rp2/rp2_pio: Validate state machine frequency in constructor. #7033
Conversation
| } else if (args[ARG_freq].u_int == 0) { | ||
| div = 0; | ||
| // Special case of 0: set clkdiv to 0. |
There was a problem hiding this comment.
Do the MicroPython PIO docs (if they exist yet) mention these special-cases of freq < 0 and freq == 0 ?
There was a problem hiding this comment.
freq < 0 is just the default case (if freq is not specified in the constructor it defaults to -1) and not necessarily something the user needs to know about (but they could pass in -1 if they wanted...).
freq == 0 is not documented.
| // Frequency given in Hz, compute clkdiv from it. | ||
| uint64_t div = (uint64_t)clock_get_hz(clk_sys) * 256ULL / (uint64_t)args[ARG_freq].u_int; | ||
| if (!(div >= 1 * 256 && div <= 65535 * 256)) { | ||
| mp_raise_ValueError(MP_ERROR_TEXT("freq out of range")); |
There was a problem hiding this comment.
Would it be helpful for this error message to give the min and max allowed SM freq for the current CPU frequency? 🤷
There was a problem hiding this comment.
It's a bit of work to do that, but could be useful (at least saves people reading the datasheet...!).
But since the bounds would be something like 1907.349 to 125e6, it makes me think that the freq argument should probably accept floats...
There was a problem hiding this comment.
Yeah, I guess there's no reason not to expose non-integer frequencies.
There was a problem hiding this comment.
In the interests of getting this fix in, let's leave it for another PR to expose non-integer frequencies.
Fixes issue micropython#7025. Signed-off-by: Damien George <damien@micropython.org>
b78c66e to
2c9af1c
Compare
Fixes issue #7025.