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.

Sign up for GitHub

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

Jump to bottom

modules: canopennode: add rxmsg callback #79472

Merged
merged 1 commit into from
Oct 16, 2024
Merged

modules: canopennode: add rxmsg callback #79472

merged 1 commit into from
Oct 16, 2024

Conversation

chgabriel79
Copy link
Contributor

Implement callback for incoming CAN messages for any of the configured filters for CANopenNode.

This can be used to wake the loop calling CO_process when a new message was received which needs processing.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

Hello @chgabriel79, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

henrikbrixandersen
henrikbrixandersen requested changes Oct 7, 2024
View reviewed changes
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. A few minor comments.

modules/canopennode/canopennode.h Outdated
*
* Setting a new callback will overwrite any existing callback.
*
* @param callback the callbak to set
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param callback the callbak to set
* @param callback the callback to set

modules/canopennode/CO_driver.c Outdated Show resolved Hide resolved
modules/canopennode/CO_driver.c Outdated
@@ -31,6 +31,8 @@ K_MUTEX_DEFINE(canopen_send_mutex);
K_MUTEX_DEFINE(canopen_emcy_mutex);
K_MUTEX_DEFINE(canopen_co_mutex);

static canopen_rxmsg_callback rxmsg_callback;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static canopen_rxmsg_callback rxmsg_callback;
static canopen_rxmsg_callback rxmsg_callback = NULL;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compliance check does not seem to like this change. Shall I revert it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry about that.

modules/canopennode/CO_driver.c Outdated Show resolved Hide resolved
@chgabriel79 chgabriel79 force-pushed the canopennode-callback branch 3 times, most recently from 17fc160 to e8b9ba9 Compare October 7, 2024 14:30
@chgabriel79
Copy link
Contributor Author

I added your suggestions, and also clarified that the callback is run in interrupt context (CAN rx callback). And I had some problems with my client not adding the files to the commit, sorry for the noise...

@chgabriel79 chgabriel79 force-pushed the canopennode-callback branch from e8b9ba9 to 191d99b Compare October 7, 2024 15:25
alexanderwachter
alexanderwachter previously approved these changes Oct 7, 2024
View reviewed changes
Copy link
Member

@alexanderwachter alexanderwachter left a comment

Choose a reason for hiding this comment

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

Looks good.
Just a thought:
Is a callback disable function necessary?
Or maybe in the API description that a NULL ptr disables the callback?

@chgabriel79
Copy link
Contributor Author

Or maybe in the API description that a NULL ptr disables the callback?

It states "Setting a new callback will replace any existing callback."

I used a similar wording to other callbacks, e.g. can_set_state_change_callback which states "Calling this function again overrides any previously registered callback." (and also does not explicitly mention passing NULL disables the callback).

@chgabriel79
modules: canopennode: add rxmsg callback
a49ccb8 
Implement callback for incoming CAN messages for any of the
configured filters for CANopenNode.

This can be used to wake the loop calling CO_process when a
new message was received which needs processing.

Signed-off-by: Christian Gabriel <[email protected]>
@chgabriel79 chgabriel79 force-pushed the canopennode-callback branch from 191d99b to a49ccb8 Compare October 8, 2024 06:08
@kartben
Copy link
Collaborator

kartben commented Oct 16, 2024

Commenting to see if that's enough to get the PR to show as green in the merge dashboard 🤷🏼

@henrikbrixandersen henrikbrixandersen added the Enhancement Changes/Updates/Additions to existing features label Oct 16, 2024
@kartben
Copy link
Collaborator

kartben commented Oct 16, 2024

@henrikbrixandersen do you want to try rebasing in case (unlikely IMO) merge dashboard is right by saying something is wrong with CI somehow?
Alternatively, just do a merge batch and pick this PR since AFAICT it is green

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen do you want to try rebasing in case (unlikely IMO) merge dashboard is right by saying something is wrong with CI somehow? Alternatively, just do a merge batch and pick this PR since AFAICT it is green

It was missing my approval after the latest force-push, so no - the merge-list had it correctly listed.

@henrikbrixandersen henrikbrixandersen merged commit 5c63839 into zephyrproject-rtos:main Oct 16, 2024
25 checks passed
@github-actions GitHub Actions
Copy link

Hi @chgabriel79!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinjaeger martinjaeger martinjaeger approved these changes

@alexanderwachter alexanderwachter alexanderwachter approved these changes

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@str4t0m str4t0m Awaiting requested review from str4t0m

Assignees

@henrikbrixandersen henrikbrixandersen

Labels
area: CAN Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chgabriel79 @kartben @henrikbrixandersen @alexanderwachter @martinjaeger @zephyrbot