search

ros-geographic-info / unique_identifier

ROS support for Universally Unique Identifiers
http://ros.org/wiki/unique_identifier
16 stars 19 forks source link

Add convenience helper functions #17

Closed peci1 closed 3 years ago

peci1 commented 3 years ago

Working with UUIDs might not seem straightforward to some people. I think this could be eased a bit by adding a few helper functions, e.g. a static method for generating IDs and a stringifying function (maybe together with operator<<). Similar could be provided for Python.

gencpp can even utilize "plugins" which allow adding code inside the message class definition, e.g. custom class methods.

It might happen that I'll send a PR for this issue, but definitely not anytime soon. If somebody else would pick this up, I can help with the gencpp plugins, I've written a few of them already.

SteveMacenski commented 3 years ago

https://github.com/ros-geographic-info/unique_identifier/blob/master/unique_id/include/unique_id/unique_id.h

I think that's already available. There's no python3 version, but the uuid library in python has a simple API version of most of those.

peci1 commented 3 years ago

Ahh, you're right. Then the ROS wiki entries are both lacking, as they do not inter-link these two packages together. I got the impression that they're two different approaches.

What's the reason for splitting the messages and their helpers to different packages, and not following e.g. the sensor_msgs way?

SteveMacenski commented 3 years ago

This pre-dates me. I was looking up the first commit on it as a joke like "I was in high school when this package was made" but found that the very first commit of this repo when it moved to GitHub was literally splitting the package https://github.com/ros-geographic-info/unique_identifier/commit/ff518a3929d7b1cf2039a9a891955ba602ab2fc2#diff-32a9d8e563f5e25b0d10078a5cac3c1791beef69e1b76f235522fa2ff414aff4. So its literally ancient.

peci1 commented 3 years ago

Okay, let's ask the dinousaurs :-D I guess that means there's no way the packages could be merged in ROS 1. I'm not sure what's the ROS 2 situation...

SteveMacenski commented 3 years ago

It could be in ROS2, but the hurdle is a little higher since its being managed by OR. I also don't see much reason to put it in the same package if its been in existence for 9 years this way. It would just make porting from ROS1 -> ROS2 that many more steps. I don't really have an opinion one way or another. I see reasons for both.

peci1 commented 3 years ago

Okay, then I'm closing this issue.

ericperko commented 3 years ago

Dinosaur here.

IIRC we split the messages and the helpers apart to improve the dependency situation. For instance, if you had an C-only embedded device using some other UUID generation library, you may not want to build and deploy all of the extra stuff for a standard set of C++ and Python APIs, but you would want the messages so that you could transmit data off the device in ROS-native format. As to why it is different than sensor_msgs, unless somebody refactored sensor_msgs along the way, that stuff is even older than ros-geographic-info :-)

I admit I don't know if this is still the best-practice in the ROS ecosystem as I haven't used ROS regularly in many years, but that's the context that comes to mind.

peci1 commented 3 years ago

Thanks for your info :) Yeah, I think ROS really suffers here from the inability do capture conditional dependencies in package.xml... Something like "if your ROS already uses rospy, then require this, but do not pull whole rospy just because of this package".