-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. W 8000 e’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The BLE workflow will broadcast publicly when the web workflow isn't activated.
60bbf69
to
63aeb11
Compare
There was a problem hiding this 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?
Does this fix #9304? |
Fixes #9291. BLE works, perfectly actually, however reloading with the ble workflow connected leads to a crash. Will test on the rest of the chips. |
I was planning on undrafting once the CI is green. |
I've reproduced this and am looking now. Thanks! |
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). |
I did a little more testing. Please, please, please, please, do not enable this for boards without psram. Current main branch: 195k usable ble takes 70k. 35% of all memory... 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? Furthermore, S3 and og ESP32 may be working well enough, but C3, which cannot have PSRAM and has less sram is not. It's actually catastrophic for me. I'm sweating at the though of this being merged. |
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. |
It may be |
Looks like we could call |
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. |
There was a problem hiding this 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.
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.