Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

publish a vector of bytes given a topic type and message definition #194

Merged
merged 11 commits into from
Sep 23, 2024

Conversation

lucasw
Copy link
Contributor

@lucasw lucasw commented Aug 16, 2024

Description

Publish a message using a topic type string and message definition string but without having a RosMessageType. One example is loading serialized data out of an mcap or rosbag and publishing it directly.

Maybe the md5sum could be computed from the definition instead of using "*", but the asterisk is the AnyMsg/ShapeShifter method (https://github.com/ros-o/ros_comm/blob/obese-devel/clients/rospy/src/rospy/names.py#L44-L53 & https://github.com/ros-o/ros_comm/blob/obese-devel/tools/topic_tools/include/topic_tools/shape_shifter.h#L115-L131, though I think under some circumstances that is updated to the actual md5sum ergo the 'shape shifter' name). I see compute_md5sum over in roslibrust_codegen but the definition needs to go through a reverse of compute_full_definition to be the right format.

Fixes

It's the publisher version of #190 but I didn't make an issue for it.

Checklist

  • [ ✔️] Update CHANGELOG.md

Comment on lines 182 to 183
|| responded_header.md5sum == Some("*".to_string())
|| conn_header.md5sum == responded_header.md5sum {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if they are both "*" there shouldn't be a connection? Or is that okay, neither will know anything about what the true md5sum is supposed to be and any byte payload can be sent.

@Carter12s
Copy link
Collaborator

This is pretty interesting...

I can't think of anywhere in the ROS ecosystem where a publisher would supply "*" as the md5sum? I think if we could find a practical usage of someone else doing that I'd be pretty comfortable with this, otherwise I'd want to test the crap out of this with rospy, roscpp, and various cli tools to make sure they don't choke on it.

I think the "right" thing to do is definitely to calculate the m5sum from the msg definition passed in. Let me take a look today at how hard I think that might be...

@Carter12s
Copy link
Collaborator

Okay I pushed an example and some documentation updates.

I can confirm at least that rostopic echo is happy to receive the data for a very basic message type, so maybe md5sum isn't required?

After looking at what it would take to compute the m5sum it'll take some work, but shouldn't be to bad. We could require the msg_definition that is passed in to be the fully expanded definition that includes the definitions of the sub messages referenced. That is probably the right call, as that is what will be needed for the message to work with something like plot_juggler. It means the user has to have access to the expanded definition (which our message generation can provide to them), I suspect the expanded definition is what is present in a rosbag, but I'm not 100% sure.

We can then break that message definition apart and recursively calculate the md5sum off of the message and its sub-definitions which isn't too hard to do.

I'd say either:

  • Confirm rospy and roscpp are happy with the publisher providing * for md5sum
    OR
  • Do the md5sum stuff

I'm happy with either.

@lucasw
Copy link
Contributor Author

lucasw commented Aug 22, 2024

Doing the md5sum sounds cleaner but possibly a pain to get just right.

rosbag store md5sums in the bag, it reads them out here:

class _ConnectionInfo(object):   
    def __init__(self, id, topic, header):
        try:
            datatype = _read_str_field(header, 'type')
            md5sum   = _read_str_field(header, 'md5sum')
            msg_def  = _read_str_field(header, 'message_definition')

(rosbag/bag.py)

I was thinking I should do the same in my mcap recorder then on playback it doesn't need to recompute them (or resort to *), but mcap convert doesn't do that so I don't want to break compatibility there.

@Carter12s
Copy link
Collaborator

Hrmmm...

I'm less familiar with MCAP what does it record? Full message definition with submessages? The fact that rosbag stores the md5sum would be really handy...

We should probably just test the "*" thing as quickly as possible with roscpp and rospy, and if it doesn't work I can get fancy with computing the md5sum.

@lucasw
Copy link
Contributor Author

lucasw commented Aug 23, 2024

It turns out rostopic echo needs the md5sums (it uses get_message_class to get a real message class to put into the rospy Subscriber), but rostopic hz works without them (it uses an AnyMsg subscriber). I might try to make md5sum from message_definition work in python as an experiment within https://github.com/lucasw/ros_one2z/blob/main/one2z/scripts/ros1_play_mcap.py#L106-L131, and maybe can get help from elsewhere, and then I could help more with the rust version. (https://robotics.stackexchange.com/questions/65290/message-md5-generation maybe is most of what I need)

I can try out roscpp subscribers to AnyMsg later.

Also I've noticed plotjuggler can't publish ros1 messages from an mcap (generated either from the official mcap convert or my mcap_record), the topics don't even show up under rostopic list- maybe not a huge task to correct though. (PlotJuggler/plotjuggler-ros-plugins#90)

@lucasw
Copy link
Contributor Author

lucasw commented Aug 23, 2024

I'm less familiar with MCAP what does it record? Full message definition with submessages?

Yes the full message definition exactly like in the connection header is stored in 'schema.data' and the message type under 'schema.name', and schema.encoding is "ros1msg" (or ros2msg or protobuf for those formats). Probably the md5sum could have been squeezed into the data field, or second parallel schema entry just to store the md5sum, and there are also metadata and attachments but mcap convert leaves those empty, I haven't tried using them.

https://mcap.dev/spec#schema-op0x03


update I see now mcap convert does store md5sums, per channel, in the channel metadata (and there is a channel per-publisher, not just per-topic, so n publishers to /tf each get their own channel with mostly duplicate channel metadata)

lucasw/mcap_tools#9 (comment)

@lucasw
Copy link
Contributor Author

lucasw commented Aug 24, 2024

I've only tried this python message-definition-to-md5 https://github.com/lucasw/mcap_tools/blob/main/mcap_tools/scripts/msg_def_md5.py for a few messages and so far so good:

./msg_def_md5.py nav_msgs/Odometry
source message type: 'nav_msgs/Odometry',
cd5e73d190d741a2f92e81eda573aca7 <- stored md5
cd5e73d190d741a2f92e81eda573aca7 <- computed md5

There's some ambiguity where the message definition strips off package names (but not always, unless 'Header'?). The package name in field is stripped off when the message being defined is in the same package, and in addition Header is always without std_msgs? I don't think there are going to be very many message with say a turtlesim/Pose and a geometry_msgs/Pose and the md5sum would turn out wrong, but I'll try creating just that message and see what happens to the definition.

@lucasw
Copy link
Contributor Author

lucasw commented Aug 31, 2024

Here's the python connection header message definition to md5sum converted to rust: lucasw@04bb108

I expect there are more than several str vs. String style/efficiency improvements to be made (and more map & filter to replace much of it) but it appears to work with handful of message test cases, I'll try it out with message publishing next.

@Carter12s
Copy link
Collaborator

LUCAS! Awesome work. I pulled in your changes adding the seperate md5sum calculation function, and then refactored the code a bit (in part so I could understand it better) and in part to improve readability.

I think the last step now is plumbing this through the .advertise_any() call and this will be ready to merge.

@lucasw
Copy link
Contributor Author

lucasw commented Sep 4, 2024

The plumbing part is sitting near the top of my commits over in my development branch- lucasw@1ddebf1 specifically- I'll try out your changes and those together tomorrow.

@lucasw
Copy link
Contributor Author

lucasw commented Sep 9, 2024

I updated the comment above about mcap #194 (comment) - it turns out there are md5sums stored by mcap convert (but not where I was expected them, in Channels instead of Schemas), which if I'd realized sooner I wouldn't have made the definition-to-md5sum conversion. Having already done it I think the conversion can stay as-is here, though a second advertise_any() could allow the caller to supply the md5sum. Maybe there's some other use case too.

Now I'm trying to sort out how to get the same connection header information that is stored in rosbags from roslibrust into mcap_record- in a rosbag there is per message information on which publisher the message came from and the publisher node name is stored, at least sometimes when the callerid is available, but in roslibrust that gets discarded, or is otherwise difficult to access? In my modifications here I store and later query the most recently received publisher connection header, but the older ones are overwritten.

I don't strictly have to conform to everything mcap convert does (and if I break compatibility I'll have to make a new mcap convert to use instead, probably in python because support for rosbags in rust I think is incomplete- or foxglove may accept PRs to change the mcap tool).

@Carter12s Carter12s merged commit 569d954 into RosLibRust:master Sep 23, 2024
5 checks passed
@Carter12s
Copy link
Collaborator

Apologies for the delay, life took me out for a bit, this is now merged!

  • I'll file an issue for accessing TopicInformation from both Publisher and Subscriber, besides the header information it would be good to give clients access to the number of connections and connection status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants