API + Documentation Feedback

[modelmat]

I find using the CTRE motor API to be rather difficult, so what I'm trying to do here is explain my thought process and how I use documentation in order to demonstrate what I find lacking.

I'm going to try to go over creating a robot project in WPILib simulation. This robot will be a motor controller with encoder (4096 CPR) controlling a 6-inch higrip wheel, and converting the encoder positions and velocities into SI units. I will compare both the WPILib documentation and CTRE documentation.

Part 1: Creating a Motor Object

CTRE

For a TalonSRX, I open the documentation.

From there I scroll down the page through a large amount of description of using the config tool until I find some sample Java code. This is not very easy to find, nor does it show C++ code.

https://user-images.githubusercontent.com/25718989/104998581-63398000-5a23-11eb-870b-2301340901de.mp4

If I look at the API Documentation you can see the namespaces and file includes required:

#include <ctre/phoenix/motorcontrol/can/TalonSRX.h>

ctre::phoenix::motorcontrol::can::TalonSRX motor{0};

But if you look at your examples, the includes are different:

#include "ctre/Phoenix.h"

WPI_TalonSRX* motor = new WPI_TalonSRX{0};

This has some issues already noted, but I'm not sure which header files which should be including (I'd lean towards the more specific one - as opposed to the style of "frc/WPILib.h" which includes everything). This also demonstrates the existence of the WPI_TalonSRX class, which is hidden in the WP/NI integration page.

WPILib

The WPILib start-documentation is much easier to find

https://user-images.githubusercontent.com/25718989/104998759-a8f64880-5a23-11eb-9d73-ccfcdba574e4.mp4

Part of the documentation for adding motors is explained here. There is also the Actuators section (also visible on the homepage) which includes documentation on the motors and how to use and initialise them. (I do realise that Actuators is not clear as to being "motors, etc")

#include <frc/PWMVictorSPX.h>

frc::PWMVictorSPX motor{0};

Part 2: Setting Motor Speeds

CTRE

From the same Example Java Code as before we are introduced to the set(ControlMode.PercentOutput, speed) class (or not shown for C++: Set(ControlMode::PercentOutput, speed)).

If you look at the examples you also can see that WPI_TalonSRX has a Set(speed) function but this is neither obvious nor mentioned in the documentation clearly. It could also be found by finding the API Documentation.

motor.Set(ControlMode::PercentOutput, speed);
// or if WPI_TalonSRX is found
motor.Set(speed);

WPILib

From the Motor Controller Actuators Page we learn about the Set(speed) command for motors, in both Java & C++.

Neither WPIlib's nor CTRE's Set() commands are difficult to understand, and both of them have easy-to-find-by-api-documentation Set() methods. This does not present much of an issue, but both could do with improvement.

motor.Set(speed);

Part 3: Creating Encoder

CTRE

There is no Encoder class within the CTRE API AFAIK, just extra methods on the motor controller. This isn't evident from the documentation.

WPILib

The sensors page then contains an encoders page, which gives you the declaration for an encoder:

#include <frc/Encoder.h>

frc::Encoder encoder{0, 1};

Part 4: Getting Encoder Values

CTRE

This part is very difficult to locate without prior knowledge of the API. To get values from the encoder, we must find the Sensors page:

From this page we must scroll all the way down through explanations of the config application to a single code example, where one of the only mentions of getSelectedSensorPosition()/getSelectedSensorVelocity() is in the documentation, in an unrelated section.

https://user-images.githubusercontent.com/25718989/104998914-e6f36c80-5a23-11eb-924b-7e91af8f84e2.mp4

There is no documentation as to what exactly these functions return within the RTD docs site as far as I can tell. You can find it with doxygen, kinda.

The Velocity() tells you to see Phoenix-Documentation for how to interpret... though I cannot find anything searching for "100ms" or "raw sensor units".

WPILib

(barring the mistakes in the pages, these should be fixed soon)

The documentation has a simple GetDistance() example: as well as a GetRate() example

The API Documentation is also (more useful) than the documentation: https://first.wpi.edu/wpilib/allwpilib/docs/release/cpp/classfrc_1_1Encoder.html

and GetRate()

Part 5: Scaling to SI Units

CTRE

The CTRE Phoenix Documentation makes no mention of this as far as I can tell via searching.

There is a ConfigSelectedFeedbackCoefficient() mentioned in the API Documentation but none of the GetSelectedSensorPosition() mention that their output changes; they all mention that they still output native units. I believe that the GetSelectedSensor functions do return the scaled values, but I don't actually have a robot to test and haven't exactly managed to figure out simulation support yet.

