merge main from practise field to working. #28

2025-10-15T23:47:02Z

Saturncorgi commented

2025-10-15 23:47:02 +00:00

(Migrated from github.com)

No description provided.

johnluetke (Migrated from github.com) reviewed

2025-10-15 23:56:13 +00:00

MobileOak (Migrated from github.com) reviewed

2025-10-15 23:59:13 +00:00

src/main/java/org/ironriders/core/RobotCommands.java

					
				@ -197,1 +186,3 @@

				   * 

				   *

				   * <p>

				   * Does this by setting the {@link

Either a comment or a better variable name would be helpful here. I know that L4 is the elevator position / coral position, but the usage of this variable is confusing. Why is it set to false in several places in the code, but then when trying to set L4 the first time does it set it to true here?

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:01:27 +00:00

src/main/java/org/ironriders/core/RobotCommands.java

					
				@ -197,1 +186,3 @@

				   * 

				   *

				   * <p>

				   * Does this by setting the {@link

It's keeping track of if it has already moved the coral piece up I think. I agree it's not super helpful

Amber-leaf commented

2025-10-16 00:02:37 +00:00

(Migrated from github.com)

yea i would add a comment here but dont have a functioning laptop rn. It is basically so that canceling going to L4 will not boost the coral in the intake twice, where boosting it is moving it up slightly

-------- Original Message --------
On Wednesday, 10/15/25 at 16:59 Carl Anderson @.***> wrote:

@MobileOak commented on this pull request.

In src/main/java/org/ironriders/core/RobotCommands.java:
     intakeCommands.set(IntakeState.STOP).unless(() -> intakeCommands.getIntake().beamBreakTriggered()));
}

public Command elevatorWristSet(ElevatorWristState state) {
switch (state) {
case L4:
   return Commands.sequence(elevatorWristCommands.setElevatorWrist(state),
   if (hasL4Boost) {
     return elevatorWristCommands.setElevatorWrist(state);
   }
   hasL4Boost = true;
Either a comment or a better variable name would be helpful here. I know that L4 is the elevator position / coral position, but the usage of this variable is confusing. Why is it set to false in several places in the code, but then when trying to set L4 the first time does it set it to true here?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: @.***>

yea i would add a comment here but dont have a functioning laptop rn. It is basically so that canceling going to L4 will not boost the coral in the intake twice, where boosting it is moving it up slightly -------- Original Message -------- On Wednesday, 10/15/25 at 16:59 Carl Anderson ***@***.***> wrote: > @MobileOak commented on this pull request. > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/core/RobotCommands.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434237482): > >> intakeCommands.set(IntakeState.STOP).unless(() -> intakeCommands.getIntake().beamBreakTriggered())); > } > > public Command elevatorWristSet(ElevatorWristState state) { > switch (state) { > case L4: > - return Commands.sequence(elevatorWristCommands.setElevatorWrist(state), > + if (hasL4Boost) { > + return elevatorWristCommands.setElevatorWrist(state); > + } > + hasL4Boost = true; > > Either a comment or a better variable name would be helpful here. I know that L4 is the elevator position / coral position, but the usage of this variable is confusing. Why is it set to false in several places in the code, but then when trying to set L4 the first time does it set it to true here? > > — > Reply to this email directly, [view it on GitHub](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#pullrequestreview-3342676506), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AZ2JPAPQAOQ7OOA2VMUPP4D3X3NWRAVCNFSM6AAAAACJJ6EEM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBSGY3TMNJQGY). > You are receiving this because your review was requested.Message ID: ***@***.***>

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:02:55 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

All the extra indenting here makes it even more difficult to read. I'm generally in favor of 4 space indents, this went from 6 spaces to 8 spaces.

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:03:33 +00:00

src/main/java/org/ironriders/core/RobotCommands.java

					
				@ -197,1 +186,3 @@

				   * 

				   *

				   * <p>

				   * Does this by setting the {@link

Fixed in #27

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:03:42 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

Does the comment above even make sense now that there's an enum instead?

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:03:55 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

This was a mistake any should be fixed in #27

Amber-leaf commented

2025-10-16 00:04:39 +00:00

(Migrated from github.com)

yea i hit format at the practice field and it did that

-------- Original Message --------
On Wednesday, 10/15/25 at 17:03 Carl Anderson @.***> wrote:

@MobileOak commented on this pull request.

In src/main/java/org/ironriders/core/RobotContainer.java:

           primaryController.rightTrigger(triggerThreshold)

                   .onTrue(robotCommands.intake())

                   .onFalse(robotCommands.stopIntake());

           primaryController.leftTrigger(triggerThreshold)

                   .onTrue(intakeCommands.set(IntakeState.SCORE))

                   .onFalse(intakeCommands.set(IntakeState.STOP));

           primaryController.leftBumper()

                   .onTrue(driveCommands.setDriveTrainSpeed(0.5))

                   .onFalse(driveCommands.setDriveTrainSpeed(1));

           primaryController.rightBumper()

                   .onTrue(driveCommands.setDriveTrainSpeed(1.5))

                   .onFalse(driveCommands.setDriveTrainSpeed(1));

```
   private void configureBindings() {
```

           DriverStation.silenceJoystickConnectionWarning(true);

All the extra indenting here makes it even more difficult to read. I'm generally in favor of 4 space indents, this went from 6 spaces to 8 spaces.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: @.***>

yea i hit format at the practice field and it did that -------- Original Message -------- On Wednesday, 10/15/25 at 17:03 Carl Anderson ***@***.***> wrote: > @MobileOak commented on this pull request. > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/core/RobotContainer.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434241256): > >> - primaryController.rightTrigger(triggerThreshold) > - .onTrue(robotCommands.intake()) > - .onFalse(robotCommands.stopIntake()); > - > - primaryController.leftTrigger(triggerThreshold) > - .onTrue(intakeCommands.set(IntakeState.SCORE)) > - .onFalse(intakeCommands.set(IntakeState.STOP)); > - > - primaryController.leftBumper() > - .onTrue(driveCommands.setDriveTrainSpeed(0.5)) > - .onFalse(driveCommands.setDriveTrainSpeed(1)); > - primaryController.rightBumper() > - .onTrue(driveCommands.setDriveTrainSpeed(1.5)) > - .onFalse(driveCommands.setDriveTrainSpeed(1)); > + private void configureBindings() { > + DriverStation.silenceJoystickConnectionWarning(true); > > All the extra indenting here makes it even more difficult to read. I'm generally in favor of 4 space indents, this went from 6 spaces to 8 spaces. > > — > Reply to this email directly, [view it on GitHub](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#pullrequestreview-3342680812), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AZ2JPAL5K2KNKOJJYMO6YQ33X3OELAVCNFSM6AAAAACJJ6EEM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBSGY4DAOBRGI). > You are receiving this because your review was requested.Message ID: ***@***.***>

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:06:19 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

fixed in #27

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:06:28 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

Please just put a newline after { and align all these comments to the same amount of indent.

Please just put a newline after `{` and align all these comments to the same amount of indent.

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:07:18 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

I think all the formatting weirdness should be fixed in #27

I think all the formatting weirdness _should_ be fixed in #27

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:07:55 +00:00

src/main/java/org/ironriders/core/RobotContainer.java

					
				@ -58,3 +51,2 @@

				    public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

				    public final TargetingCommands targetingCommands = targetingSubsystem.getCommands();

				  public final TargetingSubsystem targetingSubsystem = new TargetingSubsystem();

It probably added some of it's own, but that whole branch is for formatting fixes

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:08:01 +00:00

src/main/java/org/ironriders/drive/DriveConstants.java

					
				@ -34,15 +32,17 @@ public class DriveConstants {

				  public static final double ROTATION_CONTROL_EXPONENT = 3.0;

				  public static final double ROTATION_CONTROL_DEADBAND = 0.8;

Can probably remove the TODO here yeah?

Can probably remove the `TODO` here yeah?

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:10:03 +00:00

src/main/java/org/ironriders/drive/DriveConstants.java

					
				@ -34,15 +32,17 @@ public class DriveConstants {

				  public static final double ROTATION_CONTROL_EXPONENT = 3.0;

				  public static final double ROTATION_CONTROL_DEADBAND = 0.8;

Fixed but not yet commited

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:12:52 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

					
				@ -104,4 +109,4 @@

				   * @param rotation      Desired rotation in radians per second.

				   * @param fieldRelative If not field relative, the robot will move relative to

				   *                      its own rotation.

				   */

int invertMultiplier = invertStatus? -1 : 1;
swerveDrive.drive(translation.times(invertMultiplier), rotation * invertMultiplier, fieldRelative, false);

Even better is if you assign the false to something that lets me know what it represents.

boolean initiateRobotSelfDestruct = false;
swerveDrive.drive(translation.times(invertMultiplier), rotation * invertMultiplier, fieldRelative, initiateRobotSelfDestruct);

``` int invertMultiplier = invertStatus? -1 : 1; swerveDrive.drive(translation.times(invertMultiplier), rotation * invertMultiplier, fieldRelative, false); ``` Even better is if you assign the false to something that lets me know what it represents. ``` boolean initiateRobotSelfDestruct = false; swerveDrive.drive(translation.times(invertMultiplier), rotation * invertMultiplier, fieldRelative, initiateRobotSelfDestruct); ```

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:14:02 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

					
				@ -104,4 +109,4 @@

				   * @param rotation      Desired rotation in radians per second.

				   * @param fieldRelative If not field relative, the robot will move relative to

				   *                      its own rotation.

				   */

Even better is if you add some newlines:

boolean initiateRobotSelfDestruct = false;
swerveDrive.drive(
     translation.times(invertMultiplier),
     rotation * invertMultiplier,
     fieldRelative,
     initiateRobotSelfDestruct
);

Even better is if you add some newlines: ``` boolean initiateRobotSelfDestruct = false; swerveDrive.drive( translation.times(invertMultiplier), rotation * invertMultiplier, fieldRelative, initiateRobotSelfDestruct ); ```

Saturncorgi (Migrated from github.com) reviewed

2025-10-16 00:19:24 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

					
				@ -104,4 +109,4 @@

				   * @param rotation      Desired rotation in radians per second.

				   * @param fieldRelative If not field relative, the robot will move relative to

				   *                      its own rotation.

				   */

Fixed in #27, to be clear I'm fixing these as you go, you're not wasting your time

Saturncorgi commented

2025-10-16 00:45:25 +00:00

(Migrated from github.com)

Please do not merge until I say otherwise. I'm on my phone so I can't actually block it

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:53:04 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

Magic number 9

Magic number `9`

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:54:22 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

I just noticed this - you should use it in the method above.

MobileOak (Migrated from github.com) reviewed

2025-10-16 00:57:37 +00:00

MobileOak (Migrated from github.com) left a comment

No description provided.

Approved assuming the remaining comments are addressed.

ryanelbert5125gmail (Migrated from github.com) reviewed

2025-10-16 05:29:41 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

Yeah what Carl said. Just replace this with a constant and put it in drive constants

ryanelbert5125gmail (Migrated from github.com) reviewed

2025-10-16 06:06:49 +00:00

ryanelbert5125gmail (Migrated from github.com) left a comment

I won't approve this until the rotation inversion is fixed, but other than that it should be good to merge. The rest of my comments aside from that one were mainly nit picks and it should be fine to merge without addressing them.

src/main/java/org/ironriders/core/RobotCommands.java

Just a nit pick, but this probably should be a constant so that if this code reused the magic number 9 doesn't cause issues if their gyro isn't on CAN ID 9

src/main/java/org/ironriders/drive/DriveConstants.java

					
				@ -34,15 +32,17 @@ public class DriveConstants {

				  public static final double ROTATION_CONTROL_EXPONENT = 3.0;

				  public static final double ROTATION_CONTROL_DEADBAND = 0.8;

Just wanted to check that the SWERVE_DRIVE_MAX_SPEED = 6 and SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5 are values based on the design and limitations of the robot not random numbers based off the idea that bigger number goes faster. I don't know if would cause any issues with YAGSL, but path planner does warn that setting those values in the path planner config can cause issues.

src/main/java/org/ironriders/drive/DriveSubsystem.java

					
				@ -104,4 +109,4 @@

				   * @param rotation      Desired rotation in radians per second.

				   * @param fieldRelative If not field relative, the robot will move relative to

				   *                      its own rotation.

				   */

Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up.

If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped.

swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false)

Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up. If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped. swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false)

Amber-leaf commented

2025-10-16 17:54:41 +00:00

(Migrated from github.com)

those numbers are somewhat arbitrary, but william said they would be fine

-------- Original Message --------
On Wednesday, 10/15/25 at 23:07 Ryan Elbert @.***> wrote:

@ryanelbert5125gmail commented on this pull request.

I won't approve this until the rotation inversion is fixed, but other than that it should be good to merge. The rest of my comments aside from that one were mainly nit picks and it should be fine to merge without addressing them.

In src/main/java/org/ironriders/drive/DriveSubsystem.java:

@@ -109,14 +112,23 @@ public void drive(
Translation2d translation,
double rotation,
boolean fieldRelative) {

swerveDrive.drive(translation, rotation, fieldRelative, false);

swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * (invertStatus ? -1 : 1), fieldRelative, false);

Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up.

If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped.

swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false)

In src/main/java/org/ironriders/core/RobotCommands.java:
 return intakeCommands.set(IntakeState.EJECT);
}

public Command resetGyroAngle() {

return Commands.runOnce(() -> resetPigeon());

}

public void resetPigeon() {

driveCommands.resetRotation();

Pigeon2 pigeon2 = new Pigeon2(9);

Just a nit pick, but this probably should be a constant so that if this code reused the magic number 9 doesn't cause issues if their gyro isn't on CAN ID 9

In src/main/java/org/ironriders/drive/DriveConstants.java:

@@ -34,8 +34,8 @@ public class DriveConstants {
public static final double ROTATION_CONTROL_EXPONENT = 3.0;
public static final double ROTATION_CONTROL_DEADBAND = 0.8;

public static final double SWERVE_DRIVE_MAX_SPEED = 0.3; // m/s //TODO update for competetion after testing

public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 1; // rad/s //TODO update for Competetion after

public static final double SWERVE_DRIVE_MAX_SPEED = 6; // m/s //TODO update for competetion after testing

public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5; // rad/s //TODO update for Competetion after

Just wanted to check that the SWERVE_DRIVE_MAX_SPEED = 6 and SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5 are values based on the design and limitations of the robot not random numbers based off the idea that bigger number goes faster. I don't know if would cause any issues with YAGSL, but path planner does warn that setting those values in the path planner config can cause issues.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: @.***>

those numbers are somewhat arbitrary, but william said they would be fine -------- Original Message -------- On Wednesday, 10/15/25 at 23:07 Ryan Elbert ***@***.***> wrote: > @ryanelbert5125gmail commented on this pull request. > > I won't approve this until the rotation inversion is fixed, but other than that it should be good to merge. The rest of my comments aside from that one were mainly nit picks and it should be fine to merge without addressing them. > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/drive/DriveSubsystem.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434633682): > >> @@ -109,14 +112,23 @@ public void drive( > Translation2d translation, > double rotation, > boolean fieldRelative) { > - swerveDrive.drive(translation, rotation, fieldRelative, false); > + swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * (invertStatus ? -1 : 1), fieldRelative, false); > > Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up. > > If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped. > > swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false) > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/core/RobotCommands.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434663053): > >> return intakeCommands.set(IntakeState.EJECT); > } > > + public Command resetGyroAngle() { > + return Commands.runOnce(() -> resetPigeon()); > + } > + > + public void resetPigeon() { > + driveCommands.resetRotation(); > + Pigeon2 pigeon2 = new Pigeon2(9); > > Just a nit pick, but this probably should be a constant so that if this code reused the magic number 9 doesn't cause issues if their gyro isn't on CAN ID 9 > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/drive/DriveConstants.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434675054): > >> @@ -34,8 +34,8 @@ public class DriveConstants { > public static final double ROTATION_CONTROL_EXPONENT = 3.0; > public static final double ROTATION_CONTROL_DEADBAND = 0.8; > > - public static final double SWERVE_DRIVE_MAX_SPEED = 0.3; // m/s //TODO update for competetion after testing > - public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 1; // rad/s //TODO update for Competetion after > + public static final double SWERVE_DRIVE_MAX_SPEED = 6; // m/s //TODO update for competetion after testing > + public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5; // rad/s //TODO update for Competetion after > > Just wanted to check that the SWERVE_DRIVE_MAX_SPEED = 6 and SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5 are values based on the design and limitations of the robot not random numbers based off the idea that bigger number goes faster. I don't know if would cause any issues with YAGSL, but path planner does warn that setting those values in the path planner config can cause issues. > > — > Reply to this email directly, [view it on GitHub](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#pullrequestreview-3343129459), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AZ2JPAO4X5KJBVH275BEMRL3X4YY7AVCNFSM6AAAAACJJ6EEM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBTGEZDSNBVHE). > You are receiving this because your review was requested.Message ID: ***@***.***>

Amber-leaf commented

2025-10-16 18:26:33 +00:00

(Migrated from github.com)

william wanted the rotation to not invert and i think it is set up like that but cant check

-------- Original Message --------
On Thursday, 10/16/25 at 10:54 isabelle901 @.***> wrote:

those numbers are somewhat arbitrary, but william said they would be fine

-------- Original Message --------
On Wednesday, 10/15/25 at 23:07 Ryan Elbert @.***> wrote:
@ryanelbert5125gmail commented on this pull request.

I won't approve this until the rotation inversion is fixed, but other than that it should be good to merge. The rest of my comments aside from that one were mainly nit picks and it should be fine to merge without addressing them.

In src/main/java/org/ironriders/drive/DriveSubsystem.java:

@@ -109,14 +112,23 @@ public void drive(
Translation2d translation,
double rotation,
boolean fieldRelative) {

swerveDrive.drive(translation, rotation, fieldRelative, false);

swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * (invertStatus ? -1 : 1), fieldRelative, false);

Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up.

If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped.

swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false)

In src/main/java/org/ironriders/core/RobotCommands.java:
 return intakeCommands.set(IntakeState.EJECT);
}

public Command resetGyroAngle() {

return Commands.runOnce(() -> resetPigeon());

}

public void resetPigeon() {

driveCommands.resetRotation();

Pigeon2 pigeon2 = new Pigeon2(9);

Just a nit pick, but this probably should be a constant so that if this code reused the magic number 9 doesn't cause issues if their gyro isn't on CAN ID 9

In src/main/java/org/ironriders/drive/DriveConstants.java:

@@ -34,8 +34,8 @@ public class DriveConstants {
public static final double ROTATION_CONTROL_EXPONENT = 3.0;
public static final double ROTATION_CONTROL_DEADBAND = 0.8;

public static final double SWERVE_DRIVE_MAX_SPEED = 0.3; // m/s //TODO update for competetion after testing

public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 1; // rad/s //TODO update for Competetion after

public static final double SWERVE_DRIVE_MAX_SPEED = 6; // m/s //TODO update for competetion after testing

public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5; // rad/s //TODO update for Competetion after

Just wanted to check that the SWERVE_DRIVE_MAX_SPEED = 6 and SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5 are values based on the design and limitations of the robot not random numbers based off the idea that bigger number goes faster. I don't know if would cause any issues with YAGSL, but path planner does warn that setting those values in the path planner config can cause issues.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: @.***>

william wanted the rotation to not invert and i think it is set up like that but cant check -------- Original Message -------- On Thursday, 10/16/25 at 10:54 isabelle901 ***@***.***> wrote: > those numbers are somewhat arbitrary, but william said they would be fine > > -------- Original Message -------- > On Wednesday, 10/15/25 at 23:07 Ryan Elbert ***@***.***> wrote: > >> @ryanelbert5125gmail commented on this pull request. >> >> I won't approve this until the rotation inversion is fixed, but other than that it should be good to merge. The rest of my comments aside from that one were mainly nit picks and it should be fine to merge without addressing them. >> >> --------------------------------------------------------------- >> >> In [src/main/java/org/ironriders/drive/DriveSubsystem.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434633682): >> >>> @@ -109,14 +112,23 @@ public void drive( >> Translation2d translation, >> double rotation, >> boolean fieldRelative) { >> - swerveDrive.drive(translation, rotation, fieldRelative, false); >> + swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * (invertStatus ? -1 : 1), fieldRelative, false); >> >> Usually when the controls are inverted that is because the negatives signs that dictate the inversion for the translation were reversed. I don't think we want to invert the rotation with the translation because rotation should be constant no matter if the odometry gets messed up. >> >> If rotation were to be inverted that means that the negative sign that controls the inversion of rotation in the robot container file should get swapped. >> >> swerveDrive.drive(translation.times(invertStatus ? -1 : 1), rotation * , fieldRelative, false) >> >> --------------------------------------------------------------- >> >> In [src/main/java/org/ironriders/core/RobotCommands.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434663053): >> >>> return intakeCommands.set(IntakeState.EJECT); >> } >> >> + public Command resetGyroAngle() { >> + return Commands.runOnce(() -> resetPigeon()); >> + } >> + >> + public void resetPigeon() { >> + driveCommands.resetRotation(); >> + Pigeon2 pigeon2 = new Pigeon2(9); >> >> Just a nit pick, but this probably should be a constant so that if this code reused the magic number 9 doesn't cause issues if their gyro isn't on CAN ID 9 >> >> --------------------------------------------------------------- >> >> In [src/main/java/org/ironriders/drive/DriveConstants.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2434675054): >> >>> @@ -34,8 +34,8 @@ public class DriveConstants { >> public static final double ROTATION_CONTROL_EXPONENT = 3.0; >> public static final double ROTATION_CONTROL_DEADBAND = 0.8; >> >> - public static final double SWERVE_DRIVE_MAX_SPEED = 0.3; // m/s //TODO update for competetion after testing >> - public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 1; // rad/s //TODO update for Competetion after >> + public static final double SWERVE_DRIVE_MAX_SPEED = 6; // m/s //TODO update for competetion after testing >> + public static final double SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5; // rad/s //TODO update for Competetion after >> >> Just wanted to check that the SWERVE_DRIVE_MAX_SPEED = 6 and SWERVE_MAXIMUM_ANGULAR_VELOCITY = Math.PI * 2.5 are values based on the design and limitations of the robot not random numbers based off the idea that bigger number goes faster. I don't know if would cause any issues with YAGSL, but path planner does warn that setting those values in the path planner config can cause issues. >> >> — >> Reply to this email directly, [view it on GitHub](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#pullrequestreview-3343129459), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AZ2JPAO4X5KJBVH275BEMRL3X4YY7AVCNFSM6AAAAACJJ6EEM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNBTGEZDSNBVHE). >> You are receiving this because your review was requested.Message ID: ***@***.***>

Saturncorgi (Migrated from github.com) reviewed

2025-10-17 17:15:54 +00:00

src/main/java/org/ironriders/drive/DriveConstants.java

					
				@ -34,15 +32,17 @@ public class DriveConstants {

				  public static final double ROTATION_CONTROL_EXPONENT = 3.0;

				  public static final double ROTATION_CONTROL_DEADBAND = 0.8;

They are somewhat arbitrary, but there isn't a good way to determine them without testing we don't have time fore

Saturncorgi commented

2025-10-18 03:38:39 +00:00

(Migrated from github.com)

Pr ready

FlareTheSaberDragon (Migrated from github.com) approved these changes

2025-10-18 03:45:21 +00:00

dendsl (Migrated from github.com) reviewed

2025-10-18 03:51:23 +00:00

src/main/java/org/ironriders/lib/IronSubsystem.java

I really like the idea of a Debug vs. Release mindset in terms of publishing/printing. It is a common industry standard. For my own edification does the dashboard allow filtering out these debug publishes?

Amber-leaf commented

2025-10-18 03:53:18 +00:00

(Migrated from github.com)

yea they are placed in a separate folder in the selectior. in too tried to explain better sorry

-------- Original Message --------
On Friday, 10/17/25 at 20:51 devonshire @.***> wrote:

@dendsl commented on this pull request.

In src/main/java/org/ironriders/lib/IronSubsystem.java:

public double getDiagnostic(String name, double defaultValue) {
return SmartDashboard.getNumber(name, defaultValue);
}

/**

Publish a boolean diagnostic value to SmartDashboard with the prefix

Subsystems/{subsystem name}/.

DEBUGGING ONLY Publish a boolean diagnostic value to SmartDashboard with the prefix

debug/Subsystems/{subsystem name}/.

*/

public void debugPublish(String name, boolean value) {

SmartDashboard.putBoolean("debug/" + dashboardPrefix + name, value);

I really like the idea of a Debug vs. Release mindset in terms of publishing/printing. It is a common industry standard. For my own edification does the dashboard allow filtering out these debug publishes?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: @.***>

yea they are placed in a separate folder in the selectior. in too tried to explain better sorry -------- Original Message -------- On Friday, 10/17/25 at 20:51 devonshire ***@***.***> wrote: > @dendsl commented on this pull request. > > --------------------------------------------------------------- > > In [src/main/java/org/ironriders/lib/IronSubsystem.java](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#discussion_r2441558723): > >> public double getDiagnostic(String name, double defaultValue) { > return SmartDashboard.getNumber(name, defaultValue); > } > > /** > - * Publish a boolean diagnostic value to SmartDashboard with the prefix > - * `Subsystems/{subsystem name}/`. > + * DEBUGGING ONLY Publish a boolean diagnostic value to SmartDashboard with the prefix > + * `debug/Subsystems/{subsystem name}/`. > + */ > + public void debugPublish(String name, boolean value) { > + SmartDashboard.putBoolean("debug/" + dashboardPrefix + name, value); > > I really like the idea of a Debug vs. Release mindset in terms of publishing/printing. It is a common industry standard. For my own edification does the dashboard allow filtering out these debug publishes? > > — > Reply to this email directly, [view it on GitHub](https://github.com/IronRiders/ReefScape-PostSeason/pull/28#pullrequestreview-3352517596), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AZ2JPANYMGRD4CYKLDYMHZ33YG2NBAVCNFSM6AAAAACJJ6EEM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNJSGUYTONJZGY). > You are receiving this because your review was requested.Message ID: ***@***.***>

👍 1

Saturncorgi (Migrated from github.com) reviewed

2025-10-18 03:54:37 +00:00

src/main/java/org/ironriders/lib/IronSubsystem.java

Sorta. It's not great but it helps

johnluetke (Migrated from github.com) approved these changes

2025-10-18 04:17:02 +00:00

Amber-leaf (Migrated from github.com) approved these changes

2025-10-18 05:04:34 +00:00

Amber-leaf (Migrated from github.com) left a comment

Working at pits

Saturncorgi (Migrated from github.com) reviewed

2025-10-18 05:37:26 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

Please convert to an issue. I don't have the energy and i want this branch ready by comp

Saturncorgi (Migrated from github.com) reviewed

2025-10-18 05:38:13 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

Please convert to an issue. I don't have the energy and i want this branch ready by comp

Saturncorgi (Migrated from github.com) reviewed

2025-10-18 05:39:15 +00:00

src/main/java/org/ironriders/drive/DriveSubsystem.java

					
				@ -104,4 +109,4 @@

				   * @param rotation      Desired rotation in radians per second.

				   * @param fieldRelative If not field relative, the robot will move relative to

				   *                      its own rotation.

				   */

Please convert to an issue. I don't have the energy and i want this branch ready by comp. This does look like it could affect functionality, but it was working fine I think,