Skip to content

Fix BLE adv timeout; ESP: don't sched handler on adv finish #9395

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

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Jul 2, 2024

Fixes #9359.

  • Correct handling of advertising timeout value in both espressif and nordic common_hal_bleio_adapter_start_advertising()
    • Timeout of zero means wait forever. Use BLE_HS_FOREVER for espressif. Use BLE_GAP_ADV_TIMEOUT_GENERAL_UNLIMITED for nordic.
    • Handle zero timeout in the base routine _common_hal_bleio_adapter_start_advertising(), not in the parent routine common_hal_bleio_adapter_start_advertising(), because callers to the former expect zero to work as an unlimited timeout.
  • Espressif: remove background_callback_add_core(&bleio_background_callback() from _advertising_event(). Scheduling the background task caused advertising to start up again. This caused a tight loop of advertising starting, finishing, and restarting, slowing down CircuitPython greatly. The reschedule does not appear to be necessary: it is not done in the equivalent nordic code. There is other code to restart the BLE workflow advertising as needed.

Tested with heart rate monitor, iBBQ, and BLE UART.

@bablokb Could you test to see if this fixes #9359 for you? Thanks. It might also prevent #9362, if that error is a side effect of the tight loop mentioned above.

(Note: I have been unsuccessful in getting BLE workflow to work with PyLeap with 9.0.5 or recent builds including this, but I might be be doing something wrong.)

@dhalbert dhalbert requested a review from tannewt July 2, 2024 16:23
Copy link

@bill88t bill88t left a comment

Choose a reason for hiding this comment

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

ESP32 feels a lot snappier with this!

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks! Did you try File Glider?

@tannewt tannewt merged commit 1188d67 into adafruit:main Jul 2, 2024
324 checks passed
@dhalbert
Copy link
Collaborator Author

dhalbert commented Jul 2, 2024

Thanks! Did you try File Glider?

I tried that too and didn't get it to pair. But I need to read the Guide again.

@dhalbert dhalbert deleted the ble-timeout-espressif-events branch July 2, 2024 19:09
@bablokb
Copy link

bablokb commented Jul 3, 2024

Tested with the code mentioned in #9359:

show_colors (sleep): 3.001
show_colors (title): 0.0569992
show_colors (ui): 0.0790024
show_colors (refresh): 0.392006
show_colors (run): 0.485001

load_image (sleep): 3.003
load_image (title): 0.0540009
load_image (load): 0.107002
load_image (refresh): 1.17
load_image (run): 1.289

So we are back to normal. Thanks for tracking this down.
Regarding #9362: this is not fixed yet. I will add additional infos in that issue.

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

Successfully merging this pull request may close these issues.

Display updates extremly slow after PR#9344
4 participants