sensor_msgs/Range lacks variance field

[ejalaa12]

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

[ejalaa12]

For some reason the build is failing on the std_msgs pkg, which is completely independant from my change...
@gbiggs would you know why ?

[clalancette]

For some reason the build is failing on the std_msgs pkg

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.

[clalancette]

@ros-pull-request-builder retest this please

[clalancette]

Other than the small nit inline, I think this is a fine addition. @gbiggs @tfoote could you comment here with your thoughts?

[clalancette]

Small nit: no newline at the end of the file.

[ejalaa12]

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)

[gbiggs]

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.

[tfoote]

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.

[clalancette]

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).

[gbiggs]

@tfoote @clalancette How would you recommend going about finding the current usages? sensor_msgs is a common and broadly-used package so just finding the reverse dependencies isn't feasible.

[tfoote]

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.

[ejalaa12]

Hey, I'm not sure what you guys mean by "do due diligence"?
What concretely needs to be done ?

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.

So the idea is to find all packages that uses the Range message and notify them (by opening PRs or Issues) so that they can now add the variance when publishing those messages ?

[tfoote]

So the idea is to find all packages that uses the Range message and notify them (by opening PRs or Issues) so that they can now add the variance when publishing those messages ?

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.

[ejalaa12]

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 sensor_msgs listed in the package.xml dependencies. Then run a grep to find instance of Range in their code.
I looked for stuff like

• 'sensor_msgs::msg::Range',
• 'from sensor_msgs.msg import Range',
• 'sensor_msgs.msg.Range',

Here are the list of repos that came out. My script might have missed some...

https://github.com/mavlink/mavros.git
https://github.com/ros-simulation/gazebo_ros_pkgs.git
https://github.com/cyberbotics/webots_ros2.git
https://github.com/MRPT/mrpt.git
https://github.com/ros2/rviz.git

I'm a bit surprised that I have so few, but since I'm only looking at "officially distributed" ros packages...

[tfoote]

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.

[ejalaa12]

Hi,
In my precedent message I listed a few repos/packages that use the Range msg.
That list is the best I could get at the time.

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?
As this is a non breaking change, I think as soon as the issues have been created, I don't see a reason to wait any longer.

Thank you again

[tfoote]

+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.

[ejalaa12]

I've just created the tracking issues.
Feel free to comment them as needed.
And thank you for the response.

[clalancette]

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.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows