search

ros-geographic-info / unique_identifier

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

RFC4122 Time-Based Version 1 UUID Generation #11

Closed SomeshDaga closed 6 years ago

SomeshDaga commented 6 years ago
jack-oquin commented 6 years ago

It would be nice to add a time-based implementation to the Python interface, as well. There is a uuid1 implementation available.

SomeshDaga commented 6 years ago

@jack-oquin thanks for the heads up on the uuid1 implementation. Gave me an opportunity to compare with the results of my C++ implementation. Have added it to the python interface as well

tfoote commented 6 years ago

@SomeshDaga a few unit tests would be really great to be confident that this will keep working correctly with future updates. Since these are free functions they're relatively easy to test and there are some good examples already that you could copy from the other functions.

SomeshDaga commented 6 years ago

@tfoote Some tests have been added. I would like to add that the generation of the clock_id which is part of the uuid, is done using a random number generator. This puts an onus on the user to seed the generator, which has been documented in the docstring. If it is not seeded, it will use the default seed which is typically updated on the order of a second. This should work for most usage scenarios I can envision. Let me know what you think

jack-oquin commented 6 years ago

This looks good to me.

@mikaelarguedas: would you look this over, checking for any ROS2 migration issues?

mikaelarguedas commented 6 years ago

this looks good to me. @tfoote would have the most context as of how to convert ROS1's ros::Time to a ROS2 version (can this simply be changed to a builtin_interfaces/Time structure?)

jack-oquin commented 6 years ago

@mikaelarguedas: I don't believe builtin_interfaces exists in ROS1. I don't understand the intended migration path for ros::Time to ROS2. Presumably, some people have thoughts about it.

tfoote commented 6 years ago

We have not fullly developed the migration path for Time. The 1 to 1 mapping right now for builtin_messages/Time is std_msgs/Time and we recently removed the special aspects of builtin_msgs so they can be reintegrated as plain messages. There are also the C and C++ native datatypes to consider supporting. Likely a templated time adapter would be the best way to have portable code between ROS and ROS2. For this change I think that this is out of scope though.

jack-oquin commented 6 years ago

@SomeshDaga: Thanks for contributing!