search

ros / ros_comm

ROS communications-related packages, including core client libraries (roscpp, rospy, roslisp) and graph introspection tools (rostopic, rosnode, rosservice, rosparam).
http://wiki.ros.org/ros_comm
761 stars 912 forks source link

some fixes for the --repeat_latched option #2201

Open 1r0b1n0 opened 2 years ago

1r0b1n0 commented 2 years ago

This should fix some problems with #1850 : 1 The repeat_latched member variable bool was left in an uninitialized/random state. On my machine, even without specifying "--repeat_latched", it was sometimes activated.

2 When repeating the latched messages in splitted bags, these should still be published as latched.

Steps to reproduce : Start the following in several terminals :

after ~15 seconds stop all the nodes and the rosbag record

without the fix : when playing a splitted bag rosrun rosbag play [...]_1.bag -s 2 : running rostopic echo /chatter_latched will show nothing

with this fix : when playing a splitted bag rosrun rosbag play [...]_1.bag -s 2 : running rostopic echo /chatter_latched will show the message, because it is latched

tkazik commented 2 years ago

Nice catch, thx a lot! My 2c: I guess I would prefer the initialization to set the repeat_latched to true. As such, every bag is "contained" in itself by default. Or are there points against that?

https://github.com/ros/ros_comm/blob/785826e1235182046d7b44026208f9633a9112a8/tools/rosbag/src/recorder.cpp#L100

1r0b1n0 commented 2 years ago

Hi, I agree in my opinion the better choice would be to enable repeat_latched by default. In this case the "--repeat-latched" option can be removed/ignored.

Martin-Oehler commented 2 years ago

Can this be merged please? It contains essential fixes.

romainreignier commented 1 year ago

Hi @mjcarroll what do you think about this? Should we enable repeat-latched by default and remove the argument or keep the choice? At least, the uninitialized variable issue should be merged, this is a bugfix. Do you want a separate PR?

mjcarroll commented 1 year ago

At least, the uninitialized variable issue should be merged, this is a bugfix. Do you want a separate PR?

Yes please, I have no issues with that part. I'll need a moment to think about the other part.

1r0b1n0 commented 1 year ago

I created #2314 that will only fix the undefined variable.

romainreignier commented 1 year ago

This PR has now been updated to only include the connection_header when writing a latched topic to a bag file. This allows to keep the latched info so rosbag play can publish it as latched.