Correction for Issue #3296 - ble hanging on nrf52840 #3375
Conversation
update from main
update from main
Update from main
Pull from adafruit main
Merge from adafruit main
Update from adafruit/main
|
Excellent catch. I'm only not reviewing as "approved" so that we can talk about whether the check for 'no pin' could be moved down into the lower level routine (so that it's part of the API that it may be called with NO_PIN). This would mean that just 1 check would have to be added instead of 4, not to mention other sites that might be affected (unidirectional UART for instance) would automatically become fixed. |
|
Looking at that possibility, I see that reset_pin_number() in common-hal/microcontroller/Pin.c already checks for NO_PIN, so we would only need to add that check to never_reset_pin_number(). If you think that is a better way to deal with the issue let me know and I'll make the necessary changes. |
|
I am hoping @tannewt will have an opinion, but he's not working today. |
That looks like a good idea, but I'd also check to see if should be done on all the other ports. |
There was a problem hiding this comment.
While there's one bit that it wouldn't hurt for @hierophect to take a look at, I like this change. I looked and didn't find any more places calling reset_pin_number or never_reset_pin_number and didn't see any more to simplify.
| void reset_pin_number(uint8_t pin_port, uint8_t pin_number) { | ||
| if ( pin_number == NO_PIN ) { | ||
| return; | ||
| } | ||
|
|
||
| if (pin_port == 0x0F) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@hierophect can you take a look at the stm parts of this PR please? (I suspect that the check of pin_port check is the stm equivalent of NO_PIN, except there's an entire 'no port')
There was a problem hiding this comment.
I do not think that this stm port change is needed. The current code does not use NO_PIN . It is coded to use NULL. NULL is checked before each call to a function that would use it .
There was a problem hiding this comment.
I'd like to see this in the STM port. It looks like there are places that do not check for a valid pin object: https://github.com/adafruit/circuitpython/blob/main/ports/stm/common-hal/microcontroller/Pin.c#L100
In fact, we should probably make this more resilient by modeling after SAMD which checks validity rather that for the special value.
There was a problem hiding this comment.
Sorry I'm late to the party here. If I'm understanding this correctly @tannewt you'd rather have functions like common_hal_reset_pin check whether the incoming pin object is NULL? Rather than having the special values?
The main thing the non-object API is used for (port and number) is iterating resets - iterating through the pin objects for a microcontroller is kind of annoying, and doing it with the STM32 port and number values is easier. That said, we have done pin iteration a couple times now, so potentially the entire secondary non-object API could be removed and replaced with an object-only one that checks for pointer validity exclusively.
There was a problem hiding this comment.
@hierophect Any pin API that takes a pin pointer should check for NULL.
Any API that takes port and pin numbers should validate them. Only checking for a special value risks not catching other invalid values. When we need to set an invalid value a special value is appropriate.
In common_hal_busio_spi_never_reset() and common_hal_busio_spi_deinit() no check was being made to see if the values of self->MOSI_pin_number and self->MISO_pin_number were valid. Since, if those pins were unused they have a value of NO_PIN(0xff), the result was that values in memory past the never_reset_pins array were getting stepped on. In particular, the nrf_nvic_state.__cr_flag was being set to a non-zero value, which kept the ble IRQ from being enabled. This fix adds tests for NO_PIN before setting or resetting values.
There is also an unrelated fix, found while debugging, for an apparent typo in common_hal_busio_spi_construct() which was setting self->MISO_pin_number to mosi->number instead of miso->number.