Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows reconnect error - ESP32-BLE-Gamepad #850

Open
lemmingDev opened this issue Jan 10, 2025 · 16 comments
Open

Windows reconnect error - ESP32-BLE-Gamepad #850

lemmingDev opened this issue Jan 10, 2025 · 16 comments

Comments

@lemmingDev
Copy link

Hi @h2zero

The latest NimBLE-Arduino 2.1.3 is working with ESP32-BLE-Gamepad after a couple of users added some patches to the library

One issue we seem to have now is that on Windows (tested on Windows 10 and 11), it works fine when you pair it the first time, however when you power the board down and it automatically reconnects, gamepad input no longer works.

The main loop seems to be working as any serial output or LEDs modified in the sketch still update.

Android and perhaps Linux seem to work fine still

If you've got the time; any advice on what might need to be changed?

There is a thread with some testing feedback here: lemmingDev/ESP32-BLE-Gamepad#251

Thanks

@h2zero
Copy link
Owner

h2zero commented Jan 10, 2025

Thanks @lemmingDev, I will try to test this soon. In the meantime maybe @afpineda has some ideas?

@RaazP
Copy link

RaazP commented Jan 10, 2025

If you don't have the time to read through the linked thread, this is the only difference, when enabling the "info" flags in the nimconfig.h:

I NimBLEServer: subscribe event; attr_handle=39, subscribed: true

On re-connect, this line is missing.
When setting the flags to error/warning/critical, nothing will show.
On "debug", there's.. a lot and since I can't copy it all in once from the Arduino IDE to Notepad++, I can't make out any differences, sorry.

And what's weird: You can flash an extremely simple BLE-Gamepad button sketch, but let it tell Windows it would have 20+ buttons (not actually setting them up).
You can then flash any more complex sketch but don't "forget" the device in Windows and as long as you kept the device name identical, it will re-connect just fine forever, even after rebooting the PC.

Thanks in advance for looking into this!

@h2zero
Copy link
Owner

h2zero commented Jan 10, 2025

Hmmm, that sounds like the cccd isn't being saved, maybe need to increase CONFIG_BT_NIMBLE_MAX_CCCDS

@lemmingDev
Copy link
Author

Hmmm, that sounds like the cccd isn't being saved, maybe need to increase CONFIG_BT_NIMBLE_MAX_CCCDS

Set it to 32, but didn't fix the issue unfortunately

@h2zero
Copy link
Owner

h2zero commented Jan 11, 2025

Thanks @lemmingDev @RaazP, I was able to reproduce this and have found that the error originates in the stack, some error with the storage of the subscription state when windows subscribes to a characteristic, instead of 1 or 2 as is should be, the value is 128. Please bare with me while I troubleshoot, this area of the code is not easy to navigate.

@lemmingDev
Copy link
Author

lemmingDev commented Jan 11, 2025

Thanks for the update!

In my own testing just now, with some sketches working on reconnect, and others not, only difference I could really see in the error log was:

In a sketch where reconnecting worked:

D NimBLEAdvertising: setAdvertisementData: 02 01 06 03 19 c4 03 17 09 42 4c 45 20 44 72 69 76 69 6e 67 20 43 6f 6e 74 72 6f 6c 6c 65 72
D NimBLEAdvertising: << Advertising start
[  1323][D][BleGamepad.cpp:1508] taskServer(): [BLEGamepad] Advertising started!
D NimBLEServer: >> handleGapEvent: 
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
D NimBLEServerCallbacks: onConnParamsUpdate: default
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
D NimBLEServerCallbacks: onAuthenticationComplete: default
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
I NimBLEServer: subscribe event; attr_handle=8, subscribed: true
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
I NimBLEServer: subscribe event; attr_handle=44, subscribed: true
D NimBLECharacteristicCallbacks: onSubscribe: default
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
I NimBLEServer: subscribe event; attr_handle=39, subscribed: true
D NimBLECharacteristicCallbacks: onSubscribe: default
D NimBLEServer: << handleGapEvent
D NimBLEServer: >> handleGapEvent: 
I NimBLEServer: mtu update event; conn_handle=1 mtu=255
D NimBLEServerCallbacks: onMTUChange(): Default
D NimBLEServer: << handleGapEvent

