-
Notifications
You must be signed in to change notification settings - Fork 167
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
Make Lost Messages option ON by default #633
Make Lost Messages option ON by default #633
Conversation
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
@@ -151,11 +153,18 @@ def subscribe_and_spin( | |||
event_callbacks=event_callbacks, | |||
raw=raw) | |||
except UnsupportedEventTypeError: | |||
assert report_lost_messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: assert not no_report_lost_messages
can stay here? it expects UnsupportedEventTypeError
only if QoSEventHandler
cannot be created from rmw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed on b2b222c
I also removed the print right after it to silently continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also removed the print right after it to silently continue.
i am okay with this but i would prefer to have it to let user know that rmw cannot deliver the report of lost messages. i guess this is informative for user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I would silently continue. I think it's probably likely that many RMWs probably don't support this feature and it would be annoying to see this message every time you run ros2 topic echo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that actually makes sense too. i am okay with either way.
…ed by implementation Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
@@ -151,11 +153,18 @@ def subscribe_and_spin( | |||
event_callbacks=event_callbacks, | |||
raw=raw) | |||
except UnsupportedEventTypeError: | |||
assert report_lost_messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also removed the print right after it to silently continue.
i am okay with this but i would prefer to have it to let user know that rmw cannot deliver the report of lost messages. i guess this is informative for user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one suggestion related to printing a deprecation warning.
@@ -78,7 +77,9 @@ def add_arguments(self, parser, cli_name): | |||
parser.add_argument( | |||
'--no-str', action='store_true', help="Don't print string fields of messages") | |||
parser.add_argument( | |||
'--lost-messages', action='store_true', help='Report when a message is lost') | |||
'--lost-messages', action='store_true', help='DEPRECATED: Does nothing') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to this help message, I think we should print a deprecation warning to stderr if someone uses this. That way we can remove this option in the future with less chance of breaking someones script.
E.g. perhaps at the start of main()
below:
if args.lost_messages:
print(
"WARNING: '--lost-messages' is deprecated; lost messages are reported by default",
file=sys.stderr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed on: b3f1a9d
@@ -151,11 +153,18 @@ def subscribe_and_spin( | |||
event_callbacks=event_callbacks, | |||
raw=raw) | |||
except UnsupportedEventTypeError: | |||
assert report_lost_messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I would silently continue. I think it's probably likely that many RMWs probably don't support this feature and it would be annoying to see this message every time you run ros2 topic echo
.
@fujitatomoya It looks like the CI you posted is not referencing the correct build. |
it is weird, launcher build https://ci.ros2.org/job/ci_launcher/8341/console links different platform builds. it looks like platform build number has offset +1... i will try to rebuild. |
branch name was incorrect. |
Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Signed-off-by: Gonzalo de Pedro gonzalo@depedro.com.ar