RSDK-2526 unable to move multiaxis gantry #2168
Conversation
viambot
added
the
safe to test
This comment was marked as outdated.
This comment was marked as outdated.
viambot
added
safe to test
| jdx += len(subAxNum) | ||
|
|
||
| err = subAx.MoveToPosition(ctx, pos, worldState, extra) | ||
| if err != nil && !errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
@spieswl @nfranczak @raybjork if you'd like an update. Also, made CurrentInputs and GoToInputs fallthrough functions rather than copy-pastes of the logic to MoveToPosition and Position
There was a problem hiding this comment.
OK, that matches our patterns in arm so I like the consistency.
There was a problem hiding this comment.
This is actually opposite from what we have in arm, where GoToInputs is the function that the basic function that is called by MoveToPosition. Can you switch these to be consistent?
There was a problem hiding this comment.
Well, it's actually more consistent here, the API names are the same, but gantry.MoveToPosition is actually a functional analog to arm.MoveToJointPositions and base.MoveStraight/base.Spin. So this won't change.
There was a problem hiding this comment.
@raybjork pinging you for a response.
| g, err := newMultiAxis(ctx, deps, cfg, logger) | ||
| test.That(t, err, test.ShouldBeNil) | ||
|
|
||
| err = g.MoveToPosition(ctx, []float64{1, 2, 3, 4, 5, 6}, &referenceframe.WorldState{}, nil) |
There was a problem hiding this comment.
[minor] Add another test case for trying to call MoveToPosition with too few positions as well
| test.That(t, err, test.ShouldBeNil) | ||
| test.That(t, pos, test.ShouldResemble, []float64{6, 5, 9, 8, 7}) | ||
|
|
||
| err = g.MoveToPosition(ctx, []float64{1, 2, 3, 4, 5}, &referenceframe.WorldState{}, nil) |
There was a problem hiding this comment.
[major] add a test that MoveToPosition calls MoveToPosition on the subaxis gantries as well
There was a problem hiding this comment.
More clarification please.
There was a problem hiding this comment.
right now, it just checks that no error is returned, but a multi-axis gantry should make MoveToPosition calls to its sub-axis gantries with particular arguments based on the order of the gantries and the floats passed to the multi-axis' MoveToPosition call. The test should check that our implementation is making those calls with the correct arguments
There was a problem hiding this comment.
is what I added sufficient to what you asked for? I had it as a task on my pr description.
| err = g.MoveToPosition(ctx, []float64{1, 2, 3, 4, 5, 6}, &referenceframe.WorldState{}, nil) | ||
| test.That(t, err, test.ShouldNotBeNil) | ||
|
|
||
| pos, err := g.Position(ctx, nil) |
There was a problem hiding this comment.
[minor] it would be nice to use t.Run or have a short comment above each test case stating what is tested
| @@ -265,7 +265,7 @@ func (g *oneAxis) homeTwoLimSwitch(ctx context.Context) error { | |||
| g.positionLimits = []float64{positionA, positionB} | |||
|
|
|||
| // Go backwards so limit stops are not hit. | |||
| x := g.rotationalToLinear(0.8 * g.lengthMm) | |||
| x := g.linearToRotational(0.8 * g.lengthMm) | |||
There was a problem hiding this comment.
Oof, this looks like a frustrating bug to find! Nice work here.
| ) | ||
| } | ||
|
|
||
| jdx := 0 |
There was a problem hiding this comment.
Why did idx get renamed jdx? I'd prefer to go back to the old name, which is obviously short for "index." I don't know what jdx is short for.
There was a problem hiding this comment.
it's common in the non-software world to index using i, j, k, so i actually think in i-index, j-index, k-index.
| idx := 0 | ||
| if len(positions) != len(g.lengthsMm) { | ||
| return errors.Errorf( | ||
| "number of input positions %v does not match total gantry axes length %v", |
There was a problem hiding this comment.
Change "length" to "count." It's too easy to misread this and think that the position is supposed to be the farthest position out on the gantry (its length). (Other rephrasings are possible, too. Go with whatever you think makes it clearest that the problem is that the input has the wrong dimensionality.)
| for _, subAx := range g.subAxes { | ||
|
|
There was a problem hiding this comment.
I'd prefer never to start a block of code with a blank line. The change in indentation should be enough to indicate that something new is happening here.
| subAxNum, err := subAx.Lengths(ctx, extra) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = subAx.MoveToPosition(ctx, positions[idx:idx+len(subAxNum)-1], worldState, extra) | ||
| if err != nil { | ||
| pos := positions[jdx : jdx+len(subAxNum)] |
There was a problem hiding this comment.
👍 Very good catch with the off-by-one error here!
There was a problem hiding this comment.
Looks good! Only comment is this is opposite from how we have arms set up, so I would like to see that change.
Also (and maybe this isn't a part of this scope), I'm wondering if we should be calling motionplan from gantry.MoveToPosition the way we have it set up in arms. It won't matter 99% of the time because it will just reduce to the GoToInputs call, but there is a chance that something weird is set up with geometries that will warrant it having to plan around something in its internal robot geometries
| jdx += len(subAxNum) | ||
|
|
||
| err = subAx.MoveToPosition(ctx, pos, worldState, extra) | ||
| if err != nil && !errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
This is actually opposite from what we have in arm, where GoToInputs is the function that the basic function that is called by MoveToPosition. Can you switch these to be consistent?
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
|
Fixes an indexing error where an index was out of range in a multiaxis composed of only 3 axes with one length per axis.