and then went on to respond to button presses logging the following output for each:

D NimBLEServer: Gatt Read event
D NimBLEServer: >> handleGapEvent: 
D NimBLECharacteristicCallbacks: onStatus: default
D NimBLEServer: << handleGapEvent

But when it didn't work correctly on reconnection, it looked pretty much the same, except it seemed to be missing:

D NimBLEServer: >> handleGapEvent: 
I NimBLEServer: subscribe event; attr_handle=39, subscribed: true
D NimBLECharacteristicCallbacks: onSubscribe: default
D NimBLEServer: << handleGapEvent

and then no Gatt Read events were logged

Basically exactly the same as what @RaazP found

@afpineda
Copy link
Contributor

afpineda commented Jan 11, 2025

Thanks @lemmingDev, I will try to test this soon. In the meantime maybe @afpineda has some ideas?

In fact, I was already aware of this bug.
However, I managed to get rid of it after a computer reboot. So, I thought it was in the Windows side of things.
Another sympton pointing to Windows is that, some time ago, I was experiencing exactly the same behavior in Aurduino-ESP32 (not this library). Reported here. I found a workarround in this case (appart from switching to this library).

Regarding this issue, please note that input reports are not fully implemented according to the HID over GATT specification.
The 0x2902 characteristic descriptor is missing.
I was reluctant to fix this because what they say: "if it works, don't touch anything!!!".

@afpineda
Copy link
Contributor

In case you are wondering, adding the 0x2902 descriptor does not fix this issue. I already tried.

  /*
     * Descriptor 2902 is required by the HID over GATT spec
     * (input reports only), but it works without it.
    */

    NimBLEDescriptor *p2902 = inputReportChr->createDescriptor(
        hidReport2902DscUuid,
        NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC,
        2);
    uint8_t desc2_val[] = {0b01, 0}; // Notifications enabled by default
    p2902->setValue(desc2_val, 2);

@MaksMacioszczyk
Copy link

In case you are wondering, adding the 0x2902 descriptor does not fix this issue. I already tried.

For me, adding this descriptor kind of works. When I additionally restart Bluetooth on the Windows side after ESP32 was rebooted, I have working connection again, but it is still not the fix for me.

@afpineda
Copy link
Contributor

And what's weird: You can flash an extremely simple BLE-Gamepad button sketch, but let it tell Windows it would have 20+ buttons (not actually setting them up).
You can then flash any more complex sketch but don't "forget" the device in Windows and as long as you kept the device name identical, it will re-connect just fine forever, even after rebooting the PC.

It's a well known fact that Windows caches the HID descriptor both in USB and BLE. It's not reloaded each time the device is connected.

@h2zero
Copy link
Owner

h2zero commented Jan 11, 2025

In case you are wondering, adding the 0x2902 descriptor does not fix this issue. I already tried.

  /*
     * Descriptor 2902 is required by the HID over GATT spec
     * (input reports only), but it works without it.
    */

    NimBLEDescriptor *p2902 = inputReportChr->createDescriptor(
        hidReport2902DscUuid,
        NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC,
        2);
    uint8_t desc2_val[] = {0b01, 0}; // Notifications enabled by default
    p2902->setValue(desc2_val, 2);

This probably didn't work because NimBLE needs to create the descriptor, just adding the NIMBLE_PROPERTY::NOTIFY property to the characteristic would do it.

@afpineda
Copy link
Contributor

afpineda commented Jan 11, 2025

This probably didn't work because NimBLE needs to create the descriptor, just adding the NIMBLE_PROPERTY::NOTIFY property to the characteristic would do it.

I know. The input report characteristic is created adding that property, but the host computer can choose not to receive notifications and/or indications using the 0x2902 descriptor. Its desire should be respected, but that is another story.

The full code follows (this is "what if" code, not production code):