I only found about of this function through discussion with others. It is not easy to find through documentation or through looking at doxygen, nor is it it named similarly to the one in WPILib.

constexpr double kEncoderDistancePerPulse = 6 /* inch */ * 0.0254 * 3.14 / 4096;
motor.ConfigSelectedFeedbackCoefficient(kEncoderDistancePerPulse);
motor.GetSelectedSensorPosition(); // meters
motor.GetSelectedSensorVelocity(); // metres per 100ms
motor.GetSelectedSensorVelocity() * 10; // meters per second

Also due to the odd time units in Velocity() it is still necessary to multiply by 10 to get sane units, event with ConfigSelectedFeedbackCoefficient working.

WPILib

On the same page as before we see demonstration of the SetDistancePerPulse() function with explanation.

The doxygen documentation for all of the methods, e.g. GetPeriod() all mention that they are scaled by SetDistancePerPulse() which helps a lot.

constexpr double kEncoderDistancePerPulse = 6 /* inch */ * 0.0254 * 3.14 / 4096;
encoder.SetDistancePerPulse(kEncoderDistancePerPulse);
encoder.GetDistance(); // meters
encoder.GetRate(); // meters per second

Part 6: Simulation

CTRE

No documentation (yet?) but there are code examples it seems. There is also the API Documentation for the sim collection:

There is SetQuadratureRawPosition, which mentions

This documentation is odd, mentioning that it "integrates". This seems to be "integrate" as in combine, not mathematical integrate.

The functions are also oddly inconsistent with each other.

• SetQuadratureRawPosition() - why does this one have raw in its name whilst none of the other examples do.
• AddQuadraturePosition() - why do we have add for this but not for velocity, or why do we need add at all if we're working with WPILib's PhysicsSim classes that give us values for position and velocity directly.
• SetQuadratureVelocity()

Furthermore, there a the {Analog,Quadrature,PulseWidth} functions for each in the simulation - this contrasts with the existing API of GetSelectedSensorPosition(). Would it make more sense to have SetSelectedSensorPosition()?

These functions also take integers, despite GetSelectedSensorVelocity returning a double. They do not seem to scale by the ConfigSelectedFeedbackCoefficient - and if they did, the integer arguments make it impossible to feed them useful values. This then requires dividing by kEncoderDistancePerPulse when giving values to these functions.

