Creating an HID output report doesn't work

[TheCrypt0]

Hi,
I have a firmware that should emulate a BLE keyboard (and receive back the LEDs status for caps lock, num lock, etc.) but after updating to version 2.0.2, I cannot attach the callbacks to the output report anymore because getOutputReport returns nullptr.

The setup is very similar to this (taken from https://github.dev/Mystfit/ESP32-BLE-CompositeHID ):

void KeyboardDevice::init(NimBLEHIDDevice* hid)
{
    _input = hid->getInputReport(_config.getReportId());
    _mediaInput = hid->getInputReport(MEDIA_KEYS_REPORT_ID);
    _output = hid->getOutputReport(_config.getReportId());
    _callbacks = new KeyboardCallbacks(this);
    _output->setCallbacks(_callbacks);

    setCharacteristics(_input, _output);
}

By debugging the code, it's clear where the code fails:

/**
 * @brief Get the output report characteristic.
 * @param [in] reportId Output report ID, the same as in report map for output object related to the characteristic.
 * @return NimBLECharacteristic* A pointer to the output report characteristic.
 *                               Store this value to avoid computational overhead.
 * @return nullptr If the report is already created as an input or feature report.
 * @details This will create the characteristic if not already created.
 */
NimBLECharacteristic* NimBLEHIDDevice::getOutputReport(uint8_t reportId) {
    uint8_t               reportType;
    NimBLECharacteristic* outputReportChr = locateReportCharacteristicById(reportId, reportType);
    if ((outputReportChr != nullptr) && (reportType != 0x02)) // <------ **HERE**, the reportType is never 0x02
        // ERROR: this reportId exists, but it is not an output report
        return nullptr;
    if (outputReportChr == nullptr) {
        outputReportChr =
            m_hidSvc->createCharacteristic(inputReportChrUuid,
                                           NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::WRITE_NR |
                                               NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);
        NimBLEDescriptor* outputReportDsc = outputReportChr->createDescriptor(
            featureReportDscUuid,
            NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);
        uint8_t desc1_val[] = {reportId, 0x02};
        outputReportDsc->setValue(desc1_val, 2);
    }

    return outputReportChr;
} // getOutputReport

Here's how the same function was in version 1.4.x.

/**
 * @brief Create output report characteristic
 * @param [in] reportID Output report ID, the same as in report map for output object related to the characteristic
 * @return Pointer to new output report characteristic
 */
NimBLECharacteristic* NimBLEHIDDevice::outputReport(uint8_t reportID) {
	NimBLECharacteristic *outputReportCharacteristic = m_hidService->createCharacteristic((uint16_t)0x2a4d, NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::WRITE_NR | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);
	NimBLEDescriptor *outputReportDescriptor = outputReportCharacteristic->createDescriptor((uint16_t)0x2908, NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);

	uint8_t desc1_val[] = {reportID, 0x02};
	outputReportDescriptor->setValue((uint8_t*)desc1_val, 2);

	return outputReportCharacteristic;
}

Do you know if this is something fixable from my part or it needs an update to the library? Thank you, I really appreciate your support.

[h2zero]

@afpineda Any thoughts?

[afpineda]

Hi, @TheCrypt0.

This is the way the API is intended to work: documented here.

Returns

NimBLECharacteristic* A pointer to the output report characteristic. Store this value to avoid computational overhead.
nullptr If the report is already created as an input or feature report.

You can not assign the same report ID to two different reports (input and output).
These are the offending lines:

   _input = hid->getInputReport(_config.getReportId());
   ...
   _output = hid->getOutputReport(_config.getReportId());

If you need to read and write from/to the very same report, you should create it as a feature report.
Note that all reports must match your HID descriptor.

[afpineda]

Let me rectify my previous answer in some way.

You can not assign the same report ID to two different reports (input and output).

Even if that is true, we should not judge how the library is used.
The library should be liberal in what it accepts (see Postel's law).
In this sense, I agree to remove the nullptr return value if you also agree.

[TheCrypt0]

@h2zero @afpineda

Thank you for the reply! So saying:

You can not assign the same report ID to two different reports (input and output).

Means the BLE HID standard doesn't "allow" it to happen and we should have 2 report IDs, one for sending the keystrokes and the other to receive back the LEDs statues?

Please correct me if I'm wrong, I'm reading a book about BLE but I'm far from what you guys know.

Is it possible that the ESP32-BLE-CompositeHID project is getting this wrong?

(Btw happy new year 🎉 to y'all)

[afpineda]

Means the BLE HID standard doesn't "allow" it to happen and we should have 2 report IDs, one for sending the keystrokes and the other to receive back the LEDs statues?

Yes: create an input report, let' say ID 1, for sending the keystrokes, and an output report, let's say ID 2, to receive LED states.

Means the BLE HID standard doesn't "allow" it to happen and we should have 2 report IDs

Yes, but in an indirect way. You cannot declare a report ID twice in the HID descriptor. You cannot declare a single report as both input and output.

If your device was working prior to version 2.1.0, please post your HID descriptor. Maybe I am wrong.

Check your HID descriptor first. I recommend to use waratah to avoid mistakes.

Additional note: you could also inspect the HID descriptor from a commercial keyboard to learn how it works.
This is a screenshot from my own keyboard. Note there is at least one input report and one output report:

[afpineda]

@TheCrypt0

After looking at the hid descriptor you pointed, I definetly was wrong.
You can have input and output data in the same report (which I didn't know).
I will send a pull request to fix this.

[afpineda]

I have submitted a PR to the h2zero/NimBLE-Arduino project.

[TheCrypt0]

@afpineda

I tested your PR in a local project but unfortunately it doesn't work on my side. After the first call to getInputReport(ID) and then getOutputReport(ID) the pointer returned is for the same characteristic.

I can confirm and tested that the function locateReportCharacteristicByIdAndType called in getOutputReport keeps returning the input Characteristic for whatever reason instead of returning nullptr and allowing the function to create the correct one.

I managed to make it work with this ugly hack:

NimBLECharacteristic* NimBLEHIDDevice::getOutputReport(uint8_t reportId) {
    // NimBLECharacteristic* outputReportChr = locateReportCharacteristicByIdAndType(reportId, 0x02);
    // UGLY HACK: force outputReportChr to nullptr 
    NimBLECharacteristic* outputReportChr = nullptr;
    if (outputReportChr == nullptr) {
        outputReportChr =
            m_hidSvc->createCharacteristic(inputReportChrUuid,
                                           NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::WRITE_NR |
                                               NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);
        NimBLEDescriptor* outputReportDsc = outputReportChr->createDescriptor(
            featureReportDscUuid,
            NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE | NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::WRITE_ENC);
        uint8_t desc1_val[] = {reportId, 0x02};
        outputReportDsc->setValue(desc1_val, 2);
    }

    return outputReportChr;
} // getOutputReport

Hope this helps.

(btw: I checked my Logitech keyboard and I can confirm it has both the output and input report in the same Report ID)

---

NVM, found the issue in locateReportCharacteristicByIdAndType. It's probably just a mistake.

NimBLECharacteristic* NimBLEHIDDevice::locateReportCharacteristicByIdAndType(uint8_t reportId, uint8_t reportType) {
    NimBLECharacteristic* candidate = m_hidSvc->getCharacteristic(inputReportChrUuid, 0);
    for (uint16_t i = 1; (candidate != nullptr) && (i != 0); i++) {
        NimBLEDescriptor* dsc           = candidate->getDescriptorByUUID(featureReportDscUuid);
        NimBLEAttValue    desc1_val_att = dsc->getValue();
        const uint8_t*    desc1_val     = desc1_val_att.data();
        //reportType                      = desc1_val[1]; <----- REMOVE THIS
        if ((desc1_val[0] == reportId) && (desc1_val[1] == reportType)) return candidate;
        candidate = m_hidSvc->getCharacteristic(inputReportChrUuid, i);
    }
    return nullptr;
}

[afpineda]

NVM, found the issue in locateReportCharacteristicByIdAndType. It's probably just a mistake.

Yes. It is.
Fixed right now.

We lack a set of test units.

[h2zero]

@TheCrypt0 This should be resolved in aae70df please close this issue if it is resolved.