USBH_HID_SetProtocol() protocol parameter

[JMarq.1]

Hi,

I am using my own board based on an STM32F207ZG and STM32F2 HAL v1.9.4.

I implemented an HID Host using STM32CubeIDE (v1.19.0), simply selecting USB Host and HID class support in CubeMX and generating the code.

I was able to read key events from a standard USB desktop keyboard. However, I was not able to read any key events from a USB HID RF tag reader.

While debugging usbh_hid.c, I noticed the following:

    USBH_HID_SetProtocol(phost, 0U); generates a SET_PROTOCOL request with wValue = 0x0001  (Report Protocol)

whereas:

    USBH_HID_SetProtocol(phost, 1U); generates wValue = 0x0000  (Boot Protocol).

because this implementation:

if (protocol)
{
    phost->Control.setup.b.wValue.w = 0U;
}
else
{
    phost->Control.setup.b.wValue.w = 1U;
}

In other words, the protocol parameter passed to USBH_HID_SetProtocol() is inverted with respect to the value actually transmitted in the USB SET_PROTOCOL request URB.

I noticed that the same implementation is still present in STM32F2 HAL v1.9.6.

I am not saying that this is necessarily a bug, since the current implementation may be intentional and changing the meaning of the parameter would affect existing applications. However, I find the current naming/semantics somewhat confusing, especially because a call such as:

   USBH_HID_SetProtocol(phost, 0U);

actually selects Report Protocol, while passing 1U selects Boot Protocol.

 

Has anyone else noticed this behavior or encountered confusion/problems related to it?

Thanks for sharing your experience.

[William Romero]

Interesting observation. The inverted parameter behavior could definitely cause confusion, especially when debugging HID devices that behave differently between Boot and Report Protocol modes. I think documenting the parameter semantics more clearly would help developers avoid assuming that the function argument directly matches the USB SET_PROTOCOL wValue. It would also be useful to know whether this behavior is consistent across the other STM32 HAL versions.

[FBL]

Hi ​@JMarq.1 

Thank you for reporting the issue. 
An internal ticket CDM0065375 is submitted to dedicated team to correct the parameters of API 

[JMarq.1]

Hi ​@FBL 

I am not explicitly mentioning that this is a bug or issue. I just wanted to notice that this confused me (and maybe others) and maybe a more clarified explanation on the comments about what exactly does 0 and 1 as protocol parameter do.

BE AWARE: If the implementation is changed so that passing to the funciont a 0U means Boot Protocol and 1U means Report Protocol, this would also change the behavior of existing generated applications, with possibly terrible consecuences.

Therefore, I beg to think on the compatibility implications while reviewing this API parameters.

Thank you for forwarding this to the dedicated team.

 

[JMarq.1]

I noticed in the following link by JW that 7 years ago this behaviour seems to be detected and strange:

​@waclawek.jan said: “The "library" seeks for boot keyboard but then sets it to report mode, and even requests the report descriptor for it.”

 

 

[FBL]

Hi ​@JMarq.1 

The code is consistent with that internal MW naming, but the parameter name is confusing.
 

About Jan’s post, the stack’s behavior is only consistent if it fully transitions to report mode and then parses reports accordingly. If it still later processes data as if it were boot format keyboard data, then that mismatch could explain the strange behavior seen with some keyboards.

[JMarq.1]

Hi ​@FBL 

Two questions:

1. How can I modify usbh_hid.c code to force BootProtocol for my embedded HID host instead of the default 0U value that is hardcoded by HAL generated code and keep this modification between code generations? 

Something like:

static USBH_StatusTypeDef USBH_HID_ClassRequest(USBH_HandleTypeDef *phost)
{
   ...
      /* USER CODE BEGIN SET PROTOCOL */
    	uint8_t activateBootProtocol = FALSE;
      /* USER CODE END SET PROTOCOL */

      if (USBH_HID_SetProtocol(phost, activateBootProtocol) == USBH_OK)
   ...
}

Then I would be able to change the code for choosing BootProtocol that is my desired intention.

      /* USER CODE BEGIN SET PROTOCOL */
        uint8_t activateBootProtocol = TRUE;
      /* USER CODE END SET PROTOCOL */

 

2: Could ST possibly clarify in the routine signature the values accepted? I mean indicating what passing 0U or 1U does:
 

/**
  * @brief  USBH_Set_Protocol
  *         Set protocol State.
  * @param  phost: Host handle
  * @param  protocol : Set Protocol for HID : 1U=boot / 0U=report protocol
  * @retval USBH Status
  */
