Feature/sysid

[Orcasphynx]

No description provided.

[github-actions]

AI Code Review

Hey team! This pull request is looking great! It's fantastic to see the integration of WPILib's SysId routine into our subsystems. This is a really important step for tuning our robot effectively and getting the best performance out of our mechanisms.

Positive Highlights

I'm really impressed by several aspects of this change:

• Safety-First SysId Configuration: Your use of Volts.of(4) for the dynamic step voltage in the SysIdRoutine.Config is a smart move to prevent brownouts, which shows great foresight in robot safety and resource management.
• Clear Logging: Integrating SignalLogger.writeString to log the SysId state directly into Phoenix Tuner is a fantastic way to debug and monitor the routine's execution. It will make analysis much easier!
• WPILib Unit System: You're consistently using WPILib's type-safe units, like Volts.of(4) and RotationsPerSecond, which helps prevent common conversion errors and improves code clarity, aligning perfectly with our team's guidelines.

Suggestions

Here are a few suggestions that might further enhance this work and ensure the SysId routines function as intended:

FRC/WPILib Best Practices & Command Factory Methods

• Exposing SysId Commands: Currently, the SysIdRoutine objects are created, but the commands they generate (e.g., m_sysIdRoutineIndexer.quasistaticForwardCommand(), dynamicForwardCommand(), etc.) aren't yet exposed or used. To make full use of these, you'll want to either:

  1. Create public command factory methods in each subsystem (e.g., public Command runIndexerSysIdQuasistaticForwardCommand()) that return the SysId-generated commands.
  2. Or, directly reference these commands from your RobotContainer or another command-binding class to assign them to buttons or triggers.
     This will allow the team to actually run the SysId tests!

• SysId Measurement Input: For each SysIdRoutine.Mechanism constructor, the second parameter (getMeasurement) is currently null. This parameter is crucial for SysId to collect data, as it expects a supplier for the mechanism's velocity. Without it, SysId won't be able to collect the necessary velocity data for analysis.

  • For IndexerSubsystem.java (line 25): You could consider replacing null with something like () -> indexerMotor.getVelocity().in(RotationsPerSecond).
  • For IntakeSubsystem.java (line 35): Similarly, for the roller, it could be () -> rollerMotor.getVelocity().in(RotationsPerSecond).
  • For ShooterSubsystem.java (line 35): And for the flywheel, () -> flywheelMotor.getVelocity().in(RotationsPerSecond).

Performance and Efficiency & CTRE Position/Velocity Control

• Shooter Hood Control Mismatch: In ShooterSubsystem.java, specifically in the periodic() method (around line 89), I noticed that hoodMotor.setControl(hoodControl.withVelocity(hoodTarget.in(Rotations))); is being used. However, hoodControl is an instance of PositionTorqueCurrentFOC, which is designed for position control. It might be helpful to use hoodControl.withPosition(hoodTarget.in(Rotations)) here, as PositionTorqueCurrentFOC is expecting a position setpoint, not a velocity. Using withVelocity on a position control request could lead to unexpected behavior or errors.

Questions

To help me understand the full picture:

• How do you envision these new SysId commands being triggered? Will they be bound to specific buttons in RobotContainer, or perhaps accessed through a different interface like NetworkTables?
• Could you confirm the intended control mode for the hoodMotor in ShooterSubsystem.periodic()? Is it meant to be position control (as indicated by PositionTorqueCurrentFOC) or velocity control?

Keep up the fantastic work on improving our robot's control systems! This is a really valuable addition to our codebase.

---

This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

[github-actions]

✓ Build successful and code formatting check passed!