acmerobotics / road-runner-quickstart

FTC quickstart for https://github.com/acmerobotics/road-runner
BSD 3-Clause Clear License
193 stars 1.07k forks source link

RawEncoder applyDirection seems to overcorrect currentPosition #234

Closed kcjones closed 1 year ago

kcjones commented 1 year ago

ForwardPushTest is giving negative ticks for our left side motors (configured with SetDirection REVERSE). It appears that getCurrentPosition on our motors (GoBilda 5203s) already applies an adjustPosition method to correct for SetDirection. This appears to make the RawEncoder.applyDirection un-correct for the motor direction and generate negative ticks.

We attempted to correct for this by implementing our own version of DcMotorEx to correct for this additional adjustment. This works fine for the push tests but gives us odd behavior later in the tuning process. For example, ForwardRampLogger gives us an interesting V-Shaped graph (see attached).

Can you provide guidance on how best to move forward with tuning given this? What impact should we expect this to have as we move past the tuning process and begin coding for auton?

Screenshot 2023-09-17 at 10 19 54 AM
rbrott commented 1 year ago

ForwardPushTest is giving negative ticks for our left side motors (configured with SetDirection REVERSE). It appears that getCurrentPosition on our motors (GoBilda 5203s) already applies an adjustPosition method to correct for SetDirection. This appears to make the RawEncoder.applyDirection un-correct for the motor direction and generate negative ticks.

The direction of a RawEncoder is intended to be separate from the direction of the DcMotorEx it's constructed with. My thinking here is that it's strange in the decoupled motor/encoder case for reversing the motor to also reverse the encoder (though one can argue that independent directions are odd for the coupled case).

We attempted to correct for this by implementing our own version of DcMotorEx to correct for this additional adjustment.

It sounds like you probably just want to reverse the encoders on the left side in addition to the motors.

This works fine for the push tests but gives us odd behavior later in the tuning process. For example, ForwardRampLogger gives us an interesting V-Shaped graph (see attached).

(For those following along at home, this is indeed a forward ramp regression. I seem to have the titles switched in 0.1.3.)

Seems like some encoders are still improperly reversed. It's possible that was masked in the push test.

Can you provide guidance on how best to move forward with tuning given this? What impact should we expect this to have as we move past the tuning process and begin coding for auton?

I would

kcjones commented 1 year ago

I agree that reversing the encoder when reversing the motor direction is odd in many situations, yet DcMotorEx seems to do this anyway.

We have now upgraded to the newest build (com.acmerobotics.roadrunner:ftc:0.1.5). MecanumMotorDirectionDebugger works as expected (all wheels rolling forward) with the left motors set to REVERSE.

Despite this, the tuning opmodes continue to generate inverse tick counts on the left wheels. The new telemetry annotations in MecanumMotorDirectionDebugger are helpful in confirming negative tick counts when moving forward (thanks for adding that).

Since the stock DcMotorEx.getCurrentPosition automatically applies direction to encoder counts, won't RawEncoder.getPositionAndVelocity always reverse this direction and give us the incorrect values? I'm not sure how to separate these actions.

What mechanism should we use for reversing encoders to prevent this from happening (if not using a custom DcMotorEx)?

Also, can you clarify tuning instructions in the quickstart for mecanum using drive encoders? In the docs ForwardRampLogger is listed as being for dead wheels only, yet the AngularRampLogger instructions indicate we should use the Ramp Regression chart from the ForwardRampLogger step to identify kS & kV. In our case is it correct to proceed with the ForwardRampLogger step to get kS, kV before AngularRampLogger?

amjadj commented 1 year ago

One suggestion is to connect the par0 and par1 encoders to the right side motors (which are not REVERSED) and the perp to the left one. That way both your encoders should give you positive values on forward movement.

Also, dont reverse the par0 / par1 encoders. we got this pattern when we had par0 and par1 on left and right motor encoders with right motor being reversed, and also par 1 reversed.

On Mon, Sep 18, 2023 at 10:17 AM kcjones @.***> wrote:

I agree that reversing the encoder when reversing the motor direction is odd in many situations, yet DcMotorEx seems to do this anyway.

We have now upgraded to the newest build (com.acmerobotics.roadrunner:ftc:0.1.5). MecanumMotorDirectionDebugger works as expected (all wheels rolling forward) with the left motors set to REVERSE.

Despite this, the tuning opmodes continue to generate inverse tick counts on the left wheels.

Since the stock DcMotorEx.getCurrentPosition automatically applies direction to encoder counts, won't RawEncoder.getPositionAndVelocity always reverse this direction and give us the incorrect values? I'm not sure how to separate these actions.

What mechanism should we for reversing encoders to prevent this from happening (if not using a custom DcMotorEx)?

Also, can you clarify tuning instructions in the quickstart for mecanum using drive encoders? In the docs ForwardRampLogger is listed as being for dead wheels only, yet the AngularRampLogger instructions indicate we should use the Ramp Regression chart from the ForwardRampLogger step to identify kS & kV. In our case is it correct to proceed with the ForwardRampLogger step to get kS, kV before AngularRampLogger?

— Reply to this email directly, view it on GitHub https://github.com/acmerobotics/road-runner-quickstart/issues/234#issuecomment-1723685196, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKGCJFINR2VIL6WN2GTGKYDX3BQY3ANCNFSM6AAAAAA43U7B5Y . You are receiving this because you are subscribed to this thread.Message ID: @.***>

kcjones commented 1 year ago

One suggestion is to connect the par0 and par1 encoders to the right side motors (which are not REVERSED) and the perp to the left one. That way both your encoders should give you positive values on forward movement. Also, dont reverse the par0 / par1 encoders. we got this pattern when we had par0 and par1 on left and right motor encoders with right motor being reversed, and also par 1 reversed.

Unfortunately we don't have dead wheels yet so are relying on motor encoders. We will keep this in mind though when we get our odom modules delivered early Oct.

rbrott commented 1 year ago

Since the stock DcMotorEx.getCurrentPosition automatically applies direction to encoder counts, won't RawEncoder.getPositionAndVelocity always reverse this direction and give us the incorrect values? I'm not sure how to separate these actions.

Maybe it will help to give an interpretation of the code.

    private fun applyDirection(x: Int): Int {
        var x = x
        if (m.direction == DcMotorSimple.Direction.REVERSE) {
            x = -x
        }

        if (direction == DcMotorSimple.Direction.REVERSE) {
            x = -x
        }

        return x
    }

If m.direction matches direction, the value will be spit out unchanged (both forward will never negate; both reverse will negate twice). And then if the directions don't match the direction will be negated.

This makes the encoder direction independent of the motor direction. If you reverse the motor, the values from getCurrentPosition() / getVelocity() will be negated, and an additional negation will be applied by applyDirection() to counteract it and restore the original value.

What mechanism should we use for reversing encoders to prevent this from happening (if not using a custom DcMotorEx)?

Even if the above explanation doesn't make sense, you can always flip the encoder direction without affecting the motor direction, and the sign should change (using the direction debugger to check as always).

Also, can you clarify tuning instructions in the quickstart for mecanum using drive encoders? In the docs ForwardRampLogger is listed as being for dead wheels only, yet the AngularRampLogger instructions indicate we should use the Ramp Regression chart from the ForwardRampLogger step to identify kS & kV. In our case is it correct to proceed with the ForwardRampLogger step to get kS, kV before AngularRampLogger?

Sounds like you're confused by the sentence

Use the instructions from the ForwardRampLogger analysis to get kS, kV from the the “Ramp Regression” plot.

I meant to convey that you should use the same instructions / technique from the ForwardRampLogger (mostly following the regression tutorial), but you don't need to actually collect that data or do anything on that page.

EddieDL commented 1 year ago

I was a bit confused by this for a second as well while we were upgrading. After investigating the code that @rbrott calls out. We just called 'per.setDirection(REVERSED)'. We are using dead wheels but they are connected to the ports of our drive motors. So we would have had the same issues. This works perfectly.

In your case you would just call leftFront.setDirection(REVERSED) or whatever drive encoders need to be reversed in the constructor of the DriveLocalizer. No need for extra wrappers or anything.

kcjones commented 1 year ago

Maybe it will help to give an interpretation of the code.


This makes the encoder direction independent of the motor direction. If you reverse the motor, the values from `getCurrentPosition()` / `getVelocity()` will be negated, **and** an additional negation will be applied by `applyDirection()` to counteract it and restore the original value.

This seems so obvious now. I'm not sure how we missed it initially. Thank you for clarifying. Tick counts are now working as expected.

Sounds like you're confused by the sentence

Use the instructions from the ForwardRampLogger analysis to get kS, kV from the the “Ramp Regression” plot.

I meant to convey that you should use the same instructions / technique from the ForwardRampLogger (mostly following the regression tutorial), but you don't need to actually collect that data or do anything on that page.

It appears to me that the RR Drive Encoder Angular Ramp Regression page should be loading two graphs. rampChart and trackWidthChart? I only see trackWidthChart in my browser. I think this is the source of my confusion. Without rampChart, I couldn't extract kV, kS and trackWidth and the instructions seemed unclear.

There is a javascript error on the page when loading the rampChart that appears to point to this error:

async function newLinearRegressionChart(container: HTMLElement, xs: number[], ys: number[], options: RegressionOptions, onChange?: (m: number, b: number) => void): Promise<(xs: number[], ys: number[]) => void> {  
  if (xs.length !== ys.length) {  
    throw new Error(`${xs.length} !== ${ys.length}`); 
  }  

Here is my AngularRampLogger data: https://gist.github.com/kcjones/b0abb4de8490ae5cb42b16f8bb890427

Screenshot 2023-09-19 at 8 57 58 AM
rbrott commented 1 year ago

Yes, there should be two plots. Both the missing plot and the missing line-of-best-fit should be fixed in 0.1.6.