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

extmod/modbluetooth: Remove non-sync event support. #8945

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Jul 22, 2022

This was the original implementation before we had a way to support running
events synchronously from the BLE stack.

ESP32 was the last remaining port using this, and now that it's been
updated (in 5dbb822 & e05d0a6) we can remove this.

Removes the MICROPY_PY_BLUETOOTH_ENTER. This was previously a no-op on
ports (e.g. stm32) using sync events anyway. Replaced with
MICROPY_BEGIN_ATOMIC_SECTION where necessary (e.g. unix mpbthciport.c).

Also makes pairing&bonding enabled by default (this was effectively the
current behavior as it was enabled when sync events were enabled).

@dpgeorge
Copy link
Member

I'm not sure the zephyr port will work with synchronous events... need to check it.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #8945 (01a08ba) into master (00930b2) will decrease coverage by 0.08%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head 01a08ba differs from pull request most recent head 1591446. Consider uploading reports for the commit 1591446 to get more accurate results

@@            Coverage Diff             @@
##           master    #8945      +/-   ##
==========================================
- Coverage   98.38%   98.31%   -0.08%     
==========================================
  Files         158      159       +1     
  Lines       20940    20955      +15     
==========================================
  Hits        20602    20602              
- Misses        338      353      +15     
Files Changed Coverage Δ
ports/unix/mpconfigport.h 100.00% <ø> (ø)
py/mpthread.c 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

@jimmo
Copy link
Member Author

jimmo commented Sep 14, 2023

I have updated this PR to make a common mechanism for having RTOS tasks call into a MicroPython thread.

This replaces the MICROPY_PY_BLUETOOTH_USE_SYNC_EVENTS_WITH_INTERLOCK that was previously used specifically for ESP32, and allows the Unix (and soon, Zephyr) port to use the same functionality (and remove all the special scheduler handling in unix/mpbthciport.c).

@github-actions
Copy link

github-actions bot commented Sep 14, 2023

Code size report:


This ringbuffer-based approach was the original implementation before we
had a way to support running events synchronously from the BLE stack.

Zephyr is the last remaining port using this. ESP32 no longer requires it
as of 5dbb822 & e05d0a6. The Zephyr BLE support is about to be updated
anyway.

This commit also removes the MICROPY_PY_BLUETOOTH_ENTER/EXIT guards. This
was previously a no-op on ports using sync events (e.g. stm32). Replaced
with MICROPY_BEGIN_ATOMIC_SECTION where necessary it was used in other
locations (e.g. unix mpbthciport.c).

Also makes pairing & bonding enabled by default (this was effectively the
current behavior as it was enabled when sync events were enabled).

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Also provide a no-op mutex when threading is disabled.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This abstracts the implementation currently used by modbluetooth.c
to get BLE events from the host stack running in an RTOS task into
a MicroPython task.

This existing implementation only works when the GIL is enabled, so also
add a scheduler-based implementation for non-GIL ports (e.g. Unix).

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This replaces the bluetooth-specific implementation previously enabled when
MICROPY_PY_BLUETOOTH_USE_SYNC_EVENTS_WITH_INTERLOCK was enabled to
synchronously run a host-stack callback on a MicroPython thread.

When MICROPY_PY_THREAD_RTOS is not enabled, this is a no-op and the
call goes directly (e.g. stm32, where the host stack is running in the
scheduler).

Also enables this for esp32 by removing
MICROPY_PY_BLUETOOTH_USE_SYNC_EVENTS_WITH_INTERLOCK and adding
MICROPY_PY_THREAD_RTOS.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This provides a full implementation of the NimBLE mutex, critical section,
and semaphore. On ports without threading, this is a no-op change, but this
is necessary to make NimBLE's locking functionality work on threaded ports.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
MICROPY_EVENT_POLL_HOOK should bounce the GIL like other ports.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This replaces the previous code that used the scheduler (via thread) to
run the BLE host stack.

Now the host stack can run directly on the thread, and then
modbluetooth.c will use mp_thread_run_on_mp_thread to run the
Python-level callbacks on a MicroPython thread as necessary.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Implement a UART transport:
The way transports worked changed slightly in NimBLE 1.6.0 anyway, but
this allows us to hook the "send payload" directly, rather than having
to go byte-at-a-time via the tx callback. Also implemenets a mutex on
transmission as nothing prevents NimBLE sending both a CMD and ACL
payload at the same time.

Common implementation of mpnimbleport.c for scheduler-based ports:
Makes stm32, renesas-ra, mimxrt, rp2 use a shared implementation.

Simplify the NPL event queue:
We only need to process the single default queue, so remove the
additional logic to handle the linked list of queues. Also rename the
functions to execute the queues and run timers.

Unix:
Implement the HCI layer more like an RTOS port, where you have a task
running the host stack, and UART data is sent directly to NimBLE from
a UART IRQ. This makes it behave more like ESP32 or Zephyr, where we
only run the Python-level callbacks in a MicroPython thread.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
For _IRQ_GATTC_READ_DONE, we should wait for it to ensure that we receive
it before we sent the write (this prevents an ordering race with the
"gattc_write" vs the "_IRQ_GATTC_READ_DONE" line).

For _IRQ_GATTC_WRITE_DONE, don't print it out because we wait for it anyway
(so it will be confirmed to have been generated), and it can race with
the concurrent notify or indicate event.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Filter out escape sequences to set the color. Also allow the "TX:" and
"RX:" used in some places.

This work was funded through GitHub Sponsors.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@dmazzella
Copy link
Contributor

Are there any plans for the merge of this PR?

@jimmo
Copy link
Member Author

jimmo commented Oct 3, 2023

Are there any plans for the merge of this PR?

Yes. See #8945 (comment) -- still some more work to go on this, and need to update the Zephyr port to use it.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants