hathach
Add DWC2 Test Mode Support
According to the USB 2.0 specification under point 7.1.20 Test Mode Support, high-speed capable functions must support specific test modes to facilitate compliance testing.
This PR adds the fundamental support for that. It was tested on a NUCLEO-U5A5ZJ-Q and STM32F23E-DISCO where this is supported directly by the MCU.
The entering of the test mode is scheduled by a ~~new event~~ control complete callback so that the feature request of the host can be acknowledged prior entering it.
Edit: The test modes can be tested with the XHSETT - XHCI Electrical Test Tool from the USB Compliance Tools.
The device should enter the test mode within 3 milliseconds. Right now there is basically no additional delay and the implementation relies that the control complete callback at the CONTROL_STAGE_ACK state is only triggered when the host already received the acknowledgment for the request. Otherwise the test mode signaling from device would corrupt the USB data transfer and so the host would likely time out and so don't know if the request was successful.
I've tested test modes on my device using the EHCI tool and the tests pass.
@Rocky04, do you have an idea when this would be ready for review? Thanks!
@shuchitak Was a code change needed or are you also using the DWC2 driver? On which device / MCU have you tested it?
I just used a draft for discussion... With the recent changes I'm somewhat happy with it now.
What would be a good way to toggle that feature off? Like a flag to exclude the handling and all the entire code for it in the case the test mode feature selector should not be used completely.
@Rocky04, I tested on an xcore device. The dcd layer for xcore (https://github.com/xmos/fwk_rtos/tree/develop/modules/sw_services/usb/portable) is not part of the tinyusb repo. I added implementation for the new dcd functions (https://github.com/shuchitak/fwk_rtos/commit/08dd360817391498b14ce6469f519b7049d81e2a) and ran the EHCI tests.
What would be a good way to toggle that feature off? Like a flag to exclude the handling and all the entire code for it in the case the test mode feature selector should not be used completely.
I thought returning a STALL if dcd_check_test_mode_support() returns False was a good idea. Why would we want to turn the feature off altogether?
I like to separate the configuration from implementation. Maybe someone don't want to have the support for that feature, then he should not need to alter any code, especially not any driver code.
The dcd_check_test_mode_support() function just inform if the hardware supports it.
Sorry I meant this line, TU_VERIFY(dcd_enter_test_mode && dcd_check_test_mode_support && 0 == tu_u16_low(p_request->wIndex));
If somebody doesn't support this feature, they wouldn't implement these dcd functions and TU_VERIFY would return false.
Isn't it the same as the existing code doing this, // Only support remote wakeup for device feature TU_VERIFY(TUSB_REQ_FEATURE_REMOTE_WAKEUP == p_request->wValue);
where for the TUSB_REQ_FEATURE_TEST_MODE feature, it would return false?
Yes, but if it's already supported for a MCU people would need to delete the driver code or override the function. Instead I would prefer to toggle the feature by using a compile flag or so.
Like -DTUSB_TEST_MODE_SUPPORT=false to exclude the code for it.
Edit: The wakeup feature also depends on the configuration, even that is done via code. If there is a symbol for this it can be controlled by a compiler argument or within the TinyUSB configuration file without the need to mess with either the USB core nor the driver code.
It would work right now by placing an undefine within the TinyUSB configuration file.
Like: #undef TUP_USBIP_DWC2_TEST_MODE_SUPPORT
But this would not work as a compiler argument and is not really generic.
Hmm, I'm not really sure. We could put all the test mode related code in usbd.c in #if TUSB_TEST_MODE_SUPPORT and have the application so something like #define TUSB_TEST_MODE_SUPPORT 1 in the tusb_config.h file, so it is disabled by default?
Kind of... For this feature I would prefer an opt-out instead of an opt-in, because it should be there for Hi-Speed.
Ideally if that feature is not supported the code for it should be excluded from compiling and so to not rely on the linker to drop it afterwards.
To have it enabled by default, we could add something like
#ifndef TUSB_TEST_MODE_SUPPORT #define TUSB_TEST_MODE_SUPPORT 1 #endif
in tusb_options.h. So in tusb_config.h, the user would need to explicitly disable it by doing #define TUSB_TEST_MODE_SUPPORT 0
Hi @Rocky04, do you have any more thoughts about enabling opt-out for this feature through a #ifdef? Are you planning to add this to usbd.c? Some of the tests are also failing in the compilation stage. I don't see how these changes can fail compilation.
@shuchitak The build error looks to me like a configuration problem, it appeared while I haven't really changed the code.
CMAKE_C_COMPILER_VERSION not detected. This should be automatic.
-- Configuring incomplete, errors occurred!
For the code exclusion I currently favor to use a symbol and if that is defined to not compile the code in the first place. Because I prefer an opt-out for it and keeping it as simple as possible.
@Rocky04, I agree, a simple opt out based on a define sounds good. Something like this?
diff --git a/src/device/usbd.c b/src/device/usbd.c index 83217a869..4b30a7447 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -731,6 +731,7 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const tud_control_status(rhport, p_request); break;
+#ifndef DISABLE_TUSB_TEST_MODE_SUPPORT // Support for TEST_MODE case TUSB_REQ_FEATURE_TEST_MODE: // Only handle the test mode if supported and valid @@ -753,6 +754,7 @@ static bool process_control_request(uint8_t rhport, tusb_control_request_t const
usbd_control_set_complete_callback(process_test_mode_cb);
break;
+#endif
If this looks okay for you, do you think we could add this change and have this PR reviewed and possibly merged? I need to make a release and would be great if this could make its way into the tinyusb_src repo. Thanks!
Hi, sorry for the late response. I added the opt-out to exclude the code if the test mode is not wanted.
Sadly no-one seems to have reviewed the code so far. There aren't that many people who maintain this repro.
Thanks for your PR, I've adjusted the logic, test mode is default disabled, one can enable by set CFG_TUD_TEST_MODE=1
As mentioned above the Test Mode is mandatory for Hi-Speed per specification.
As mentioned above the Test Mode is mandatory for Hi-Speed per specification.
It's mandatory for compliance test not for end use, same like WiFi RED test which is mandatory but test firmware is not publicly available.
Yeah, but these are there for a reason. Only because no one cares and don't send in a device doesn't mean it should be disabled per default. It's not that this feature is only enabled just when doing the test.