Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:  +144 +0.044% PICO[incl -100(data)]
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Codecov Report

Merging #10739 (33b403d) into master (b3cd41d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10739   +/-   ##
=======================================
  Coverage   98.40%   98.40%           
=======================================
  Files         155      155           
  Lines       20543    20543           
=======================================
  Hits        20215    20215           
  Misses        328      328           

Note: I think I addressed the review comments so far. We're still waiting for some news on a fix for an issue from Infineon, so we'd prefer if this was not merged yet.

We're still waiting for some news on a fix for an issue from Infineon, so we'd prefer if this was not merged yet.

OK. But note that this is blocking #10635 because that PR needs to update cyw43-driver as well, which in turn requires pico-sdk 1.5.0. Will pico-sdk need updating with the fix from Infineon? If yes then we can wait. If no then would be good to just update pico-sdk to 1.5.0 (and cyw43-driver) and do the minimal changes to the rp2 port to work with the new pico-sdk (that's hopefully very minimal work). Then BT can be added as a second step, as done in this PR.

See #10764 which updates pico-sdk to 1.5.0 and cyw43-driver to the latest. That's a very simple change, then this PR can be easily rebased on that once it's merged.

See #10764 which updates pico-sdk to 1.5.0 and cyw43-driver to the latest. That's a very simple change, then this PR can be easily rebased on that once it's merged.

@peterharperuk Impersonating my little one, are we there yet?

@samveen We're still waiting to see if we can get an update from Infineon. In the meantime, feel free to try this PR out for yourself.

@peterharperuk can you provide any clarity as to what this issue is? I've been able to get this PR working on my Pico but not quite up to feature parity with the ESP32 stack and I'm wondering if that's related...

It's just a stability issue. What features are missing?

It seems that remote writes to a characteristic (BLE.gattc_write) only work if the mode option is set 1 instead of the default 0. It isn't the end of the world but I think it breaks part of aioble because code that worked on an ESP32 errors when writing on a Pico W.

I've also been able to get pairing and pairing with a passkey working in "Legacy Pairing" mode but enabling "LE Secure" mode so as to allow for bonding does not work, it just seems to get stuck. I realize those options are disabled in this PR so I totally understand if that's too in the weeds for you right now.

@FlantasticDan Could you point me towards an example that demonstrates the problem with gattc_write mode set to zero? I believe you, but the ble_simple_central.py example seems to work okay and that appears to be using write-without-response.

@peterharperuk I think I have gotten to the bottom of this and I fear I may have steered you a little off course. My apologies. I was seeing that my writes were erroring and assumed it was because of that write mode. Both write modes are working as expected. What's actually happening is the write response handler (only used when mode = 1) never get's called because the _IRQ_GATTC_WRITE_DONE event is not emitting the correct value_handle. My error was interpreting the timeout that I was getting as a "failed to write" and not what it actually was: "the write was never confirmed."

To test this I'm running ble_simple_peripheral.py on a TinyS3 and ble_simple_central.py on a Pico W using this PR's build of micropython. I've made a few changes to ble_simple_central.py to test this bug:

# ble_simple_central.py
        elif event == _IRQ_GATTC_WRITE_DONE:
            conn_handle, value_handle, status = data
            print("TX complete", value_handle) # line 147 modified print statement to see the value_handle of the response event
# . . .
    with_response = True # line 228, was False, changed to True

    i = 0 # line 230
    print("RX Handle:", central._rx_handle) # added this statement to confirm outbound write value_handle
    while central.is_connected():
# . . .

Running this test sees all writes successfully reach the TinyS3 but where it's interesting is in the Pico W stdout:

MicroPython v1.19.1-863-g3e4244f01-dirty on 2023-02-24; Raspberry Pi Pico W with RP2040
Type "help()" for more information.
>>> import ble_simple_central
>>> ble_simple_central.demo()
service (64, 10, 14, UUID(0x1800))
service (64, 15, 18, UUID(0x1801))
service (64, 19, 65535, UUID('6e400001-b5a3-f393-e0a9-e50e24dcca9e'))
Connected
RX Handle: 24
TX 0_
RX <memoryview>
RX <memoryview>
RX <memoryview>
TX complete 65535

