Return an error if start_capability requested for a capability that is already running #79
Conversation
|
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. |
|
Can you rebase now that I merged #77? Thanks! |
The start_capability service returns an error code if it cannot start a capability because it is already running.
jonbinney
force-pushed
the
start-capability-response
branch
from
1911fdc to
5a24d95
Compare
|
Rebased. Passes tests on my desktop. |
|
Passes on Travis too, I'll start the logical review now. |
|
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? |
Return an error if start_capability requested for a capability that is already running
|
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) |
Addresses #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.