BLE: BLECharacteristic::setValue() cleanup and optimization
#11751
Conversation
|
|
👋 Hello Kolcha, 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 ...
Review and merge process you can expect ...
|
Kolcha
force-pushed
the
ble-cleanup
branch
2 times, most recently
from
05c13c0 to
3fa7ff5
Compare
There is no need to pass primitive types by reference, especially by non-const reference as it was in BLECharacteristic. Pass primitive types by value instead, as it should be, and used everywhere else except this one odd class. This allows allows to pass function result as `setValue()` argument without creating temporary variable.
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the BLECharacteristic::setValue() methods by changing parameter passing from non-const references to pass-by-value for primitive types and simplifying the implementation by directly using parameter addresses instead of creating temporary variables.
- Changes parameter passing from non-const reference to pass-by-value for primitive types (uint16_t, uint32_t, int, float, double)
- Removes manual bit manipulation and temporary array creation in favor of direct memory address usage
- Leverages ESP32's little-endian architecture to eliminate unnecessary byte-by-byte copying
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| libraries/BLE/src/BLECharacteristic.h | Updates method signatures to pass primitive types by value instead of non-const reference |
| libraries/BLE/src/BLECharacteristic.cpp | Simplifies implementation by using direct parameter addresses and sizeof() instead of manual byte manipulation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
It may be worth to test and verify it using other MCU SoCs, such as ESP32-S3 and ESP32-C3. Based on BLE specification, it shall be little-endian, therefore if you try sending an int value, you will see that the hex values are arranged in little-endian format. The "human readable" value 0xAABBCCDD will for instance be transmitted and received as 0xDDCCBBAA. The change takes into consideration the MCU endianless when compiling the code.
|
Test Results 76 files 76 suites 13m 8s ⏱️ Results for commit 5edf464. ♻️ This comment has been updated with latest results. |
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.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Another suggestion of change here (using reinterpret_cast<uint8_t*>): |
ESP32 is little-endian architecture, so the code that filled temporary arrays created from integers basically did nothing. Pass primitives as 'buffers' to generic `setValue()` instead. Also use sizeof() instead of hardcoded sizes as the argument.
There is no technical need to pass non-const data there. Also it is a common practice to pass const data to the functions with write semantics, as they not supposed to modify the data. This allows to pass the data from read-only sources without removing the constness what is not always safe.
Pass String by const reference to many `setValue()` functions to avoid copying it. Even there are not so much data inside these strings, it just looks too odd to copy them every time. Also this change decreases binary size (~200 bytes in my case).
By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes
Checklist
This entire section above can be deleted if all items are checked.
Description of Change
The change is just a code cleanup following common practices, no behavior change introduced.
BLECharacteristic::setValue()by value instead of by non-const reference.There is no technical need to passing values by non-const reference there, non-const reference makes usage inconvenient (function return can't be passed directly as argument, temporary variable required), passing by value eliminates this inconvenience.
setValue()implementations for primitive types.There is no technical need to create all those temporary variables, the address of the argument can be used directly. Such "integer to array" conversion is not required, because it basically does nothing, as ESP32 is little-endian architecture.
There is no technical need to pass non-const data there. This allows to pass the data from read-only sources without removing the constness what is not always safe.
Pass String by const reference to many
setValue()functions to avoid copying it.Test Scenarios
I have tested my Pull Request on Arduino-esp32 core v3.3.0 with ESP32 Board with this scenario:
uint16_tand 4 arefloatRelated links
nothing related found