-
Notifications
You must be signed in to change notification settings - Fork 107
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
sensor_msgs/Range lacks variance field #181
Conversation
036fd2a
to
5f4d777
Compare
For some reason the build is failing on the |
Our PR builds are currently broken due to the transition to Ubuntu 22.04. We'll manually run CI on it once it has been reviewed. |
@ros-pull-request-builder retest this please |
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.
sensor_msgs/msg/Range.msg
Outdated
|
||
float32 variance # variance of the range sensor | ||
# 0 is interpreted as variance unknown | ||
|
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.
Small nit: no newline at the end of the file.
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! Thanks for reviewing my PR. I just removed the newline at the end of the file in the latest commit.
(sorry for the force-push I keep forgetting to sign-off with git commit -s
)
4e09f4f
to
7e0b01e
Compare
I think this is a good addition. There are now some sensors on the market that can provide estimated error for each range measurement, rather than just having a value in the data sheet. |
Going ahead with this makes sense. We should do some due diligence about reviewing current usages and providing recommended migrations for existing usage patterns. That we can add to changelogs as well as potentially open some example pull requests based on the audit of code using the message. |
Thanks for the responses. In that case, I think we have agreement that this is the way to go. However, we are now in a freeze for Humble, and we still need to do the due diligence that @tfoote mentioned. So we'll hold off on this for two weeks until we come out of the freeze (the week of April 18th). |
@tfoote @clalancette How would you recommend going about finding the current usages? |
Yes there's a lot, but cloning all the reverse depends and grepping isn't that bad. It just takes a little bit of patience. |
Hey, I'm not sure what you guys mean by "do due diligence"?
So the idea is to find all packages that uses the |
Basically yes. If the list is too long to do that we may want to reconsider this process. Along with that we should be adding documentation and linking to it from the issues opened that talks about the required migration steps in different use cases. In many cases like this with a pure addition like this it generally won't break anything. But if the variance would be useful it should be at least passed through potentially if the values are being copied. |
Is the distribution.yaml a good place to start ? I created a very naive script that goes through all those repo, filter the one that have
Here are the list of repos that came out. My script might have missed some... https://github.com/mavlink/mavros.git I'm a bit surprised that I have so few, but since I'm only looking at "officially distributed" ros packages... |
Yeah, that's the right place to look. The Range message has never had a lot of sensors providing it so it has definitely been less used. Some in the ultrasonics space too. Because this list is relatively short, it this change can be accompanied with PRs for all of those projects relatively easily. And by linking to them provide clear documentation of what will need to change as an example to other code bases. |
Hi, Should we just open Issues to those repo to notify them of the change? If so, should I do it, or would it make more sense that a developer from Ros does it? Also, do we have to wait for those issues to be opened before merging this? Thank you again |
+1 to opening tracking issues on the repositories above and then we can merge this into rolling to get it in before Iron. As you said we don't expect this to break builds with a pure extension so we don't need to wait for a response. |
I've just created the tracking issues. |
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.
This looks good to me with green CI. Thanks for doing the additional work of finding other repositories that use this. I'm going to run CI on it, and if there are no issues or objections, I'll merge it in later this week.
Fixes ros2#180 Signed-off-by: Alaa <ejalaa12@gmail.com>
Signed-off-by: Alaa <ejalaa12@gmail.com>
7e0b01e
to
736e693
Compare
Fixes #180
As stated in the original issue, the Range msg lacks the variance field as opposed to most sensor msgs.
A similar issue exist for ros1, and here the discussion about it: ros/common_msgs#142