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

[rclpy_examples] QoS examples #132

Merged
merged 11 commits into from
Nov 3, 2016
Merged

[rclpy_examples] QoS examples #132

merged 11 commits into from
Nov 3, 2016

Conversation

mikaelarguedas
Copy link
Member

Add reliable/best effort talker listener example

@mikaelarguedas mikaelarguedas added the in review Waiting for review (Kanban column) label Oct 31, 2016
@mikaelarguedas mikaelarguedas self-assigned this Oct 31, 2016
parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter)
parser.add_argument(
'-r', '--reliability', type=int, default=0, choices=[0, 1],
help='0: reliable, 1: best effort')
Copy link
Member

Choose a reason for hiding this comment

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

When being called reliability I would expect the value 1 to actually be reliable.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for pointing it out, fixed in 1ce6239

parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter)
parser.add_argument(
'-r', '--reliability', type=int, default=0, choices=[0, 1],
help='1: reliable, 0: best effort')
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other options planned? If not I would suggest using a boolean flag --reliable instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hesitated to add other profiles that's why I settled for a integer parameter. Given that this is supposed to be an example I think we can keep it simple and restrict ourselves to only 2 qos profiles.

@dirk-thomas
Copy link
Member

Does it make sense to create new scripts rather then extending the already existing ones?

@mikaelarguedas
Copy link
Member Author

I see your point, this comes down to how simple do we want the examples to be, should the basic pub/sub examples expose how to configure the Quality of Service? or does it make sense to have another script showing how to handle QoS on top of the basic example?
IMO the code in this PR could be extended to expose more quality of service settings in the future and could be used as a full QoS example/tutorial and should stay dissociated from the minimalist pub/sub.
(this PR also has the goal of giving a starting point for ros2/demos#85).

@mikaelarguedas
Copy link
Member Author

thanks

@mikaelarguedas
Copy link
Member Author

If nobody has strong opinions about #132 (comment) I'm going to merge this as is

@dirk-thomas
Copy link
Member

Sure, go ahead with the merge.

@mikaelarguedas mikaelarguedas merged commit b30e0bb into master Nov 3, 2016
@mikaelarguedas mikaelarguedas deleted the qos_examples branch November 3, 2016 18:58
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Nov 3, 2016
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