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

Add null support for constants and defaults #208

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

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Mar 18, 2024

Purpose

Finish out the Null support in ROSIDL_python.

Tests Performed

We've tested this on humble and it works great! Note, I added a bunch more checks for equality that should protect against regressions in similar areas.

Ticket

Fixes #207

Attribution

This work was contributed on behalf of the MacCreadyWorks department at AeroVironment, Inc

@mjcarroll
Copy link
Member

I think this brings up kind of a larger question. By IEEE-754 definition nan != nan, so if we have a message with a single float data field, both set to nan, should those messages be equal to each other?

@Ryanf55
Copy link
Author

Ryanf55 commented Mar 28, 2024

I think this brings up kind of a larger question. By IEEE-754 definition nan != nan, so if we have a message with a single float data field, both set to nan, should those messages be equal to each other?

To me, they are the same message. You are checking equality of the messages and practically means the contents are the same.

For purposes of the equality, I use the operator just before a call to publish to check if the message is the same. If it's the same, publishing the same thing is a waste of CPU. That said, we can't assume that use case for everyone. As implemented, I vote for this behavior, but am open to other options.

@mjcarroll
Copy link
Member

I'm going to bring this up at our next team meeting, because I think you aren't wrong, but want to make sure that we think through the potential impacts.

@Ryanf55
Copy link
Author

Ryanf55 commented Jun 5, 2024

I'm going to bring this up at our next team meeting, because I think you aren't wrong, but want to make sure that we think through the potential impacts.

Did you get any feedback? I'm still carrying all these patches internally, and would love to get them merged!

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.

Message equality fails with NaN values for floats/doubles
4 participants