RSDK-8840 - Add set speed and acceleration function to xArm doCommand #4382
Conversation
viambot
added
the
safe to test
components/arm/xarm/xarm.go
Outdated
| if val, ok := cmd["set"]; ok { | ||
| // The command expects a float64 slice of length 2, describing the desired speed and acceleration. | ||
| // We specify our values in degrees and here they are converted into radians. | ||
| interfaceList, err := utils.AssertType[[]interface{}](val) |
There was a problem hiding this comment.
Rather than forcing setting of speed and acceleration together as a list, let's add "set_speed" and "set_acceleration" as separate commands.
| if !ok { | ||
| return nil, errors.New("could not get float64 from interfaceList[1]") | ||
| } | ||
| x.speed = utils.DegToRad(speed) |
There was a problem hiding this comment.
Both of these need to be checked that they are greater than zero.
components/arm/xarm/xarm.go
Outdated
| @@ -255,5 +255,25 @@ func (x *xArm) DoCommand(ctx context.Context, cmd map[string]interface{}) (map[s | |||
| if _, ok := cmd["load"]; ok { | |||
| return x.getLoad(ctx) | |||
| } | |||
| if val, ok := cmd["set"]; ok { | |||
There was a problem hiding this comment.
When this is modularized we should add some good documentation to DoCommand describing to a user how it should be interacted with.
There was a problem hiding this comment.
My preference is that this simply goes away, and speed/acceleration are specified as part of a time-anchored trajectory.
There was a problem hiding this comment.
For the time being since we are just using this for a single demo I would prefer to keep this functionality in the DoCommand.
I could make a ticket to track this improvement though, i.e. removing set_speed & set_acceleration in favor of specifying them "as part of a time-anchored trajectory."
What do you think?
There was a problem hiding this comment.
This definitely should stay in a DoCommand for this PR. The discussed change above is in the far-off future.
There was a problem hiding this comment.
Sure. Its not a super actionable ticket until a bunch of other stuff happens first but I suppose no harm in making it. Keeping it in DoCommand is definitely the correct scope here
components/arm/xarm/xarm.go
Outdated
| } | ||
| speed, ok := interfaceList[0].(float64) | ||
| if !ok { | ||
| return nil, errors.New("could not get float64 from interfaceList[0]") |
There was a problem hiding this comment.
can use utils.AssertType here to cut down on the number of unique errors we have
…es are greater than zero
viambot
added
safe to test
components/arm/xarm/xarm.go
Outdated
| @@ -251,9 +251,33 @@ func (x *xArm) DoCommand(ctx context.Context, cmd map[string]interface{}) (map[s | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| return map[string]interface{}{}, nil | |||
There was a problem hiding this comment.
I've added this since we were returning an error on move_gripper when there was none in actuality.
There was a problem hiding this comment.
This should be solved differently. Instead, a flag should be set if at least one command is run. We do not want to return per-command, or else you cannot set speed and acceleration simultaneously.
There was a problem hiding this comment.
lmk what you think of the current solution.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
components/arm/xarm/xarm.go
Outdated
| @@ -242,6 +242,10 @@ func (x *xArm) DoCommand(ctx context.Context, cmd map[string]interface{}) (map[s | |||
| if err = x.setGripperMode(ctx, false); err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Can we wrap the above few lines in a command as well? Setting speed with a DoCommand should not fail if no gripper is attached.
viambot
added
safe to test
Add to the xArm's DoCommand.
This DoCommand is used within the wine pouring demo.
This allows us to set a higher acceleration and speed for when we want to de-pour.
A higher speed and acceleration prevents the wine from dribbling out of the bottle on the de-pour.
This is being added so that people other than me can run the demo here in nyc.