Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
microbit-carlos opened this issue Nov 9, 2022 · 2 comments

Comments

@microbit-carlos
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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

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

No branches or pull requests

3 participants