common_interfaces icon indicating copy to clipboard operation
common_interfaces copied to clipboard

sensor_msgs/Range lacks variance field

Open ejalaa12 opened this issue 2 years ago • 12 comments

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: https://github.com/ros/common_msgs/issues/142

ejalaa12 avatar Mar 17 '22 09:03 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 ?

ejalaa12 avatar Mar 17 '22 10:03 ejalaa12

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 avatar Mar 17 '22 12:03 clalancette

@ros-pull-request-builder retest this please

clalancette avatar Mar 22 '22 19:03 clalancette

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.

gbiggs avatar Mar 24 '22 05:03 gbiggs

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.

tfoote avatar Apr 05 '22 05:04 tfoote

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

clalancette avatar Apr 05 '22 12:04 clalancette

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

gbiggs avatar Jul 06 '22 22:07 gbiggs

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.

tfoote avatar Jul 06 '22 23:07 tfoote

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 ?

ejalaa12 avatar Jul 07 '22 17:07 ejalaa12

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.

tfoote avatar Jul 07 '22 23:07 tfoote

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

ejalaa12 avatar Aug 09 '22 11:08 ejalaa12

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.

tfoote avatar Sep 22 '22 22:09 tfoote

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

ejalaa12 avatar Mar 08 '23 23:03 ejalaa12

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

tfoote avatar Mar 10 '23 18:03 tfoote

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

ejalaa12 avatar Mar 13 '23 11:03 ejalaa12

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

clalancette avatar Mar 13 '23 20:03 clalancette