[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

Make Lost Messages option ON by default #633

Conversation

gonzodepedro
Copy link
Contributor

Signed-off-by: Gonzalo de Pedro gonzalo@depedro.com.ar

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
Copy link
Collaborator
@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.

looks good to me.

@@ -151,11 +153,18 @@ def subscribe_and_spin(
event_callbacks=event_callbacks,
raw=raw)
except UnsupportedEventTypeError:
assert report_lost_messages
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Member

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.

Copy link
Collaborator

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

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.

@fujitatomoya
Copy link
Collaborator

CI:

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

Copy link
Member
@jacobperron jacobperron left a 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')
Copy link
Member

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)

Copy link
Contributor Author

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

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.

@jacobperron
Copy link
Member

@fujitatomoya It looks like the CI you posted is not referencing the correct build.

@fujitatomoya
Copy link
Collaborator

@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.

@fujitatomoya
Copy link
Collaborator
fujitatomoya commented Apr 29, 2021

CI: (now links look okay.)

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

@fujitatomoya
Copy link
Collaborator

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>
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.

4 participants