CMSIS-DAPv2: blink LED on bulk interface activity

[mbrossard]

Fix for #714

[microbit-carlos]

As it turns out, the micro:bit online editors (MakeCode & Python) both constantly pull serial data when connected via WebUSB. This is done to offer some useful features like detecting errors printed to serial and highlighting them in the editor.

With this fix, unfortunately, the editors are constantly blinking the DAPLink activity LED as soon as the micro:bit is WebUSB connected, and therefore we cannot tell the difference between the micro:bit seating idle vs flashing (or the user interacting with the target serial), which is technically worse for our users, as seeing the flashing LED activity is quite useful and the behaviour we've described in a lot of the micro:bit documentation.

One option we are considering is seeing if there is a way to check the bulk activity traffic, and if possible maybe filter out activity coming from serial data (of course, that would have to be behind a build flag or hook).
@mathias-arm Until we have a chance to investigate a workaround like this, could we have this fix behind a build flag?

[mbrossard]

Do you think this could work: https://github.com/mbrossard/DAPLink/tree/pr/uart-dap-led?

[microbit-carlos]

Thanks @mbrossard, unfortunately that didn't work. I think DAP_ProcessVendorCommand() likely executes inside DAP_queue_execute_buf(), so main_blink_hid_led(MAIN_LED_FLASH) would overwrite the state set in that patch:

DAPLink/source/usb/bulk/usbd_bulk.c

Lines 80 to 81 in 5e04123

	if (DAP_queue_execute_buf(&DAP_Cmd_queue, USBD_Bulk_BulkOutBuf, DataInReceLen, &rbuf)) {
	main_blink_hid_led(MAIN_LED_FLASH);

Doing this as quick hack did work (vendor IDs 3 and 4 are serial read/write), but not sure how to best integrate it in DAPLink.
236e1c4

[microbit-carlos]

The call stack goes:

• DAP_queue_execute_buf() ->
  • DAP_ExecuteCommand() ->
    • Here It can execute DAP_ProcessCommand() one or multiple times ->
      • Here the DAP command IDs are checked for the first time and processed, and it might call DAP_ProcessVendorCommand()

I guess we could probably add main_blink_hid_led(MAIN_LED_FLASH) at the top of DAP_ProcessCommand() and main_blink_hid_led(MAIN_LED_DEF) when DAP_ProcessVendorCommand() processes the serial read/write DAP commands?

I guess the only problem is that "philosophically" it's not great to set the HID LED inside of the DAP commands module, and on the other hand checking the DAP command in the USB traffic processing layer is also not ideal. But I'm not sure if there is a better alternative.

[mathias-arm]

I think we could move the main_blink_hid_led() calls in source/usb/bulk/usbd_bulk.c and source/usb/hid/usbd_user_hid.c to be in DAP_queue_execute_buf() and have the usbd_hid_no_activity() apply to both HID and Bulk.

I prefer modifying DAP_queue_execute_buf() because it's a DAPLink function, when DAP_ExecuteCommand() is from CMSIS-DAP.

[microbit-carlos]

We could use a similar hook to usbd_hid_no_activity in source/usb/bulk/usbd_bulk.c:
microbit-foundation@86ab399

Ideally we'd use the same hook, but if the function declaration needs to go into a header file I'm not sure what's the best place to put it, any suggestions?

[mathias-arm]

I agree there should only be one hook. The "HID" LED should rather be considered the "DAP" LED. So I would be in favor of renaming usbd_hid_no_activity() to usbd_dap_no_activity() for instance. Both source/usb/bulk/usbd_bulk.c and source/usb/hid/usbd_user_hid.c import DAPLINK_MAIN_HEADER (main_interface.h) for main_blink_hid_led(), so that would be my suggestion.

I think the default hook could ignore the DAP commands that call main_blink_cdc_led().

[microbit-carlos]

The easiest way to use a single hook might be to move it to DAP_queue_execute_buf() as you suggested before.

Which approach would you prefer then:

• Single hook called in usbd_bulk.c and usbd_user_hid.c that might require the weak function to be declared in a header file
  • Similar to microbit-foundation@86ab399
  • Which header file would the declaration go? rl_usb.h is likely an RTX header that shouldn't be touched, so the alternative might be to create a new one?
• Single hook moved to DAP_queue.c
  • As shown in microbit-foundation@c718b87
• Or something else?

[microbit-carlos]

Okay, I've created both PRs, feel free to merge whichever you prefer if it's ready to go (comments with improvements, maybe with the naming, are welcomed).

• Update hook to skip blinking on WebUSB HID serial traffic (option 1). #1017
• Update hook to skip blinking on WebUSB HID serial traffic #1018