Some parts have wrong or missing ISP commands

ATtiny43U: the EEPROM memory read command has only 31 bits. It should be

   read = "1  0  1  0   0  0  0  0   0 0 0 x  x x x x",
          "0  0 a5 a4  a3 a2 a1 a0   o o o o  o o o o";

as the EEPROM has 64 bytes; a5 is currently missing.

The lock memory misses the read instruction altogether; it should be

        read            = "0 1 0 1  1 0 0 0   0 0 0 0  0 0 0 0",
                          "x x x x  x x x x   o o o o  o o o o";

I suggest adding the following code just before the return statement in the function parse_cmdbits(OPCODE *op) of the grammar file config_gram.y so an error (or warning if one removes the rv assignment) is thrown when there are too few bits specified in a command:

  if(bitno > 0) {
    yyerror("Too few opcode bits in instruction");
    rv = -1;
  }

ATtiny87 and ATtiny167:

The EEPROM write commands for these parts contains two a8 bits each; the second ones should be a7, respectively.

The addresses in the ISP commands actually behave more systematic than the highly flexible encoding scheme suggests, which leads to quite a few opportunities for specification errors in the .conf file. ISP commands only ever deal with 16-bit addresses (word addresses for flash, byte addreses for EEPROM) which appear in Bytes 2 and 3 of the 4-byte command sequence in the same sequence of bits. In fact, adding the warning

          // address bits in ISP commands are offset by 8 if a0 .. a15: warn if otherwise
          if(op->bit[bitno].bitno < 16 && op->bit[bitno].bitno != bitno-8)
            yyerror("Unusual bit position %d for a%d?", bitno-8, op->bit[bitno].bitno);

below line 1609 in config_gram.y uncovers these types of specification errors in .conf.

Speaking of the function parse_cmdbits(OPCODE *op) in the grammar file: line 1588 holds the assignment

   op->bit[bitno].bitno = 8*(bitno/8) + bitno % 8;

The rhs can be shortened to bitno, because n%m is defined as n-m*(n/m). This code is supposed to provide a default address line for the lone bit signifier a, ie, when not accompanied by a number (as in a11). However, it would be much, much, much neater if the default assignment was instead

  op->bit[bitno].bitno = bitno >= 8? bitno-8: 0;

Then, all the numbered address bit assignments (with the notable exception of a16 for the load extended address command) automatically fall in place, and could simply be written in the .conf file as a without numbers. In the very unlikely event that a part surfaces that jumbles up address bits there is still the fall-back syntax of meticulously describing where address bits are mapped to in the command bytes.

Making use of the suggested default assignment and using a bits in the .conf file without numbers (except for the single occurrence of a16) removes the human-error source of getting the pesky numbers for the address lines right in the .conf file.

ATmega164P, ATmega164PA, ATmega164A and ATmega64M1:

These miss EEPROM and flash ISP commands and other vital parameters.

My understanding of the current .conf file is that ATmega164P inherits its properties from ATmega324p and then redeclares eeprom and flash sections upon which the grammar file zaps the previously inherited memory description. Hence, the ISP commands (and probably a lot more vital parameters) went missing in this case.

The same for ATmega641M1, which inherits from ATmega328 and then, again, redeclares eeprom and flash without going on to specify all parameters.

Could be solved by

Changing the grammar so that values from an existing memory declaration of the same type are inherited
Specifying all parameters of redeclared memories in avrdude.conf (this issue could affect other, non ISP parts)
Both of the above

I have a slight preference for the first as this makes neater use of the concept of inheritance.

Either way, the documentation of the treatment of memory sections in inherited parts deserve an update. The inline docu in avrdude.conf says "New memory definitions are added to the definitions inherited from the parent" but it does not say what happens when a previously existing memory declaration from the parent is re-issued.

ATmega329, ATmega3290, ATmega3290A, ATmega3290P, ATmega3290PA, ATmega329A, ATmega329P, ATmega329PA, ATmega649, ATmega6490, ATmega6490A, ATmega6490P, ATmega649A, ATmega649P, ATtiny1634 and ATtiny1634R:

The writepage command for flash misses vital address lines. As an example, for the ATmega649, the flash writepage command is defined as

        writepage       = "  0   1   0   0      1   1   0   0",
                          "  x   x   x a12    a11 a10  a9  a8",
                          " a7   x   x   x      x   x   x   x",
                          "  x   x   x   x      x   x   x   x";

It has 65536 bytes flash in pages of 256 bytes, so one needs bits a7 ... a14 from the word address to identify the page to write. The two don't care bits x x before a12 need to be replaced by a14 a13.

Similarly, for the other 15 parts.

AT90PWM2, AT90PWM216, AT90PWM2B, AT90PWM3, AT90PWM316, AT90PWM3B, AT90USB1286, AT90USB1287, ATmega32U4, ATtiny167, ATtiny44, ATtiny441, ATtiny44A, ATtiny84, ATtiny841, ATtiny84A and ATtiny87:

A similar problem exists for the writepage command, this time for EEPROM. As an example, for the ATtiny167 the eeprom writepage command is defined as

  writepage = "  1   1   0   0      0   0   1   0",
        "  0   0   x   x      x   x   x   x",
        "  0   0  a5  a4     a3  a2   0   0",
        "  x   x   x   x      x   x   x   x";

It has 512 bytes EEPROM, so needs the x 0 0 before a5 replaced by a8 a7 a6; this time it's byte addresses, not word addresses.

Similarly, for the the other 16 parts' eeprom writepage commands.

Summarising, some 25% of ISP capable parts appear to have serious errors in their .conf definition.

In addition, there is a galore of extra address bits in ISP commands which are specified despite not being available on that part (as it has not that much flash or EEPROM). In practice, this does not matter, because those address bits will be set as 0 by avrdude, and therefore will match the x or 0 pattern that one would typically expect. 59 parts are affected by this with the worst offender being ATmega8U2 with 21 superfluous address bits. Attached a list avrdude-conf-extra-addr-bits.txt with all 423 occasions.

Umm, thanks for the analysis! I feel like this gotta be fixed before rolling the v7.0 release.

Running avrdude -p \* with my modified main.c reproduces the analysis. I wrote this to explore how systematic the ISP commands are: look carefully at the penultimate parameter in the .desc lines which is a bitfield that expresses which ISP commands are available in which parts. Outliers in this number told me where to look for problems...

$ avrdude -p \* | grep ^.desc  | cut -d, -f11 | sort | uniq -c 
    150  0x00
      4  0x05
      7  0x35
      2  0x37
      6  0x77
      6  0x7f
      4  0xf1
      4  0xf7
    129  0xff

$ grep ne.AD_ main.c
#define AD_SPI_EN_CE_SIG      1
#define AD_SPI_PROGMEM_PAGED  2
#define AD_SPI_EEPROM         4
#define AD_SPI_EEPROM_PAGED   8
#define AD_SPI_LOCK          16
#define AD_SPI_FUSE0         32
#define AD_SPI_FUSE1         64
#define AD_SPI_FUSE2        128

... then wondering which parts have three fuses but no eeprom/flash commands (0xf1)

$ avrdude -p \* | grep 0xf1,

showed one set of culprits

I wonder whether avr_set_addr() should be rewritten to ignore the .conf file and do the right thing. The new function would take four parameters

AVRMEM *mem
int what // one tries to do (AVR_OP_READ, ...)
unsigned char cmd[4]
int address

There are 14 cases with 5 different things to do:

calibration read, signature read copy the lower 16 bits of address & 3 to cmd[1] and cmd[2]
eeprom loadpage_lo, flash loadpage_lo, flash loadpage_hi copy the lower 16 bits of address & (mem->page_size - 1) to cmd[1] and cmd[2]
eeprom read, eeprom write, flash read_hi, flash read_lo, flash write_hi, flash write_lo copy the lower 16 bits of address to cmd[1] and cmd[2]
eeprom writepage, flash writepage copy the lower 16 bits of address & ~(mem->page_size - 1) to cmd[1] and cmd[2]
flash load_ext_addr copy the lower 16 bits of address>>16 to cmd[1] and cmd[2]

This assumes that, as is currently the case, address is a word address for flash, a byte address for eeprom and an index for signature and calibration memories. It also assumes that page_size is set correctly ;)

Strictly speaking, different parts have a different number of calibration bytes, but there is no ISP part with more than 4 calibration bytes. Hence, the suggested method still works. Note that the current .conf file, too, wrongly suggests for some parts there is an address to take when there is actually only one calibration byte: ATtiny87, ATtiny167, ATtiny24, ATtiny44, ATtiny84, ATtiny24A, ATtiny44A, ATtiny84A, ATtiny43U, ... So doing the same in the suggested new avr_set_addr() function would not be out of character.

The documentation could say that from avrdude version 7.0 onwards the address bits in those ISP command specifications that need one are ignored and created automatically because too many errors kept happening in the .conf files.

Given the many serious errors in address part of ISP opcodes (even when curated by experts!), the sheer number of .conf files out there, and that new parts with ISP command interfaces are unlikely to appear (now that UPDI and PDI interfaces are en vogue), I am convinced that above approach will be less error-prone than the current specification format in .conf.

Well, rather than doing any last-minute hacks in the code, I'd like to fix the broken .conf entries, and then ship v7.0. The ideas to improve the code are very welcome, but I've learned in all those years that this kind of last-minute change is quite risky, and must be avoided.

Fair enough.

Yes, the code that creates SPI programming commands is a bit proliferated (what with 25 calls to set the address bits alone). Replacing this function would probably require as much attention to detail as putting the .conf file straight.

The advantage of the latter is that avrdude itself with my for test purposes temporarily modified test main.c (here is a new more comprehensive one) will show you if you've overlooked an address-bit problem in flash/eeprom/signature/calibration ISP commands

  $ avrdude -p\* | grep ^.cmdbit | grep -v outside.addressable

This doesn't cater for the missing ISP commands, though

Commit 4bcd0eaa1d54 is supposed to fix all the mistakes you found. Thanks so much for it! You're welcome to cross-check the new conf file. The only thing I could not find was the EEPROM in ATmega32U4 - I think it is correct.

About your changes to the config file evaluation, I think in particular the automatic cross-checks make a lot of sense, but I don't like to introduce code changes before releasing v7.0, so I just wanted to fix the config file mistakes by now.

The only thing I could not find was the EEPROM in ATmega32U4 - I think it is correct

Does that not miss the a2 in writepage? (eeprom pagesize is declared to be 4)

You're welcome to cross-check the new conf file.

Don't ask me! Better ask PR #934.

If you run that AVRDUDE with your suggested .conf it will tell you:

avrdude: /usr/local/bin/../etc/avrdude.conf:9645 defines ATmega64M1 SPI programming command read for eeprom
  - its address bits are not compatible with memory size 2048 and/or pagesize 8, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        read            = "1 0 1 0 0 0 0 0",
                          "0 0 0 x x a10 a9 a8 a7 a6 a5 a4 a3 a2 a1 a0",
                          "o o o o o o o o";
avrdude: /usr/local/bin/../etc/avrdude.conf:9650 defines ATmega64M1 SPI programming command write for eeprom
  - its address bits are not compatible with memory size 2048 and/or pagesize 8, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        write           = "1 1 0 0 0 0 0 0",
                          "0 0 0 x x a10 a9 a8 a7 a6 a5 a4 a3 a2 a1 a0",
                          "i i i i i i i i";
avrdude: /usr/local/bin/../etc/avrdude.conf:9655 defines ATmega64M1 SPI programming command loadpage_lo for eeprom
  - its address bits are not compatible with memory size 2048 and/or pagesize 8, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        loadpage_lo     = "1 1 0 0 0 0 0 1",
                          "0 0 0 0 0 0 0 0 0 0 0 0 0 a2 a1 a0",
                          "i i i i i i i i";
avrdude: /usr/local/bin/../etc/avrdude.conf:9660 defines ATmega64M1 SPI programming command writepage for eeprom
  - its address bits are not compatible with memory size 2048 and/or pagesize 8, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        writepage       = "1 1 0 0 0 0 1 0",
                          "0 0 x x x a10 a9 a8 a7 a6 a5 a4 a3 0 0 0",
                          "x x x x x x x x";
avrdude: /usr/local/bin/../etc/avrdude.conf:9680 defines ATmega64M1 SPI programming command read_lo for flash
  - its address bits are not compatible with memory size 65536 and/or pagesize 256, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        read_lo         = "0 0 1 0 0 0 0 0",
                          "0 a14 a13 a12 a11 a10 a9 a8 a7 a6 a5 a4 a3 a2 a1 a0",
                          "o o o o o o o o";
avrdude: /usr/local/bin/../etc/avrdude.conf:9685 defines ATmega64M1 SPI programming command read_hi for flash
  - its address bits are not compatible with memory size 65536 and/or pagesize 256, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        read_hi         = "0 0 1 0 1 0 0 0",
                          "0 a14 a13 a12 a11 a10 a9 a8 a7 a6 a5 a4 a3 a2 a1 a0",
                          "o o o o o o o o";
avrdude: /usr/local/bin/../etc/avrdude.conf:9690 defines ATmega64M1 SPI programming command loadpage_lo for flash
  - its address bits are not compatible with memory size 65536 and/or pagesize 256, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        loadpage_lo     = "0 1 0 0 0 0 0 0",
                          "0 0 0 x x x x x x a6 a5 a4 a3 a2 a1 a0",
                          "i i i i i i i i";
avrdude: /usr/local/bin/../etc/avrdude.conf:9695 defines ATmega64M1 SPI programming command loadpage_hi for flash
  - its address bits are not compatible with memory size 65536 and/or pagesize 256, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        loadpage_hi     = "0 1 0 0 1 0 0 0",
                          "0 0 0 x x x x x x a6 a5 a4 a3 a2 a1 a0",
                          "i i i i i i i i";
avrdude: /usr/local/bin/../etc/avrdude.conf:9700 defines ATmega64M1 SPI programming command writepage for flash
  - its address bits are not compatible with memory size 65536 and/or pagesize 256, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        writepage       = "0 1 0 0 1 1 0 0",
                          "0 a14 a13 a12 a11 a10 a9 a8 a7 0 x x x x x x",
                          "x x x x x x x x";
avrdude: /usr/local/bin/../etc/avrdude.conf:12743 defines ATmega16U4 SPI programming command writepage for eeprom
  - its address bits are not compatible with memory size 512 and/or pagesize 4, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        writepage       = "1 1 0 0 0 0 1 0",
                          "0 0 x x x 0 0 a8 a7 a6 a5 a4 a3 a2 0 0",
                          "x x x x x x x x";
avrdude: /usr/local/bin/../etc/avrdude.conf:12934 defines ATmega32U4 SPI programming command writepage for eeprom
  - its address bits are not compatible with memory size 1024 and/or pagesize 4, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        writepage       = "1 1 0 0 0 0 1 0",
                          "0 0 x x x 0 a9 a8 a7 a6 a5 a4 a3 a2 0 0",
                          "x x x x x x x x";
avrdude: /usr/local/bin/../etc/avrdude.conf:16408 defines ATtiny1634 SPI programming command writepage for flash
  - its address bits are not compatible with memory size 16384 and/or pagesize 32, and external SPI programming may not work
  - consider the following suggestion and consult the datasheet:
        writepage       = "0 1 0 0 1 1 0 0",
                          "0 0 0 a12 a11 a10 a9 a8 a7 a6 a5 a4 x x x x",
                          "x x x x x x x x";

And brace yourself when you run avrdude -v .... There are a lot of address lines used in part descriptions that simply do not exist in that part. Not a biggie. Avrdude will still work. But this is an indication of copy/paste operations that have never really been looked at.

I don't like to introduce code changes before releasing v7.0

Why not? The changes suggested in PR #934 are non-functional, ie, they do not affect the functionality of AVRDUDE. The worst that can happen is that an AVRDUDE warning is wrong but I don't think this likely! OK, I would say this, wouldn't I? :)

AVRDUDE's .conf grammar over-models SPI programming commands to the extent that not even authors/maintainers cope well with checking them. This really is better done by an algorithm, not a human. Similarly, I would never trust myself typing a15 a14 a13 a12 a11 a10 a9 a8 a7 a6 a5 a4 a3 a2 a1 a0 correctly. I'd always write a program for that (echo $(for i in {15..0}; do echo a$i; done) since you asked).

The only thing I could not find was the EEPROM in ATmega32U4 - I think it is correct

Does that not miss the a2 in writepage? (eeprom pagesize is declared to be 4)

Ah OK, I missed that one.

Thanks, I also fixed the remaining issues above.

I don't like to introduce code changes before releasing v7.0

Why not?

Because my experience tells me that code changes right before a release are a bad idea. They require a thorough review, but I'm already pretty late with the release (target date was end of 2022Q1), so I don't want to spend any time into such a review.

If I'm mis-doing something with that .conf file changes, they affect a single device or two. If I (and you) have missed something in the overall code, this puts a risk on the entire usability of the release.

But I'm all for changing something after the release. As long as we keep it compatible with the existing config file, we could even change it in a way that omits all those explicit address bits. If we then also change the inheritance principles (extend rather than replace), quite possible we might be able to supply "generic" ISP commands for a good number of parts.

I'm already pretty late with the release (target date was end of 2022Q1)

I don't think .conf is ready yet: I suspect there to be more and potentially significant copy/paste errors in the part descriptions. I only had a program check eeprom/flash size/pagesize and signature against ATDF files along a sanity check for the address bits of SPI commands. I strongly recommend other properties be programmatically cross-checked that can be read off ATDF files.

Also note there are a couple of small things that PR #934 corrects en passant: The syntax comment at the top in .conf misses the offset component of a memory, and the mode, delay, blocksize and readsize parameters are mistakenly put under part rather than the memory component where they belong. You might want to do that for your release (always neat when the documentation/comments are up to date).

But I'm all for changing something after the release.

I recommend you look at PR #934. AVRDUDE warning about wrong address bits is useful for people's own .conf files or when they conjure up their own own part descriptions. Should you get onside the principle/algorithm/implementation, this has the powerful potential to populate the address bits automatically where needed (as it is only a trivial change from warning to overriding).

If we then also change the inheritance principles (extend rather than replace), quite possible we might be able to supply "generic" ISP commands for a good number of parts

... getting rid of much repetitive ISP commands in .conf that are difficult to get right. Neat!

BTW, if you do that don't forget to also extended the grammar to allow NULL assignments for SPI commands: Imagine a 128 kB flash part inherits its properties from a 256 kB flash part. The latter will have the load_ext_addr command defined, but the (inherited) 128 k flash part then needs to overwrite load_ext_addr = NULL. This is a real-world case: the current .conf details such an inheritance.

avrdudes / avrdude

Some parts have wrong or missing ISP commands #915