[sidenote that I'm still looking into, the values returned by GetSelectedSensor*() seem to be rounded)

motor.GetSimCollection().SetQuadratureRawPosition(physicsSim.GetDistance() / kEncoderDistancePerPulse);
motor.GetSimCollection().SetQuadratureVelocity(physicsSim.GetVelocity() / 10 / kEncoderDistancePerPulse);

Note that this is an easy to do footgun (albeit not one that translates to the real world as it only affects the sim GUI), because it is easy to add integer values for distances when these should be taking the raw ticks per 100ms. In addition, the extra *10 or /10 is also easy to forgot and mess up.

WPILib

The WPILib simulation paradigm involves the creation of EncoderSim or XXXSim classes which hook into the XX classes when in simulation mode and change the values of Encoder.GetXX().

There is an example for multiple different types of subsystems within the documentation page, as well as a full tutorial for the Drivetrain.

#include <frc/sim/EncoderSim.h>
frc::sim::EncoderSim encoderSim{encoder};
encoderSim.SetDistance(physicsSim.GetDistance());
encoderSim.SetRate(physicsSim.GetRate());

Conclusion

I hope this can explain part of the reason for why the CTRE API is a bit un-intuitive and hard to figure out, due to the inconsistency within the API as well as inadequate and confusing documentation.

I would love to see the following improvements

• Inconclusions of coded tutorials a la the "Drivetrain Simulation Tutorial" where the ways of doing certain common constructions are explained using the CTRE API rather than WPILib-native motor objects. This should help explain the ways you can do things.
• Inclusion of how you convert "raw sensors units" to physical distances using either the ConfigSelectedFeedbackCoefficient or other methods where necessary, and updating of documentation to explain what functions return value scaled by the coefficients.
• IMO, the CTRE API should provide objects such as Encoders and EncoderSims that match the API provided by WPILib such that it is easy to just replace the type of the variable and it will continue to work throughout the rest of the robot project without need for any changes. This also makes most of the WPILib Documentation relevant for your motor controllers, without the need to copy/adjust, excepting where you have additional features.
  • This might include the addition of separate Encoder classes. I am unaware of any official reasoning aside from speculation behind having the motor controller control the functions of the encoder and other items. If there is a good reason I'd be happy to hear it.

[JCaporuscio]

First, thank you for the issue tracker - this kind of tracker is extremely helpful in understanding where our documentation can be improved.

I want to address a few of your points, and then list some of the changes we're going to make to improve these items. Apologies since it's a bit of a wall of text.

Single parameter set:

This set function only exists to provide compatibility with the WPI Speedcontroller interface (and WPI objects that require it). We can do a better job of documenting this fact, but users not being aware of this function doesn't necessarily concern us since it's not truly intended to be user-facing in our classes.

Encoder Class:

Our API is centered around pairing software objects with individual CAN devices. When we were designing the API there were several paths we could have taken (including the more hardware-abstracted software model like WPI uses). Ultimately we decided on the path that was the most straightforward to grasp for both hardware and software folks - each object is a CAN device, and functions are then things that CAN device can do/read. Users who are more experienced with software and want a more abstract model then have the option to add abstraction interface layers.

Now that's not saying we're not open to adding more abstracted classes - given the rise of simulation and multiple requests from users for this type of abstraction to be built-in, we're evaluating what makes sense and how it should be implemented. For example, we could potentially add an Encoder class to our WPIAPI library - this would also implement any relevant interfaces from WPILib. I'm not going to 100% commit to this now as there are a lot of factors that have to be looked at, but we're open to it.

I do want to look at this statement though:

IMO, the CTRE API should provide objects such as Encoders and EncoderSims that match the API provided by WPILib such that it is easy to just replace the type of the variable and it will continue to work throughout the rest of the robot project without need for any changes.

Keep in mind our libraries need to support multiple use cases, including use cases outside of FRC. While we do look at WPILib's implementations when writing libraries so as to commonize when it makes sense, existing WPILib APIs are not a driving factor for our core design decisions. BUT this is also the reason our WPIAPI exists, so we can add things that are specific to integration with WPILib. So if/when we feel it's important to spend the development effort adding additional API elements to match WPI, this is where it would go.

Unit Scaling:

So Phoenix doesn't have unit scaling currently. This is why you couldn't find documentation with explicit instructions for doing so. We explained in the blog post (but we'll add it to the main docs as well) that the switch to doubles this year was pre-emptive so that when we do release a true scaling feature it doesn't break the API potentially mid-season. This is also why the values you see returned are still in integer intervals.

The existing function for FeedbackCoefficient was created for two primary purposes - averaging a sensor sum and scaling non-intuitive units to be slightly more intuitive (see Pigeon's 8192 per rotation when used as a remote sensor, that we usually scale to 3600 per rotation which is tenths of a degree). There's also a historical limitation (since fixed) where sensors for Aux PID had an upper bound and the coefficient helped avoid that bound. So you'll typically only see the FeedbackCoefficient referenced in documentation that specifically requires it, like our sensor sum or remote sensor section. Additionally, since true unit scaling is not yet released any scaled values will still be integers (this is something we want to correct in the near future).

For this reason, we'll update the documentation to more accurately reflect the state of scaling (as mentioned in the blog), but we likely won't call out the FeedbackCoefficient explicitly (other than where the use cases call for it) except to mention that it's currently a niche function.

As far as scaling during simulation, this is why in the example I created explicit helper functions. https://github.com/CrossTheRoadElec/Phoenix-Examples-Languages/blob/master/Java%20General/DifferentialDrive_Simulation/src/main/java/frc/robot/Robot.java#L183

Sim:

Our SimCollection is a mirror of the existing SensorCollection. It exists to provide software input hooks for everything that would normally be a hardware input - Selected Sensor is a software construct, not a hardware one. This means each independent input needs its own function, just like each hardware input is distinct. There are situations where users have multiple hardware inputs wired to a Talon SRX data port (ie a mag encoder has both pulse-width and quadrature), and the simulated inputs need to allow for this. This is also why the inputs are integers - they're reflecting hardware input values.

Additionally, the functions provided are intended to address a variety of use cases. Even within FRC, WPILib's physics simulators are not the only way to do things. The Add function exists because we wanted to support the use case of a simulator generating relative sensor deltas.

This documentation is odd, mentioning that it "integrates". This seems to be "integrate" as in combine, not mathematical integrate.

Nope, it's a mathematical integration. There's a unique challenge with relative sensors - users may call to change the position of the relative sensor in robot code (such as setting position to 0 when homing a mechanism), but your simulator likely doesn't have knowledge of this (especially if it's communicating via websockets). We elected to solve this entirely on the robot code side, so the change in "raw" position is mathematically integrated into the true position. This allows the true position to be reset and changed arbitrarily in "normal" (non-sim) robot code and not be overwritten by the sim input when running simulation. Fun fact: this is the same thing that hardware quadrature implementations do.

Documentation Changes

All of the above being said, we definitely think there are changes we can make both to help improve the accessability issues you brought up and to codify some of my explanations above so that others don't run into the same questions/confusion you did. Major doc improvements have been on our radar for some time, but it's obvious with the advent of simulation and the structure of the season that they're more critical than ever.

What we're going to do:

Restructure:

We're going to break up our doc pages into major sections and add a new major section specifically for getting started with Phoenix software, independent of our hardware bring-ups. This should help address questions on how our API works vs. WPILib's, TalonSRX vs WPI_TalonSRX, common calls such as setting output and reading sensor values, and how to use the simulation API. This likely won't be an extremely long section but it should address all the questions of getting up and running with a project and basic functionality (including sim).

This is where we'll include code tutorials like the ones you mentioned - they'll likely start fairly small and basic and then we'll add to them as it makes sense. Keep in mind that while there may be similarities they likely won't be identical to WPILib's.

Integrating blog and backround info

We're going to add (in the new section described above and/or where it makes sense in existing docs) information callouts that may only exist in blog posts or tribal knowledge currently, such as the fact that there is no unit scaling yet or that the single parameter set exists only in WPI_ classes (and why).

We're going to prioritize these changes for this week.

[modelmat]

Keep in mind our libraries need to support multiple use cases, including use cases outside of FRC. While we do look at WPILib's implementations when writing libraries so as to commonize when it makes sense, existing WPILib APIs are not a driving factor for our core design decisions. BUT this is also the reason our WPIAPI exists, so we can add things that are specific to integration with WPILib. So if/when we feel it's important to spend the development effort adding additional API elements to match WPI, this is where it would go.

I'd like to respectfully disagree w/r/t the importance of matching APIs. Having the Phoenix API match that of WPILib allows for both easier integration with existing WPILib helper classes (as already evidenced by the addition of Set() for DifferentialDrive/SpeedController) as well as conforming to existing expectations of how you can do certain things which assists when writing code (e.g. the expectation of GetDistance() rather than GetSelectedSensorPosition()). I can agree for not modifying an existing API used by other customers, but in WPI-* specific subclasses the addition of aliased names can assist greatly when working within the framework of WPILib for the rest of the robot.

Additionally, since true unit scaling is not yet released any scaled values will still be integers (this is something we want to correct in the near future).

Okay, thanks.

As far as scaling during simulation, this is why in the example I created explicit helper functions. https://github.com/CrossTheRoadElec/Phoenix-Examples-Languages/blob/master/Java%20General/DifferentialDrive_Simulation/src/main/java/frc/robot/Robot.java#L183

Ah... that example doesn't exist for C++. That would probably have helped me understand :)

Nope, it's a mathematical integration. There's a unique challenge with relative sensors - users may call to change the position of the relative sensor in robot code (such as setting position to 0 when homing a mechanism), but your simulator likely doesn't have knowledge of this (especially if it's communicating via websockets). We elected to solve this entirely on the robot code side, so the change in "raw" position is mathematically integrated into the true position. This allows the true position to be reset and changed arbitrarily in "normal" (non-sim) robot code and not be overwritten by the sim input when running simulation. Fun fact: this is the same thing that hardware quadrature implementations do.

See this discussion on Discord I started because I wasn't sure if I was just confused and didn't really want to derail this GitHub issue.

We're going to prioritize these changes for this week.

Okay, that's great :)

P.S.

Apologies since it's a bit of a wall of text.

... did you see how much I had written :) I should be apologising.

[JCaporuscio]

Wanted to give a brief update - this is still being worked on. I don't have an exact status update or ETA but I didn't want folks to feel like this is just being ignored.

The changes may be larger than we originally targeted and major structural changes will take a bit (and depend on some other development items).

In the meantime if anyone has additional doc suggestions feel free to add them to this issue tracker so they're all in one place. Of particular help will be any specific points of confusion or items that are hard to find.

[ghost]

The biggest point of confusion for me has always been why motor.set(speed) is only in WPI_*, not the regular classes. I don't see much of a reason why not