Fixes a bug where incorrect Tx power level would be advertised.

[sanastasiou]

C3 and S3 have a workaround where different power levels could be set for e.g. advertising or scanning. The workaround does not take into account advertisement data, which keeps sending "default" channel with incorrect dbM value to the advertisement packet.

The fix should be backwards compatible since it only adds an optional parameter when Tx level is added into the advertisement packet.

Tested and verified that I now see -3 dbM when previously setting the Tx Level like this:

NimBLEDevice::setPowerLevel(ESP_PWR_LVL_N3, ESP_BLE_PWR_TYPE_ADV);

Without the fix some default value is transmitted.

[thekurtovic]

If you are only setting the TX power for advertising, you could change the line to omit specifying the power type instead.
The change to getPower will break parity with the parent repo esp-nimble-cpp, where NimBLEDevice::getPower must work for non-esp32 platform.
Alternative could be to let NimBLEAdvertisementData::addTxPower have the value specified by the caller for this case.
i.e.

/**
 * @brief Adds Tx power level to the advertisement data.
 * @param [in] power The power value to be set in the advertisement.  
 * @return True if successful.
 */
bool NimBLEAdvertisementData::addTxPower(int8_t power) {
    uint8_t data[3];
    data[0] = BLE_HS_ADV_TX_PWR_LVL_LEN + 1;
    data[1] = BLE_HS_ADV_TYPE_TX_PWR_LVL;
# ifndef CONFIG_IDF_TARGET_ESP32P4
    data[2] = power;
# else
    data[2] = 0;
# endif
    return addData(data, 3);
} // addTxPower

/**
 * @brief Adds Tx power level to the advertisement data.
 * @return True if successful.
 */
bool NimBLEAdvertisementData::addTxPower() {
    return addTxPower(NimBLEDevice::getPower());
} // addTxPower

[sanastasiou]

Hi @thekurtovic, I am not only setting the Tx power for advertising but also in the device itself. So that's how I found the issue in the first place. The retrieval of the power in the addTxPower is not correct without the fix.

I could do the 2nd solution you proposed, although I find it kind of redundant since the device should "know" with which power it currently transmits.

Let me know what change you prefer and I'll update the PR. Alternatively I could use NimbleDevice::getPowerLevel() and put that into the add TxPower or even build the advertisement packet myself but I guess that's not so user friendly.

[thekurtovic]

I am not only setting the Tx power for advertising but also in the device itself

Just to rule out any confusion, NimBLEAdvertising::addTxPower merely includes additional data within the advertisement payload so a central device can see the value while scanning, similar to how a string is shown for the name. If you only ever call NimBLEDevice::setPowerLevel one time, for setting the advertising TX power, then it's not necessary to specify the type for most applications. Disregard if you call NimBLEDevice::setPowerLevel for multiple power types with unique values.

I agree it should know which power type to use, your solution makes sense in a vacuum.
Let's see what @h2zero thinks, maybe my assessment is wrong.

[h2zero]

Thanks @sanastasiou, this makes sense, but as @thekurtovic has mentioned it would break the API when building for non-esp devices.

I have a couple ideas:

• Add the tx power level in dbm as a parameter passed to the addTxPower function as @thekurtovic suggested.
• Keep the default/advertising/scan power in sync at all times, so setting one will set them all to the same value, which is how the NimBLE stack works with non-esp devices.
• Add functions for getAdvPower and getScanPower as esp only functions.

[sanastasiou]

Hi @h2zero so if I understand correctly, the AddTxPower should also call NimbleDevice::setPowerLevel for Advertising and also set the power in dbM in the advertising package right ?

[h2zero]

What I meant was inside setPowerLevel we can keep the default power level in sync when the type is something else. However I think instead that we could introduce a member function and variable in NimBLEAdvertising and NimBLEExtAdvertising that stores the power level locally in dbm and is called from setPowerLevel/setPower so that addTxPower will just apply that value instead of having to call NimBLEDevice::getPower.

[sanastasiou]

@h2zero Do we even need this of extAdv class?

/**
 * @brief Adds Tx power level to the advertisement data.
 */
void NimBLEExtAdvertisement::addTxPower() {
    m_params.include_tx_power = 1;
} // addTxPower

[h2zero]

Yes, it is an advertisable data in extended advertising, however the data used is stored in m_params.tx_power and this flag enables or disables advertising of it.

[sanastasiou]

@h2zero @thekurtovic thanks but I think I'll create an issue instead and you guys can fix this. I am pretty sure that whatever I implemented wouldn't be quite right. ExtAdvertising already has a member to store dbM, simple advertising doesn't and needs one setPowerLevel doesn't pass the dbM rather esp_ble_power_type_t so man one to either convert this to dbM with all the corner cases, or call the getPowerLevel which does this for free but this is what you wanted to avoid in the first place. Pretty sure I'd forget other corner cases too. So I am closing this PR since I have a locally working solution, but thanks for the effort put here.

[h2zero]

Thank you for your efforts here @sanastasiou, your solution is perfectly acceptable and I greatly appreciate you bringing this to attention. This isn't the easiest library to maintain, that is for sure, this issue in particular is tricky because each chip manufacturer can implement tx power in their own way, no specification for it. I will definitely ensure you are credited with the final fix and hope you will approve the solution presented.

[sanastasiou]

@h2zero hey no worries happy to help and thanks for the only sane ble library out there 🥇