8000 extmod/modbluetooth: Addressing modes, Unix H4 UART, reliability fixes by jimmo · Pull Request #6343 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

extmod/modbluetooth: Addressing modes, Unix H4 UART, reliability fixes #6343

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

Closed
wants to merge 22 commits into from

Conversation

jimmo
Copy link
Member
@jimmo jimmo commented Aug 17, 2020

Work done in collaboration with @andrewleech

This started as just an implementation of NimBLE on Unix but ended up with lots of dependent changes and bugs fixed along the way. Ideally this would have been multiple PRs but I've tried to split each part into distinct commits. Other than the binding refactor (which is unfortunately very difficult to split), each commit is quite small.

Bug fixes:

  • Fix handling of duration_ms on gap_scan
  • Fix UART RX IDLE handler, improves latency on handling incoming HCI UART data.
  • Implement the NimBLE mutex
  • Fix race between the READ_REQUEST "hard" IRQ and the other scheduler-invoked IRQs
  • Made it possible to use active scan mode.
  • Writing to a characteristic (as server) now notifies subscribed clients.
  • STM32 HCI UART time 8000 out was too low (leading to occasional TX failures leading to loss of HCI sync)

Code cleanup:

  • Refactoring to simplify port/driver/stack/binding interfaces, improving the startup and shutdown process.
  • Make debugging/logging macros consistent and clearer across the BLE-related files

Unix port:

  • Allow nimble_malloc to work on Unix
  • Implement support for H4 UART (typically via USB UART adaptor), via btstack and NimBLE
    This allows (for example) a Zephyr HCI controller running on an nRF to be used from the Unix port.
  • Extract out common code from the libusb mode and the stm32 port.

