Blink an LED until USB is enumerated

[esden]

If we blink an LED until USB becomes enumerated it is clear that there is a USB connectivity issue. A common user problem is the use of charge only USB cables, that lack data lines.

It probably makes most sense to blink the RED led, possibly using the morse subsystem?

[krogozinski]

@esden @dragonmux I'm keen to work on this as a first issue, if no objections from you

[dragonmux]

Certainly, please do! Our mental plan for this was to start by poking in the SysTick handler and accessing the USB stack state to see if a connection has been established in whatever way we can without modifying libopencm3 if possible.

[esden]

No objections. :) If you end up modifying locm3 for this, please submit your changes up stream: https://github.com/libopencm3/libopencm3 and to our fork: https://github.com/blackmagic-debug/libopencm3

[ALTracer]

That is a useful feature.
I had a go at this as well. I had in mind non-blocking platform_timeout and some (most) code in super-loop, without intervening in other source files much.

Since there already are two usb_set_config callbacks (for serial and for DFU), I used usb_get_config() of usb_serial to check for PC presence. Alternatively, there is an unused (void)value in dfu_set_config() callback. Or you could register your 3rd callback, library ABI statically reserves 4 slots. I think it's unlikely for BMF to become a multi-config composite device on f103 platforms with usbd and 8 flexible endpoints, but it may be necessary for f4 platforms with dwc_otg and 4 pairs of endpoints to actually support traceswo capture. That is a whole different story.

I started by appending the top-most super-loop with a gpio_toggle+timeout reload on timeout expiry (for non-hosted).
However, there are problems with that, this code wasn't reached. Several routines behind bmp_poll_loop(), gdb_getpacket()/gdb_if_getchar() and gdb_if_update_buf() in gdb_if.c seem to busy-wait on usb_get_config() returning non-zero, so the super-loop actually does not run to completion for me to be able to do that. I tried altering gdb_if logic to avoid that, but it started returning \x04 symbols up its stack and other meaningless activity. bool gdb_serial_dtr is initialized as true which doesn't help.
I discovered this after connecting my blackpill-f411ce as "target" to platform=swlink/bluepill-plus as BMD probe. These boards have their single blue LED mapped to IDLE_RUN, not ERROR (which is manipulated by SET_ERROR_STATE(morse_update()) in sys_tick_handler, but you could blink any [one] other LED in this way, morse is non-reentrant on its morse_msg state.)

Using morse for this looked too fishy. I needed another all-platform universal way to blink the idle LED from boot until usb_get_config() returns non-zero, and ... a smaller blocking loop up front seemed appropriate as PoC. Yes, this prevents any other activity in firmware (besides sleeping in __WFI), like pending feature of offline mass-programming. But as the blinky serves a purpose to indicate lack of USB connection, maybe the offline mode code should run before it. There's no RTOS, no task_yield, no software thread scheduler. After all, other OEM purely online debug adapters don't do much else when powered but not USB-connected. I haven't worked with combined function devices which are both online debug probes and offline mass programmers, like AT-Link (non-EZ: + and Pro).

PoC, tested on blackpill-f411ce (+uhubctl or a power bank):

diff --git a/src/main.c b/src/main.c
index 496cfd02..b528ec2d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -32,6 +32,9 @@
 #ifdef ENABLE_RTT
 #include "rtt.h"
 #endif
+#if PC_HOSTED == 0
+#include "usb.h"
+#endif
 
 /* This has to be aligned so the remote protocol can re-use it without causing Problems */
 static char pbuf[GDB_PACKET_BUFFER_SIZE + 1U] __attribute__((aligned(8)));
@@ -77,6 +80,16 @@ int main(int argc, char **argv)
 	(void)argc;
 	(void)argv;
 	platform_init();
+
+	platform_timeout_s nousb_blinky;
+	platform_timeout_set(&nousb_blinky, 1000U);
+
+	while (!usb_get_config()) {
+		if (platform_timeout_is_expired(&nousb_blinky)) {
+			gpio_toggle(LED_PORT, LED_IDLE_RUN);
+			platform_timeout_set(&nousb_blinky, 1000U);
+		}
+	}
 #endif
 
 	while (true) {

[krogozinski]

@ALTracer your patch above is almost identical to the PoC I had as well. Below is what my thought process was after the PoC:

• I would prefer not to add platform-specific code into main.c, with #if PC_HOSTED or otherwise
• I would prefer not to include usb.h into main.c either. My feeling was that any code in main.c should be limited to using interfaces provided by platform.h
• I would like to stay as close to the original suggestion and use the morse subsystem. If the feedback for this is negative, another solution can be implemented.

I understand your point about the morse subsystem being non-reentrant. I too am concerned about this. The lack of a scheduler and associated synchronisation primitives makes this tricky. I think a concept of message priorities could also help.

Perhaps these are other issues that could be raised and assessed on their merit.

[dragonmux]

Closing as fixed by #1616 which implemented this feature.