Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAN: Loopback() #286

Open
wants to merge 5 commits into
base: feature/can_bus
Choose a base branch
from
Open

Conversation

LinjingZhang
Copy link
Contributor

By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md
--- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

CONTRIBUTING.md also tells you what to expect in the PR process.

Description
Add loopback functionalities for CAN lib.

Related Issue
NA

Context
Add loopback() function.
Basically writing application code instead of a library... but no idea how to do better using XMCLib.

@ederjc
Copy link
Member

ederjc commented Jul 5, 2024

Thanks LJ! I can't review a lot here. I could test it of course, if you describe the test setup. For me it's fine to have this WiP on the feature branch. Can you create follow-up tickets for the open todos?
In the example there is still the namespace ifx. Please refer to this comment.

@ederjc ederjc removed their request for review July 5, 2024 05:24
@@ -0,0 +1,61 @@
// Copyright (c) Linjing Zhang. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big topic this header about the licensing :)
We can talk about it.

@@ -202,14 +220,63 @@ CANXMC::~CANXMC() {}

int CANXMC::observe()
{
XMC_CAN_NODE_SetInitBit(_XMC_CAN_config->can_node);
// TOOD: LJ: no idea what this is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tood? xD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol I will fix it

XMC_CAN_Enable(CAN_xmc);
/* Configuration of CAN Node and enable the clock */
XMC_CAN_InitEx(CAN_xmc, _XMC_CAN_config->can_clock , _XMC_CAN_config->can_frequency);
if (XMC_CAN_STATUS_SUCCESS == XMC_CAN_NODE_NominalBitTimeConfigureEx( CAN_NODE1, &CAN_NODE_bit_time_config)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, keep that in mind for the whole file. We don´t have a formating tool yet, but at least within a file we can keep some consistancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I formatted locally using clang style.
I can also add this: https://github.com/marketplace/actions/clang-format-code-formatter for style checking. Is it ok?
That way we don't get PRs with a lot of formatting changes... like the opened one...

Copy link
Member

@OlafFilies OlafFilies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so far looks good to me.

@LinjingZhang
Copy link
Contributor Author

Thanks LJ! I can't review a lot here. I could test it of course, if you describe the test setup. For me it's fine to have this WiP on the feature branch. Can you create follow-up tickets for the open todos? In the example there is still the namespace ifx. Please refer to this comment.

Hi Julian, for loopback you need no extra hardware setup. You could test it with a single XMC1400 2Go. I did not test it on XMC4000 series, maybe you can do that.

For The namespace, I talked once with Jens (related ticket: https://jirard.intra.infineon.com/browse/DESMAKERS-3828), namespace offers several benefits, for example avoiding naming conflicts with third-party libraries. I also referenced this post: https://luckyresistor.me/2014/10/31/how-and-why-to-use-namespaces/
From a user's point of view, one line of using namespace wouldn't give me much trouble. Would you consider keeping it?

@ederjc
Copy link
Member

ederjc commented Jul 5, 2024

If you tested it on the 2Go it's fine for the feature branch.

Regarding the namespace it's a matter of hw abstraction which should not be handled in the application code. I would like to allow our device to work also with third party code and vice versa. But I can check with Jens next week :)

For me it's fine to merge this to the feature branch if Juan and Olaf agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants