Hi Christian,
On 6/6/24 11:52 AM, Christian Marangi wrote:
On Thu, Jun 06, 2024 at 11:12:11AM +0200, Quentin Schulz wrote:
Hi Christian,
On 6/5/24 9:21 PM, Christian Marangi wrote:
This series expand the STATUS LED framework with a new color
and a big new feature. One thing that many device need is a way
to communicate to the user that the device is actually doing
something.
This is especially useful for recovery steps where an
user (for example) insert an USB drive, keep a button pressed
and the device autorecover.
There is currently no way to signal the user externally that
the bootloader is processing/recoverying aside from setting
a LED on.
A solid LED on is not enough and won't actually signal any
kind of progress.
Solution is the good old blinking LED but uboot doesn't
suggest (and support) interrupts and almost all the LED
are usually GPIO LED that doesn't support HW blink.
I haven't used it yet but we do have a cyclic framework now for things
happening in the background. I think this is a good use-case for this
framework? Something would set the blinking frequency (could be from CLI
directly, or as part of board files, or architecture, etc...) and the LED
would just blink then. This would allow to highlight stages in the boot
process, though that is not like an activity LED so if you're stuck in a
stage, you wouldn't know if something is still happening or if you're really
stuck (e.g. no packet on TFTP or TFTP very slow). The benefit is that it
would be way less intrusive than patching all commands that could make use
of that LED. Right now, this only adds support to MTD, SPI and TFTP, but
what about MMC, NVMe, USB, other net stuff (e.g. wget), etc...
Can you hint me on where is this framework? Checking the tftp code i
couldn't find extra call to it. Maybe it's attached to the schedule()
function?
https://docs.u-boot.org/en/latest/develop/cyclic.html
Also notice that it's really not a one setting since almost all device
have GPIO LEDs and doesn't have a way to support HW Blink so the
"activity" function needs to be called multiple time to increase the
counter and toggle the LED.
Cyclic callback would be called twice per expected blink period, where
you would toggle the GPIO (essentially making it 50% duty cycle, but
could be more fine-grained if you want a different duty cycle).
Also this have the extra feature that you can check if something gets
stuck in the process. The lifecycle is:
- Turn on the ACTIVITY LED at the start of the thing
- Blink once in a while (for very little task this might not happen)
- Turn off the ACTIVITY LED at the end of the thing
Soo if something goes wrong the LED would never turn OFF but would stay
solid ON.
Yes, that's something that wouldn't be covered by cyclic framework here.
It all depends what you want to do, if it's an activity LED, then we
need to hook ourselves deep into frameworks where stuff is actually
happening. If it's just to specify which stage of the boot we reached,
then cyclic would be good enough probably (register for stage 1,
unregister stage1+register for stage2 for different frequency, etc...).
More than happy to rework this to a less intrusive implementation.
Maybe this can be generalized to some generic API like task_start(),
task_processing() and task_end()? Might make more sense than having to
add specific LED function to each function?
This also likely would introduce a hit in performance if we need to
toggle the GPIO in the same thread that we do TFTP/storage medium
reading/writing? I assume we could still adapt cyclic to make it spawn a
one time event instead of looping (e.g. by unregistering itself at the
end of its own callback?).
(AFAIK linux kernel have something similar used for all the trace
framework so having something in uboot to trace these kind of operation
might be interesting)
Indeed, that's what's being done with ledtrig_.* functions, they are
however scheduled on a workqueue and called from the subsystem directly.
I'm a bit confused also as to why we control the LED blinking from the
cmd/ ? E.g. for cmd/mtd.c I would assume that the changes made to the
mtd subsystem should be enough to handle those? Similarly, since UBI is
for use over NAND/MTD, shouldn't that already be handled by the MTD
subsystem, and if not, why not in the UBI subsystem instead of the
CMD_UBI? One of the issues is that you may not necessarily go through
the cmd/ to do stuff with storage medium or network (e.g. directly from
board files).
For the net protocols, why not hook this to the net_[sg]et_*_handler for
example so it's protocol-agnostic? No clue how difficult this would be,
or if you'd rather have something like per-protocol activity?
Finally, maybe we also want to have a Kconfig symbol per "type" of
activity to control what should be "monitored", and I would also suggest
if we go this route to have a Kconfig symbol for the frequency per
"type" of activity as well, so that one can know which activity is
happening right now.
Nothing against this patch series or its current implementation, just
throwing ideas :)
Cheers,
Quentin
