Skip to content

Conversation

@Srijan-Murai
Copy link
Contributor

Made flywheel subsystem. Had to delete old shooter branch because of weird issues but both are identical.

@drewbxyz
Copy link
Member

Hey! Take a look here for some comments: #1


public default void launchRing() {}

public default void setRunning(boolean runIntake) {}

Choose a reason for hiding this comment

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

The setRunning method appears to contribute no additional functionality to the IO. I would suggest removing it

// make lower a follower of upper
leftFX.setControl(new Follower(FlywheelConfigs.rightFXID, false));
// add motors to sim
// PhysicsSim.getInstance().addTalonFX(leftFX);

Choose a reason for hiding this comment

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

You would want the PhysicsSim lines uncommented for simulation in order to add TalonFX to it.


@Override
public void setRollerDuty(double power) {
leftFX.setControl(new DutyCycleOut(power));

Choose a reason for hiding this comment

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

It is more efficient to reuse the same instance of the control request DutyCycleOut and simply use the withOutput method to change the value of the output upon calling this method. I would also note that for flywheel control, it may be worth considering more robust control alternatives such as velocity control, but this is ok for testing for the time being.

Copy link
Member

Choose a reason for hiding this comment

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

(try motion magic torque current velocity)

Choose a reason for hiding this comment

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

(try motion magic torque current velocity)

See this for an example of what you could use

Choose a reason for hiding this comment

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

See javadoc for import path. Otherwise if it isn't showing up, you might want to sync vendordeps

@Srijan-Murai
Copy link
Contributor Author

I will remove launchRing method next commit I think it just does the setRoller method's job


@Override
public void setRoller(double power) {
leftMotor.setControl(new MotionMagicVelocityTorqueCurrentFOC(power));

Choose a reason for hiding this comment

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

Please see my previous comment on reusing control requests


public default void setRoller(double power) {}

public default void launchRing() {}

Choose a reason for hiding this comment

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

Remove

public Flywheel(FlywheelIO io) {
// Recording inputs from roller
this.io = io;
// Logger.processInputs("Flywheel", inputs); error

Choose a reason for hiding this comment

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

Uncomment

Copy link
Member

@samperlmutter samperlmutter left a comment

Choose a reason for hiding this comment

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

looking much better. mostly just cleanup now

Comment on lines 40 to 41
public void setRoller(double power) {
leftMotor.setControl(motionRequest.withVelocity(power));
Copy link
Member

Choose a reason for hiding this comment

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

power isn't quite the right name for this parameter right? what is the parameter representing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to velocity

Copy link
Member

@samperlmutter samperlmutter left a comment

Choose a reason for hiding this comment

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

looks good!

@Srijan-Murai Srijan-Murai merged commit a85b275 into main Jan 21, 2026
1 check passed
@Srijan-Murai Srijan-Murai deleted the flywheel branch January 21, 2026 01:38
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.

4 participants