USB HID OUT endpoint trouble

[chris-se]

I'm having issues with getting a USB HID device with an OUT endpoint to work. My goal here is to use the USB HID protocol to provide a generic communication channel to a device with which I can then control the device from a PC. HID was chosen because it doesn't require drivers on any operating system, and many libraries for the PC side are available.

Environment:
Chip used: STM32F407VG (in 3.3V mode with 32.768kHz + 25MHz external oscillators)
Programming via SWD programming header with a STLink3 (official, not a clone)
Used current master branch (2021-06-22) of both STM32_XPD and USBDevice repositories

I've observed the following behavior with STM32_XPD + USBDevice:

• device enumerates correctly to the operating system
• send 64 bytes via HID to device: appears to work (send appears to be successful), but interrupt handle on device is not triggered (checked via gdb via STLink)
• receive 64 bytes via HID from device: timeout (because the handler is not executed and no report is ever sent back -- if reports are sent back from the main loop manually, the can be received by the application)
• I can repeat that as often as I like (same behavior)
• however: send less than 64 bytes via HID to device (e.g. 63 bytes): handler is executed, but second "data" argument of SetReport handler is set to NULL (seen in debugger), so garbage is sent back (but something is sent back)
• after the send with less than 64 bytes the USB stack on the device appears to have gone into some undefined state and nothing appears to work correctly anymore and I have to reset the device to perform further USB operations (the device itself is not hung totally, if I add a button and some LEDs and some simple logic, those will still work -- and the debugger just tells me that the USB interrupt isn't called anymore for some reason.

The following code was used for setup:

mcu_hwconfig.h:

#ifndef MCU_HWCONFIG_H_
#define MCU_HWCONFIG_H_

#include <xpd_usb.h>
#include <xpd_gpio.h>

#ifdef __cplusplus
extern "C"
{
#endif

#define USB_DP_PIN          PA12
#define USB_DM_PIN          PA11
#define USB_VBUS_PIN        PA9

extern void SystemClock_Config(void);
extern void HwConfig_USB_Bind(void);
extern void HwConfig_USB_Init();

#ifdef __cplusplus
}
#endif

#endif /* MCU_HWCONFIG_H_ */

mcu_hwconfig.c:

#include <xpd_rcc.h>
#include <xpd_nvic.h>

#include <usbd.h>
#include <usbd_dfu.h>
#include <usbd_hid.h>

#include "mcu_hwconfig.h"

#include <string.h>

static USB_HandleType h_usb_handle;
USB_HandleType *const g_usb_handle = &h_usb_handle;

static const RCC_PLL_InitType pllconf = {
    .State = ENABLE,
    .Source = HSE,
    .M = HSE_VALUE_Hz / 1000000,
    .N = 336,
    .P = 4,         /* 1 * 336 / 4 = PLLP =  84000000 -> SYSCLK */
    .Q = 7          /* 1 * 336 / 7 = PLLQ =  48000000 -> USB */
};

static const GPIO_InitType usb_pinconf =
{
    .Mode = GPIO_MODE_ALTERNATE,
    .Pull = GPIO_PULL_FLOAT,
    .Output.Type  = GPIO_OUTPUT_PUSHPULL,
    .Output.Speed = VERY_HIGH,
    .AlternateMap = GPIO_OTG_FS_AF10
};

/* System clocks configuration */
void SystemClock_Config(void)
{
    RCC_eHSE_Config(OSC_ON);
    RCC_ePLL_Config(&pllconf);

    /* System clocks configuration */
    RCC_eHCLK_Config(PLL, CLK_DIV1, 3);

    RCC_vPCLK1_Config(CLK_DIV2);
    RCC_vPCLK2_Config(CLK_DIV1);
}

void HwConfig_USB_Bind(void)
{
    GPIO_vInitPin(USB_DM_PIN, &usb_pinconf);
    GPIO_vInitPin(USB_DP_PIN, &usb_pinconf);
    USB_INST2HANDLE(g_usb_handle, USB_OTG_FS);
}

/** @brief USB device configuration */
const USBD_DescriptionType hdev_cfg = {
    .Vendor = {
        .Name           = "Sample Vendor https://www.example.com/",
        .ID             = 5824,
    },
    .Product = {
        .Name           = "HID Test Device",
        .ID             = 1503,
        .Version.bcd    = 0x0100,
    },
#if (USBD_SERIAL_BCD_SIZE > 0)
    .SerialNumber       = (USBD_SerialNumberType*)DEVICE_ID_REG,
#endif
    .Config = {
        .Name           = "HID Test Device",
        .MaxCurrent_mA  = 500,
        .RemoteWakeup   = 0,
        .SelfPowered    = 0,
    },
}, *const dev_cfg = &hdev_cfg;

static void HID_GetReport(void* itf, USBD_HID_ReportType type, uint8_t reportId);
static void HID_SetReport(void* itf, USBD_HID_ReportType type, uint8_t* data, uint16_t length);

__alignment(USBD_DATA_ALIGNMENT)
static const uint8_t HID_Report[] __align(USBD_DATA_ALIGNMENT) = {
    0x06, 0x00, 0xFF,
    0x0A, 0x00, 0xFF,
    0xA1, 0x01,
    0x15, 0x00,
    0x25, 0xFF,
    0x75, 0x08,
    0x95, 0x40,
    0x09, 0x01,
    0x81, 0x00,
    0x09, 0x02,
    0x91, 0x00,
    0x09, 0x03,
    0xB1, 0x02,
    0xC0
};

static const USBD_HID_ReportConfigType HID_ReportConfig = {
        .Desc = HID_Report,
        .DescLength = sizeof(HID_Report),
        .MaxId = 0,
        .Input.MaxSize = 64,
        .Input.Interval_ms = 5,
        .Output.MaxSize = 64,
        .Output.Interval_ms = 5,
};

static USBD_HID_AppType hid_app = {
    .Name = "HID Test Device",
    .SetReport = &HID_SetReport,
    .GetReport = &HID_GetReport,
    .Report = &HID_ReportConfig,
};

static USBD_HID_IfHandleType h_hid_handle = {
    .App = &hid_app,
    .Config.InEpNum = 0x81,
    .Config.OutEpNum = 0x01,
};
static USBD_HID_IfHandleType *const g_hid_handle = &h_hid_handle;

void HwConfig_USB_Init(void)
{
    NVIC_SetPriorityConfig(OTG_FS_IRQn, 1, 1);
    NVIC_SetPriorityConfig(OTG_FS_WKUP_IRQn, 1, 2);

    USBD_Init(g_usb_handle, dev_cfg);
    USBD_HID_MountInterface(g_hid_handle, g_usb_handle);

    NVIC_EnableIRQ(OTG_FS_IRQn);

    USBD_Connect(g_usb_handle);
}

void OTG_FS_IRQHandler(void)
{
    USB_vIRQHandler(g_usb_handle);
}




void HID_GetReport(void* itf, USBD_HID_ReportType type, uint8_t reportId)
{
    (void) itf;
    (void) type;
    (void) reportId;
}

void HID_SetReport(void* itf, USBD_HID_ReportType type, uint8_t* data, uint16_t length)
{
    uint8_t buffer[64];

    if (type == HID_REPORT_OUTPUT) {
        if (length > 64)
            length = 64;
        // Reverse the data, send it back
        for (int i = 0; i < length; ++i)
            buffer[length - i - 1] = data[i];
        USBD_HID_ReportIn(g_hid_handle, buffer, length);
    }
}

main.c:

#include <usbd_dfu.h>
#include <usbd_hid.h>
#include <usbd.h>

#include <xpd_flash.h>
#include <xpd_rcc.h>
#include <xpd_utils.h>
#include <xpd_gpio.h>

#include "mcu_hwconfig.h"

#include <string.h>

int main(void)
{
    /* Reset hardware to default state */
    XPD_vDeinit();
    RCC_vDeinit();

    // Basic initialization
    XPD_vInit();
    FLASH_vPrefetchBuffer(ENABLE);

    // Configure pinout
    HwConfig_USB_Bind();

    // Configure system clock
    SystemClock_Config();

    // Configure USB
    HwConfig_USB_Init();

    while (1)
        XPD_vDelay_ms(1000);
    
    return 0;
}

On the computer side, the following code via HIDAPI:

#ifdef WIN32
#include <windows.h>
#endif
#include <hidapi.h>

#include <iostream>
#include <cstddef>
#include <cstdlib>
#include <cstring>

int main()
{
    hid_device *handle;

    // Initialize the hidapi library
    (void) hid_init();

    // Open the device using the VID, PID,
    // and optionally the Serial number.
    handle = hid_open(0x16c0, 0x05df, NULL);

    // Write some data
    uint8_t buf[65];
    ssize_t len;
    memset(buf, 0, sizeof(buf));
    std::cout << "Sending: ";
    // hid_write requires the report id in the first byte,
    // since we don't use report ids, set it to 0
    // (only 64 bytes will be transferred to the HID device)
    buf[0] = 0;
    for (int i = 0; i < 64; ++i) {
        buf[i + 1] = rand();
        if (i > 0)
                std::cout << ' ';
        std::cout << int(buf[i + 1]);
    }
    std::cout << std::endl;
    len = hid_write(handle, buf, 65);
    std::cout << "Write result: " << len << std::endl;

        // Read some data
    std::cout << "Reading data..." << std::endl;
        len = hid_read(handle, buf, 64);
    std::cout << "Read result: " << len << std::endl;

    std::cout << "Received: ";
    for (int i = 0; i < len; ++i) {
        if (i > 0)
            std::cout << ' ';
        std::cout << int(buf[i]);
    }
    std::cout << std::endl;

    // Close device
    hid_close(handle);

    // Finalize the hidapi library
    (void) hid_exit();

    return 0;
}

For comparison, I created a very simple project with STM32CubeIDE and used the ST-integrated library instead of XPD and this one, and there I can get the USB HID communication to work. (The ST library has other drawbacks, which is why I attempted to use this one, because I liked the design a lot more.)

I've used the debugger via the STLink device to attempt to get more information, and step through parts of the XPD and/or USBDevice code, but I don't understand the code well enough to easily see what's wrong. (From a plain reading of the code that is triggered by the USB interrupt it seems correct to me at first glance.)

So it appears to me that either I'm using USBDevice wrong, in which case it would be great to know how, or there's a bug there, and I'm not quite sure how to find it.

If necessary I can provide more information, such as a tarball with the complete code, or USB traces on Linux/Windows, or gdb expressions taken at certain places, but the bug report is long enough as-is, so I'll wait for responses first.

Many thanks in advance!

[lukebayes]

Sorry I can't be of much help in solving this particular problem, but I wanted to check in with a couple things:

1. We're using this project on multiple physical products (with composite USB devices) that are in production and working well, so I'm sure there's a way to get it working.

2. This is a really great way to report issues, the amount of detail and tone are both super!

[benedekkupper]

Hello Christian,

First of all, cudos for taking the leap to use STM32_XPD as well as this library, I don't see many people taking it over the Cube HAL (I can't blame them as there's no full support in XPD). Secondly, let me recommend the HidReportDef macros that will make your report descriptor humanly readable. Also, I fully share Luke's opinion about your issue reporting.

With these out of the way, let's get into it:

send 64 bytes via HID to device: appears to work (send appears to be successful), but interrupt handle on device is not triggered (checked via gdb via STLink)

1. Please make sure that you have #define USBD_HID_OUT_SUPPORT 1 in your usbd_config.h
2. The HID interface can receive OUT reports via two paths:

• Through the control channel, a SetReport request (HID_REQ_SET_REPORT). This path is always available (it's buffered in CtrlData), but the host drivers usually don't use this path (for OUT reports at least, for FEATURE reports this is the only path available).
• Through the HID interface's OUT endpoint. For this path you need to provide a buffer to receive the data into, before it will be successful. The recommended way to do this is to define an Init callback for your HID application, where you call USBD_HID_ReportOut(), and to also call USBD_HID_ReportOut() from the SetReport callback if you want to keep this path always open for reception.

receive 64 bytes via HID from device: timeout (because the handler is not executed and no report is ever sent back -- if reports are sent back from the main loop manually, the can be received by the application)

When you're trying to send data the length of which matches the maximum packet size of the endpoint, it might be necessary to send a zero length packet afterwards, so the host knows that the data chunk is complete. Whether this is needed depends on multiple factors, e.g. host driver design.

however: send less than 64 bytes via HID to device (e.g. 63 bytes): handler is executed, but second "data" argument of SetReport handler is set to NULL (seen in debugger), so garbage is sent back (but something is sent back)
after the send with less than 64 bytes the USB stack on the device appears to have gone into some undefined state and nothing appears to work correctly anymore and I have to reset the device to perform further USB operations (the device itself is not hung totally, if I add a button and some LEDs and some simple logic, those will still work -- and the debugger just tells me that the USB interrupt isn't called anymore for some reason.

That is a behavior that I didn't expect. I'd like to test this myself as well, could you provide the source code for me for debugging? Thanks in advance.

[chris-se]

Hi Benedek,

many thanks for the quick response, and many thanks for the XPD and USBDevice libraries!

Your feedback was very helpfui, what was missing was that I wasn't calling USBD_HID_ReportOut() anywhere, because I didn't know that was necessary (I thought I'd just get an interrupt whenever data was available). I've now added that to the two places you suggested, and it now works as expected. :-)

Also thanks for your suggestion for HidReportDef, I'll definitely take a look at that.

I do have a quick follow-up question though: which of the following logic would you recommend?

HID_SetReport(...) {
    handle_report(...);
    // wait for next
    USBD_HID_ReportOut(...);
}

or:

int request_present;

HID_SetReport(...) {
    request_present = 1;
}

// in main loop:
while (!request_present) /* wait */;
handle_request(...);
USBD_HID_ReportOut(...);

I assume the latter is better unless handling a request is always quick, correct?

Thanks again!

As for the weird case I had when I transferred less than 64 bytes -- the weird behavior only occurs when I don't call USBD_HID_ReportOut() -- but since that function may not be called immediately after receiving a report, it could be the case that this may be trigger-able from the PC by sending to things quickly back to back (haven't tried though). If you want to reproduce this, I've uploaded a ZIP file with both the project for the MCU as well as the code that may be run on the PC to trigger it:

https://gist.github.com/chris-se/aef26cf3d9fbe76fadc7afac1dc0bf06#file-github_issue29-zip

Just unpack the ZIP file, clone the XPD_STM32 & USBDevice repositories into there (so that the XPD_STM32 directory is next to the main.c file).

To build the MCU code:

# current working directory is where this was unpacked
mkdir build
cd build
cmake -DCMAKE_TOOLCHAIN_FILE=../cmake/linux-toolchain.cmake
make
# upload hid_github29.hex

You may need to adapt linux-toolchain.cmake if your embedded compiler is not in /usr/bin or not called exactly arm-none-eabi-gcc.

My setup currently assumes that you have a STM32F407VG, so if that's not the case, you'll need to adapt a couple of things.

To build the test code for talking with the device:

# current working directory is where this was unpacked
cd test
mkdir build
cd build
cmake ..
make
# you can now run ./hid_github29_test:
# attempt to send 64 bytes:
./hid_github29_test 64
# attempt to send 63 bytes:
./hid_github29_test 63

Hope that helps with reproducing this issue.

(Note that when I do add USBD_HID_ReportOut() in the correct places the 63 byte variant works perfectly, so this only occurs if a smaller-size packet is received when USBD_HID_ReportOut() hasn't been called previously.)

[benedekkupper]

Sorry for the delayed response on your open question. How you schedule receiving the next packet depends on the use case, I usually go with a 2 page buffer, so I can already receive another report while processing the last received one hasn't been started in the super loop.

I'll leave this ticket open until I can get to investigating the 63 byte issue.

[FelixGruetzmacher]

Thank goodness for this discussion. We also didn't know about having to call USBD_HID_ReportOut() and were wondering why we weren't getting any output reports on our device. With those calls in place, the world is round again.