Skip to content

Conversation

lucasssvaz
Copy link
Member

Description of Change

This pull request fixes the missing static members initialization in BLESecurity.

Tests scenarios

Tested locally

Related links

Closes #11671

@lucasssvaz lucasssvaz self-assigned this Jul 31, 2025
@lucasssvaz lucasssvaz requested a review from SuGlider as a code owner July 31, 2025 11:57
@lucasssvaz lucasssvaz added the Area: BLE Issues related to BLE label Jul 31, 2025
Copy link
Contributor

github-actions bot commented Jul 31, 2025

Messages
📖 This PR seems to be quite large (total lines of code: 1200), you might consider splitting it into smaller PRs

👋 Hello lucasssvaz, we appreciate your contribution to this project!


📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 586e497

Copy link
Contributor

github-actions bot commented Jul 31, 2025

Test Results

 76 files   76 suites   15m 4s ⏱️
 38 tests  38 ✅ 0 💤 0 ❌
241 runs  241 ✅ 0 💤 0 ❌

Results for commit 586e497.

♻️ This comment has been updated with latest results.

@lucasssvaz lucasssvaz mentioned this pull request Jul 31, 2025
1 task
Copy link
Contributor

github-actions bot commented Jul 31, 2025

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

MemoryFLASH [bytes]FLASH [%]RAM [bytes]RAM [%]
TargetDECINCDECINCDECINCDECINC
ESP32C50‼️ +7K0.00⚠️ +0.940⚠️ +8520.00‼️ +3.37
ESP32S30‼️ +2K0.00⚠️ +0.350⚠️ +560.00⚠️ +0.17
ESP32C30⚠️ +16360.00⚠️ +0.260⚠️ +720.00⚠️ +0.32
ESP32C60‼️ +4K0.00⚠️ +0.520⚠️ +2480.00‼️ +1.14
ESP32H20‼️ +2K0.00⚠️ +0.290⚠️ +2440.00‼️ +1.18
ESP320‼️ +4K0.00⚠️ +0.360⚠️ +2560.00⚠️ +0.65
Click to expand the detailed deltas report [usage change in BYTES]
TargetESP32C5ESP32S3ESP32C3ESP32C6ESP32H2ESP32
ExampleFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAMFLASHRAM
libraries/BLE/examples/Beacon_Scanner‼️ +7K⚠️ +852⚠️ +1680⚠️ +48⚠️ +1044⚠️ +72‼️ +4K⚠️ +248‼️ +2K⚠️ +236‼️ +4K⚠️ +256
libraries/BLE/examples/Client‼️ +7K⚠️ +852⚠️ +1736⚠️ +56⚠️ +1098⚠️ +64‼️ +4K⚠️ +240‼️ +2K⚠️ +244‼️ +4K⚠️ +248
libraries/BLE/examples/Client_secure_static_passkey------------
libraries/BLE/examples/EddystoneTLM_Beacon‼️ +7K⚠️ +852‼️ +2K⚠️ +32⚠️ +1466⚠️ +64‼️ +3K⚠️ +232⚠️ +1897⚠️ +236‼️ +3K⚠️ +148
libraries/BLE/examples/EddystoneURL_Beacon‼️ +7K⚠️ +852‼️ +2K⚠️ +32⚠️ +1636⚠️ +72‼️ +4K⚠️ +240‼️ +2K⚠️ +244‼️ +4K⚠️ +256
libraries/BLE/examples/Notify‼️ +7K⚠️ +852⚠️ +1644⚠️ +56⚠️ +948⚠️ +64‼️ +3K⚠️ +240⚠️ +2041⚠️ +244‼️ +3K⚠️ +248
libraries/BLE/examples/Scan‼️ +7K⚠️ +852⚠️ +1660⚠️ +48⚠️ +1044⚠️ +72‼️ +4K⚠️ +248‼️ +2K⚠️ +236‼️ +4K⚠️ +256
libraries/BLE/examples/Server‼️ +7K⚠️ +852⚠️ +1688⚠️ +48⚠️ +1038⚠️ +56‼️ +4K⚠️ +240‼️ +2K⚠️ +244‼️ +3K⚠️ +256
libraries/BLE/examples/Server_multiconnect‼️ +7K⚠️ +852⚠️ +1628⚠️ +56⚠️ +956⚠️ +64‼️ +3K⚠️ +240⚠️ +2033⚠️ +244‼️ +3K⚠️ +248
libraries/BLE/examples/Server_secure_static_passkey------------
libraries/BLE/examples/UART‼️ +7K⚠️ +852⚠️ +1628⚠️ +48⚠️ +956⚠️ +56‼️ +3K⚠️ +240⚠️ +2033⚠️ +244‼️ +3K⚠️ +256
libraries/BLE/examples/Write‼️ +7K⚠️ +852⚠️ +1692⚠️ +56⚠️ +1038⚠️ +64‼️ +4K⚠️ +240‼️ +2K⚠️ +244‼️ +3K⚠️ +248
libraries/BLE/examples/iBeacon‼️ +7K⚠️ +852⚠️ +1628⚠️ +48⚠️ +958⚠️ +56‼️ +3K⚠️ +240⚠️ +2035⚠️ +244‼️ +3K⚠️ +256

@lucasssvaz lucasssvaz marked this pull request as draft August 10, 2025 03:05
@lucasssvaz lucasssvaz changed the title fix(ble): Add initialization of static members fix(ble): Fix BLESecurity Aug 10, 2025
@lucasssvaz lucasssvaz changed the title fix(ble): Fix BLESecurity fix(ble): Fix BLESecurity and add examples Aug 10, 2025
@lucasssvaz lucasssvaz force-pushed the fix/bt_component branch 2 times, most recently from 3f549d3 to f83d7f3 Compare August 30, 2025 20:05
@lucasssvaz lucasssvaz marked this pull request as ready for review September 1, 2025 11:50
Copilot

This comment was marked as outdated.

@me-no-dev me-no-dev requested a review from Copilot September 4, 2025 05:58
@me-no-dev me-no-dev added the Status: Review needed Issue or PR is awaiting review label Sep 4, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes missing static member initialization in the BLESecurity class and adds comprehensive security examples. The changes improve BLE security functionality by properly initializing static variables, enhancing error handling, and providing clearer security configuration APIs.

  • Initializes missing static members in BLESecurity class to prevent undefined behavior
  • Improves security callback handling and removes deprecated server/client callback methods
  • Adds comprehensive secure BLE server and client examples with static passkey authentication

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
BLESecurity.h/cpp Adds static member initialization, new security APIs, and improved callback handling
BLEServer.h/cpp Removes deprecated security callbacks and adds advertise-on-disconnect functionality
BLEClient.h/cpp Removes deprecated security callbacks and improves security integration
BLEDevice.h/cpp Moves security level handling to BLESecurity class and adds stack detection
BLEUtils.h/cpp Fixes const correctness and logging macro usage
BLECharacteristic.h/cpp Adds authorization support and improves permission handling
BLEDescriptor.h/cpp Updates permission type from uint8_t to uint16_t
BLERemoteCharacteristic.cpp Adds security enabled checks before attempting secure connections
BLERemoteDescriptor.cpp Adds security enabled checks before attempting secure connections
Examples Adds comprehensive secure BLE server and client examples with documentation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Rob58329
Copy link
Contributor

Rob58329 commented Sep 4, 2025

@lucasssvaz

I note that your above "BLESecurity.cpp" has the static class function "setAuthenticationMode()" containing:

uint8_t BLESecurity::m_authReq = 0;
...
void BLESecurity::setAuthenticationMode(uint8_t auth_req) {
  log_d("setAuthenticationMode: auth_req=%d", auth_req);
  m_authReq = auth_req;
#if defined(CONFIG_BLUEDROID_ENABLED)
  esp_ble_gap_set_security_param(ESP_BLE_SM_AUTHEN_REQ_MODE, &m_authReq, sizeof(uint8_t));
#elif defined(CONFIG_NIMBLE_ENABLED)
  BLESecurity::setAuthenticationMode(
    (auth_req & BLE_SM_PAIR_AUTHREQ_BOND) != 0, (auth_req & BLE_SM_PAIR_AUTHREQ_MITM) != 0, (auth_req & BLE_SM_PAIR_AUTHREQ_SC) != 0
  );
#endif
}

But I wonder why it is necessary to have "uint8_t BLESecurity::m_authReq = 0;" at all, as would it not make more sence just to use the passed "auth_req" directly, or instead to just to define it inside the function itself?

(And similarly for m_iocap, m_initKey and m_respKey?)

