RSDK-4120 add MoveThroughJointPositions to Arm interface #4508
Conversation
viambot
added
the
safe to test
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
viambot
added
safe to test
viambot
added
safe to test
Motion should call GoToInputs, because we move things that are not arms, e.g. gantries. This PR should update the implementation of GoToInputs on arms so that that calls MoveThroughJointPositions. |
Availability
Quality
Performance
The above data was generated by running scenes defined in the
|
viambot
added
safe to test
viambot
added
safe to test
viambot
removed
the
safe to test
viambot
added
the
safe to test
|
https://github.com/viamrobotics/rdk/blob/main/components/arm/client.go#L148 |
viambot
added
safe to test
Yes, thank you for catching this. Just updated. |
There was a problem hiding this comment.
LGTM, I am very excited about this being merged!
I've left a comment about where the MoveOptions struct should live, the names we give to the field of that particular struct. It's your final call to decide these things.
I've also left a comment asking for clarification to understand why we convert from rad to deg and then back from deg to rad when doing fromProto and then toProto.
🚀
| ) | ||
|
|
||
| // MoveOptions define parameters to be obeyed during arm movement. | ||
| type MoveOptions struct { |
There was a problem hiding this comment.
I think it might make more sense for this struct to live in arm.go? What are your thoughts?
Having it live in pb_helpers.go kind of obscures its presence/makes it harder to find
There was a problem hiding this comment.
I think its pretty easy to find and the struct is simple enough that it kind of feels like a code artifact that is a helper. Going to leave this as is for now and mentally flag this as something to change down the line as we change arms more
components/arm/pb_helpers.go
Outdated
| ) | ||
|
|
||
| // MoveOptions define parameters to be obeyed during arm movement. | ||
| type MoveOptions struct { | ||
| MaxVel, MaxAcc float64 |
There was a problem hiding this comment.
do you think it makes sense to add units here?, e.g. MaxVelRads, MaxAccRads?
There was a problem hiding this comment.
Sorry I should've thought of this when I left my original comment, thoughts on the units actually being in degrees since that are easier to mentally reason about? We could then handle the conversion in the backend.
If not, that's also ok :)
There was a problem hiding this comment.
No the units should be in radians because this is the struct we will be operating on. The protobuf struct is in degrees
components/arm/pb_helpers.go
Outdated
| ) | ||
|
|
||
| // MoveOptions define parameters to be obeyed during arm movement. | ||
| type MoveOptions struct { | ||
| MaxVel, MaxAcc float64 |
components/arm/pb_helpers.go
Outdated
| acc = *protobuf.MaxAccDegsPerSec2 | ||
| } | ||
| return &MoveOptions{ | ||
| MaxVel: utils.DegToRad(vel), |
There was a problem hiding this comment.
If not set, these here default to 0.
I could see a naive module implementer making <= 0 an invalid value, rather than a "use the default" value.
A comment on the struct should specify expected behavior for not-positive values.
There was a problem hiding this comment.
Yeah my intention was to have these default to zero if unset and handle default values at the driver level (per Miko's suggestion earlier in this review) I think thats a better way to handle it than trying to come up with some default at the package level.
For example if I am an arm driver I see an incoming options struct with zero values, I know then that they were unspecified since zero would result in no motion. Or I could make the bizarre implementation choice of allowing a user to specify a zero speed. But either way it is up to the module author here, and this code is just a passthrough. That track with your expecations?
viambot
added
safe to test
viambot
added
safe to test
The actual work involved here is pretty minor I think. The real work will come in properly implementing the arm modules we are supporting. Opening this up as a draft PR for now to get some initial feedback, while I write tests for the code. Open questions: