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

Current message template file does not correctly handle float constant expresssions. #20444

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bl0wfi-sh
Copy link

Changing msg.h.em template to properly handle const references of type float. Without this float values will be interpreted as int's and loose decimal resolution.

Describe problem solved by this pull request

The current message template interprets all constant reference variables as int's regardless of what type is actually defined in the .msg file.

Example .msg file showing how a float might be used.

Ex: custom_actuator_test.msg
float32 CUSTOM_ACTUATOR_MIN = 3.141592653589793238

With the original template file 'msg.h.em' the value of CUSTOM_ACTUATOR_MIN would become 3 instead of the intended value of 3.141592653589793238

Describe your solution

To properly handle floats the 'msg.h.em' file was augmented.

To handle the creation of accurate #define values the following changes were made at line 85.
@{ for constant in spec.constants: if "float" in constant.type: print('#define %s_%s %s'%(spec.short_name.upper(), constant.name, float(constant.val))) else: print('#define %s_%s %s'%(spec.short_name.upper(), constant.name, int(constant.val))) }@

Finally to handle the accurate creation of static constexp variables the following changes were made at line 125.
if "float" in type_name: print('\tstatic constexpr %s %s = %s;'%(type_px4, constant.name, float(constant.val))) else: print('\tstatic constexpr %s %s = %s;'%(type_px4, constant.name, int(constant.val)))

Describe possible alternatives

I do not see any other alternative if someone wants to have a 'const expression' of type float32/64 inside there uORB message, and they don't want to loose decimal accuracy.

Test data / coverage

Tested on the bench with a custom message and a custom module that prints the const expression value of the message endlessly.

Additional context

NA

…at type. Without this float values will be interpreted as int's and loose decimal resolution.
Copy link
Contributor

@junwoo091400 junwoo091400 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Wasn't aware of this pitfall 🥶

@bl0wfi-sh
Copy link
Author

We may want to consider adding additional support for other constant types like string's. This can help px4 become more agnostic with ROS message as defined here http://wiki.ros.org/msg.

I have no seen any .msg files that hold constants of type string but that does not mean they won't be useful to someone.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Can you rebase to fix the conflict?

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

Successfully merging this pull request may close these issues.

None yet

3 participants