search

espressif / arduino-esp32

Arduino core for the ESP32
GNU Lesser General Public License v2.1
13.47k stars 7.38k forks source link

function setBatteryLevel() in BLEHIDDevice.cpp is invalid #6708

Closed BG2CRW closed 2 years ago

BG2CRW commented 2 years ago

Board

ESP32 DevModule

Device Description

a ble keyboard use ESP32 DevModule

Hardware Configuration

BLE Server

Version

v2.0.2

IDE Name

arduino ide

Operating System

win10

Flash frequency

40MHz

PSRAM enabled

no

Upload speed

921600

Description

BLEHIDDevice hid; hid->setBatteryLevel(level);

esp32实现的蓝牙键盘(server 模式)，用这种方法进行电量设置，在win10系统下显示键盘电量。 在初次连接时，电量显示正常，电量也可变化。但是断开连接后重新连接，电量始终显示为初次连接时的电量，且更改变量赋值后，win10系统显示变量仍然无变化。

I want to use this way to make my BLE keyboard change battery level in win10. but it is invalid. When I connect my keyboard first time, it is work, meanwhile, win10 can show my real-time battery level. but after disconnect and reconnect, win10 can only show its last battery level, and can't show my realt-ime battery level, when I change my battery level variable.

Sketch

BLEHIDDevice hid;
hid->setBatteryLevel(level);

I use this library https://github.com/T-vK/ESP32-BLE-Keyboard, it is also mentioned by https://github.com/T-vK/ESP32-BLE-Keyboard/issues/99

Debug Message

no debug message

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

BG2CRW commented 2 years ago

I find this page https://github.com/espressif/arduino-esp32/pull/4517, it seems to deal with this bug, but I think it doesn't work

BG2CRW commented 2 years ago

oh, I fix this bug, in BLEHIDDevice.cpp #45

this is origon:

BLE2904* batteryLevelDescriptor = new BLE2904();
batteryLevelDescriptor->setFormat(BLE2904::FORMAT_UINT8);
batteryLevelDescriptor->setNamespace(1);
batteryLevelDescriptor->setUnit(0x27ad);

m_batteryLevelCharacteristic = m_batteryService->createCharacteristic((uint16_t) 0x2a19, BLECharacteristic::PROPERTY_READ | BLECharacteristic::PROPERTY_NOTIFY);
m_batteryLevelCharacteristic->addDescriptor(batteryLevelDescriptor);
m_batteryLevelCharacteristic->addDescriptor(new BLE2902());

this is my fix:

BLE2904* batteryLevelDescriptor = new BLE2904();
batteryLevelDescriptor->setFormat(BLE2904::FORMAT_UINT8);
batteryLevelDescriptor->setNamespace(1);
batteryLevelDescriptor->setUnit(0x27ad);

m_batteryLevelCharacteristic = m_batteryService->createCharacteristic((uint16_t) 0x2a19, BLECharacteristic::PROPERTY_READ | BLECharacteristic::PROPERTY_NOTIFY);
m_batteryLevelCharacteristic->addDescriptor(batteryLevelDescriptor);
m_batteryLevelCharacteristic->addDescriptor(new BLE2904());
mrengineer7777 commented 2 years ago

https://github.com/espressif/arduino-esp32/blob/0b3f1a9fa9bb5cc4df9169fe9130f324cbcba9db/libraries/BLE/src/BLEHIDDevice.cpp#L45

Looks like you've found a possible bug. Changing from new BLE2902() to new BLE2904() causes it to work?

BG2CRW commented 2 years ago

https://github.com/espressif/arduino-esp32/blob/0b3f1a9fa9bb5cc4df9169fe9130f324cbcba9db/libraries/BLE/src/BLEHIDDevice.cpp#L45

Looks like you've found a possible bug. Changing from new BLE2902() to new BLE2904() causes it to work?

yes, it works. I don't know why. I only think it should same as "BLE2904* batteryLevelDescriptor = new BLE2904()"

SuGlider commented 2 years ago

@BG2CRW Thank you for reporting and investigating it!

I think it is neessary to investigate the issue.

Descriptor 0x2904 aka "Characteristic Presentation Format" is already added by Line 44.

Line 45 adds another Descriptor 0x2902, which is the notify descriptor - necessary for notifying Win10 that the value has changed.

BG2CRW commented 2 years ago

I’m looking forward to the right code.

BG2CRW commented 2 years ago

if line 45 is 2902, it won't change battery level since the second connect. if line 45 is 2904, it will change battery level, but if I delete the bond on my PC and reconnect it to my PC, the battery icon won't appear. I need help!

SuGlider commented 2 years ago

Based on a fast research, Battery Level Service uses 2901 and 2902, instead of 2904 and 2902.

I need to confirm it and then create the code for the 2901, which is different from 2904.

BG2CRW commented 2 years ago

Thank you. It looks like a lot of work.

BG2CRW commented 2 years ago

@SuGlider I copy BLE2904.h and BLE2904.cpp as BLE2901.h and BLE2901.h, and replace 2901 as 2904. Then change 2904 in BLEHIDDevice.cpp.
It can compile and the result is same as 2904. So it not work.

BG2CRW commented 2 years ago

hello, do you have any idea about it? I can code it by myself.

SuGlider commented 2 years ago

After some investigation, it seems that Windows only shows the battery once connected. Win10 and Win11 don't have a "real-time" update feature for the battery indicator in the Bluetooth Setting window.

Some keyboards will offer a separated application that monitors the battery and display it as an icon in the Taskbar. Some other will also pop an alert when the battery goes down, under a certain level.

But it sounds like Windows itself doesn't monitor the battery from BLE/Bluetooth devices.

BG2CRW commented 2 years ago

But, if you test it like me. first, burn it when line 45 is "2902", and connect with your WIN11. Second, change line 45 as 2904 and burn it. Then, the battery level can change real-time whenever connect it to your PC.

BG2CRW commented 2 years ago

Meanwhile, I use the NRF library to code keyboard, it can change battery level real-time. I trust that, esp32 also can change bettery level real-time on win11.

SuGlider commented 2 years ago

@BG2CRW - where in Windows you see the battery level changing? In the Bluetooth settings? Could you send a screenshot of the window where you see the battery indicator?

BG2CRW commented 2 years ago

image

BG2CRW commented 2 years ago

you can use this test .https://github.com/T-vK/ESP32-BLE-Keyboard

and change the main function as:

include "BleKeyboard.h"

BleKeyboard bleKeyboard;

void setup() { Serial.begin(115200); Serial.println("Starting BLE work!"); bleKeyboard.begin(); }

void loop() { if(bleKeyboard.isConnected()) { bleKeyboard.setBatteryLevel(0); delay(1000); bleKeyboard.setBatteryLevel(25); delay(1000); bleKeyboard.setBatteryLevel(50); delay(1000); bleKeyboard.setBatteryLevel(75); delay(1000); bleKeyboard.setBatteryLevel(100); delay(1000); }

delay(5000); }

SuGlider commented 2 years ago

OK, thanks. Question: Do you see the number (%) changing in the same window from the screenshot?

BG2CRW commented 2 years ago

step1: if line 45 is "m_batteryLevelCharacteristic->addDescriptor(new BLE2902());", you can find that the battery level change once connect, but don't change after reconnect.

step2: ”//“ line 45, and reconnect to your PC(don't need to remove from the list and rescan), you can find that it can change.

step3: if you delete the connection and rescan/connect, you can find that the battery icon disappear.

step4: you return the step1.

BG2CRW commented 2 years ago

whether you have reproduced my phenomenon？

BG2CRW commented 2 years ago

@SuGlider That depends on what line 45 is.

SuGlider commented 2 years ago

OK... I just got it to work... which is actually weird. No change to the library...

I just compiled and uploaded the code... I can see the INPUT device name ESP32 Keyboard, connected and its battery changes from 0, 25, 50, 75, 100% with the battery icon changing as well (empty to full). When it is 0%, a little window shows up in the bottom right corner saying that the keyboard has no battery.

In other words... it works and I just can't reproduce the issue. It took a while to connect to show up and connect to the Windows 11...

Important detail:

If I reset the ESP32 board, it says it has reconnected, but the Battery indicator stops updating. I have to remove it from the paired devices and then scan it again, connect it again and then it works fine.

This is the final sketch (few changes to the one you sent me):

#include "BleKeyboard.h"

BleKeyboard bleKeyboard;

void setup() {
  Serial.begin(115200);
  Serial.println("Starting BLE work!");
  bleKeyboard.begin();
}

