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

some fixes for the --repeat_latched option #2201

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

1r0b1n0
Copy link
Contributor

@1r0b1n0 1r0b1n0 commented Nov 4, 2021

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 :

  • roscore
  • publish a latched topic : rostopic pub -l /chatter_latched std_msgs/String "data: 'blabla'"
  • publish a periodic topic : rostopic pub -r 2 /periodic std_msgs/Empty
  • launch a rosbag record : rosrun rosbag record -a --repeat-latched --duration 5 --split

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

@1r0b1n0 1r0b1n0 changed the title add missing repeat_latched initialisation some fixes for the --repeat_latched option Nov 5, 2021
@tkazik
Copy link

tkazik commented Jan 17, 2022

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?

repeat_latched(false),

@1r0b1n0
Copy link
Contributor Author

1r0b1n0 commented Feb 9, 2022

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
Copy link

Can this be merged please? It contains essential fixes.

@romainreignier
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

1r0b1n0 commented Feb 15, 2023

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

@romainreignier
Copy link

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.

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.

6 participants