Skip to content
This repository has been archived by the owner on Oct 27, 2020. It is now read-only.

REV Through Bore Encoder can easily overflow ExH firmware velocity counter #241

Open
Windwoes opened this issue Feb 26, 2020 · 4 comments
Open

Comments

@Windwoes
Copy link
Member

Windwoes commented Feb 26, 2020

According to REV's website, the Through Bore Encoder has:

Quadrature Resolution: 2048 Cycles per Revolution (8192 Counts per Revolution)

But, the Expansion Hub firmware stores velocity as 16-bit signed integers:

int16_t     motor0velocity_cps;  // counts per second
int16_t     motor1velocity_cps;
int16_t     motor2velocity_cps;
int16_t     motor3velocity_cps;

The maximum value for a signed 16-bit integer is +-32,768.

If we do 32768 / 8192 we find that the REV Through Bore encoder will overflow the velocity counter at 4RPS, which is only 240RPM!

The good news is that the position is stored as a 32-bit signed int, which isn't going to overflow in a 2:30 match.

@willtoth
Copy link

One note here is this only impacts the communication between the hub and the phone, the internal control uses 32-bit velocity

@guberti
Copy link

guberti commented Mar 3, 2020

This has been a major problem for team 8802 as well - because the REV encoders overflow 16-bit signed integers, we can't tune the built-in PID to work with REV encoders as inputs without some really gross hacks.

@Windwoes
Copy link
Member Author

Windwoes commented Mar 3, 2020

@guberti that seems to go against what @willtoth said. His comment seems to indicate that the built in PID would be fine, it would be a custom PID that would cause issues.

@NoahBres
Copy link

NoahBres commented Mar 4, 2020

The issue Gavin is referring to is that to tune the built in PID, you graph the actual velocity vs target velocity using getVelocity(). However, getVelocity() seems to be the issue here as it’s part of the communication between rev hub to phone.
Only way to get around this is to calculate velocity yourself but that isn’t totally reliable as the packets aren’t time stamped. It should be good enough but having getVelocity() work would be a lot nicer.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants