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

Queue-Sizes Configurable Through Parameters #47

Closed
wants to merge 1 commit into from

Conversation

Jmeyer1292
Copy link
Contributor

Greetings,

Per #46, this PR allows the user of a ROS action server or action client to configure the queue size of all publishers / subscribers through a parameter interface.

Specifically, actionlib_server_sub_queue_size, actionlib_server_pub_queue_size, actionlib_client_sub_queue_size, and actionlib_client_pub_queue_size may be set. If these parameters are not set, the assumed defaults make the behaviour of the library identical to the current logic.

I understand actionlib is core infrastructure for the ROS community, and I will work with you as necessary to make this PR satisfactory if it is not already. Please review and provide feedback.

int pub_queue_size;
int sub_queue_size;
n_.param("actionlib_client_pub_queue_size", pub_queue_size, 1);
n_.param("actionlib_client_sub_queue_size", sub_queue_size, 10);

Choose a reason for hiding this comment

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

Should the default values be the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@safrimus You are absolutely right. I apologize for waiting three days to respond to this, I didn't see it... Will fix right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the changes and did a rebase, but it's not currently showing up on Github. I cloned the repository to a new directory to test, and my changes show up there. I'll let Github's front end have a few hours to register the change.

@mikaelarguedas
Copy link
Member

@ros-pull-request-builder can you retest this now that merge conflicts have been resolved?

action client and action server's C++ implementation.
@Jmeyer1292
Copy link
Contributor Author

I did a reword on the commit message in order to trigger a new test round.

@mikaelarguedas
Copy link
Member

Sorry for the delay. Is it possible to add a basic test/launchfile, setting the queue-sizes through parameters?
Thanks!

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Jun 22, 2016

@Jmeyer1292 would you mind adding the same configuration on the python server/client for feature parity? Otherwise I'll open a PR against your branch before merging this

@Jmeyer1292
Copy link
Contributor Author

@mikaelarguedas Now I'm sorry for the delays. I will attempt to address both the test launch files and the python feature parity.

@mikaelarguedas mikaelarguedas mentioned this pull request Aug 15, 2016
@mikaelarguedas
Copy link
Member

@Jmeyer1292 FYI: created another pull request with the changes discussed on Jun 22nd

@Jmeyer1292
Copy link
Contributor Author

@mikaelarguedas +1, thanks for carrying this forward.

@mikaelarguedas
Copy link
Member

closing this in favor or #55

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