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

Add in a warning for a KeepLast depth of 0. #2048

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

clalancette
Copy link
Contributor

It really doesn't make much sense to have a KeepLast depth of 0; no data could possibly be stored. Indeed, the underlying DDS implementations don't actually support this. It currently "works" because a KeepLast depth of 0 is assumed to be system default, so the RMW layer typically chooses "1" instead. But this isn't something we should be encouraging users to do, so add a warning.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

I'll note that I could easily be convinced to make this throw an exception instead. That would be a much stronger warning to users not to do this, but it does break existing practice so has some risk of downstream breakage. Happy to discuss it.

Copy link
Collaborator

@alsora alsora left a comment

Choose a reason for hiding this comment

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

I usually think that user-specified settings that do not make sense should result in immediate crashes that are not possible to ignore, to make the robot fail as soon as possible.

However, here there's an additional complexity: what happens if someone changes the QoS at runtime using the qos_parameters_overrides?
In this case we should rather reject the change as we don't want people to be able to kill robots that are already running.
This probably is out of scope for the tiny change that you did here.

@ivanpauno
Copy link
Member

However, here there's an additional complexity: what happens if someone changes the QoS at runtime using the qos_parameters_overrides?
In this case we should rather reject the change as we don't want people to be able to kill robots that are already running.

The parameters overrides are only read once at startup, they cannot be changed aferwards.

It really doesn't make much sense to have a KeepLast depth of 0;
no data could possibly be stored.  Indeed, the underlying DDS
implementations don't actually support this.  It currently "works"
because a KeepLast depth of 0 is assumed to be system default,
so the RMW layer typically chooses "1" instead.  But this isn't
something we should be encouraging users to do, so add a warning.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

clalancette commented Nov 29, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

CI is clean here, so merging.

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.

None yet

4 participants