refactor of docking server

[mikeferguson]

This is still a WIP - opening for visibility

[mikeferguson]

This is ready to review - addresses everything that is crossed out in the design doc

I'll do another pass of documentation (especially around coordinate frame assumptions) once we switch to using the upstream controller.

[mikeferguson]

I walked through the whole docking loop - step by step (I've got some chicken scratch we can probably turn into a state diagram later):

• every recoverable error is now thrown
• revamped where we check num_retries so that we can rethrow on the n+1
• fully consistent handling of the result message - it's now properly filled in for success/failure and num_retries
• updated the test to also run through the preemption so I could look at the printed log warnings - they are correct

[mikeferguson]

@SteveMacenski - I'm going to merge this so I can start working on top of graceful controller - if you have any comments after the latest revamp - just leave them here and I'll get them addressed in another PR/commit

[SteveMacenski]

@mikeferguson

• L402, throw the exception ex
• approachDock & resetApproach, I noticed that loop doesn't have a max timeout condition. Those could both use the same param + throw FailedToControl type
• L280 throw the exception e
• L584-L590 I think should be before L578-L581 so that cancel/preempt can be done if disable charging hasn't yet returned true. Our uses have that instantaneous, but it could be otherwise Added to graceful refactor
• L604 TODO seems important and I agree that should throw something. Probably just UNKNOWN failure?
• undockRobot's break for cancel/preempt calls a terminate_current not terminate_all Added to graceful refactor
• Should L274, L288, L324 caching happen if we fail to dock? I would think not since we didn't successfully dock

[mikeferguson]

On the first point (L402) - it's my understanding that plain "throw" is actually preferred since it won't recreate the exception: https://stackoverflow.com/questions/2360597/c-exceptions-questions-on-rethrow-of-original-exception - but I'll admit, I don't use exceptions all that much, and so I'm not 100% sure

[SteveMacenski]

Oh, if that works, that works. I didn't know that throw in that context would implicitly throw the exception in the scope. I'd just test to verify that you get the right contextual errors and that the error_codes are populated (if it works for one, it'll work for all). I didn't expect C++ to be that non-explicit about it. I learned something new today

[mikeferguson]

re: stashDockData - it probably needs a rename - it also handles cleaning up that raw pointer that gets created when not using the dock database

[SteveMacenski]

If you added a success field, you could use that whether or not to store it or just clean it