NimBLECharacteristic *NimBLEHIDDeviceFix::getInputReport(uint8_t reportId)
{
    NimBLECharacteristic *inputReportChr =
        getHidService()->createCharacteristic(
            hidReportChrUuid,
            NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::NOTIFY | NIMBLE_PROPERTY::READ_ENC);

    NimBLEDescriptor *inputReportDsc =
        inputReportChr->createDescriptor(
            hidReportChrDscUuid,
            NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::READ_ENC,
            2);
    uint8_t desc1_val[] = {reportId, 0x01};
    inputReportDsc->setValue(desc1_val, 2);

    /*
     * Descriptor 2902 is required by the HID over GATT spec
     * (input reports only), but it works without it.
     * Uncomment the following lines if you want to add it.
    */

    NimBLEDescriptor *p2902 = inputReportChr->createDescriptor(
        hidReport2902DscUuid,
        NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC,
        2);
    uint8_t desc2_val[] = {0b01, 0}; // Notifications enabled by default
    p2902->setValue(desc2_val, 2);

    return inputReportChr;
} 

@afpineda
Copy link
Contributor

afpineda commented Jan 11, 2025

@h2zero, This excerpt from your migration guide was unknown to me:

Descriptors are now created using the NimBLECharacteristic::createDescriptor method.
BLE2902 or NimBLE2902 class has been removed.
NimBLE automatically creates the 0x2902 descriptor if a characteristic has a notification or indication property assigned to it.
It was no longer useful to have a class for the 0x2902 descriptor as a new callback NimBLECharacteristicCallbacks::onSubscribe was added to handle callback functionality and the client subscription status is handled internally.

Note: Attempting to create a 0x2902 descriptor will trigger a warning message and flag it internally as removed and will not be functional.

So, is the 0x2902 descriptor already there ?

Update:
I searched along the code base, but I was not able to locate the piece of code where the 0x2902 descriptor is created.

It should be created here :

NimBLECharacteristic::NimBLECharacteristic(const NimBLEUUID& uuid, uint16_t properties, uint16_t maxLen, NimBLEService* pService)
    : NimBLELocalValueAttribute{uuid, 0, maxLen}, m_pCallbacks{&defaultCallback}, m_pService{pService} {
    setProperties(properties);
} // NimBLECharacteristic

It should check for NIMBLE_PROPERTY::NOTIFY or INDICATE, but setProperties() is inherited from NimBLELocalValueAttribute and does note create the descriptor:

void setProperties(uint16_t properties) { m_properties = properties; }

I'm lost here. I can confirm that any attempt to create the descriptor is blocked, so my previous code is useless:

 NimBLEDescriptor::NimBLEDescriptor(const NimBLEUUID& uuid, uint16_t properties, uint16_t max_len, NimBLECharacteristic* pCharacteristic)
    : NimBLELocalValueAttribute{uuid, 0, max_len}, m_pCallbacks{&defaultCallbacks}, m_pCharacteristic{pCharacteristic} {
    // Check if this is the client configuration descriptor and set to removed if true.
    if (uuid == NimBLEUUID((uint16_t)0x2902)) {
        NIMBLE_LOGW(LOG_TAG, "Manually created 2902 descriptor has no functionality; please remove.");
        setRemoved(NIMBLE_ATT_REMOVE_HIDE);
    }
...

@h2zero
Copy link
Owner

h2zero commented Jan 11, 2025

@afpineda The descriptor is created automatically in the NimBLE Host, not in this wrapper. All that is is required is to add NIMBLE_PROPERTY::NOTIFY to the characteristic

@h2zero
Copy link
Owner

h2zero commented Jan 11, 2025

Original issue should be resolved in lemmingDev/ESP32-BLE-Gamepad#257

This is an upstream oversight that I will pursue a long term solution to, the workaround in the above PR seems to be satisfactory for now.

@afpineda
Copy link
Contributor

I confirm that calling notify() from NimBLEServerCallback::onConnect() or NimBLEServerCallback::onAuthenticationComplete() will cause this issue. I suggest to warn users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants