search

osrf / capabilities

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

Return an error if start_capability requested for a capability that is already running #79

Closed jonbinney closed 10 years ago

jonbinney commented 10 years ago

Addresses https://github.com/osrf/capabilities/issues/78

This is a first pass; I'm open to suggestions on how to do this better. I wasn't sure how detailed of information to return to the callee; I settled on telling them whether the capability they requested is "RUNNING", "STARTING", or "STOPPING". Giving more detailed info (like "stopping" vs. "terminated") seemed like it exposed a bit too much internal state. Giving less detailed info (only that the call failed, for instance) seemed like not quite enough for many use cases.

After these changes, one of the launch_manager tests hangs - I'm having a hard time debugging it. I didn't change anything in the launch manager code, so I'm not sure what is going on.

wjwwood commented 10 years ago

Sorry Jon, I've not had time to look at this yet. I'll try to spend some time on it today, I first need to fix the failing unit tests which existed before your pull request.

wjwwood commented 10 years ago

Can you rebase now that I merged #77? Thanks!

jonbinney commented 10 years ago

Rebased. Passes tests on my desktop.

wjwwood commented 10 years ago

Passes on Travis too, I'll start the logical review now.

wjwwood commented 10 years ago

The coverage percentage will go down, but these will be especially difficult to be coax out in a coverage test. I think this will be fine.

The other thing to consider is deployment. This not only adds constants to the message file which will change the MD5sum of the message, but we also change the return type, which might be problematic for people consuming this in C++. With that being said I think the impact will be minimal since I imagine that most people are not using this currently.

@bit-pirate do you have any feedback on releasing this into indigo/hydro/groovy?

jonbinney commented 10 years ago

If there is existing C++ code that relies on this service call returning a simple bool, I could modify my changes to keep the "success" bool, and just add an additional integer flag that has an error code if that bool is false. Then any existing code should continue to work (after a recompile, though)