The writes occur at value_handle 24 but their responses come in as value handle 65535. I suspect this 65535 is a hardcoded bug somewhere (haven't gotten to look very hard for it yet) because the same handle is also returned for all writes to all handles in my Blackmagic Camera use case as well. So it seems to me the Pico W is correctly receiving the write response but is delivering it to the user code in correctly. It's also possible it's a BTstack bug and not an issue with this PR, I don't have an alternate BTstack micropython board to test on. I'd be curious to see if you can replicate this behavior? Or maybe I'm totally misunderstanding and this is the expected Bluetooth behavior (I'm new to all this BT stuff). Thanks!

@FlantasticDan this is a known issue with our btstack bindings. I have been working on this in the past week (in between other things).

v1.20 will include feature parity with NimBLE for all the GATT functionality (and hopefully also l2cap channels), which will mean that aioble will work identically.

@FlantasticDan the background is that btstack doesn't provide the value handle in the event, so it's up to us to track it.

It would be nice if we could make btstack do this but
I discussed it with the btstack developer here bluekitchen/btstack#439 and we concluded that it wasn't too hard for micropython to do it instead. (Based on what I learnt there it seems we can actually simplify our bindings a bit).

I'll have a PR for this ready soon.

@peterharperuk Yes, just to confirm this is completely unrelated to the issue you're tracking with infineon.

@jimmo that's awesome, thanks for all your work on that. I think I just came across that issue in the source code 😅

Sorry for steering you astray @peterharperuk

np

Hi there, I've tried out this PR, and noticed two bugs/instabilities, that could be addressed here or resolved as separate PRs/issues, as they are closely related but not strictly implied by this change:

• Randomly missing hci-callbacks: When I use bluetooth and enable bonding, I noticed that btstack sometimes misses to send replies, even though an appropriate timer, with zero delay is in mp_btstack_runloop_timers, the main loop is not executed. I traced this down to a bug (I believe) in mp_bluetooth_hci_poll_in_ms. What happens in my case: btstack creates multiple timers, one after 30s (probably to abort the bonding), and one for immediate execution, this causes mp_bluetooth_hci_poll to register a new alarm in roughly 30s. But before that alarm expires new hci packages are handled and processed, creating a new "immediate" timer, which then expires, triggers a mainloop run and and causes mp_bluetooth_hci_poll to register another 29.x-s alarm. This repeats until all pico timer slots are full with roughly 30s-timers, and adding a new "1ms"-timer fails, causing the bluetooth stack to hang until the pairing times out. I would suggest to resolve this like this:

 void mp_bluetooth_hci_poll_in_ms(uint32_t ms) {
+    if(poll_timer_id != 0){
+      cancel_alarm(poll_timer_id);
+    }
     poll_timer_id = add_alarm_in_ms(ms, mp_bluetooth_hci_timer_callback, NULL, true);
 }

• When I use random bluetooth addresses, the btstack fails to initialize with 25% probability. This is caused, by the corresponding code not setting the correct address type flags in the configuration used here:

@@ -592,26 +601,26 @@ STATIC void set_random_address(void) {
         bd_addr_t static_addr;
 
         #if MICROPY_BLUETOOTH_USE_MP_HAL_GET_MAC_STATIC_ADDRESS
 
         DEBUG_printf("set_random_address: Generating static address using mp_hal_get_mac\n");
         mp_hal_get_mac(MP_HAL_MAC_BDADDR, static_addr);
-        // Mark it as STATIC (not RPA or NRPA).
-        static_addr[0] |= 0xc0;
 
         #else
 
         DEBUG_printf("set_random_address: Generating random static address.\n");
         btstack_crypto_random_t sm_crypto_random_request;
         volatile bool ready = false;
         btstack_crypto_random_generate(&sm_crypto_random_request, static_addr, 6, &btstack_static_address_ready, (void *)&ready);
         while (!ready) {
             MICROPY_EVENT_POLL_HOOK
         }
 
         #endif // MICROPY_BLUETOOTH_USE_MP_HAL_GET_MAC_STATIC_ADDRESS
+        // Mark it as STATIC (not RPA or NRPA).
+        static_addr[0] |= 0xc0;
 
         DEBUG_printf("set_random_address: Address generated.\n");
         gap_random_address_set(static_addr);
     }

@peterharperuk I think we can now drop the commits that prevent the use of WiFi and BT together. Do you want me to do that?

We also need to wait for releases/tags of pico-sdk and btstack.

@dpgeorge I've reverted the two commits that stopped bt and wifi working. Yes - we have to wait for btstack and pico-sdk. Are you going to wait to update cyw43-driver?

Thanks @peterharperuk. I've now cleaned up the commits here (squashing the relevant ones), and have also added commits to update to the latest submodules for pico-sdk, cyw43-driver and btstack.

I tested it and everything works as far as I can tell. In particular there are no more issues with BT and WiFi running at the same time on Pico W.

Apart from waiting for tags on the three submodules, this is ready to go in.

This is now merged!

Thanks for your help, and all the work!

Thank you for all the hard work. I have a question. When updating the submodules with that PR, commit 825a957 shows a diff:

diff --git a/lib/btstack b/lib/btstack
index 1635e36d0..77e752abd 160000
--- a/lib/btstack
+++ b/lib/btstack
@@ -1 +1 @@
-Subproject commit 1635e36d06821af8b61302509e91bfcc1ade84c4
+Subproject commit 77e752abd6a0992334047a48038a5a3960e5c6bc

However,. git submodules lists:
77e752abd6a0992334047a48038a5a3960e5c6bc lib/btstack (v1.4-2373-g77e752abd)
Same hash value, but different version number.

I'm not sure where v1.4-2373 is coming from.

Guys, while I just sat here observing progress, thanks for all the hard and pro work. Really appreciated!

Well deserved official praise on The official Site, BTW

Same hash value, but different version number.

You probably just need to run git pull --tags in lib/btstack. Then git describe --tags will work.

The Pico W firmware nightly builds include this work from version v1.20.0-206-g33b403dfb onward.

Thank you for the Music et. al.

You probably just need to run git pull --tags in lib/btstack

That's what I did, and git tags returns all tags:

robert@hh3:~/Downloads/MicroPython/micropython/lib/btstack$ git tag
v1.1
v1.2
v1.2.1
v1.3
v1.3.2
v1.4
v1.4.1
v1.5.0
v1.5.1
v1.5.2
v1.5.3
v1.5.4
v1.5.5
v1.5.5-extra
v1.5.6
v1.5.6.1
v1.5.6.2
v1.5.6.2-extra
robert@hh3:~/Downloads/MicroPython/micropython/lib/btstack$ git describe --tags
v1.5.6.2

I tried as well a deinit of btstack and a new init. No change.

However,. git submodules lists:
77e752abd6a0992334047a48038a5a3960e5c6bc lib/btstack (v1.4-2373-g77e752abd)

@robert-hh my repo also shows that same line when running git submodule. I suspect it's because v1.4 is the most recently annotated git tag of btstack, while all the other ones are not annotated. If you do git describe it does say v1.4-2373-g77e752abd, whereas git describe --tags returns v1.5.6.2. I guess git submodule defaults to showing annotated tags first.

Thanks. I was just a little bit nervous about the state of my local clone.