search

CrossTheRoadElec / Phoenix-api

CTRE Phoenix language API (targets FIRST Robotics Competition, Linux, RaspPi, Windows)
Other
39 stars 28 forks source link

selectedFeedbackCoefficient behavior not fully documented / unideal #72

Closed ItayZiv closed 3 years ago

ItayZiv commented 4 years ago

The behavior of the feedback coefficient is (as far as I can gather), that it rounds the values. This is both not fully documented and not ideal.

It would be much more beneficial if instead a coefficient could be set, so the units the user sees is something like a meter/foot, with the current system, this would result if very inaccurate control of a mechanism, because of the big units. If it would work more like WPILib's encoder's setDistancePerPulse where it doesn't round said values. This is especially important because it comes very useful in Phoenix tuner if you can look at the actual values and not have to do the division manually.

Another thing that would be nice, is if we could set a separate value for velocity, since currently if we want to get in anything other than units per 100ms, you have to either divide in code or set the coefficient to a different value, which makes the position coefficient inaccurate.

It would be very simple to test this feature, not sure about implementation.

JCaporuscio commented 4 years ago

There's actually two separate points to what you're discussing.

First, the feedback coefficient does not round. https://www.ctr-electronics.com/downloads/api/cpp/html/classctre_1_1phoenix_1_1motorcontrol_1_1can_1_1_base_motor_controller.html#ad35fc9302f7770fcd407c314bb7f1475 Per the API docs, maximum value is 1, and resolution is in increments of 1/(2^16). If you enter a value that doesn't match the resolution, the motor controller has to use the closest valid value.

Having a system that allows you to scale your units (and time base) is something we've had on the docket for a while. For it to work intuitively it's a bit more complex than just using FeedbackCoefficient. We have a version of these changes in house - we were hoping to release them for the 2020 season but for various reasons were unable to do so. It will likely be one of the early off-season releases this summer.

ItayZiv commented 4 years ago

Yes, I understand that it doesn't round the coefficient, but the position/velocity is rounded (or I assume so since it is returned as a int?).

Having an option to scale position/velocity (the way it named really doesn't matter), would be amazing, especially for tuning stuff with phoenix tuner.

This probably could be 2 different issues, but I thought they were quite related.

To be clear, would this new feature include scaling for velocity separately from position? As this can be very useful to set units for a drivetrain for example to meters and meters per second (Which also would let it interface very well with WPILibs various features)

JCaporuscio commented 4 years ago

Ah, I understand what you mean by rounding now. It's not really rounding, it's that all position and velocity data is an integer, period. This would change as part of the unit scaling changes (which is one reason we couldn't release it mid-season, it's a breaking API change).

To elaborate, there are two things that can be scaled in the new feature - the sensor data itself (ie position) and the timebase of the sensor data (so per second, per minute, etc.)

ItayZiv commented 4 years ago

Yeah that’s what I thought, anyways that sounds very cool. Can’t wait to try it out!

JCaporuscio commented 3 years ago

Closing since issue is old