osrf / capabilities

Implements the concept of capabilities as part of the robots-in-concert system.
Other
8 stars 26 forks source link

add timeout to CapabilitiesClient #46

Closed bit-pirate closed 10 years ago

bit-pirate commented 10 years ago

Creating a client should return false or raise an exception, when the capability server is not available.

I suggest to implement here (https://github.com/osrf/capabilities/blob/issue_19/src/capabilities/client.py#L57) a timeout handling similar to the implementation in the capabilities.service_discovery (http://docs.ros.org/hydro/api/capabilities/html/_modules/capabilities/service_discovery.html#spec_index_from_service).

wjwwood commented 10 years ago

Both seem to take timeout's, do you mean that it should behave differently? I don't exactly see what you mean here, can you give me some more details?

bit-pirate commented 10 years ago

I basically like to check for an existing capability server when initialising the CapabilitiesClient class, i.e. checks for the existence of the use_capability and free_capability services. In this way I know from the start, if I can use capabilities or not.

Adding

self.__wait_for_service(self.__use_capability, timeout)
self.__wait_for_service(self.__free_capability, timeout)

in __init__ would probably suffice.

Hope this makes it clearer.

wjwwood commented 10 years ago

I would prefer to add a new function to the client like wait_for_services(timeout), that way the user can choose to only __init__ and not block on services being available, but then you can call wait_for_services right after construction if you want to ensure the services are ready before later calling use_capability or free_capability.

Does that seem reasonable?

bit-pirate commented 10 years ago

Does that seem reasonable?

That would be fine.

However, I wonder what use an instance of the CapabilityClient class would have, if it is not guaranteed that the capability server is available?

wjwwood commented 10 years ago

It's just about being able to create an instance and not have it block on the other node's availability.

bit-pirate commented 10 years ago

Roger that.

wjwwood commented 10 years ago

Btw, this has been added to both #19 and #48.

bit-pirate commented 10 years ago

Successfully tested with #48. We can close this issues, once that PR is merged.