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

Actually pass the parent pointer to periodic updates. #196

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

Y-Less
Copy link

@Y-Less Y-Less commented Feb 28, 2023

add_can_lib_update_callback, like many other callback methods, takes a parentPointer and stores said pointer in CanLibUpdateCallbackInfo. However, unlike basically every other example of callbacks that I could find this meta-data is not actually passed to the callback itself, in fact the only place it seems to be used is in remove_can_lib_update_callback to confirm that you're removing the correct instance.

This updates the signatures and calls to actually pass the parent pointer to the callbacks themselves.

@Y-Less Y-Less temporarily deployed to Integrate Pull Request February 28, 2023 04:22 — with GitHub Actions Inactive
@ad3154
Copy link
Member

ad3154 commented Feb 28, 2023

Hey there! Welcome to the project, and thanks for your contribution!

I think this is good for consistency, especially if we ever refactor the network manager to be a multiple instance object like was described in #161, in which case we will need to make this change anyways.

One thing I will note is that many compilers will complain about passing a value that is not used. One way to avoid this is to just omit the variable name from the consumer function. For example, if you change void update_CAN_network(void *parentPointer) to void update_CAN_network(void *) in the example files, most compilers will be happy and not generate a warning at compile time.

If you don't mind going back through and changing that in the example files, that would be most appreciated, and I'd be happy to approve your PR and get it merged in!

Thanks,

Adrian

@ad3154 ad3154 self-assigned this Feb 28, 2023
@ad3154 ad3154 added the enhancement New feature or request label Feb 28, 2023
@ad3154
Copy link
Member

ad3154 commented Feb 28, 2023

address_claim_tests.cpp Also appears to be failing - just need to update that file similar to how you updated the example files and it'll be happy again.

@Y-Less Y-Less temporarily deployed to Integrate Pull Request March 2, 2023 07:03 — with GitHub Actions Inactive
@Y-Less
Copy link
Author

Y-Less commented Mar 2, 2023

Done. I'm not sure how I missed that add_can_lib_update_callback instance in the test when I did the initial changes. Oh well...

@sonarcloud
Copy link

sonarcloud bot commented Mar 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 8 Code Smells

75.0% 75.0% Coverage
0.0% 0.0% Duplication

@ad3154 ad3154 merged commit 1f4178c into Open-Agriculture:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants