Add 60k RPM support to voxl-esc driver #49
Conversation
|
|
||
| // Limit RPMs to prevent overflow when converting to int16_t | ||
| if (rpm0 > UINT16_MAX) { rpm0 = UINT16_MAX-5; } if (rpm1 > UINT16_MAX) { rpm1 = UINT16_MAX-5; } | ||
| if (rpm2 > UINT16_MAX) { rpm2 = UINT16_MAX-5; } if (rpm3 > UINT16_MAX) { rpm3 = UINT16_MAX-5; } |
There was a problem hiding this comment.
This is confusing to me. The comment says it is trying to prevent overflow of int16_t. So why would UINT16_MAX-5 accomplish that?
There was a problem hiding this comment.
When using extended range rpm command we divide the value passed in by 2 before sending to the ESC. Requested rpms passed in can theoretically be larger than UINT16_MAX since they are int32_t (RPMs can be negative), which would mean that it's possible when we divide the value for extended range packet that it is still larger than UINT16_MAX, which would cause overflow when assigning the value to the data array (int16_t). I.e. if a value of 2,000,000 is passed in, the divided value is 1,000,000 which is still above UINT16_MAX but when assigned to the int16_t data variable it would be truncated to 16960.
After thinking about it more, I think it might be a good idea to add some range limiting to the negative end of the range as well as maybe limit the standard range too since the input is int32_t... or we could leave it up to the user to input the correct values... thoughts?
There was a problem hiding this comment.
Let's let Alex submit his comments to see what his ideas are.
There was a problem hiding this comment.
From Alex:
in the limit values for regular (not extended rpm) the limits should be +-/32k (2^15), not UINT16_MAX . For the extended rpm range, the limit will be +/- 65K (but lets use slightly smaller limit as you already started doing)
I updated this in my latest commit (3dd7d21). I added max and min for both the standard and extended range... -32k to 32k and -65k to 65k, respectively
| @@ -1038,6 +1063,12 @@ bool VoxlEsc::updateOutputs(bool stop_motors, uint16_t outputs[MAX_ACTUATORS], | |||
| _esc_chans[i].rate_req = 0; | |||
|
|
|||
| } else { | |||
| if (_extended_rpm) { | |||
| if (outputs[i] > 65530) outputs[i] = 65530; | |||
There was a problem hiding this comment.
Perhaps a constant definition of 65530 and 32766 with some comments about why those values were chosen
|
From Alex (referring to qc_esc_packet.c line 154) : I updated this in the latest commit (3dd7d21) and removed the double and int16_t casts |
Changes
Testing