Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finalize unit testing (gear ratios and clamping) #141

Merged
merged 13 commits into from
Feb 10, 2024

Conversation

Oliver-Cushman
Copy link
Contributor

added clamping and gear ratios so that robot is safe!

@Oliver-Cushman
Copy link
Contributor Author

By the way I'm not sure if my usage of the constant ShooterConstants.PIVOT_GEAR_RATIO is preferable to using a conversion factor, however I could not get conversion factors to produce the desired effect as it seems they multiply both input and output.

Copy link
Member

@GalexY727 GalexY727 left a comment

Choose a reason for hiding this comment

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

This makes me wonder, should a Neo be forced to have a min and max output when constructing it (or as an optional method that can get called during config) that runs MathUtil.clamp() for all of the setter methods? That way we don't have to worry about it being or not being in every subsystem and can just make sure that Neo is handling all of the repeated yet required work.

return Commands.runOnce(() -> claw.setTargetVelocity(0));
return runOnce(() -> setTargetPercent(TrapConstants.CLAW_STOP_PERCENT));
}
public void setIntakingTimestamp() {
Copy link
Member

Choose a reason for hiding this comment

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

The name set******* makes me think that we are expecting a parameter, consider changing to update*******

@@ -100,7 +101,7 @@ public Command setRestAngleCommand() {
}

public double getAngle() {
return pivot.getPosition() * ShooterConstants.PIVOT_MAX_ANGLE_DEGREES;
return pivot.getPosition() * ShooterConstants.PIVOT_MAX_ANGLE_DEGREES / ShooterConstants.PIVOT_GEAR_RATIO;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed-- we should have this done inside of the Neo's positionConversionFactor instead.

Copy link
Member

Choose a reason for hiding this comment

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

the ratio is 1/14 btw if u care, I'll remember it if you don't wanna do that now

angle,
ShooterConstants.PIVOT_LOWER_LIMIT_DEGREES,
ShooterConstants.PIVOT_UPPER_LIMIT_DEGREES);
pivot.setTargetPosition(angle / ShooterConstants.PIVOT_MAX_ANGLE_DEGREES * ShooterConstants.PIVOT_GEAR_RATIO);
Copy link
Member

Choose a reason for hiding this comment

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

This gets taken care of by positionConversionFactor

public static final double SHOOTER_VELOCITY_CONVERSION_FACTOR = 1.0;
public static final double PIVOT_POSITION_CONVERSION_FACTOR = 1.0;

public static final double PIVOT_GEAR_RATIO = 100.8;
Copy link
Member

Choose a reason for hiding this comment

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

who is he!

Comment on lines 136 to 137
public static final double SHOOTER_RPM_LOWER_LIMIT = -4000;
public static final double SHOOTER_RPM_UPPER_LIMIT = 4000;
Copy link
Member

Choose a reason for hiding this comment

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

I think these can be set to +-NeoMotorConstants.UHHSOMETHING_MAX_FREE_SPEED

src/main/java/frc/robot/util/Constants.java Show resolved Hide resolved
Comment on lines 195 to 205
public static final double CLAW_OUTTAKE_PERCENT = -50;
public static final double CLAW_INTAKE_PERCENT = 50;
public static final double CLAW_STOP_PERCENT = 0;
public static final double TRAP_PLACE_POS = 0.48;
public static final double AMP_PLACE_POS = 0.48;

public static final double ELEVATOR_TOP_LIMIT = 0.48;
public static final double ELEVATOR_BOTTOM_LIMIT = 0;

public static final double CLAW_LOWER_PERCENT_LIMIT = 100;
public static final double CLAW_UPPER_PERCENT_LIMIT = -100;
Copy link
Member

Choose a reason for hiding this comment

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

target percents are values from [-1,1]

Comment on lines 437 to 442
public static final double INTAKE_PERCENT_UPPER_LIMIT = 100;
public static final double INTAKE_PERCENT_LOWER_LIMIT = -100;

public static final double INDEXER_PERCENT_UPPER_LIMIT = 100;
public static final double INDEXER_PERCENT_LOWER_LIMIT = -100;

Copy link
Member

Choose a reason for hiding this comment

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

target percents are values from [-1,1]
we can divide that out in Neo if you want, but I think it's fine to reflect that in constants

@PatribotsProgramming PatribotsProgramming linked an issue Feb 9, 2024 that may be closed by this pull request
5 tasks
@Oliver-Cushman
Copy link
Contributor Author

I found that setting the velocity conversion factor rather than the position conversion factor resolved the issues I was having with pivot. However, it does not seem that position conversion factor is working as intended as getPosition() uses velocity conversion factor. Changing this might require some refactoring of our subsystems so it might be wise to have another issue relating to tweaking the Neo class and our subsystems in regards to conversion factors.

@Jacob1010-h
Copy link
Member

if you want I could take this off of alex's hand so we could merge this as soon as possible

@Oliver-Cushman
Copy link
Contributor Author

if you want I could take this off of alex's hand so we could merge this as soon as possible

I’m pretty sure pivot is good now, I’ll test in lunch and then we should be good to merge

@Oliver-Cushman
Copy link
Contributor Author

if you want I could take this off of alex's hand so we could merge this as soon as possible

I’m pretty sure pivot is good now, I’ll test in lunch and then we should be good to merge

Works as intended in sim

@Oliver-Cushman Oliver-Cushman requested review from Jacob1010-h and GalexY727 and removed request for GalexY727 February 9, 2024 20:16
Copy link
Member

@Jacob1010-h Jacob1010-h left a comment

Choose a reason for hiding this comment

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

Everything in here looks very well coded ! I like it a lot. This should be good to merge. Make sure to test again once it's in sim-dev (just to make sure)

@@ -101,8 +101,9 @@ public static final class ShooterConstants {
public static final int RIGHT_SHOOTER_CAN_ID = 12;
public static final int SHOOTER_PIVOT_CAN_ID = 13;

public static final double SHOOTER_VELOCITY_CONVERSION_FACTOR = 1;
public static final double PIVOT_POSITION_CONVERSION_FACTOR = 1;
public static final double SHOOTER_VELOCITY_CONVERSION_FACTOR = 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

this is not used and is safe to remove if you want

public static final double PIVOT_POSITION_CONVERSION_FACTOR = 1;
public static final double SHOOTER_VELOCITY_CONVERSION_FACTOR = 1.0;
// degrees
public static final double PIVOT_POSITION_CONVERSION_FACTOR = 360;
Copy link
Member

Choose a reason for hiding this comment

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

Oops I see, this does look more correct, please ignore the comment about this being 1

@Jacob1010-h
Copy link
Member

ill wait for alex's review in case he sees something that I don't

@Jacob1010-h Jacob1010-h merged commit e900b6f into sim-dev Feb 10, 2024
@Jacob1010-h Jacob1010-h deleted the Finalize-unit-testing-(gear-ratios-and-clamping) branch February 10, 2024 04:01
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.

Component Unit Tests
3 participants