Skip to content
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

Bluetooth-classic: release BLE memory when BT classic only is requested #8051

Merged
merged 11 commits into from
Nov 29, 2023

Conversation

ferbar
Copy link
Contributor

@ferbar ferbar commented Apr 9, 2023

Checklist

  1. Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
  2. Please provide related links (eg. Issue which will be closed by this Pull Request)
  3. Please update relevant Documentation if applicable
  4. Please check Contributing guide

Description of Change

If CONFIG_BTDM_CONTROLLER_MODE_BR_EDR_ONLY is defined BLE is disabled, therefore the allocated memory can get freed to save about 14kB ram.

Tests scenarios

Arduino-esp32:2,0.6 (current master) with ESP32 under platformio with and without -DCONFIG_BTDM_CONTROLLER_MODE_BR_EDR_ONLY=y

Related links

#6451

ferbar added 3 commits April 9, 2023 01:00
…E_BR_EDR_ONLY is set

BLE memory can be released if bluetooth-classic - only is requested
#define BT_MODE ESP_BT_MODE_CLASSIC_BT
#elif defined(CONFIG_BTDM_CONTROLLER_MODE_BTDM)
#define BT_MODE ESP_BT_MODE_BTDM
Copy link
Member

Choose a reason for hiding this comment

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

does switching defines do anything here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is to tell btStart() that I want to use bluetooth-classic only. CONFIG_BTDM_CONTROLLER_MODE_BTDM is defined in a default aduino-esp32 configuration, so it is the first #if that matches and can not be overridden. When we switch the defines BR_EDR_ONLY can be forced with a -DCONFIG_BTDM_CONTROLLER_MODE_BR_EDR_ONLY.

If you prefer an other #define like ARDUINO_BLUETOOTH_EDR_ONLY or a new function for BT-classic initialization please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

how about you instead -DBT_MODE=ESP_BT_MODE_CLASSIC_BT yourself and only #ifndef BT_MODE do the config check.

@@ -34,16 +34,28 @@ bool btStarted(){

bool btStart(){
esp_bt_controller_config_t cfg = BT_CONTROLLER_INIT_CONFIG_DEFAULT();
#if CONFIG_IDF_TARGET_ESP32
#ifdef CONFIG_BTDM_CONTROLLER_MODE_BR_EDR_ONLY
Copy link
Member

Choose a reason for hiding this comment

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

This would need to check #if BT_MODE == ESP_BT_MODE_CLASSIC_BT instead

#elif BT_MODE == ESP_BT_MODE_BLE
esp_bt_controller_mem_release(ESP_BT_MODE_CLASSIC_BT);
#endif
cfg.mode=BT_MODE;
Copy link
Member

Choose a reason for hiding this comment

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

cfg.mode is only available on ESP32. You should have kept the whole block filtered for ESP32 target. Also as far as I remember, doing mem release of the classic BT will also break BLE. If BT/BLE is not used, that memory is already cleared when Arduino is starting up. So keep just esp_bt_controller_mem_release(ESP_BT_MODE_BLE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, you were too fast.

  1. esp_bt_controller_mem_release(ESP_BT_MODE_CLASSIC_BT) crashes the esp32 without an error message when using BLE
  2. unexpectedly #if BT_MODE == ESP_BT_MODE_CLASSIC_BT doesn't work. The preprocessor only likes integer values, I will find a solution. Thinking of:
   cfg.mode=BT_MODE;
   if(cfg.mode == ESP_BT_MODE_CLASSIC_BT) {
     esp_bt_controller_mem_release(ESP_BT_MODE_BLE);
   }

tomorrow....

Copy link
Member

Choose a reason for hiding this comment

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

#if CONFIG_IDF_TARGET_ESP32
cfg.mode=BT_MODE;
if(cfg.mode == ESP_BT_MODE_CLASSIC_BT) {
  esp_bt_controller_mem_release(ESP_BT_MODE_BLE);
}
#endif

Looks like a good plan 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@me-no-dev Is it not possible to clear memory used by BT_CLASSIC if I'm using only BLE? In the case of Rainmaker, we often run into low memory since both Classic BT and BLE are on during WiFi provisioning. Tagging @shahpiyushv.

Copy link
Member

Choose a reason for hiding this comment

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

@sanketwadekar as far as I remember and it seems that @ferbar confirms it above, it is not possible to release classic memory even if you are using just BLE

@VojtechBartoska VojtechBartoska added this to the 2.0.8 milestone Apr 12, 2023
@VojtechBartoska VojtechBartoska added the Resolution: Awaiting response Waiting for response of author label Apr 12, 2023
@VojtechBartoska
Copy link
Contributor

@me-no-dev Can you please check? Updates are there, thanks @ferbar.

@ferbar
Copy link
Contributor Author

ferbar commented Apr 13, 2023

If you are fine with my changes I would squash them before the PR merge.

@me-no-dev me-no-dev modified the milestones: 2.0.8, 3.0.0 Apr 17, 2023
@me-no-dev me-no-dev added Status: Pending Merge Pull Request is ready to be merged Area: BLE Issues related to BLE and removed Resolution: Awaiting response Waiting for response of author labels Apr 17, 2023
@CLAassistant
Copy link

CLAassistant commented May 6, 2023

CLA assistant check
All committers have signed the CLA.

@ferbar
Copy link
Contributor Author

ferbar commented Oct 26, 2023

@me-no-dev do you need any additional information?

@VojtechBartoska VojtechBartoska modified the milestones: 3.0.0, 3.0.0-Aplha3 Nov 28, 2023
@me-no-dev me-no-dev merged commit dbf0b18 into espressif:master Nov 29, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Issues related to BLE Status: Pending Merge Pull Request is ready to be merged
Projects
Development

Successfully merging this pull request may close these issues.

5 participants