espressif / esp-nimble

A fork of NimBLE stack, for use with ESP32 and ESP-IDF
Apache License 2.0
76 stars 49 forks source link

NimBLE connection reattempts overwrite ble_gap_connect callback argument #65

Closed robin96c closed 3 months ago

robin96c commented 3 months ago

When I initiate a connection by calling ble_gap_connect, I pass my GAP event handler callback function and callback argument to it.

All the events related to this connection correctly call the callback function with the callback argument. However in some cases, I noticed the callback argument changes.

After some digging I found that the NimBLE reconnect attempts are the culprit, as they overwrite the callback argument: https://github.com/espressif/esp-nimble/blob/9478bc510fa9fc792d98efbc80dc547b97a13b99/nimble/host/src/ble_gap.c#L1032

The callback function is not overwritten, so this leads to my application callback being called with a different callback argument. Disabling the reattempts in sdkconfig (CONFIG_BT_NIMBLE_ENABLE_CONN_REATTEMPT=n) fixed the issue for me. But I don't think this is intended behavior that the application callback argument is overwritten?

rahult-github commented 3 months ago

Hi @robin96c ,

I think you are referring to nimble-1.3.0-idf (mostly v4.4 ). This issue was fixed on later releases. Please find attached patch which should fix this. Please let me know if issue is still observed.

dangling_pointer.txt

robin96c commented 3 months ago

Hello @rahult-github

Thank you for the quick response! I'm running idf v5.1.1 on an esp32s3.

I tried your patch but unfortunately the issue remains. I think the issue is that, even with your patch, the connection reattempt calls ble_gap_connect with the connection description as the new callback argument. https://github.com/espressif/esp-nimble/blob/9478bc510fa9fc792d98efbc80dc547b97a13b99/nimble/host/src/ble_gap.c#L1035 https://github.com/espressif/esp-nimble/blob/9478bc510fa9fc792d98efbc80dc547b97a13b99/nimble/host/src/ble_gap.c#L5482

And as far as I can tell, my original callback argument (passed when the application originally called ble_gap_connect) is never restored after this connection reattempt.

rahult-github commented 3 months ago

Hi @robin96c ,

May i request you to share your application code changes done. I can try the same at my end to reproduce issue and debug more.

robin96c commented 3 months ago

Hi @rahult-github,

In the meantime I did a quick test with release v5.2.1, the issue still remains. I adapted the blecent and bleprph examples to showcase the issue.

If you apply this patch to idf v5.2.1 and flash one device with the blecent example and one device with bleprph example you should be able to reproduce the issue :) https://github.com/espressif/esp-idf/tree/v5.2.1 NimBLE_connection_reattempt_issue_v5_2_1.txt

Connection re-attempts would trigger quite randomly (hard to reproduce), so I adapted the bleprph example that it automatically disconnects with a response code that triggers the re-attempts. You will see that once the central initiates the re-attempt, it will call the applications blecent_gap_event with a different cb_arg. On top of that I noticed a crash if I choose a random cb_arg that is not pointing to a valid address (more explanation in the patch file).

Thanks for looking into it!

rahult-github commented 3 months ago

Hi @robin96c ,

Can you please check if attached patch helps resolve your issue ?

Patch is to be assigned in $IDF_PATH/components/bt/host/nimble/nimble path

callback_assign.txt

robin96c commented 3 months ago

Hi @rahult-github,

Perfect, this resolved the issue, thank you!