Louisza patch phy #405
Louisza patch phy #405
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Coded PHY (long range) advertising and transmit power control to the ArduinoBLE library. The changes enable developers to use Bluetooth Low Energy's extended range capabilities and adjust transmission power for better range or power efficiency.
- Adds two new public methods:
enableCodedPHYAdvertising()andsetTxPower() - Updates library properties to reflect the custom modifications
- Implements the new functionality using the underlying BLE interface
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/BLEDevice.h | Declares new methods for Coded PHY advertising and TX power control |
| src/BLEDevice.cpp | Implements the new methods with proper BLE interface calls |
| library.properties | Updates library name and version to indicate custom modifications |
| // Set TX power here if not using setTxPower separately | ||
| gap.setTxPower(4); |
There was a problem hiding this comment.
Hardcoded TX power value (4) should be made configurable or removed. This conflicts with the separate setTxPower() method and creates unexpected behavior where enabling Coded PHY always sets power to 4 dBm.
| // Set TX power here if not using setTxPower separately | |
| gap.setTxPower(4); | |
| // TX power should be set via setTxPower() if needed |
Copilot uses AI. Check for mistakes.
| void BLEDevice::enableCodedPHYAdvertising(bool enabled) { | ||
| if (!_ble_interface.hasInitialized()) return; | ||
|
|
||
| ble::AdvertisingParameters advParams( | ||
| ble::advertising_type_t::NON_CONNECTABLE_UNDIRECTED, |
There was a problem hiding this comment.
The advertising type is hardcoded to NON_CONNECTABLE_UNDIRECTED, which may not be appropriate for all use cases. Consider making this configurable or using the existing advertising configuration.
| void BLEDevice::enableCodedPHYAdvertising(bool enabled) { | |
| if (!_ble_interface.hasInitialized()) return; | |
| ble::AdvertisingParameters advParams( | |
| ble::advertising_type_t::NON_CONNECTABLE_UNDIRECTED, | |
| void BLEDevice::enableCodedPHYAdvertising(bool enabled, ble::advertising_type_t advType /* = ble::advertising_type_t::NON_CONNECTABLE_UNDIRECTED */) { | |
| if (!_ble_interface.hasInitialized()) return; | |
| ble::AdvertisingParameters advParams( | |
| advType, |
Copilot uses AI. Check for mistakes.
|
|
||
| ble::AdvertisingParameters advParams( | ||
| ble::advertising_type_t::NON_CONNECTABLE_UNDIRECTED, | ||
| ble::adv_interval_t(ble::millisecond_t(200)) |
There was a problem hiding this comment.
The advertising interval is hardcoded to 200ms. This magic number should be made configurable or defined as a named constant to improve maintainability.
| ble::adv_interval_t(ble::millisecond_t(200)) | |
| ble::adv_interval_t(ble::millisecond_t(DEFAULT_ADVERTISING_INTERVAL_MS)) |
Copilot uses AI. Check for mistakes.
| auto& gap = _ble_interface.gap(); | ||
| gap.setAdvertisingParameters(_adv_handle, advParams); | ||
|
|
||
| // Set TX power here if not using setTxPower separately |
There was a problem hiding this comment.
The comment is unclear about when this automatic TX power setting occurs versus when setTxPower() should be used. This could confuse users about the intended API usage pattern.
Copilot uses AI. Check for mistakes.
|
This sounds like exactly what I have been looking for. |
No description provided.