avrdudes / avrdude

AVRDUDE is a utility to program AVR microcontrollers
GNU General Public License v2.0
747 stars 138 forks source link

Make UPDI programmers less verbose during initialization #1084

Closed mcuee closed 2 years ago

mcuee commented 2 years ago

Similar #1081 (fixed by #1083), I think the UPDI programmers have the same issue, probably not limited to the following serialupdi and pickit4_updi, but also others.

Ref: from the run log of @MCUdude https://github.com/avrdudes/avrdude/pull/1078#issuecomment-1221609012

$ ./avrdude -cserialupdi -P/dev/cu.usbserial-1410 -patmega4808 -Uflash:w:blink_4808.hex -qq && echo OK
avrdude: UPDI link initialization OK
avrdude: NVM type 0: 16-bit, page oriented write
avrdude: Entering NVM programming mode
avrdude: Leaving NVM programming mode
OK

$ ./avrdude -cpickit4_updi -patmega4808 -Uflash:w:blink_4808.hex -qq && echo OK
         Vtarget                      : 5.06 V
         JTAG clock megaAVR/program   : 1000 kHz
         JTAG clock megaAVR/debug     : 100 kHz
         PDI/UPDI clock Xmega/megaAVR : 100 kHz

OK
mcuee commented 2 years ago

Some INFO may need to be downgraded to NOTICE for serialupdi.c. https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L160 https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L165 https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L170 https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L183 https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L588 https://github.com/avrdudes/avrdude/blob/e5edecf95ea3a8dafc45309914ebb1a2127047f5/src/serialupdi.c#L656

mcuee commented 2 years ago

Some INFO may need to be downgraded to NOTICE for jtag3.c.

https://github.com/avrdudes/avrdude/blob/main/src/jtag3.c#L2453 https://github.com/avrdudes/avrdude/blob/main/src/jtag3.c#L2460 https://github.com/avrdudes/avrdude/blob/main/src/jtag3.c#L2468 https://github.com/avrdudes/avrdude/blob/main/src/jtag3.c#L2476 https://github.com/avrdudes/avrdude/blob/main/src/jtag3.c#L2484

stefanrueger commented 2 years ago

Maybe a good idea to agree on a verbosity level house style and then, whoever sees clear style violations, change the verbosity level straight on the code rather than going via Issues, PRs, discussions etc? How about sth like

  1. MSG_INFO for errors outside AVRDUDE's remit why it cannot start the job (eg, file not found, cannot open port, ...)
  2. MSG_INFO for error messages to explain why AVRDUDE cannot do a job and had to exit (eg, verification error)
  3. MSG_INFO in a very few situations to keep the user appraised (initialised, reading, writing, verified, thank you)
  4. MSG_NOTICE for more detail that is likely to assist in debugging
  5. ...

The house style could be put as comments in https://github.com/avrdudes/avrdude/blob/38aa1313f94fb2ece97ea9a87d5384e8b7cb2b89/src/avrdude.h#L40-L45

It's probably hard to come up with a house style as I predict amongst $n$ developers there are at least $n+1$ different tastes: my preferences alone depend on the time of day (before-lunch grumpy, after-lunch mellow ;)

It will be a worthwhile exercise, though, given the rich tapestry of messages and info that AVRDUDE affords the user.

One word of caution: Some will use the AVRDUDE executable in their workflow/GUI and have programs read what AVRDUDE has to say; they will not be amused by having to rewrite their code, because AVRDUDE developers changing the verbosity level back and forth for info that's important to their workflow/GUI. Hence, I would advocate to make changes a) systematically and b) sparingly. (I only created PR #1083 as this instance seemed gratuitously verbose in absence of -v).

There is one thing I miss: a silent option, where AVRDUDE doesn't say anything and just exits with 0 (all good) or 1 (a programming problem occurred, could not do the job) akin to cmp -s which suppresses all normal output. This to use avrdude in pipelines or bash scripts. Right now I use >& /dev/null or -l /dev/null for this, but having a shortcut -v=0, -S or -Q for this silent option would be neat. My pref is allowing -v[=]<num> as stand-alone option, eg, -v0, -v2 -v=-1, whilst keeping the -vvvvp m328p syntax alive. This to avoid introducing yet another option. I wouldn't be averse to still get messages for problems outside AVRDUDE's remit (external error, category 0 above), and this is what cmp -s actually does (suppress all normal output). This would require splitting MSG_INFO into three types MSG_EXTERR (value 0), MSG_ADERR (value 1), MSG_INFO (value 2), ... and setting the default verbosity level to 2 so the user does not see any change in AVRDUDE's behaviour (except for the terminal use that sets the level directly).

I like the orthogonal -q option for quelling progress messages (ie, messges of a certain type, not of a certain verbosity level). They are independent of -v, which is good (and thankfully the comment in Line 40 above about -qq is wrong).

Anyway, bigger fish to fry, this may be for another day.

mcuee commented 2 years ago

@stefanrueger

Very well thought out reply. Thanks. I did not think too much when I raise the issue -- I was just thinking that avrdude is not so consistent with the messages.

MCUdude commented 2 years ago

it would be useful to have an option (-qq) where no output is printed and all we're left with is the return code.

IMO, quell should perhaps be dealt with inside avrdude_message. At the moment it has to be dealt with "manually" like shown below:

https://github.com/avrdudes/avrdude/blob/3e49f078b3ffc8c0e647fc3600da8ea69e47e487/src/main.c#L1054-L1057

How about something like this:

int avrdude_message(const int msglvl, const char *format, ...)
{
    int rc = 0;
    va_list ap;
    if (verbose >= msglvl && quell_progress < 2) {
        va_start(ap, format);
        rc = vfprintf(stderr, format, ap);
        va_end(ap);
    }
    return rc;
}