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