void loop() {
  if (bleKeyboard.isConnected()) {
    Serial.println("Setting Level to 0%");
    bleKeyboard.setBatteryLevel(0);
    delay(2000);
    Serial.println("Setting Level to 25%");
    bleKeyboard.setBatteryLevel(25);
    delay(2000);
    Serial.println("Setting Level to 50%");
    bleKeyboard.setBatteryLevel(50);
    delay(2000);
    Serial.println("Setting Level to 75%");
    bleKeyboard.setBatteryLevel(75);
    delay(2000);
    Serial.println("Setting Level to 100%");
    bleKeyboard.setBatteryLevel(100);
    delay(2000);
  } else {
    Serial.println("Not connected yet...");
  }
  Serial.println("Pause of 5 seconds...");
  delay(5000);
}
SuGlider commented 2 years ago

This is in Portuguese... Google Lens can translate it from the image, if necessary. I used it to read the details in your screenshot. ;-)

image

SuGlider commented 2 years ago

In summary:

It seems it is necessary to remove the ESP32 Keyboard from the list of paired devices and then scan/add it again in order to display and to update the Battery Indicator. Not sure if this is an ESP32 issue or a Windows issue...

BG2CRW commented 2 years ago

Yes, I want to say is that need to scan/add it again in order to display and to update the Battery Indicator. Only reset ESP32,the Battery Indicator won't update.

BG2CRW commented 2 years ago

BUT, this time, you can try to do the step2: ”//“ line 45 in BLEDevice.cpp, and reconnect to your PC, you can find that it can update.

BG2CRW commented 2 years ago

I'm sure it is the problem in ESP32. Because my NRF52840 works well. Although I haven't read its library.

SuGlider commented 2 years ago

OK. I'll work in this issue and try to find out what is going on. Can you post the NFR HID source code link that you have used?

SuGlider commented 2 years ago

I think that the issue may be in the BLE bonding process. It may be missing in the ESP32 protocol layer. I have enabled some debug messages and there are some request that the ESP32 doesn't know how to answer. It is possible that Windows demands strong bonding when pairing by exchanging encrypted keys.

If this is the case... it may take a big effort and time.

SuGlider commented 2 years ago

BLE bonding has to do with reconnection and running BLE like if it had never disconnected before, in a seamless way. Everything points to it, given that it only works on the first Pairing attempt and it fails when reconnecting.

Changing or commenting out Line 45 doesn't seem to be the way to solve it definitively. Battery Level Service needs the 2902 characteristic. It just doesn't need the 2904.

BG2CRW commented 2 years ago

Yes, I know it is not a right way. Now, my project use this feature to recognize if this is a new connect and comment or not line 45 to solve this problem for my product users. This is a huge bug for my product users, but I can't take back and change their motherboard.

BG2CRW commented 2 years ago

this is ble_bas.c in NRF sdk.

