-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add missing wrappers for uavcan/si #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @joelsa ☕ 👋
Thank you very much for your contribution. I love it, it's mostly perfect. If you fix the namespace thing then you can copy this scheme for all missing types. Also I'll fix up the CI error for you sometime today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment, sorry I had mis-clicked.
I've fixed the broken CI build in #134. Please rebase your PR on 107-Arduino-UAVCAN:master. |
Thanks! I hope rebasing and then force-pushing my forked branch is the correct way? I did not see any other way since it could not be fast-forwarded. |
Yes, in case of such a rebase you'll need a force-push (nothing wrong with |
Good morning @joelsa 👋 ☕ How do you do? Any chance you can finish the implementation of the wrappers? Cheers, Alex |
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
=======================================
Coverage 85.64% 85.64%
=======================================
Files 17 17
Lines 641 641
=======================================
Hits 549 549
Misses 92 92 Continue to review full report at Codecov.
|
Hi 👋 Now it looks to me like the wrappers are almost complete, but the following five are still missing:
Shall I implement those too? |
Terrific, thank you! 👍 Looks like there's an issue with all the function names for With regards to the still-missing-types - that would be very neat for completeness, even if those types will never be used without composition into another type. I'd suggest another PR for those though. |
There is https://github.com/107-systems/107-Arduino-UAVCAN/tree/master/src/types/uavcan/si/sample/_torque as well as https://github.com/107-systems/107-Arduino-UAVCAN/tree/master/src/types/uavcan/si/sample/torque They are identical, do you want me to remove the redundant one, or is there a particular reason for the duplication? |
I think it might have been a generation issue with Nunavut or something similar? In any case, the data types was only used in |
Yes, already fixed: OpenCyphal/nunavut#188 |
Memory usage change @ 67dceff
Click for full report table
Click for full report CSV
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thank you very much! ❤️
You allready resolved everything in a perfect way! Thank you! In the (possibly nearer) future there shall be automatic header generation, re-running the C headers was / is on my todo-list. |
This PR is intended as a draft PR related to #98. I started with
uavcan/si/sample/acceleration
. I plan on creating wrappers like this for alluavcan/si
data types, but I would be very grateful if you @aentinger are able to check if this is the correct approach and how you intended it to be done before I move on to the rest.