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

update documentation for rosbag2 composable player #1799

Closed

Conversation

berndpfrommer
Copy link
Contributor

all: true should read all_topics: true

This was brought to my attention on the robotics stack exchange here

@berndpfrommer berndpfrommer requested a review from a team as a code owner September 4, 2024 22:54
@berndpfrommer berndpfrommer requested review from gbiggs and jhdcs and removed request for a team September 4, 2024 22:54
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm.

record_options.all_topics = node.declare_parameter<bool>("record.all_topics", false);

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@berndpfrommer Could you please elaborate why it is should be all_topics instead of all?

Inspight the fact that we added recently a new CLI options like --all-topics and --all-service we still have --all CLI option which will "Record all topics and services (Exclude hidden topic)"

@berndpfrommer
Copy link
Contributor Author

I had a superficial look at the README, saw only "all_topics" mentioned, and uncritically assumed the failure report on stack exchange was correct.
I wonder if the user is on the latest update. Do I understand correctly that all will only work if this commit is being used? That one is fairly recent...

@MichaelOrlov
Copy link
Contributor

Ahh you are right, I've already forgotten that we had a compatibility issue in yaml parser and that I've fixed in the #1664.

Anyway, in the current state, everything should be up to date, even on a new Jazzy release and there is no need to update documentation.

@berndpfrommer
Copy link
Contributor Author

OK, closing this PR then.

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.

3 participants