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

Issue 19 one bond per user #48

Merged
merged 10 commits into from
Mar 14, 2014
Merged

Issue 19 one bond per user #48

merged 10 commits into from
Mar 14, 2014

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Mar 11, 2014

This is an alternative implementation to issue #19.

It uses one bond per client instead of one bond per capability "use".

@bit-pirate for review

@wjwwood
Copy link
Member Author

wjwwood commented Mar 13, 2014

@bit-pirate bump on the review

I think this pull request is superior to #19.

The main difference is that #19 establishes one bond per call to /use_capability, whereas this pr establishes one bond per client to the capability server.

#19 work flow:

  • For each capability:
    • Call ~use_capability
    • Establish bond
    • Later call ~free_capability or break bond

This pull requests work flow:

  • Call ~establish_bond
  • Establish the bond
  • For each capability:
    • Call ~use_capability
    • Later call ~free_capability
  • Break bond to free any remaining uses which were not expilicitly free'ed

The entire work flow is hidden by the client API, for both pull requests, this is how the client would use the capabilities.client Python API:

from capabilities.client import Client
client = Client()
# Use the line below if the capability_server has a different name
# client = Client(capability_server_node_name='/capability_server_node_name')
client.wait_for_services()  # Optional
client.use_capability('foo_pkg/Foo')
client.use_capability('foo_pkg/Foo')
client.use_capability('bar_pkg/Bar')
client.free_capability('foo_pkg/Foo')
client.shutdown()


def shutdown(self):
"""Cleanly frees any used capabilities."""
self._bond.break_bond()
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a check, if self._bond exists.

"""
# If no bond has been established, establish one first
if self._bond is None:
if self.establish_bond(timeout) is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

timeout needs to be a rospy.Duration

@bit-pirate
Copy link
Contributor

PR #50 fixes the two issues mentioned above. Apart from that this PR looks really nice. I tested freeing/using capabilities and corner cases, such as killing the app manager after an app depending on caps has been started, and it works well (caps get freed correctly)! 👍

client: adds check for existing bond and fixes timout type
wjwwood added a commit that referenced this pull request Mar 14, 2014
@wjwwood wjwwood merged commit 25f1fbe into master Mar 14, 2014
@wjwwood wjwwood deleted the issue_19_one_bond_per_user branch March 14, 2014 21:10
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.

2 participants