Skip to content

Conversation

@worker-bob
Copy link
Contributor

Author - Your name

Description

What code was changed & why was it changed, any bugs it fixed

Checklist:

Mark with X's once you have tested

  • [ x] Follows the style guidelines of this project
  • [ x] Builds with no errors off robot
  • [x ] Main branch merged into this branch
  • [ x] Changes generate no new errors or warnings
  • Code deploys and runs as expected on robot
  • [x ] Commented code, especially in hard-to-understand areas

@worker-bob worker-bob requested review from Ammelll and rift10 November 1, 2025 19:25
Copy link
Contributor

@rift10 rift10 left a comment

Choose a reason for hiding this comment

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

i think a lot of these values you can move to the constants file, maybe make a separate section in each constants file for sim constants
also, did you simulate it? i got an error after simulating (ports issue)

Comment on lines -156 to -164
// Logger.processInputs("Swerve/Gyro", gyroInputs);
// Logger.recordOutput("Swerve/Pose", getPose());
// Logger.recordOutput("Swerve/Field Pose/Best Pose with offset", getBestCoralAutoAlign());
// Logger.recordOutput(
// "Swerve/Field Pose/Best Source", getBestSourceAutoAlign().getDegrees());
// Logger.recordOutput("Swerve/Field Pose/Robot Angle", getRotation().getDegrees());
// Logger.recordOutput(
// "Swerve/Auto Align/Offset Transform", getBestReefTagNoOffset().minus(getPose()));

Copy link
Contributor

Choose a reason for hiding this comment

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

add these back

public class GlobalConstants {

private static RobotType kRobotType = RobotType.BETA;
private static RobotType kRobotType = RobotType.SIMBOT;
Copy link
Contributor

Choose a reason for hiding this comment

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

change this back before merge


private ElevatorIO io;
private ElevatorIOInputsAutoLogged inputs = new ElevatorIOInputsAutoLogged();
public ElevatorIOInputsAutoLogged inputs = new ElevatorIOInputsAutoLogged();
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this public now?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would be better to make a getter

return;
}
runVolts(controller.calculate(eSim.getPositionMeters(), Units.feetToMeters(eleHeight)));
runVolts(controller.calculate(eSim.getPositionMeters(), Units.feetToMeters(setpointMeters)));
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this feet to meters if we're passing in meters


private IntakePivotIO io;
private IntakePivotIOInputsAutoLogged inputs = new IntakePivotIOInputsAutoLogged();
public IntakePivotIOInputsAutoLogged inputs = new IntakePivotIOInputsAutoLogged();
Copy link
Contributor

Choose a reason for hiding this comment

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

again why public

180.0 // initial angle placeholder
);
DCMotor.getKrakenX60Foc(2),
310 / 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number

Copy link
Contributor

Choose a reason for hiding this comment

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

also if this is gearing shouldn't it match the constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For all these values, i already had pid vaules tuned to them so i didn't want to retune the sim with something i already knew works

Copy link
Contributor

Choose a reason for hiding this comment

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

i still think you should add this to the switch case in the constants file... and do this for all the subsystem sim files

DCMotor.getKrakenX60Foc(2),
310 / 3,
SingleJointedArmSim.estimateMOI(0.3, 10),
.3,
Copy link
Contributor

Choose a reason for hiding this comment

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

magic number

Copy link
Contributor

Choose a reason for hiding this comment

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

still lwk a magic number


private WristIO io;
private WristIOInputsAutoLogged inputs = new WristIOInputsAutoLogged();
public WristIOInputsAutoLogged inputs = new WristIOInputsAutoLogged();
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Comment on lines 125 to 132
driver.povUp(); // ! Unbound
driver.povDown(); // ! Unbound
driver.povLeft(); // ! Unbound
driver.povRight(); // ! Unbound

Copy link
Contributor

Choose a reason for hiding this comment

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

put these back

}

public void initiateSimulatedSequence() {
((BeambreakIOSim) io).initiateSequence();
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting.... lol

Comment on lines +50 to +60
public void setIntake(boolean val) {
intakeTriggered = val;
}

public void setHandoff(boolean val) {
handoffTriggered = val;
}

public void setEnd(boolean val) {
endTriggered = val;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you dont wanna use @ setters?

180.0 // initial angle placeholder
);
DCMotor.getKrakenX60Foc(2),
310 / 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

i still think you should add this to the switch case in the constants file... and do this for all the subsystem sim files

DCMotor.getKrakenX60Foc(2),
310 / 3,
SingleJointedArmSim.estimateMOI(0.3, 10),
.3,
Copy link
Contributor

Choose a reason for hiding this comment

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

still lwk a magic number

Comment on lines 30 to 33
private static final LoggedTunableNumber l1 = new LoggedTunableNumber("Wrist/Gains/l1", 0);
private static final LoggedTunableNumber l2 = new LoggedTunableNumber("Wrist/Gains/l2", 3);
private static final LoggedTunableNumber l3 = new LoggedTunableNumber("Wrist/Gains/l3", 2);
private static final LoggedTunableNumber l4 = new LoggedTunableNumber("Wrist/Gains/l4", 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

while you're at it, there are still /gains/ in the path

Comment on lines +18 to +23
private static final double intakePivotOffsetX = Units.inchesToMeters(22.746);
private static final double intakePivotOffsetY = Units.inchesToMeters(9.577);

private static final double elevatorToGround = Units.inchesToMeters(5.36);
private static final double startingElevatorHeight = 0.9;
private static final double mechanismOffset = Units.inchesToMeters(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

add k's in front

Comment on lines 84 to 86
public void runPosition(double angle) {
io.runPosition(angle);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no

};

public static final double kBottomPosition =
public static final double kTolorance =
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi this is spelled wrong

Comment on lines 112 to 119
public void runPosition(ElevatorGoal goal) {
currentGoal = goal;
io.runPosition(currentGoal.getEleHeight().getAsDouble());
// io.runPosition(currentGoal.getEleHeight().getAsDouble());
}

public void runPosition(double meters) {
io.runPosition(meters);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no

return false;
}

private final SysIdRoutine sysIdRoutine = new SysIdRoutine(
Copy link
Contributor

Choose a reason for hiding this comment

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

why is all this stuff here btw, could it possibly be moved?
also it doesn't seem to be used??

rift10
rift10 previously approved these changes Nov 7, 2025
@rift10 rift10 merged commit 8979910 into main Nov 7, 2025
2 checks passed
@rift10 rift10 deleted the Sim branch November 7, 2025 03:00
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