Make all float->int/frac conversions round to nearest by default #2065
Conversation
…nearest (which is a backwards incompatible change, but I think OK), and add ability to turn it off
|
All looks sensible, and |
| @@ -155,6 +160,9 @@ static inline void pwm_config_set_phase_correct(pwm_config *c, bool phase_correc | |||
| */ | |||
| static inline void pwm_config_set_clkdiv(pwm_config *c, float div) { | |||
| valid_params_if(HARDWARE_PWM, div >= 1.f && div < 256.f); | |||
| #if PICO_PWM_CLKDIV_ROUND_NEAREST | |||
| div += 0.5f / (1 << PWM_CH0_DIV_INT_LSB); // round to the nearest fraction | |||
There was a problem hiding this comment.
Sorry - why are we using PWM_CH0_DIV_INT_LSB here rather than PWM_CH0_DIV_FRAC_MSB+1 - PWM_CH0_DIV_FRAC_LSB. Isn't it coincidence that it's the right value? It confuses me because there are 8 bits for the pwm int.
There was a problem hiding this comment.
That's (almost) exactly what I asked earlier - see my resolved comments above.
There was a problem hiding this comment.
i used what was used in the code in the rest of the function... feel free to commit a new change to this PR to fix all of them to be more sensible
There was a problem hiding this comment.
Thanks, but see that the original code still uses the "wrong" values, we should fix all.
Perhaps worth adding a "const int frac_bit_count = (BLAH);" to each function
There was a problem hiding this comment.
wish I hadn't asked!
…oping that INT_LSB is in the right place
|
Latest commit just pushed. Let me know if it goes "too far" ( 🫣 ) and I'll revert it and push a simpler version without the |
|
nice; i think I'd prefer REG_FIELD_WIDTH() and as such it probably belongs in |
…k-dividers more consistent
lurch
force-pushed
the
clkdiv_rounding
branch
from
87255fb to
4b84880
Compare
Done! 👍 |
Under control of specific vars, but defaulted by
PICO_CLKDIV_ROUND_NEARESTwhich now defaults to1This fixes #1931 which was PIO specific, but it seemed sensible to be consistent across all APIs