Skip to content

fix(gearing): Intake/Outake now takes in rpm in desired output, then uses gearing to get desired motor output#75

Open
RemiIqbal wants to merge 1 commit intoRoboLancers:mainfrom
RemiIqbal:dev-gearing-fix
Open

fix(gearing): Intake/Outake now takes in rpm in desired output, then uses gearing to get desired motor output#75
RemiIqbal wants to merge 1 commit intoRoboLancers:mainfrom
RemiIqbal:dev-gearing-fix

Conversation

@RemiIqbal
Copy link
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [X ] My code follows the style guidelines of this project
  • [X ] Someone have performed a self-review of my code
  • [X ] I have commented my code, particularly in hard-to-understand areas
  • [X ] My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • [X ] New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@BananasAmIRite BananasAmIRite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@@ -56,8 +56,10 @@ public void periodic() {
// Will only run once; For a continuous method, see runAtVelocity(Supplier<AngularVelocity>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment to say angular velocity of what (motor shaft vs output shaft)

also add what type of velocity inputs.velocity represents

// Physical constants
public static final double kMomentOfInertia = 0.01;
public static final double kGearing = 1;
public static final double kGearing = 37 / (3 * 38);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write this more verbosely (eg. in each stage of the gearing)
Like: (1 / 3) * (37 / 38)

public static final AngularVelocity kCoralOuttakeRPM = RPM.of(-1500);
public static final AngularVelocity kCoralStallRPM = RPM.of(500);
public static final AngularVelocity kAlgaeKnockRPM = RPM.of(-3000);
public static final AngularVelocity kCoralIntakeRPM = RPM.of(9243.243);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't look correct. The original RPM is the motor RPM, so this should be the output shaft RPM, which should be lower in magnitude, not higher.

You may want to add dedicated methods to convert between motor and output shaft RPMs to avoid confusion.

endEffectorController.calculate(inputs.velocity.in(RPM), velocity.in(RPM))
+ feedforward.calculate(velocity.in(RPM));
endEffectorController.calculate(
inputs.velocity.times(CoralEndEffectorConstants.kGearing).in(RPM),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say inputs.velocity should be the motor RPM itself, so you don't need to multiply that part. You''d still have to multiply the inputted velocity to convert it from output RPM to motor RPM.

Also, in the IO implementation of this, make sure to put the conversion factors as 1 instead of our gearing. Otherwise, your inputs.velocity would be in output shaft RPM and PID values would change drastically if you decided to change the gearing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants