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.

Sign up for GitHub

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

Correction for Issue #3296 - ble hanging on nrf52840 #3375

Merged
merged 8 commits into from
Sep 10, 2020
Merged

Correction for Issue #3296 - ble hanging on nrf52840 #3375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eac639a
Merge pull request #16 from adafruit/main
DavePutz Jul 25, 2020
a44c09e
Merge pull request #17 from adafruit/main
DavePutz Aug 1, 2020
1f75953
Merge pull request #19 from adafruit/main
DavePutz Aug 3, 2020
0311c34
Merge pull request #21 from adafruit/main
DavePutz Aug 18, 2020
43aef8a
Merge pull request #22 from adafruit/main
DavePutz Aug 20, 2020
6357093
Merge pull request #23 from adafruit/main
DavePutz Aug 30, 2020
ed6cc64
Corrected issue with BLE not working on nrf52840
siehputz Sep 4, 2020
2690fae
Moved checks for invalid pin to common-hal/microcontroller/Pin.c
siehputz Sep 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ports/atmel-samd/common-hal/microcontroller/Pin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ void reset_all_pins(void) {
}

void never_reset_pin_number(uint8_t pin_number) {
if (pin_number >= PORT_BITS) {
return;
}

never_reset_pins[GPIO_PORT(pin_number)] |= 1 << GPIO_PIN(pin_number);
}

Expand Down
6 changes: 6 additions & 0 deletions ports/esp32s2/common-hal/microcontroller/Pin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ bool apa102_mosi_in_use;
bool apa102_sck_in_use;

void never_reset_pin_number(gpio_num_t pin_number) {
if (pin_number == -1 ) {
return;
}
never_reset_pins[pin_number / 32] |= 1 << pin_number % 32;
}

Expand All @@ -54,6 +57,9 @@ void common_hal_never_reset_pin(const mcu_pin_obj_t* pin) {

// Mark pin as free and return it to a quiescent state.
void reset_pin_number(gpio_num_t pin_number) {
if (pin_number == -1 ) {
return;
}
never_reset_pins[pin_number / 32] &= ~(1 << pin_number % 32);
in_use[pin_number / 32] &= ~(1 << pin_number % 32);

Expand Down
2 changes: 1 addition & 1 deletion ports/nrf/common-hal/busio/SPI.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self, const mcu_pin_obj_t *

if (miso != NULL) {
config.miso_pin = miso->number;
self->MISO_pin_number = mosi->number;
self->MISO_pin_number = miso->number;
claim_pin(miso);
} else {
self->MISO_pin_number = NO_PIN;
Expand Down
3 changes: 3 additions & 0 deletions ports/nrf/common-hal/microcontroller/Pin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ void reset_pin_number(uint8_t pin_number) {


void never_reset_pin_number(uint8_t pin_number) {
if (pin_number == NO_PIN) {
return;
}
never_reset_pins[nrf_pin_port(pin_number)] |= 1 << nrf_relative_pin_number(pin_number);
}

Expand Down
7 changes: 7 additions & 0 deletions ports/stm/common-hal/microcontroller/Pin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ void reset_all_pins(void) {

// Mark pin as free and return it to a quiescent state.
void reset_pin_number(uint8_t pin_port, uint8_t pin_number) {
if ( pin_number == NO_PIN ) {
return;
}

if (pin_port == 0x0F) {
return;
}
Comment on lines 72 to 79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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')

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Expand All @@ -88,6 +92,9 @@ void reset_pin_number(uint8_t pin_port, uint8_t pin_number) {
}

void never_reset_pin_number(uint8_t pin_port, uint8_t pin_number) {
if ( pin_number == NO_PIN ) {
return;
}
never_reset_pins[pin_port] |= 1<<pin_number;
// Make sure never reset pins are also always claimed
claimed_pins[pin_port] |= 1<<pin_number;
Expand Down