Addressing:

  • Clarify behavior for obtaining an identity address (use controller's public address and then fallback to static address)
  • On btstack, allow using a Zephyr controller's static address (generated from the FICR register on nRF)
  • On btstack+nimble, allow using mp_hal_get_mac as as a static address.
  • Implement a Python API for switching between public and static address modes (and preliminary support for RPA/NRPA)
  • Make querying the MAC address return the currently used address.
  • Implement preliminary support for MAC rotation (when using RPA/NRPA)

This is tested using the multitests on:

  • PYBD btstack
  • PYBD nimble
  • Unix USB
  • Unix btstack
  • Unix nimble
  • ESP32 nimble

More testing required on

  • WB55 bstack
  • WB55 nimble
    (I'm working on those right now)

TODO:

  • Notify subscribed clients for btstack.
  • Add multitests for address modes and subscriptions.

jimmo and others added 19 commits August 14, 2020 15:48
This will allow the HCI UART to use a non-heap mp_irq_obj_t, which avoids needing to make a root pointer for it.

Misc cleanups to mpirq.c to remove unused functionality.
Also use correct format specifier for %zu.

Always prefix with "btstack:" or "nimble:".
Previously the interaction between the different parts was different on each port and each stack. This commit defines common interfaces between them and implements them for cyw43, btstack, nimble, stm32, unix.
Generally a controller should either have its own public address hardcoded, or
loaded by the driver (e.g. cywbt43).

However, for a controller that has no public address where you still want a
long-term stable address, this allows you to use a static address generated by
the port. Typically on STM32 this will be an LAA, but a board might override
this.
2ms used previously was not long enough and the result is that it loses HCI
sync.

Also print error on tx failure to make this more obvious in the future.
int flags = O_RDWR | O_NOCTTY | O_NONBLOCK;
uart_fd = open(uart_device_name, flags);
if (uart_fd == -1) {
DEBUG_printf("Unable to open port %s", uart_device_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better as an exception that's then user visible - currently if the port is missing or cannot be opened it's a silent fail here and the rest of the startup just looks broken

Copy link
Member Author

Choose a reason for hiding this comment

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

Done -- and added the plumbing to make this work for btstack and nimble. On btstack it can detect the init failure, on nimble it now leads to an init timeout.

uart_tx_data(&mp_bluetooth_hci_uart_obj, (void *)buf, len, &errcode);
if (errcode != 0) {
printf("\nmp_bluetooth_hci_uart_write: failed to write to UART %d\n", errcode);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In my branch I've changed this if to assert(errcode == 0);

My updated commit is here, it includes your comment and removes the extra comment/printf below:
andrewleech@32bf807

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is too important to be a debug-only notification...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, I forgot that asserts are disabled in non-debug builds... yep perhaps a static flag that can be checked at a higher layer will be needed, perhaps something that can then trigger a stack/hci reset.

MP and NimBLE must be on the same core (for synchronisation of the BLE ringbuf and the MP scheduler).

However, in the current IDF versions (3.3 and 4.0) there are issues (e.g. micropython#6343) with running NimBLE on core 1.

This change makes it possible to reliably run the BLE multitests.
@jimmo jimmo force-pushed the modbluetooth-1.13-fixes branch from 4a6a5a8 to 6546f70 Compare August 19, 2020 00:48
This allows `ble.active(1)` to fail correctly if the HCI controller is
unavailable.

Also avoids an infine loop in the NimBLE event handler where NimBLE doesn't
correctly detect that the HCI controller is unavailable and keeps trying to
reset.

Also fixes an issue where GATT service registrations were left allocated,
which led to a bad realloc if the stack was activated multiple times.
@dpgeorge
Copy link
Member

The following commits from this PR have been applied to master (they are minor and fix existing bugs) (the hash refers to the commit in master):

  • 3d9a7ed extmod/btstack: Implement GAP scan duration_ms parameter.
  • 0bc2c1c extmod/modbluetooth: Fix race between READ_REQUEST and other IRQs.
  • a80a146 extmod/bluetooth: Support active scanning in BLE.gap_scan().

jimmo added a commit to jimmo/micropython that referenced this pull request Sep 1, 2020
MP and NimBLE must be on the same core (for synchronisation of the BLE ringbuf and the MP scheduler).

However, in the current IDF versions (3.3 and 4.0) there are issues (e.g. micropython#6343) with running NimBLE on core 1.

This change makes it possible to reliably run the BLE multitests.
dpgeorge pushed a commit to dpgeorge/micropython that referenced this pull request Sep 4, 2020
MP and NimBLE must be on the same core (for synchronisation of the BLE ringbuf and the MP scheduler).

However, in the current IDF versions (3.3 and 4.0) there are issues (e.g. micropython#6343) with running NimBLE on core 1.

This change makes it possible to reliably run the BLE multitests.
@dpgeorge
Copy link
Member
dpgeorge commented Sep 4, 2020

Superseded by #6405 (with some notes there about what was not included).

@dpgeorge dpgeorge closed this Sep 4, 2020
jimmo added a commit to jimmo/micropython that referenced this pull request Sep 4, 2020
MP and NimBLE must be on the same core (for synchronisation of the BLE ringbuf and the MP schedul
5D39
er).

However, in the current IDF versions (3.3 and 4.0) there are issues (e.g. micropython#6343) with running NimBLE on core 1.

This change makes it possible to reliably run the BLE multitests.
jimmo added a commit to jimmo/micropython that referenced this pull request Sep 7, 2020
MP and NimBLE must be on the same core (for synchronisation of the BLE ringbuf and the MP scheduler).

However, in the current IDF versions (3.3 and 4.0) there are issues (e.g. micropython#6343) with running NimBLE on core 1.

This change makes it possible to reliably run the BLE multitests.
@dpgeorge
Copy link
Member
dpgeorge commented Sep 8, 2020

Changes here were merged (somewhat modified) through #6405, see 2310998 through 126f972

@dpgeorge
Copy link
Member
dpgeorge commented Sep 8, 2020

Thanks @jimmo and @andrewleech for all the effort!

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request May 12, 2022
add board.LED to Metro M4 AirLift LIte and PyRuler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0