Hi, this summer my team and I decided to convert our TalonFX code from Phoenix 5 to Phoenix 6. My mentor and I have been using the API documentation and the code builds, but it doesn’t actually work on the real robot. When trying out our shooter, the motors don’t move at all. Here’s the link to the working shooter subsystem file from phoenix 5 and the link to the new shooter subsystem file. What am I missing?
In addition to the Migration Guide, make sure you also check out some of our examples, such as our VelocityClosedLoop example.
Looking at your code, here are some of my observations:
- It doesn’t look like you actually apply the PID configs or peak outputs anywhere;
setPIDParameters()andsetPeakOutputs()modify the Configuration object but never apply it to the device. Note that configs are blocking in both Phoenix 5 and Phoenix 6, so you should not callsetPIDParameters()andsetPeakOutputs()periodically. Instead, you should try to configure PID and peak outputs in the constructor with the rest of the configs.
If you need multiple sets of PID gains for one motor, we support three gain slots, and you can select which slot to use in the control request object usingwithSlot(). - There are a lot of references to the legacy Phoenix 5 unit types, which makes it hard to tell whether you’re actually using the correct units everywhere. For example,
getRpm()is definitely wrong, as it should just returngetVelocity().getValueAsDouble() * 60.0.
I would recommend plugging your old gains into the gain calculators in the migration guide and copying the new numbers directly into your code, rather than adding code to do the P5->P6 conversions. Since everything is in rotations and RPS now, it looks like the only unit conversion you should need is RPM to RPS (divide by 60) and vice versa.- We also typically recommend using
VelocityVoltageinstead ofVelocityDutyCycle, as it performs more consistently with changes in battery voltage. You basically just need to multiply all your duty cycle gains by 12.0 V to get voltage gains.
- We also typically recommend using
- It looks like
shootCustom()doesn’t actually change the requested velocity in the control request. You really only need oneVelocityDutyCycleinstance per motor; you can change theVelocityparameter in each function. For example:
private final VelocityDutyCycle shooterVelocity = new VelocityDutyCycle(0);
public void shootLow() {
SwitchedCamera.setUsbCamera(Ports.UsbCamera.SHOOTER_CAMERA);
shooterMaster.setControl(shooterVelocity.withVelocity(targetLowVelocityRPS));
isShooting = true;
}
public void shootCustom(double custom_rpm) {
SwitchedCamera.setUsbCamera(Ports.UsbCamera.SHOOTER_CAMERA);
shooterMaster.setControl(shooterVelocity.withVelocity(custom_rpm / 60.0));
isShooting = true;
}
1 Like
Okay, thank you, I’ll make the changes and test it out at our next meeting.
If you’re interested, our team did two rewrites this summer.
Our 2023-24 competition code is based on Phoenix-5 and in our first project we converted it into Phoenix-6 without changing the logic.
This branch - GitHub - FRC999/2024Competition at 2024PostSeasonPhoenix6 has a rewritten code, while this one - GitHub - FRC999/2024Competition at post-WNE has the original. That will hopefully make it easy to compare.
We’re also currently working on a full rewrite to implement “native” CTR swerve logic as generated via Phoenix Tuner X, but rewritten to more “regular” Command Robot framework.
That code is still being modified. Almost all of it is done, but none tested on the robot yet.
Here is the link to the branch: GitHub - FRC999/2025SwervePrototype
1 Like
Thanks! Will definitely take a look.
As an update - we completed and tested the rewrite of the “native” CTR swerve code (the one generated by Phoenix Tuner X, but rewritten by our team in more “normal” way, compliant with regular Command Robot template).
GitHub - FRC999/2025SwervePrototype
Note that the code complained a little when our TalonFX did not have Pro license installed. We planned to do it anyway, so when we did it, the warnings went away. Nevertheless, it actually worked even without Pro license.
The main advantage of the “native” code - we did research on their “internal” library code - meaning - we figured out how exactly it was controlling the motors. So, it does not actually use Velocity PID, but rather converts velocity PID into a Voltage PID using an algorithm, which I guess CTR came up with and tested. Since it’s their hardware, I would assume they would do so better and “smoother” than the regular velocity PID.
The regular teleop driving feels about the same. May be a bit smoother and precise, less jerky on lower speeds.
We also liked an ability to add on-demand telemetry directly to the chassis object, and the fact that it provided access to the information from the individual swerve modules.
All-in-all - the code is about 50-60% smaller than the swerve code we ran in 2023-2024 season, primarily because the actual swervemodule operations are “hidden” in the CTR libraries. However, we do not see any detriment in the functionality so far.
Another big difference - the wheel turn angles are tracked exclusively by cancoders. In our 2023/24 code we used internal encoders, so the turn PID will run on 1ms cycle. However, with Pro remote Cancoders also can run on 1ms cycle (to the TalonFX over CAN). So, hardware PID works just as well with Cancoders now.
The next step for us is to test (and fix if needed) PathPlanner auto.
What do you mean by this, in particular?
CTRE worked closely with the WPILib development team when writing the most recent version of their swerve template; the Tuner X generated code is compliant with the intended Command-based architecture.
1 Like
Their generated code is certainly compliant. But it needed a bit of a retrofit to make it easier to use with auto and other things:
- Have all constants in Constants.java (they have it in a TunerConstants.java; all constants are static, so it’s compliant, just in a different file)
- Have all commands portable in separate command classes, in case extra telemetry/troubleshooting need to be added to the individual commands.
For instance, manual teleop drive in a generated code is done as
drivetrain.setDefaultCommand( // Drivetrain will execute this command periodically
drivetrain.applyRequest(() -> drive.withVelocityX(-joystick.getLeftY() * MaxSpeed) // Drive forward with
// negative Y (forward)
.withVelocityY(-joystick.getLeftX() * MaxSpeed) // Drive left with negative X (left)
.withRotationalRate(-joystick.getRightX() * MaxAngularRate) // Drive counterclockwise with negative X (left)
));
where the applyRequest is done as
public Command applyRequest(Supplier<SwerveRequest> requestSupplier) {
return run(() -> {
this.setControl(requestSupplier.get()) ;
}
);
}
We do not really like that as it makes it difficult to troubleshoot if something is not working and you want to find the cause (e.g. print the details of the SwerveRequest that is sent to the chassis object)
In fact, in the generated code the Command code is inside the file that is actually a Subsystem. We try to clearly separate a Subsystem, which we only use as a class to describe possible behavior of the hardware vs the Command, which provides an action via calls to the exposed subsystem methods.
We also do not use AutoBuilder with PathPlanner for the same reason - because we want to see the in-flight details of the commands to the chassis generated by the PP controller in case the robot does something we do not expect. We use FollowPathHolonomic class instead, which allows us to easily insert troubleshooting telemetry, from tracking what joystick input values are processing to the response we get.
Another example is - the generated code creates the chassis object in TunerConstants -
public static final CommandSwerveDrivetrain DriveTrain = new CommandSwerveDrivetrain(DrivetrainConstants, FrontLeft,
FrontRight, BackLeft, BackRight);
But we thought that this code really belongs in a subsystem, since it’s a subsystem that does initialization of all of the subsystem-related hardware. We moved createModuleConstants calls to the subsystem to follow the same logic. In other words, we prefer to have numbers in the constants, but anything that coonfigures/touches the hardware in the respoctive subsystem.
The generated code also does not directly expose the IMU object. This is not needed for teleop driving, but may be needed for other purposes. First, in the auto we “snap” the robot to the start of the trajectory, something that AutoBuilder does by itself, but if you run the trajectory in a command, you need to handle that manually. Second, n 2023/24 we used IMU to track the chassis roll/pitch for other purposes not related to driving.
In our team’s opinion follow these rules makes the code much easier to follow (e.g. when the command classes do only what command classes suppose to do, and subsystem classes do what subsystem classes suppose to do).
Another example is that the telemetry logger for the chassis is defined in the RobotContainer -
private final Telemetry logger = new Telemetry(MaxSpeed);
Since this logger is only for the chassis, we though it should be defined either in a subsystem that defined chassis object, or in a separate subsystem responsible for telemetry (we usually create SmartDashboardSubsystem to produce telemetry; in this particular case since the telemetry is registered with the chassis class directly, we thought it would be best to place it here).
On a cosmetic side of things we do not like defining swerve modules in a named fashion (e.g. FrontLeft), but rather define them in the ENUM to treat the constants in the array-like fashion. We keep the order of the modules submitted to the createModuleConstants corresponding to the names in the ENUM (e.g. MOD0). This allows us to catch typos if someone will type some constant as int for one module and double for another one.
To summarize all this - the CTR-generated code works absolutely fine. However, in-depth review with the students showed that the fine details of it was hard to follow. E.g. when we asked a group of students to explain in detail and follow the code in Java how the specific joystick input causes a particular command to be issued to a specific motor controller (including tracing that process in CTR libraries call-by-call), they were getting lost quickly. Hence, our retrofit was to make a code much easier to understand, follow and troubleshoot by our team rather than doing any functional change.
We thought that other teams may benefit from easier-to-understand code as well.
This is not currently advised; you can troubleshoot just fine in a factory function. The encapsulation breakages required to call subsystem methods from external command classes is worse than any gain in modularity.
Separating a command into its own class file doesn’t really make it any easier to log diagnostic state about that command. You still have to insert logging code into the command body.
1 Like
Could you elaborate on the encapsulation breakage, please?
We expose the subsystem methods only for specific functions (e.g. if you want the robot to move, we expose a “drive” method that takes joystick methods in a subsystem; if we need to read or set IMU, we expose relevant methods). Meaning, we do not expose subsystem hardware-related objects, only the methods that are required to perform a specific function on the hardware.
Anything that writes to protected state (e.g. moving a motor) should only be exposed as a Command mutexed by a Subsystem. If a Subsystem has a public non-Command method that changes the output of a motor, then any piece of the code that has access to that subsystem can race with the command scheduler for control of that motor’s output.
Whether you modify the motor through an accessor method or direct public access doesn’t make any difference. What matters is that you’re not guaranteeing the right invariants to keep your hardware from doing unsafe/unpredictable things.
Re: encapsulation - yes, I see your point.
So, we have an agreement (which is obviously not a guarantee) - we call subsystem methods ONLY from commands, and all commands are interruptible and mutexed by the subsystems they’re calling.
I agree that it does not provide a true encapsulation for the reason you mentioned, but as long as the global rule is followed (and it always was in our case throughout the years), it should not make a difference. I still think that this setup makes the code easier to follow by the students vs subsystems running commands.
As an example, we thought about switching to more advanced framework, like ROS2. However, we found that it has a significant learning barrier to entry for students, especially the ones that start with no prior programming knowledge. Our code is designed by students (with mentor guidance), so we want to make sure the students on the programming team understand the meaning and purpose of every single line of it.
Nevertheless, proper encapsulation is an important concept, and I will think how we can enforce it in some other way. The point is - this is really what interruptible commands are for - to eliminate resource contention. And we use that feature extensively. Also putting commands in a subsystem, while will enforce the encapsulation, will make the subsystem very bloated. In our code we usually have 30+ commands, which are often designed by different students, and they know that the only thing they should worry about is the command logic, not the interaction with other unrelated commands. They also know that they need to design the command in a way that it will safely handle a condition when it’s cancelled by some other command (e.g. auto-driving cancelled by manual interrupt; shooting sequence cancelled by some other button - helps much if the drivers see a gamepiece being stuck, so they can eject it instead, for instance)
Re: logging of the commands - we found that it’s much easier for less experienced team members to insert logging statements that troubleshoot hardware vs trying to see how to modify the in-line command properly.
One example is a trajectory - e.g. your auto suppose to move the robot forward, and it actually moves sideways or turns.
So, you want to determine very quickly - did you actually tell the robot to move forward? Is that the typo with the inputs where X and Y are misplaced, or is that an issue with IMU, or you have a problem with the drive code? The quick solution - when you have a value from a controller, instead of actually forwarding it to the drive code, just print it to the dashboard. Once troubleshooting is done, just uncomment the original and comment out the call to the logger. That saved us quite a bit during the competition if we find a bug in something that was not fully tested.
1 Like
Even under the factory model, any command that can be built from other commands component-wise can be moved entirely out of the Subsystem file itself without doing anything in particular to allow it, so long as the component commands are in the subsystem’s public contract.
If you approach the first step of defining a subsystem as defining a minimal contract of public Commands (instead of public methods), then that contract should not bloat the class. Past that base contract, you’re free to put more-complicated commands wherever you want.
If you really want bespoke Command subclasses but also want to encapsulate safely, you should move commands that directly use subsystem hardware into a shared package with the subsystem and make the hardware access methods package-private. This enforces the workflow you’ve described with Java’s type system, and is how Java expects this sort of thing to be represented.
1 Like
The last way (encapsulate with packages) looks very attractive, and it will require very minimal changes to our way of doing things. The command sequences in our case usually run individual commands which are defined separately. So, we can “tie” the most granular command to just one subsystem. Hence, we indeed can have subsystems and all commands that directly use it defined in the same package.
If you can provide or point to some java code examples of the first two ways, it would be much appreciated.
1 Like
Just to explain some of the reasons why the generated swerve code is the way it is right now:
Putting the Tuner-generated constants in a separate file makes it easier to regenerate the constants when your swerve setup changes (as we have a “Generate constants only” button). Additionally, a lot of teams have been (to some extent) moving towards having one constants file per subsystem, rather than one massive constants file, so having a separate TunerConstants doesn’t differ much from already common practices.
We agree that this is something that goes against WPILib recommended best practices. If you wanted to fix this right now, I would just make the SwerveDrivetrainConstants and four SwerveModuleConstants instances public and construct the CommandSwerveDrivetrain directly inside RobotContainer (saving it to a non-static variable). We will be doing something similar with the 2025 swerve generator.
Since SwerveDrivetrainConstants and SwerveModuleConstants are just classes that encapsulate all constants necessary for swerve, constructing them directly in the drivetrain didn’t seem appropriate to us, as that clutters the drivetrain code. The drivetrain just cares about the five objects, not everything used to construct them.
It actually does; getPigeon2() gets the Pigeon 2 instance, getRotation3d() pulls out just a Rotation3d from the Pigeon 2 (which includes pitch and roll), and getState().Pose.getRotation() pulls out the robot heading as a Rotation2d.
The reason why Telemetry is currently constructed in the RobotContainer is because it could be easily extended to telemeterize other subsystems. If you want it to specifically be a SwerveTelemetry class, then I would agree that it is more reasonable to construct it directly inside the drivetrain and call registerTelemetry() from the constructor.
On a separate note, you shouldn’t really have a Subsystem for telemetry, as there aren’t really any hardware resources that need to be protected by the command-based framework (the purpose of Subsystem). Instead, telemetry should generally be a regular class, as it is in our generated code.
As for why we do commands the way we do, I think Oblarg has done a great job explaining that. If you want to add some telemetry to the commands, you can easily do so within the lambdas registered with the drivetrain. With that said, some teams do prefer moving the SwerveRequest objects into the subsystem and exposing methods like
private final SwerveRequest.FieldCentric m_drive = new SwerveRequest.FieldCentric()
.withDeadband(MaxSpeed * 0.1).withRotationalDeadband(MaxAngularRate * 0.1) // Add a 10% deadband
.withDriveRequestType(DriveRequestType.OpenLoopVoltage); // I want field-centric
// driving in open loop
public Command drive(DoubleSupplier vx, DoubleSupplier vy, DoubleSupplier omega) {
return applyRequest(() ->
m_drive.withVelocityX(vx.getAsDouble())
.withVelocityY(vy.getAsDouble())
.withRotationalRate(omega.getAsDouble())
);
rather than constructing and applying everything from RobotContainer. Our generated swerve code doesn’t do this because it’s more flexible as a template to construct the requests and apply them from RobotContainer.
2 Likes
First, I want to thank you and the rest of the CTR for making such great hardware and working on this software. I do not believe our team would have been able to make a comparable one s a summer project, for instance. Since it makes our main drive code about 3 times smaller, we plan to use it as our main competition drive code (with changes that I mentioned) for the 24/25 season.
The more we look into it and go through the details, the more we like the architectural decisions that went into it. For instance, we worked with the field odometry today, and we really like that it’s encapsulated in the chassis object. In the past we tracked odometry separately, and we had very messy handling of the IMU. We tested today that the chassis snap method that snaps the robot to the pose is IMU-independent, which makes a big difference in handling/restoring IMU before/after trajectory.
On the constants - yes, I have seen that trend as well. Not sure if I really like it, but it is what it is. We currently do it via subclasses in Constants. And yes, sometimes it gets annoying when trying to find a specific constant.
Re: IMU - yes, found it, thank you for the note. Moreover, with the exposure of the telemetry and poses through the chassis object, we may not need to expose IMU directly for drive.
Re: Telemetry - I see your point. In our case the SmartdashboardSubsystem is it. I agree that subsystem should manage a piece of hardware. So, in our case we try not to have any telemetry outside it. That way we do not chase SmartDashboard updates all over the place.
In summary, I think you really did great work. Our team did not mind a rewrite to make the code more “familiar”, since it gave students the opportunity to go through it line by line and actually understand how it does what it suppose to do. We also walked through the code of the actual library, at least the parts that make the robot actually drive. As I said before, it’s working absolutely fine, with the template code or our rewrite, and we really like the advantages that it gives us. This code was the sole reason we bought the Pro license this time, since we really saw how we can still stick to the hardware PID even with remote devices (we really do not like software PID running on Rio; with auto trajectories we do not have a choice, but everything else is hardware PID).
In the past it took us multiple sessions to update and calibrate field odometry correctly, and here we did it today, in one evening session. You cannot get much more time and effort saving than that.
2 Likes