search

ros-geographic-info / geographic_info

ROS packages for geographic information
http://ros.org/wiki/geographic_info
60 stars 61 forks source link

Add GeoPoint Validation #56

Closed Ryanf55 closed 4 months ago

Ryanf55 commented 4 months ago

Purpose

GeoPoint is a message type with comments in the header on the limits for the values. I have a lot of code duplication in ROS nodes by checking this over and over.

As a solution, I propose packaging a validation function that can be used by anyone.

Details

I added a new CMake target, along with tests. The validation function uses [[nodiscard]] so I bumped up the C++ requirement. It also includes a comprehensive test suite for all the cases in the comments.

Copyright

The repo is set to BSD, but I can't figure out what to put in the C++ files to make the ament_copyright linter happy.

Attribution

This works is contributed from AeroVironment, Inc, under the existing BSD license of the repo. I did not add license texts to the work here because "BSD" is not a valid license, and the ROS tooling does not support unknown licenses. Regardless, the files contributed into this PR fall under the license declared in the package.xml, or any newer version of BSD if the maintainers desire it.

Details: https://robotics.stackexchange.com/questions/109592/how-to-use-ament-lint-commons-ament-copyright-on-a-bsd-licensed-package-such/109593#109593

Related

Tully did the same thing here: https://github.com/ros2/common_interfaces/blob/rolling/sensor_msgs/CMakeLists.txt#L68

See this PR: https://github.com/ros2/common_interfaces/pull/183

Maybe we could get one of their reviewers to give this a pass?

Ryanf55 commented 4 months ago

Hi Steve, any chance we (ie you) could spin a bloom release for this in rolling within a week and see how it sticks?

SteveMacenski commented 4 months ago

Next Friday is my bloom day, so I'll get to it then in bulk with everything else. I know there's the rolling deadline (which I asked this morning to get a timeline for https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/3) which actually would make any new PRs auto-reject, if I'm reading this right. Else, I could potentially move it up a week, but it seems moot.

Ryanf55 commented 4 months ago

Next Friday is my bloom day, so I'll get to it then in bulk with everything else. I know there's the rolling deadline (which I asked this morning to get a timeline for https://discourse.ros.org/t/preparing-ros-2-rolling-for-the-transition-to-ubuntu-24-04/35673/3) which actually would make any new PRs auto-reject, if I'm reading this right. Else, I could potentially move it up a week, but it seems moot.

Perfect, thanks!

P3TE commented 4 months ago

As someone still on ROS2 foxy, this change broke my build. If anyone experiences the same problem, consider using the tag 1.0.4

git fetch --tags
git checkout -b 1.0.4 tags/1.0.4