/**

define NRF_LOG_MODULE_NAME ble_bas

if BLE_BAS_CONFIG_LOG_ENABLED

define NRF_LOG_LEVEL BLE_BAS_CONFIG_LOG_LEVEL

define NRF_LOG_INFO_COLOR BLE_BAS_CONFIG_INFO_COLOR

define NRF_LOG_DEBUG_COLOR BLE_BAS_CONFIG_DEBUG_COLOR

else // BLE_BAS_CONFIG_LOG_ENABLED

define NRF_LOG_LEVEL 0

endif // BLE_BAS_CONFIG_LOG_ENABLED

include "nrf_log.h"

NRF_LOG_MODULE_REGISTER();

define INVALID_BATTERY_LEVEL 255

/*@brief Function for handling the Write event.

void ble_bas_on_ble_evt(ble_evt_t const p_ble_evt, void p_context) { if ((p_context == NULL) || (p_ble_evt == NULL)) { return; }

ble_bas_t * p_bas = (ble_bas_t *)p_context;

switch (p_ble_evt->header.evt_id)
{
    case BLE_GATTS_EVT_WRITE:
        on_write(p_bas, p_ble_evt);
        break;

    default:
        // No implementation needed.
        break;
}

}

/*@brief Function for adding the Battery Level characteristic.

ret_code_t ble_bas_init(ble_bas_t p_bas, const ble_bas_init_t p_bas_init) { if (p_bas == NULL || p_bas_init == NULL) { return NRF_ERROR_NULL; }

ret_code_t err_code;
ble_uuid_t ble_uuid;

// Initialize service structure
p_bas->evt_handler               = p_bas_init->evt_handler;
p_bas->is_notification_supported = p_bas_init->support_notification;
p_bas->battery_level_last        = INVALID_BATTERY_LEVEL;

// Add service
BLE_UUID_BLE_ASSIGN(ble_uuid, BLE_UUID_BATTERY_SERVICE);

err_code = sd_ble_gatts_service_add(BLE_GATTS_SRVC_TYPE_PRIMARY, &ble_uuid, &p_bas->service_handle);
VERIFY_SUCCESS(err_code);

// Add battery level characteristic
err_code = battery_level_char_add(p_bas, p_bas_init);
return err_code;

}

/*@brief Function for sending notifications with the Battery Level characteristic.

ret_code_t ble_bas_battery_level_update(ble_bas_t * p_bas, uint8_t battery_level, uint16_t conn_handle) { if (p_bas == NULL) { return NRF_ERROR_NULL; }

ret_code_t         err_code = NRF_SUCCESS;
ble_gatts_value_t  gatts_value;

if (battery_level != p_bas->battery_level_last)
{
    // Initialize value struct.
    memset(&gatts_value, 0, sizeof(gatts_value));

    gatts_value.len     = sizeof(uint8_t);
    gatts_value.offset  = 0;
    gatts_value.p_value = &battery_level;

    // Update database.
    err_code = sd_ble_gatts_value_set(BLE_CONN_HANDLE_INVALID,
                                      p_bas->battery_level_handles.value_handle,
                                      &gatts_value);
    if (err_code == NRF_SUCCESS)
    {
        NRF_LOG_INFO("Battery level has been updated: %d%%", battery_level)

        // Save new battery value.
        p_bas->battery_level_last = battery_level;
    }
    else
    {
        NRF_LOG_DEBUG("Error during battery level update: 0x%08X", err_code)

        return err_code;
    }

    // Send value if connected and notifying.
    if (p_bas->is_notification_supported)
    {
        ble_gatts_hvx_params_t hvx_params;

        memset(&hvx_params, 0, sizeof(hvx_params));

        hvx_params.handle = p_bas->battery_level_handles.value_handle;
        hvx_params.type   = BLE_GATT_HVX_NOTIFICATION;
        hvx_params.offset = gatts_value.offset;
        hvx_params.p_len  = &gatts_value.len;
        hvx_params.p_data = gatts_value.p_value;

        if (conn_handle == BLE_CONN_HANDLE_ALL)
        {
            ble_conn_state_conn_handle_list_t conn_handles = ble_conn_state_conn_handles();

            // Try sending notifications to all valid connection handles.
            for (uint32_t i = 0; i < conn_handles.len; i++)
            {
                if (ble_conn_state_status(conn_handles.conn_handles[i]) == BLE_CONN_STATUS_CONNECTED)
                {
                    if (err_code == NRF_SUCCESS)
                    {
                        err_code = battery_notification_send(&hvx_params,
                                                             conn_handles.conn_handles[i]);
                    }
                    else
                    {
                        // Preserve the first non-zero error code
                        UNUSED_RETURN_VALUE(battery_notification_send(&hvx_params,
                                                                      conn_handles.conn_handles[i]));
                    }
                }
            }
        }
        else
        {
            err_code = battery_notification_send(&hvx_params, conn_handle);
        }
    }
    else
    {
        err_code = NRF_ERROR_INVALID_STATE;
    }
}

return err_code;

}

ret_code_t ble_bas_battery_lvl_on_reconnection_update(ble_bas_t * p_bas, uint16_t conn_handle) { if (p_bas == NULL) { return NRF_ERROR_NULL; }

ret_code_t err_code;

if (p_bas->is_notification_supported)
{
    ble_gatts_hvx_params_t hvx_params;
    uint16_t               len = sizeof(uint8_t);

    memset(&hvx_params, 0, sizeof(hvx_params));

    hvx_params.handle = p_bas->battery_level_handles.value_handle;
    hvx_params.type   = BLE_GATT_HVX_NOTIFICATION;
    hvx_params.offset = 0;
    hvx_params.p_len  = &len;
    hvx_params.p_data = &p_bas->battery_level_last;

    err_code = battery_notification_send(&hvx_params, conn_handle);

    return err_code;
}

return NRF_ERROR_INVALID_STATE;

}

endif // NRF_MODULE_ENABLED(BLE_BAS)

BG2CRW commented 2 years ago

BLE_UUID_REPORT_REF_DESCR is 0x2908

SuGlider commented 2 years ago

@BG2CRW, 0x2908 is related to HID only, not to Battery Level. Thanks for sending all this information!

It is now fixed by setting the notification to 1 by default on the Battery Level Service creation in the ESP32 BLE Arduino Library. Check #6864 for details.

BG2CRW commented 2 years ago

Thanks a lot!