Skip to content

Conversation

@blaze-developer
Copy link
Collaborator

No description provided.

@blaze-developer blaze-developer self-assigned this Jan 27, 2026
@blaze-developer
Copy link
Collaborator Author

Butches Comments:

General

Add support for the sysid() commands. There will be commands for quasistatic and for dynamic both forward and reverse
for each motor that needs to be characterized. So, four for the pivot motor and four for the spinner motor.
Digging into sysid will help you learn more about tunning and these command help us in the tuning processes.

The set pivot velocity capability will not be needed, but does not hurt anything.

IntakeSubsystem.java

In the method isIntakeDeployed() you compare the actual angle of the intake to the deployed angle. However, the
actual angle will not be exactly the deployed angle so you need to check if the angle is close. Note the units library
provides the isNear() method to help with this.

Same basic comment for isIntakeStowed()

IntakeIOHardware.java

The can bus for the intake will likely not be the can bus for the drivebase (which is CompTunerConstants.kCANBus).
I did this for the intake prototype code because that was the easiest way to wire the prototype. For the main robot,
we should likely pass the CAN bus into the constructor so higher level code can decide what goes on which bus.
So, the IntakeIOHardware class would take a CANBus object as a constructor argument.

The PID controllers generally have P, I, D, V, A, G, and S parameters. Many of these are zero, but lets put the code
in place all the way to the constants file so we have the plumbing in place to set these values if we choose to. Theoritically
The gravity (G) parameter might be necessary for the intake (but its so light we probably won't use it).

On the forward and reverse soft limits, I would have different constants from the deployedAngle and stowedAngle. The reason
is we want the limits to set where we may do damage to the robot. But the deployAngle will be based on where we collect best
and we may change it as we go.

For the status signals, all of them are 50 Hz except the pivot angle which is 20 Hz. However the only two that we might need
more updates for is the pivot angle and the rollerAngularVelocity as these are the two values we are trying to manage closely. I would
make these two be 50 hz and the others 20 hz.

In the setRollerVoltage you take the Voltage value in the method argument (volts) and extract it as a double (volts.in(Volts))
and pass the double in to the withOutput() method. The withOutput() method has two forms, one with double and one with Voltage.
If you use the Voltage version of this, there are not these conversions that have to be done. Same basic comment on setPivotAngle()
setRollerVelocity(), and setPivotVelocity().

Copy link
Collaborator Author

@blaze-developer blaze-developer left a comment

Choose a reason for hiding this comment

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

Here

//Current Limit Configurations
pivotConfigs.CurrentLimits.StatorCurrentLimit = IntakeConstants.currentLimit;
pivotConfigs.CurrentLimits.StatorCurrentLimitEnable = true;
pivotMotor.getConfigurator().apply(pivotConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pivotMotor.getConfigurator().apply(pivotConfigs);

When applying configs, it is recommended / has been useful in the past to use the tryUntilOk() method from PheonixUtil. This trys the configuration up to a specified number of times until it returns an OK status signal.

We should also only have to run this configuration once per motor after the configuration has been entirely created, this call should be removed, and the next one should be changed to tryUntilOk and kept.

pivotConfigs.Slot0.kD= IntakeConstants.pivotKD;
pivotConfigs.Slot0.kV= IntakeConstants.pivotKV;
pivotConfigs.Slot0.kI= IntakeConstants.pivotKI;
pivotMotor.getConfigurator().apply(pivotConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pivotMotor.getConfigurator().apply(pivotConfigs);

Same as above

pivotConfigs.SoftwareLimitSwitch.ForwardSoftLimitThreshold= IntakeConstants.deployedAngle.in(Degrees);
pivotConfigs.SoftwareLimitSwitch.ReverseSoftLimitEnable= true;
pivotConfigs.SoftwareLimitSwitch.ReverseSoftLimitThreshold= IntakeConstants.stowedAngle.in(Degrees);
pivotMotor.getConfigurator().apply(pivotConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pivotMotor.getConfigurator().apply(pivotConfigs);

Same as above, remove this redundant call.

pivotConfigs.MotionMagic.MotionMagicCruiseVelocity= IntakeConstants.pivotCruiseVelocity.in(DegreesPerSecond);
pivotConfigs.MotionMagic.MotionMagicAcceleration= IntakeConstants.pivotCruiseAcceleration.in(DegreesPerSecondPerSecond);
pivotConfigs.MotionMagic.MotionMagicJerk= IntakeConstants.pivotMaxJerk;
pivotMotor.getConfigurator().apply(pivotConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pivotMotor.getConfigurator().apply(pivotConfigs);
tryUntilOk(5, () -> pivotMotor.getConfigurator().apply(pivotConfigs, 0.25));

Same as above, keep this as it is the last one and after pivotConfigs has been configured fully.

//Current Limit Configurations
rollerConfigs.CurrentLimits.StatorCurrentLimit= IntakeConstants.currentLimit;
rollerConfigs.CurrentLimits.StatorCurrentLimitEnable= true;
rollerMotor.getConfigurator().apply(rollerConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
rollerMotor.getConfigurator().apply(rollerConfigs);

Same as above.

rollerConfigs.Slot0.kD= IntakeConstants.rollerKD;
rollerConfigs.Slot0.kV= IntakeConstants.rollerKV;
rollerConfigs.Slot0.kI= IntakeConstants.rollerKI;
rollerMotor.getConfigurator().apply(rollerConfigs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
rollerMotor.getConfigurator().apply(rollerConfigs);
tryUntilOk(5, () -> rollerMotor.getConfigurator().apply(rollerConfigs, 0.25));

Same as above, keep this one.

rollerCurrentAmpsSignal = rollerMotor.getSupplyCurrent();
pivotCurrentAmpsSignal = pivotMotor.getSupplyCurrent();

BaseStatusSignal.setUpdateFrequencyForAll(50.0, rollerAngularVelocitySignal, rollerAppliedVoltsSignal, rollerCurrentAmpsSignal, pivotAngularVelocitySignal, pivotAppliedVoltsSignal, pivotCurrentAmpsSignal);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These can also use the tryUntilOk method.

Comment on lines +24 to +30
Logger.recordOutput("Intake/PivotAngle", inputs.PivotAngle);
Logger.recordOutput("Intake/RollerVoltage", inputs.RollerAppliedVolts);
Logger.recordOutput("Intake/PivotAngularVelocity", inputs.PivotAngularVelocity);
Logger.recordOutput("Intake/RollerAngularVelocity", inputs.RollerAngularVelocity);
Logger.recordOutput("Intake/RollerCurrentAmps", inputs.RollerCurrentAmps);
Logger.recordOutput("Intake/PivotCurrentAmps", inputs.PivotCurrentAmps);
Logger.recordOutput("Intake/PivotAppliedVolts", inputs.PivotAppliedVolts);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
Logger.recordOutput("Intake/PivotAngle", inputs.PivotAngle);
Logger.recordOutput("Intake/RollerVoltage", inputs.RollerAppliedVolts);
Logger.recordOutput("Intake/PivotAngularVelocity", inputs.PivotAngularVelocity);
Logger.recordOutput("Intake/RollerAngularVelocity", inputs.RollerAngularVelocity);
Logger.recordOutput("Intake/RollerCurrentAmps", inputs.RollerCurrentAmps);
Logger.recordOutput("Intake/PivotCurrentAmps", inputs.PivotCurrentAmps);
Logger.recordOutput("Intake/PivotAppliedVolts", inputs.PivotAppliedVolts);

We don't really need to log these as outputs, they are already logged periodically as inputs, I think we should save on log file space and remove these calls.

Comment on lines 34 to 60
public void setRollerVoltage(Voltage volts) {
io.setRollerVoltage(volts);
}

public void setPivotAngle(Angle angle) {
io.setPivotAngle(angle);
}

public void stopRoller(){
io.setRollerVoltage(Volts.of(0));
}

public void stowIntake(){
setPivotAngle(IntakeConstants.stowedAngle);
}

public void deployIntake(){
setPivotAngle(IntakeConstants.deployedAngle);
}

public void setRollerVelocity(AngularVelocity velocity) {
io.setRollerVelocity(velocity);
}

public void setPivotVelocity(AngularVelocity velocity) {
io.setPivotVelocity(velocity);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These should either be primarily exposed as Commands or command options should also be exposed. Either I can go in and do this, or you can if you feel up to it.

import edu.wpi.first.units.measure.AngularVelocity;
import edu.wpi.first.units.measure.Angle;

public class IntakeCommands {
Copy link
Collaborator Author

@blaze-developer blaze-developer Jan 27, 2026

Choose a reason for hiding this comment

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

The majority of these, if not all of them, can be moved directly into the IntakeSubsystem, meaning its easier around the codebase to reference these commands, for example, instead of writing:

IntakeCommands.stowIntakeCommand(intake_)

We could write

intake_.stow()

When we start to involve multiple subsystems, factories like this become more useful.

null, //Default ramp rate of the voltage of 1 V/s, according to Phoenix 6 documentation
stepVoltage_pivot,
timeOut_pivot,
(state) -> SignalLogger.writeString("state", state.toString()) //Logging the state of the routine
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of using the SignalLogger, AdvantageKit can be used here described in this documention..

We don't use the CTRE signal logger that saves log files, meaning we need to do it in our wpilog.

null, //Default ramp rate of the voltage of 1 V/s, according to Phoenix 6 documentation
stepVoltage_roller,
timeOut_roller,
(state) -> SignalLogger.writeString("state", state.toString()) //Logging the state of the routine
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

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