-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make: print spinning icon while flashing/resetting #15970
make: print spinning icon while flashing/resetting #15970
Conversation
f603cb9
to
92ad5b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this one. I do agree that flashing output is too verbose, so we definitely could suppress a part of it. But I definitely think we need some output: flash progress, or at least "Flashing started, Flashing done", the flasher used, maybe there is ways of launching the flashers in amore silent way as well no?
I'm looking into this and it's not easily achievable. Anyway this PR is broken in its current state: it works well with JLink (the programmer I used initially) but with openocd and pyocd it doesn't. |
3a089a8
to
6cf7fd8
Compare
@fjmolinas after your initial review, I completely reworked this PR. Now a new programmer wrapper tool is introduced (written in Python) to silence the output of flashers/reset and print a spinning icon instead. I tried the programmer.py tool with several programmers and found a couple of issues:
|
really? all those extra lines to get a spinning icon? |
Not really. The goal is to keep a way to also print the programmer output as before (using PROGRAMMER_QUIET=0). For the spinning icon, the Python script (and subprocess) makes this easy to implement and read. |
a5380c1
to
3a750a0
Compare
Could this potentially make IDE integration harder (or alternatively easier)? |
Do you have an example of IDE integration ? The only problem I could see is the ascii escape codes used for colors, the carriage return and line clear. But I guess modern terminals should handle that. |
Something like a GUI progress bar based on parsed terminal output e.g. (I'm thinking for instance of the Ubuntu update manager which does something like that). |
I guess this could actually make the integration easier, since one could hook into that. |
In theory, it's possible to redirect the stdout/stderr of subprocess to anything. For the progress bar, the "progress" makes little sense since we don't have any information about the underlying flash progression. We could estimate it based on the firmware size/programmer but I don't think that's worth. Here the spinning icon make more sense IMHO: it's simpler and shows activity. |
I would count the usual "Cylon visor" for such a case also as a "progress" bar ;-) |
3a750a0
to
85075fd
Compare
5e710a6
to
5a8094e
Compare
Squashed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK on my side but would like some more voices.
If we change "programmer" with e.g., "other-kind-of-tool" and fixup the help messages, this would be a generic "run tool and show spinner instead of output unless an error occurs", right? |
Are you sure "other-kind-of-tool" (or maybe "run-tool-and-show-spinner-instead-of-output-unless-an-error-occurs") would be a good name ? In the build/flash/reset/term usual process, I don't see where the spinning icon could be helpful other than with flash and reset. The build output as it is now is small enough while giving useful information, so it don't really need to be wrapped like the programmer. The pattern Do you have another tool in mind where this spinning icon would be worth ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say we can still change the name later if we add more use cases for this.
Now it would just be confusing.
What I meant is that having a tool that runs a command, printing a spinner and only outputs on error code (or stderr), could be re-used. "other-kind-of-tool" was just to point out that with changed text and name, programmer.py would be that generic tool. |
wget, git, git-cache, anything that takes time. With the current build system, "make info-boards-supported" with the spinner printed to stderr, so users know something is happening. |
You can disable the spinning icon by adding PROGRAMMER_QUIET=0 to the command line. I know it's not perfect but pyocd is the only programmer with this behavior and it will be hard to print the menu automatically (unless we revert that PR). |
But why even change the behavior then? Is it really worth to have a spinning icon then? It looks nice, yes...but IMHO I would really prefer the old way when there is no way to provide this menu. It feels a bit like a regression. |
If you don't want the spinning icon, you can add PROGRAMMER_QUIET=0 to the command line and you'll have the menu (it works, I just tried it).
I agree that for this particular case, an interactive menu printed when multiple boards are connected, there is a regression. From There's no easy way to determine automatically that multiple boards (recognized by pyocd) are connected, this is really a pyocd thing. Your problem could be fixed by simply disabling the spinning icon when pyocd is used. |
But that requires the user to know about this option. Currently the help text
is only printed if the flashing was successful. If programming fails due to the new option, the user has no idea how to disable it (or that it could be related to the failure in the first place) I think we should employ a similar approach as with spell check here: Have the option to revert it quickly if it turns out to be a nuisance. I do like eyecandy, but if it comes at the price of reduced usability, that's a problem. |
But even with this option, I would have to use it every time now. And writing For the eyecandy: Eyecandy is cool, but with pyocd we have now even less information than before. Before, there was a very informative progress bar. Now it's just the spinning icon. In case of my problem the spinning icon was not helpful. The icon was spinning, but it didn't do anything and there was no hint that there was a problem or that the progress got stuck. With a progress bar you would see that it doesn't move. |
I agree that in this case, it is a regression, for now I opened a PR to disable the spinner for pyocd. Unless similar functionality is implemented later on in the wrapper. If more issues pop up later on we can re-examine this feature and this fix. |
I want to retroactively withdraw my ACK on this PR. |
Contribution description
This is an RFC PR that introduces 2 variables to control the output of the flasher. Most of the time it's just noise, especially when pasting them as test output in a PR. The build system also provides the QUIET variable to silent the compiler output and replace it with a nice output. The idea of this PR is to something similar for the flasher and by default, only display the flasher command.
FLASHERLOG and FLASHER_OUTPUT variables are introduced to give a way to control the flasher output.
The proposed behavior is as follows:
$(BINDIR)/$(PROGRAMMER).log
Maybe there's a better way to handle this but the proposed solution in this PR is quite simple. The other possibility that I see is to add a wrapper tool that runs the flasher in background with a nice activity printed during flash (dots or progress bar or whatever).
Testing procedure
Issues/PRs references
Depends on
#16046,#16047,#16050