8000 Improve ESP BLE and turn on BLE workflow by tannewt · Pull Request #9325 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Improve ESP BLE and turn on BLE workflow #9325

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 13, 2024
Merged

Improve ESP BLE and turn on BLE workflow #9325

merged 3 commits into from
Jun 13, 2024

Conversation

tannewt
Copy link
Member
@tannewt tannewt commented Jun 11, 2024 •
edited by dhalbert
Loading

The BLE workflow will broadcast publicly when the web workflow isn't activated.

EDIT by @dhalbert: fixes #9291 as well, due to the ESP-IDF upgrade.

@tannewt tannewt added this to the 9.1.0 milestone Jun 11, 2024
@tannewt tannewt requested a review from dhalbert June 11, 2024 23:16
@tannewt tannewt mentioned this pull request Jun 11, 2024
Merged
@tannewt
Improve ESP BLE and turn on BLE workflow
63aeb11 
The BLE workflow will broadcast publicly when the web workflow
isn't activated.
@tannewt tannewt force-pushed the esp_ble_follow_up branch from 60bbf69 to 63aeb11 Compare June 11, 2024 23:21
dhalbert
dhalbert requested changes Jun 12, 2024
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Some minor comments. This is still marked "Draft"; did you want to change that?

@dhalbert dhalbert mentioned this pull request Jun 12, 2024
Closed
@dhalbert
Copy link
Collaborator

Does this fix #9304?

@bill88t
Copy link
bill88t commented Jun 12, 2024

Fixes #9291.

BLE works, perfectly actually, however reloading with the ble workflow connected leads to a crash.
Only tested on ESP32-S3 so far.

IMG_2024-06-12-17-01-43-899

Will test on the rest of the chips.

@tannewt
Copy link
Member Author
tannewt commented Jun 12, 2024

Does this fix #9304?

I haven't tried it. I was hoping @tyeth would test for me. I did fix some issues with characteristic current_value.

@tannewt
Copy link
Member Author
tannewt commented Jun 12, 2024

Some minor comments. This is still marked "Draft"; did you want to change that?

I was planning on undrafting once the CI is green.

@tannewt
Copy link
Member Author
tannewt commented Jun 12, 2024

BLE works, perfectly actually, however reloading with the ble workflow connected leads to a crash.
Only tested on ESP32-S3 so far.

I've reproduced this and am looking now. Thanks!

@tyeth
Copy link
tyeth commented 8000 Jun 12, 2024

Should get a look at it tomorrow most likely (retesting #9304), but only got an ESP32v2 not s3 this time (away on vacation / working holiday).

@bill88t
Copy link
bill88t commented Jun 12, 2024 •
edited
Loading

I did a little more testing.

Please, please, please, please, do not enable this for boards without psram.
ESP32-S3 doesn't have the memory for this..

Current main branch: 195k usable
This PR: 125k usable

ble takes 70k. 35% of all memory...
It will break a lot of applications.

If ble has to stay, can we please at least have a CIRCUITPY_NOBLE flag or something like this so that all the memory doesn't get allocated?
Or at least can we check as to if something is paired before taking all that ram?

Furthermore, S3 and og ESP32 may be working well enough, but C3, which cannot have PSRAM and has less sram is not.
It has 120k usable as is, with the PR that would be 50k..
How is someone supposed to do web stuff with 50k idk.

It's actually catastrophic for me. I'm sweating at the though of this being merged.

@tannewt
Copy link
Member Author
tannewt commented Jun 12, 2024

I was hoping that the RAM wouldn't be impacted until ble was imported. I'll take a look at it but may want this merged in the meantime.

@bill88t
Copy link
bill88t commented Jun 12, 2024

It may be displayio with the status bar is triggering the import.

@tannewt
Copy link
Member Author
tannewt commented Jun 12, 2024

Looks like we could call esp_bt_mem_release() when ble hasn't been used and we're out of memory. That releases all of the BLE code so bleio can't be used afterwards.

@tyeth
Copy link
tyeth commented Jun 13, 2024 •
edited
Loading

Does this fix #9304?

Sorry not yet. Hard fault to safemode (tested on lilygo T-Display S3 which I forgot I'd brought with me). I ran your latest CI assets from this PR.
I can see the values for accelerometer characteristic changing when viewed with Nrf Connect app. Also a circuitpython service shows up in the Bluefruit App, which is encouraging, and code.circuitpython.org did connect to the board and see the file transfer service, but never finished connecting (might be a result of me messing with BLE things in my code.py)

@dhalbert dhalbert marked this pull request as ready for review June 13, 2024 17:22
dhalbert
dhalbert approved these changes Jun 13, 2024
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Merging as is now to make it easier to test and to clear other merges.

@dhalbert dhalbert merged commit 03e42a8 into adafruit:main Jun 13, 2024
97 checks passed
This was referenced Jun 15, 2024
Closed
Closed
@bablokb bablokb mentioned this pull request Jun 21, 2024
Closed
@dhalbert dhalbert mentioned this pull request Jun 27, 2024
Closed
@bablokb bablokb mentioned this pull request Jul 3, 2024
Closed
@Neradoc Neradoc mentioned this pull request Mar 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

5875
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.1.0
Development

Successfully merging this pull request may close these issues.

ESP32-S3 ADC use causes crashes when WiFi in use
4 participants
@tannewt @dhalbert @bill88t @tyeth
0