PS. Thankyou for the new "static passkey" examples!

@me-no-dev me-no-dev added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Sep 4, 2025
@me-no-dev
Copy link
Member

@lucasssvaz please take care of the typos https://github.com/espressif/arduino-esp32/actions/runs/17464095894/job/49595433898?pr=11681#step:9:29

@lucasssvaz
Copy link
Member Author

@Rob58329 These values are accessed directly by other classes by declaring them as friend classes.

@lucasssvaz
Copy link
Member Author

@me-no-dev Done

@me-no-dev me-no-dev merged commit 9a127fc into espressif:master Sep 4, 2025
55 checks passed
@lucasssvaz lucasssvaz deleted the fix/bt_component branch September 4, 2025 16:31
@Rob58329
Copy link
Contributor

Rob58329 commented Sep 5, 2025

@lucasssvaz
Note that for me your "Secure_server_with_static_passkey.ino" example does not work with my NodeMCU ESP32.

Speifically if you delete all saved bonds (eg. using the code below), my Android phone can connect to the ESP32 fine, and even though it gets the pop-up "Bluetooth paring request" box, you click on "pair" and it pairs without asking for or checking any paring code).

(In fact I cant currently get any of my old SDK v3.1.1 paring-code software to work!)

(I am using Windows10 with Arduino IDE v1.8.19 and https://github.com/espressif/arduino-esp32 as at 5Sep25)

The "delete saves bonds" code I am using is:

void loop() {
  while (Serial.available()) {
    char c=Serial.read();
    if (c>=32) Serial.write(c);
    else Serial.println();
    if (c=='d') show_bonded_devices();
    if (c=='e') remove_all_bonded_devices();
  }
  delay(100);
}

char bda_str[18];
char *bda2str(const uint8_t *bda) {
  sprintf(bda_str, "%02x:%02x:%02x:%02x:%02x:%02x", bda[0], bda[1], bda[2], bda[3], bda[4], bda[5]);
  return bda_str;
}


void show_bonded_devices() { 
  int dev_num = esp_ble_get_bond_device_num();
  esp_ble_bond_dev_t *dev_list = (esp_ble_bond_dev_t *)malloc(sizeof(esp_ble_bond_dev_t) * dev_num);
  esp_ble_get_bond_device_list(&dev_num, dev_list);
  Serial.printf("\nBonded devices number: %d\n", dev_num);
  for (int i = 0; i < dev_num; i++) {Serial.printf("Found bonded device #%d = %s\n", i+1, bda2str(dev_list[i].bd_addr));}
  free(dev_list);
}

void remove_all_bonded_devices() { // static void __attribute__((unused)) remove_all_bonded_devices(void) {
  Serial.println("\nRemoving all bonded devices...");
  int dev_num = esp_ble_get_bond_device_num();
  esp_ble_bond_dev_t *dev_list = (esp_ble_bond_dev_t *)malloc(sizeof(esp_ble_bond_dev_t) * dev_num);
  esp_ble_get_bond_device_list(&dev_num, dev_list);
  for (int i = 0; i < dev_num; i++) esp_ble_remove_bond_device(dev_list[i].bd_addr);
  free(dev_list);
}

@lucasssvaz
Copy link
Member Author

@Rob58329 I tested everything with the nrf connect and it was working as expected. Could you please try erasing the flash before the sketch upload to clear the nvs ?

@Rob58329
Copy link
Contributor

Rob58329 commented Sep 5, 2025

@Rob58329 I tested everything with the nrf connect and it was working as expected. Could you please try erasing the flash before the sketch upload to clear the nvs ?

@lucasssvaz I've just done a "Flash memory erased successfully in 6.0 seconds." and then re-flashed your Secure_server_with_static_passkey.ino" example . But with my ESP32 (orig) I still can connect to the ESP32 using BLE with "nRF Connect" on my Android phone without enterning any PIN code and read the "Secure Hello World" message (as well as the "Insecure Hello World" message).

PS. also tried on a brand new NodeMCU ESP32 which has never been flashed before and it too connected without needing to enter any PIN (after pressing the "PAIR" option on the phone it just connects without giving you the option to enter a PIN).

(Perhaps the BLE on the ESP32 (orig) works differently to that on the ESP32C6 etc?)

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
None yet
Development

Successfully merging this pull request may close these issues.

undefined reference to `BLESecurity::m_authReq'
4 participants