USBH_StatusTypeDef USBH_HID_SetProtocol(USBH_HandleTypeDef *phost,
                                        uint8_t protocol)

 

[FBL]

I agree ​@JMarq.1 

It would indeed make it easier to control for host applications that need to set Boot Protocol, while also keeping the choice visible and maintainable. Also, explicitly stating what 0U and 1U mean would help avoid ambiguity.

Thanks again for taking the time to report this

[Pavel A.]

Note that the “classic” USBH library does not have USBH_HID_GetProtocol at all. Only USBH_HID_SetProtocol. The “get” request does not exist in the USB HID spec.

https://github.com/STMicroelectronics/stm32-mw-usb-host/blob/a5cc3798cd50f6b86322e7091ccf7e3eadd5be47/Class/HID/Src/usbh_hid.c#L691

The device type (HID_KEYBRD_BOOT_CODE) and support for “boot” mode is detected in the interface descriptor

https://github.com/STMicroelectronics/stm32-mw-usb-host/blob/a5cc3798cd50f6b86322e7091ccf7e3eadd5be47/Class/HID/Src/usbh_hid.c#L179

and it is not the same value as for the HID Set Protocol request. This looks like the root cause for the confusion.

 

https://github.com/STMicroelectronics/stm32-mw-usb-host/blob/a5cc3798cd50f6b86322e7091ccf7e3eadd5be47/Class/HID/Src/usbh_hid.c#L766

TL;DR use this library code for prototyping, then get real and find (or write) production level code.

HID device that supports “report” mode and has HID report descriptor starts in the report mode by default - even if the device descriptor has  bInterfaceSubClass =1 (“boot” interface). This value only indicates that the “boot” mode is supported.

(Note: this info is from AI!  Being lazy, hasn’t checked in the full HID spec).

To activate the “boot” mode, host sends Set Interface with wValue=0 - or as the OP proposes, USBH_HID_SetProtocol(phost, activateBootProtocol).  Again, some examples skip this step.

 

​@William Romero 

>  It would also be useful to know whether this behavior is consistent across the other STM32 HAL versions.

The USB “classic middleware” is common for several STM32 packages (included as a git submodule). The code is literally same.

 

[JMarq.1]

Thank you @FBL and @Pavel A. for your replies.

I also believe there are other changes worth considering before getting to Set_Protocol, as Set_Protocol needs to be set after the correct Boot Keyboard interface is found and selected.

If a system supports only Boot Protocol-capable keyboards (no mice, for example because the GUI does not support mouse interaction), I think it would be preferable to search specifically for the HID_KEYBRD_BOOT_CODE interface.

Default usbh_hid.c code:

// File usbh_hid.c:

interface = USBH_FindInterface(phost,
                               USBH_HID_CLASS,
                               HID_BOOT_CODE,
                               0xFFU);  /* Thiis selects the first KEYBOARD or MOUSE interface */

The 0xFFU protocol parameter means that the first HID Boot interface is selected, regardless of whether it is a Boot Keyboard or Boot Mouse.

Suggested usbh_hid.c code for a system that supports Boot Keyboards only, to explicitly search for the Boot Keyboard interface:

// File usbh_hid.c:

interface = USBH_FindInterface(phost,
                               USBH_HID_CLASS,
                               HID_BOOT_CODE,
                               HID_KEYBRD_BOOT_CODE);  /* Thiis selects KEYBOARD BOOT interface */

With the default code, the first interface returned could be the Mouse interface on devices exposing both Keyboard and Mouse interfaces. In that case, the desired Keyboard interface might not be selected.

Once the correct interface has been selected, I would also suggest assigning the IN and OUT pipes based on the actual endpoint descriptors, rather than assuming that Ep_Desc[0] is always the IN endpoint.

// File usbh_hid.c:

