microbit-foundation / micropython-microbit-v2

Temporary home for MicroPython for micro:bit v2 as we stablise it before pushing upstream
MIT License
44 stars 25 forks source link

`pin.set_analog_period()` out of range values do not throw exception. #142

Open microbit-carlos opened 1 year ago

microbit-carlos commented 1 year ago

In V1 pin0.set_analog_period_microseconds(255) and pin0.set_analog_period(1001) throw ValueError, but in V2 the values are instead not updated (they are not clamped, so the set method silently fails to update the value).

V1:

>>> pin0.set_analog_period_microseconds(255)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid period
>>> pin0.set_analog_period(1001)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid period

V2:

>>> pin16.set_analog_period(262)
>>> pin16.get_analog_period_microseconds()
262000
>>> pin16.set_analog_period(263)
>>> pin16.get_analog_period_microseconds()
262000
>>> pin16.set_analog_period(-1)
>>> pin16.get_analog_period_microseconds()
262000

From manual testing the min value for V2 is 0 and max value is 262 ms.

dpgeorge commented 1 year ago

Not sure if CODAL should propagate out an error from NRF52PWM::setPeriodUs if the period cannot be set, or if MicroPython bindings should check the range (from empirically determined min/max values).

martinwork commented 1 year ago

If this is the right place, it looks like the V1 range checking is done in micropython code. https://github.com/bbcmicrobit/micropython/blob/master/source/microbit/microbitpin.cpp#L163 https://github.com/bbcmicrobit/micropython/blob/master/source/lib/pwm.c#L195

V2 micropython looks for an error value -1, which would occur if CODAL setAnalogPeriodUs returned an error. https://github.com/microbit-foundation/micropython-microbit-v2/blob/master/src/codal_port/microbit_pin.c#L141 https://github.com/microbit-foundation/micropython-microbit-v2/blob/master/src/codal_app/microbithal.cpp#L148

But, setAnalogPeriodUs only returns an error if the pin is not an analogue output, and microbit_hal_pin_set_analog_period_us ensures that it is. https://github.com/lancaster-university/codal-nrf52/blob/master/source/NRF52Pin.cpp#L638

Out of range values do create an error return from NRF52PWM::setPeriodUs, and don't affect the getAnalogPeriodUs() value. https://github.com/lancaster-university/codal-nrf52/blob/master/source/NRF52PWM.cpp#L133

CODAL setAnalogPeriodUs could check the return value from setPeriodUs and pass the error back, but I guess a change like that could create problems for other existing code.

CODAL and DAL match, in that both only return an error from setAnalogPeriodUs if the pin is not an analogue output, but the DAL stores and returns any value it is sent, and out of range values do not cause an error return anywhere. https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitPin.cpp#L415 https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/DynamicPwm.cpp#L172 https://github.com/lancaster-university/mbed-classic/blob/master/targets/hal/TARGET_NORDIC/TARGET_MCU_NRF51822/pwmout_api.c#L266