extmod/modbluetooth: Addressing modes, Unix H4 UART, reliability fixes (v2) #6405
Conversation
|
Have you got any info/idea of what's wrong with the "notify subscribed connections" change? |
The commit itself is not necessarily wrong. It's that it makes the |
|
Ah yes, I see what you mean! |
| endif | ||
|
|
||
| ifeq ($(MICROPY_BLUETOOTH_BTSTACK),1) | ||
| MICROPY_BLUETOOTH_BTSTACK_H4 ?= 1 |
| @@ -38,6 +38,18 @@ CFLAGS += $(shell pkg-config libusb-1.0 --cflags) | |||
| LDFLAGS += $(shell pkg-config libusb-1.0 --libs) | |||
| endif | |||
|
|
|||
| ifeq ($(MICROPY_BLUETOOTH_BTSTACK_H4),1) | |||
There was a problem hiding this comment.
does this H4 stuff belong in this commit?
| if (mp_bluetooth_btstack_state == MP_BLUETOOTH_BTSTACK_STATE_STARTING || mp_bluetooth_btstack_state == MP_BLUETOOTH_BTSTACK_STATE_ACTIVE || mp_bluetooth_btstack_state == MP_BLUETOOTH_BTSTACK_STATE_HALTING) { | ||
| // Pretend like we're running in IRQ context (i.e. other things can't be running at the same time). | ||
| mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION(); | ||
| #if MICROPY_BLUETOOTH_BTSTACK_H4 |
There was a problem hiding this comment.
does this belong in this commit?
ports/unix/mpbtstackport_common.c
Outdated
| // Pretend like we're running in IRQ context (i.e. other things can't be running at the same time). | ||
| mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION(); | ||
| #if MICROPY_BLUETOOTH_BTSTACK_H4 | ||
| void mp_bluetooth_hci_poll_h4(void); |
There was a problem hiding this comment.
this decl can be removed
ports/unix/mpbtstackport_common.c
Outdated
| // hci_dump_open(NULL, HCI_DUMP_STDOUT); | ||
|
|
||
| #if MICROPY_BLUETOOTH_BTSTACK_USB | ||
| void mp_bluetooth_btstack_port_init_usb(void); |
There was a problem hiding this comment.
this decl can be removed
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.
So that the IRQ handler does not need to be traced by the GC.
And prefix the debug message with "btstack:" or "nimble:", depending on the context. Also use correct format specifier for %zu.
e51ef91 to
b911293
Compare
Previously the interaction between the different layers of the Bluetooth stack was different on each port and each stack. This commit defines common interfaces between them and implements them for cyw43, btstack, nimble, stm32, unix.
b911293 to
8a70bf0
Compare
This commit adds support for using Bluetooth on the unix port via a H4 serial interface (distinct from a USB dongle), with both BTstack and NimBLE Bluetooth stacks. Note that MICROPY_PY_BLUETOOTH is now disabled for the coverage variant. Prior to this commit Bluetooth was anyway not being built on Travis because libusb was not detected. But now that bluetooth works in H4 mode it will be built, and will lead to a large decrease in coverage because Bluetooth tests cannot be run on Travis.
Changes `BLE.config('mac')` to return a tuple (addr_mode, addr).
Adds `BLE.config(addr_mode=...)` to set the addressing mode.
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.
The 2ms used previously was not long enough and it could lose HCI sync. Also print error on tx failure to make this more obvious in the future.
MicroPython and NimBLE must be on the same core, for synchronisation of the BLE ringbuf and the MicroPython scheduler. However, in the current IDF versions (3.3 and 4.0) there are issues (see e.g. micropython#5489) with running NimBLE on core 1. This change - pinning both tasks to core 0 - makes it possible to reliably run the BLE multitests on esp32 boards.
This allows `ble.active(1)` to fail correctly if the HCI controller is unavailable. It 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. Furthermore, it fixes an issue where GATT service registrations were left allocated, which led to a bad realloc if the stack was activated multiple times.
1d49c67 to
126f972
Compare
|
The issue with cores on the esp32 comes down to MICROPY_BEGIN_ATOMIC_SECTION being aliases to portENTER_CRITICAL_NESTED which is not multi-core safe. I can't find info on this: does MICROPY_BEGIN_ATOMIC_SECTION have to support nesting? Because if not, MICROPY_PY_BLUETOOTH_ENTER could use portENTER_CRITICAL, which is multi-core safe. |
|
@tve I'd like to fix this (and really get to the bottom of the threading/multicore stuff on ESP32) but haven't had a chance to really get into the details.
I've noticed this previously, and wondered why this isn't a problem more generally (outside BLE). I've never looked into the multi-threading implementation on ESP32. I'm guessing this works currently (despite not being multi-core-safe) for Python threads because all MicroPython tasks still run on the same core.
Out of curiosity, what do you mean by getting "both cores back" (given that all MicroPython threads will always be on the same core). I guess there's enough overhead of other ESP32 functionality running that having a dedicated core for all MicroPython threads is still a win.
I know on Unix when implementing the equivalent I had to use a recursive pthread mutex, but that might have been specifically because of the way the BLE code uses the mutex. But would moving it to use portENTER_CRITICAL actually solve the slow-startup issue that the pinning is working around? I think @dpgeorge said he could reproduce in an IDF example (and it's fixed in later IDF versions?). (Ref #5489 (comment) ). |
Because most (everything?) provided by esp-idf is multicode-safe and python is constrained to one core. One other place this comes up is with wifi-based communication. For std wifi (sockets) this is handled in the esp-idf network stack, which is one reason the esp32's modsocket doesn't use low level LwIP APIs 'cause they're not multi-core safe. Another place is ESP-NOW support, whose esp-idf api is not multi-core safe per-se either, which is why Glenn had to add a ring buffer (how many ring buffer implementations do we have, in addition to the one esp-idf/freertos provide?).
What I mean is that MP can run on a core relatively undisturbed by whatever RF stuff esp-idf does, which does impact latencies.
I'm unfortunately not familiar with the bluetooth support. So far I'm having a hell of a time getting a simple BLE client for a heart rate sensor to connect/reconnect reliably after something happens, but to be fair, it could also be on my end. |
This is #6343 with some minor changes: