Extended parameters (-x and -E) improvements

[MCUdude]

Adresses #1697.

I've added the -x autoreset to the AVR109 programmer so that compatible bootloaders can utilize this feature. I've also added the -x noautoreset to the arduino and wiring programmers.

@stefanrueger I didn't touch the Urclock implementation, since the way it deals with extra parameters is so different than the other. It would be great if you could look into this!

This PR is still a draft, so feedback is welcome.

[stefanrueger]

@MCUdude Very cool. Will review/add/comment in the next few days

[stefanrueger]

This looks wrong (because it probably is): Must be PDATA(pgm) not PDATA(&pgm).I think this sets an unknown memory address to 1 (but not autoreset).

[MCUdude]

It works this way, and I got a segfault if not. I'll look into it again

[stefanrueger]

I think I know what happens: arduino_initpgm() is called before pgm->setup() which allocates the pdata structure. So, pgm->cookie is NULL here (as it hasn't been initialised yet) and you get a seg-fault when you access it. PDATA(&pgm)->autorest is a memory location in the PROGRAMMER sructure which is at the offset of autoreset from where the cookie pointer lives. This is defo not what you want! Either set PDATA(pgm)->autoreset in pgm->setup() or later but before the parameters are parsed. Or invert the logic: Use a variable noautoreset that gets automatically initialised to zero. So you don't need to initialise that.

[stefanrueger]

@MCUdude You'll have seen a couple of review comments. What I haven't done is compiling or testing the code (but you have the scope to see the number of auto-reset spikes, so that's best verified by yourself that the code does what you want it to do).

I'll push code for urclock's -xnoautoreset functionality. There is a ton of other no... extended options without the _ between no and feature that is being switched off. So, for consistency in urclock.c I'll drop the _.

[stefanrueger]

And... this PR requires documentation in two places: avrdude.1 and avrdude.texi

[MCUdude]

Thanks for your feedback @stefanrueger! I have some comments though.

BTW one thing we should look into that would make the extended parameters more helpful to a struggling user would be to print the -xhelpoutput when the user provided an incorrect extended parameter. At the moment it's just an error, with no additional information. I think printing the help menu when there is an error could be helpful.

[stefanrueger]

more helpful to a struggling user would be to print the -xhelp output

Vvv good point! It's a lot of work to add this to what feels like 1500 programmers, but a great idea. (I think urclock already does this, so it's only needed for 1499 programmers :)

[MCUdude]

Vvv good point! It's a lot of work to add this to what feels like 1500 programmers, but a great idea. (I think urclock already does this, so it's only needed for 1499 programmers :)Vvv good point! It's a lot of work to add this to what feels like 1500 programmers, but a great idea. (I think urclock already does this, so it's only needed for 1499 programmers :)

How about something like this (separate print function):
EDIT: Here's a better way that doesn't require a separate print function:

static int arduino_parseextparms(const PROGRAMMER *pgm, const LISTID extparms) {
  const char *extended_param;
  int attempts;
  int rv = 0, help = 0;

  for (LNODEID ln = lfirst(extparms); ln; ln = lnext(ln)) {
    extended_param = ldata(ln);

    if (sscanf(extended_param, "attempts=%i", &attempts) == 1) {
      PDATA(pgm)->retry_attempts = attempts;
      pmsg_info("setting number of retry attempts to %d\n", attempts);
      continue;
    }

    if(str_eq(extended_param, "noautoreset")) {
      PDATA(pgm)->autoreset = false;
      continue;
    }

    if (str_eq(extended_param, "help")) {
      help = 1;
      rv = LIBAVRDUDE_EXIT;
    }

    if (!help) {
      pmsg_error("invalid extended parameter %s\n", extended_param);
      rv = -1;
    }
    msg_error("%s -c %s extended options:\n", progname, pgmid);
    msg_error("  -xattempts=<n> Specify the number <n> of connection retry attempts\n");
    msg_error("  -xnoautoreset  Don't toggle RTS/DTR lines on port open to prevent a hardware reset\n");
    msg_error("  -xhelp         Show this help menu and exit\n");
    return rv;
  }
  return rv;
}

This will result in the following output:

$ ./avrdude -carduino -patmega328p -P /dev/cu.usbmodem14201 -xinvalid
avrdude error: invalid extended parameter invalid
avrdude -c arduino extended options:
  -xattempts=<n> Specify the number <n> of connection retry attempts
  -xnoautoreset  Don't toggle RTS/DTR lines on port open to prevent a hardware reset
  -xhelp         Show this help menu and exit
avrdude error: unable to parse extended parameter list

[stefanrueger]

How about something like this

Output looks great; and yes, both ways would work.

[MCUdude]

@stefanrueger I've now pushed a fix for all programmers that supports extended parameters. I've done some minor code tweaks, but the main feature is that the help text gets printed if an invalid parameter is specified. Please review!

[MCUdude]

BTW I'll update the docs one #1842 is merged, to prevent merge conflicts.

[stefanrueger]

@MCUdude I think your technique of printing the help message on invalid parameters is very elegant. Great code! I particularly like that the help message code resides inside the for() loop. This allows those valid -x parameters with invalid assignment values to break; out of the look forgoing the full help message (which would distract from the nature of the invalid value).

Seeing that this is a really neat usability improvement for AVRDUDE, would you want to extend your technique to the parsing of exit parameters (-E)? It's three places

• par_parseexitspecs()
• flip2_parseexitspecs()
• linuxspi_parseexitspecs()

Please review!

Great code; a couple of comments

• Please double check jtagmkII_parseextparms(): help never set; help preamble msg_error("%s -c %s extended options:\n", progname, pgmid); output in wrong place. I predict avrdude -c jtag2 -xhelp won't be what you want.
• WIRINGPDATA(&pgm)->autoreset = true; is still a problem
• I'd prefer str_eq(pgm->type, "Arduino") over str_eq(pgmid, "arduino") in stk500_setup() to be consistent when Arduino-like programmers are meant

fix for all programmers that supports extended parameters

All? The commit overlooked

• serprog_parseextparms()
• stk500v2_parseextparms()
• usbasp_parseextparms()

While reviewing the xyz_parseextendedparms() functions I also noticed that some of the error messages for invalid values on valid parameters did not make the context sufficiently clear (so a user who has carefully built a 2 mile long CLI line might struggle to figure out to which part of the command line the message actually refers to). I took the liberty to push two commits that makes those error messages clearer mostly by saying Invalid value in -x <user setting>. Also massaged the help messages a bit, eg, inserting a space between -x and the parameter.

Please review my last two commits!

I also happened upon a bug in old code where str_eq(str_eq(extended_param, "jtagchain=") clearly needed to be str_starts(extended_param, "jtagchain="). Fixed.

[MCUdude]

Thanks for the feedback @stefanrueger! I've done some work to -E as well, and I've added help text here as well (-E help). But it would be great if you had any ideas for better help text for these exitspec parameters. The ones I came up with are a bit too verbose I think.

EDIT: I've also fixed the broken auto-reset implementation in wiring.c, so this PR is ready for another review!

[stefanrueger]

The next line needs to be rv = LIBAVRDUDE_EXIT; not return ...

I recommend checking all 22 treated programmers by running

avrdude -c ... -x invalid
avrdude -c ... -x help

to see whether there are surprises.

[stefanrueger]

too verbose

I don't think so with the possible exception of replacing programming session by programming so that the help text squeezes nicely into 80 cols.

All in all it's a really cool PR that substantially improves usability. Thanks a lot! Happy to merge once you confirm that you tested all programmers like here.

[MCUdude]

I've just tested all the programmers that support extended parameters. Things are looking good! Docs need to be updated, but I think it's best to wait until #1842 has been merged.

[MCUdude]

Speaking of docs, now is a good time to finally resolve #1711 by adding a new chapter or section to the avrdude.texi docs here?