static USBH_StatusTypeDef USBH_HID_InterfaceInit(USBH_HandleTypeDef *phost)
{
  . . .

  // Initial states for HID state machine
  HID_Handle->state     = HID_INIT;
  HID_Handle->ctl_state = HID_REQ_INIT;

  /* Initialize both pipes in case there is no OUT interface */
  HID_Handle->InPipe    = 0U;
  HID_Handle->OutPipe   = 0U;

  /* Check fo available number of endpoints */
  /* Find the number of EPs in the Interface Descriptor */
  /* Choose the lower number in order not to overrun the buffer allocated */
  max_ep = ((phost->device.CfgDesc.Itf_Desc[interface].bNumEndpoints <= USBH_MAX_NUM_ENDPOINTS) ?
             phost->device.CfgDesc.Itf_Desc[interface].bNumEndpoints : USBH_MAX_NUM_ENDPOINTS);

  /* Decode endpoint IN and OUT address from interface descriptor */
  for (num = 0U; num < max_ep; num++)
  {
    if (phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].bEndpointAddress & 0x80U)
    {
    	// MANDATORY: IN Pipe Interrupt present (all HID Boot IN are Interrupt)
      HID_Handle->ep_addr   = phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].bEndpointAddress;	// Backward compatibility. Same value than InEp.
      HID_Handle->length    = phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].wMaxPacketSize;
      HID_Handle->poll      = phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].bInterval;
      HID_Handle->InEp      = phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].bEndpointAddress;
      HID_Handle->InPipe    = USBH_AllocPipe(phost, HID_Handle->InEp);

      if (HID_Handle->poll  < HID_MIN_POLL)
        HID_Handle->poll = HID_MIN_POLL;

      /* Open pipe for IN endpoint */
      USBH_OpenPipe(phost, HID_Handle->InPipe, HID_Handle->InEp, phost->device.address,
                    phost->device.speed, USB_EP_TYPE_INTR, HID_Handle->length);

      USBH_LL_SetToggle(phost, HID_Handle->InPipe, 0U);
    }
    else
    {
    	// OPTIONAL: Should the OUT Pipe Interrupt exist, keyboard LEDs must be controlled through this endpoint instead of control endpoint (EP0).
      HID_Handle->OutEp = (phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].bEndpointAddress);
      HID_Handle->OutPipe  = USBH_AllocPipe(phost, HID_Handle->OutEp);

      /* Open pipe for OUT endpoint */
      USBH_OpenPipe(phost, HID_Handle->OutPipe, HID_Handle->OutEp, phost->device.address,
                    phost->device.speed, USB_EP_TYPE_INTR,
										phost->device.CfgDesc.Itf_Desc[interface].Ep_Desc[num].wMaxPacketSize);

      USBH_LL_SetToggle(phost, HID_Handle->OutPipe, 0U);
    }
  }

  . . .
}

According to USB HID specs, the LED Output Report could then be sent either through the Interrupt OUT endpoint, when one is declared, or through the Control endpoint using Set_Report(Output) when no Interrupt OUT endpoint is available. Some code like the following one could be used:

HID_HandleTypeDef *HID_Handle = (HID_HandleTypeDef *) hUsbHostFS.pActiveClass->pData;

if (HID_Handle->OutPipe == 0U)
{
	status = USBH_HID_SetReport(&hUsbHostFS, 2, 0, &maskLEDs, 1);
}
else 
{
	status = USBH_InterruptSendData(&hUsbHostFS, &maskLEDs, 1, HID_Handle->OutPipe);
}

I have not tested the Interrupt OUT case yet, as I currently do not have a keyboard with an Interrupt OUT endpoint.

NOTE: I suspect that the configuration containing the Boot Keyboard interface should also be explicitly selected. At the moment, however, I have not investigated this part yet and am assuming that the first configuration contains the Boot Keyboard interface. The selection for a sepecific configuration should be done in the user callback function:
 

// File usb_host.c:

static void USBH_UserProcess  (USBH_HandleTypeDef *phost, uint8_t id)
{
  ...

    case HOST_USER_SELECT_CONFIGURATION:
      /* Search for the configuration that presents Boot Keyboard interface */
    break;
  ...
}

Any suggestions or recommendations on implementing a USB Boot Keyboard host are more than welcome.

[Pavel A.]

> Any suggestions or recommendations on implementing a USB Boot Keyboard host are more than welcome.

If you want to support only “boot” keyboards (which is reasonable) - check for them properly, select the boot mode properly. Don’t crash if user plugs in something unsupported. Test a lot to achieve coverage of all keyboards that you need to support. If a keyboard behaves weirdly, reject it and inform the user.

If you want to support fancy keyboards (multifunction/multi-configuration devices, full HID report mode, exotics key codes, exotic typematic behavior) - help is available here.

[JMarq.1]

Hi ​@FBL 

The ticket CDM0065375 can be followed outside ST?

The forum administrator is asking me to close and select the best answer. Knowing where to follow the ticket would be appreciated.

Thanks