add a wrapper around motion/builtin DoCommand utilities #4384
Conversation
viambot
added
the
safe to test
| @@ -512,3 +508,28 @@ func (ms *builtIn) execute(ctx context.Context, trajectory motionplan.Trajectory | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| // DoPlan is a helper function to wrap doPlan (a utility inside DoCommand) with types that are easier to work with. | |||
| func DoPlan(ctx context.Context, ms motion.Service, req motion.MoveReq) (motionplan.Trajectory, error) { | |||
There was a problem hiding this comment.
Not a fan of these. The DoCommand work we are doing should be explicitly temporary, I would prefer not to entrench the temporary fix by creating public methods that use them.
The lack of quality of life we experience from DoCommand should motivate us to get this feature into the real API. Building up a parallel infrastructure makes that harder.
There was a problem hiding this comment.
I don't think we should be concerned about breaking our package interface with our current userbase, so that argument isnt compelling to me. But also the upside of this work is pretty marginal so I'm fine not to merge it. @nfranczak was asking for this and has more opinions I think
There was a problem hiding this comment.
Would getting this feature into the real API mean creating a planning service and then a separate execution service? I am curious on what you think the real solution is explicitly. Would it be work defined by RSDK-8850?
I understand your preference for not creating public methods that use temporary solutions since that is tech debt + backwards incompatible. However, if the ultimate goal is to remove DoCommand, then haven't we already reached a backwards incompatible state?
I also think that by making our lives easier at the moment, we are not rushing ourselves to create the proper solution as it might take some time.
This PR builds on #4287 by adding wrapper functions to the builtin package to make using DoCommand more ergonomic.