Skip to content
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

Developer options to describe parts and extend avrdude.conf syntax #1040

Merged
merged 37 commits into from
Aug 2, 2022

Conversation

stefanrueger
Copy link
Collaborator

This (untested) PR

  • Provides developer options to describe parts as AVRDUDE understands them
  • Corrects the default bit number for lone 'a' in config_gram.y
  • Enhances the avrdude.conf grammar
    • New syntax: readback = 0x80 0x7f;
    • New lexer.l capability to scan negative decimal integers or reals
    • New syntax opcode = NULL; for SPI programming
    • New syntax memory "name" = NULL;
    • New syntax ((pp|hvsp)_controlstack|(eeprom|flash)_instr) = NULL;
    • Extend rather than reset memory entries in avrdude.conf
    • New compact alternative specification for SPI opcodes, eg,
      loadpage_lo = "0100.0000--000x.xxxx--xxaa.aaaa--iiii.iiii";
  • Updates the avrdude.conf introductory documentation
  • Provides own part_match(p, s) function to support Windows portability
  • Provides avr_set_addr_mem() to set addresses in SPI opcodes within boundaries
  • Makes AVRDUDE warn whenever address bits in avrdude.conf SPI commands miss or are misplaced

The syntax for the part description option is avrdude -p <wildcard>/<flags>

$ avrdude -p */h
avrdude: flags for developer option -p <wildcard>/<flags> not recognised
Wildcard examples:
         * all known parts
  ATtiny10 just this part
  *32[0-9] matches ATmega329, ATmega325 and ATmega328
      *32? matches ATmega329, ATmega32A, ATmega325 and ATmega328
Flags (one or more of the characters below):
       c  check address bits in SPI commands and output errors
       d  description of core part features
       o  opcodes for SPI programming parts and memories
       S  show entries of avrdude.conf parts with all values
       s  show entries of avrdude.conf parts with necessary values
       r  show entries of avrdude.conf parts as raw dump
       w  wd_... constants for ISP parts
       *  all of the above except s
       t  use tab separated values as much as possible
Note:
  -p *      same as -p */*

The function avr_set_addr_mem(AVRMEM *mem, int opnum, unsigned char *cmd, unsigned long addr) is meant to replace avr_set_addr(OPCODE *op, unsigned char *cmd, unsigned long addr) in future. avr_set_addr_mem() has more information about the context of the task in that it knows the memory size, memory page size, whether or not the memory is a flash memory (which gets words addressees supplied) and, crucially, knows which SPI operation it is meant to compute the address bits for. avr_set_addr_mem() first computes the interval of bit numbers that must be supplied for the SPI command to stand a chance to work. The function only sets those address bits that are needed. Once all avr_set_addr() function calls have been replaced by avr_set_addr_mem(), the SPI commands that need an address can afford to declare in avrdude.conf all 16 address bits in the middle two bytes of the SPI command. This over-declaration will be corrected during runtime by avr_set_addr_mem(). One consequence of this is that parts can inherit smaller or larger memories from parents without the need to use different SPI codes in avrdude.conf. Another consequence is that avr_set_addr_mem() can, and does, tell the caller whether vital address bits were not declared in the SPI opcode. During parsing of avrdude.conf this might be utilised to generate a corresponding warning. This will uncover problematic SPI codes in avrdude.conf that in the past went undetected.

I still have to get the hang of a workflow with git rebase. Apologies for the non-linear history :(

@dl8dtl, could I ask for your review? This work relates to the Extend rather than reset memory entries in avrdude.conf that might be useful, and for hardening avrdude.conf entries against mistakes.

For testing, I would suggest to run a few avrdude -p ATmega328P/... commands.

Here one fun application: How much time does it roughly take for a bootloader on the ATtiny85 to emulate chip erase? Answer

expr $(avrdude -p ATtiny85/st | grep num_pages | cut -f5 ) \* $(avrdude -p ATtiny85/st | grep chip_erase_delay | cut -f4 ) / 1000
576

This assumes flash->num_pages * part->chip_erase_delay is a reasonable estimate considering SPM page erase might take roughly ads long, but no longer than an SPI programming chip erase.

Well, it won't be long that the world thinks What does Microchip know about their parts, when I can ask AVRDUDE about a part? :)

stefanrueger and others added 30 commits May 24, 2022 16:56
…on -p \*

  -p \*/c  check address bits in SPI commands
  -p \*/d  description of core part features
  -p \*/o  opcodes for SPI programming parts and memories
  -p \*/s  show avrdude.conf entries of parts
  -p \*/ss show full avrdude.conf entry as tab separated table
  -p \*/w  wd_... constants for ISP parts
  -p \*/\* all of the above except -p \*/s
  -p \*    same as -p\*/\*
When an SPI command has a lone 'a' the initialisation now is as would be
expected by all commands that take an address. Atmel's opcodes for SPI
programming are consistent in this respect. This commit makes specifying
the bit number in avrdude.conf optional. Instead of

 read_lo = "0 0 1 0 0 0 0 0  0 0 a13 a12 a11 a10 a9 a8  a7 a6 a5 a4 a3 a2 a1 a0  o o o o o o o o";

one can now use

 read_lo = "0 0 1 0 0 0 0 0  0 0 a a a a a a  a a a a a a a a  o o o o o o o o";
This commit changes the philosophy whenever avrdude.conf encounters the
same memory of a part for the second time or whenever a memory is
described that, through inheritance, already existed: AVRDUDE no longer
zaps the memory, it rather extends it.

Therefore, avrdude.conf.in's entry for ATmega128RFA1, which inherits from
the ATmega2561, needs a line `load_ext_addr = NULL;` in its flash memory
description to zap the inherited load_ext_addr SPI command.

Other than this, avrdude.conf.in needs no other change in order to effect
the same internal representation proving earlier updates to the .conf.in
file correct that manually ensured inheritance of memory contents.
As the address bit numbers in the SPI opcodes are highly systematic, they
don't really need to be specified. Each bit can therefore be described as one
of the characters 0 (always 0), 1 (always 1), x (don't care, but will be set
as 0), a (a copy of the correct bit of the byte or word address of read,
write, load, pagewrite or load extended address command of memories with more
than one byte), i (input bit for a load/write) or o (output bit from a read).
The bits therefore do not need to be individually separated.

If a string in the list of strings that describe an SPI opcode does *not*
contain a space *and* is longer than 7 characters, it is interpreted as a
compact bit-pattern  representation. The characters 0, 1, x, a, i and o will
be recognised as the corresponding bit, whilst any of the characters ., -, _
or / can act as arbitrary visual separators, which are ignored. Examples:

  loadpage_lo = "0100.0000--000x.xxxx--xxaa.aaaa--iiii.iiii";

  loadpage_lo = "0100.0000", "000x.xxxx", "xxaa.aaaa", "iiii.iiii";

  loadpage_lo = "0100.0000", "000x.xxxx.xxaa.aaaa", "iiii.iiii";

  loadpage_lo = "0100.0000-000x.xxxx--xxaa.aaaa-iiii.iiii";

  loadpage_lo = "0100.0000/000x.xxxx/xxaa.aaaa/iiii.iiii";

The compact format is an extension of the current format, which remains
valid. Both, the compact and the traditional specification can be mixed in
different strings, albeit not in the same string:

  load_ext_addr = "0100.1101", "0000.0000.0000", "0 0 0 a16", "0000.0000";
…ndaries

The function avr_set_addr_mem(AVRMEM *mem, int opnum, unsigned char *cmd,
unsigned long addr) is meant to replace avr_set_addr(OPCODE *op, unsigned
char *cmd, unsigned long addr) in future.

avr_set_addr_mem() has more information about the context of the task in that
it knows the memory size, memory page size, whether or not the memory is a
flash memory (which gets words addressees supplied) and, crucially, knows
which SPI operation it is meant to compute the address bits for.

avr_set_addr_mem() first computes the interval of bit numbers that must be
supplied for the SPI command to stand a chance to work. The function only
sets those address bits that are needed. Once all avr_set_addr() function
calls have been replaced by avr_set_addr_mem(), the SPI commands that need an
address can afford to declare in avrdude.conf all 16 address bits in the
middle two bytes of the SPI command. This over-declaration will be corrected
during runtime by avr_set_addr_mem(). One consequence of this is that parts
can inherit smaller or larger memories from parents without the need to use
different SPI codes in avrdude.conf. Another consequence is that
avr_set_addr_mem() can, and does, tell the caller whether vital address bits
were not declared in the SPI opcode. During parsing of avrdude.conf this
might be utilised to generate a corresponding warning. This will uncover
problematic SPI codes in avrdude.conf that in the past went undetected.
@stefanrueger
Copy link
Collaborator Author

For MSYS2 bash, I tried different ways and none of them are working.

There must be a way of quoting for that shell... Try compiling the following echo.c program

#include <stdio.h>

int main(int c, char **v) {
  for(int i=0; i<c; i++)
    printf("%s%c", v[i], i+1==c? '\n': ' ');
}

... and see how you can execute it, so you get the argument */c printed. What happens when you type a.out */c, a.out "*/c" or a.out \*/c, ...

@mcuee
Copy link
Collaborator

mcuee commented Jul 23, 2022

There must be a way of quoting for that shell... Try compiling the following echo.c program

... and see how you can execute it, so you get the argument */c printed. What happens when you type a.out */c, a.out "*/c" or a.out \*/c, ...

Finally got this working. I need to use */c or better "*//c". *//* does not work but "*//* works.

$ ./echo_msys2_shell.exe "*/*"
C:\work\avr\avrdude_test\readline\echo_msys2_shell.exe *C:/msys64/*

$ ./echo_msys2_shell.exe "*/c"
C:\work\avr\avrdude_test\readline\echo_msys2_shell.exe *C:/

$ ./echo_msys2_shell.exe "*//c"
C:\work\avr\avrdude_test\readline\echo_msys2_shell.exe */c

$ ./echo_msys2_shell.exe "*//*"
C:\work\avr\avrdude_test\readline\echo_msys2_shell.exe */*

$ ./avrdude.exe -p "*//c" | head
.cmderr AT90S4414       flash-read_hi   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-read_lo   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_hi  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_lo  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       eeprom-read     bit 16 outside addressable space should be x or 0 but is a8
.cmderr AT90S4414       eeprom-write    bit 16 outside addressable space should be x or 0 but is a8

.cmderr ATmega64        eeprom-read     bit 19 outside addressable space should be x or 0 but is a11
.cmderr ATmega64        eeprom-write    bit 19 outside addressable space should be x or 0 but is a11

$ ./avrdude.exe -p "*//*" | head
    desc                = "ATtiny11";
    id                  = "t11";
    family_id           = "";
    hvupdi_variant      = -1;
    stk500_devcode      = 0x11;
    avr910_devcode      = 0x00;
    chip_erase_delay    = 20000;
    pagel               = 0x00;
    bs2                 = 0x00;
    signature           = 0x1e 0x90 0x04;

@stefanrueger
Copy link
Collaborator Author

@mcuee Yes, you need to somehow protect the wildcards for the part name to be evaluated by the shell that you use. Maybe leaving out the space does the trick in some of the shells: -p*/c

Copy link
Contributor

@dl8dtl dl8dtl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the question library vs. main CLI only, the changes are fine with me.

@@ -133,6 +133,9 @@ add_library(libavrdude
confwin.c
crc16.c
crc16.h
developer_opts.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether we want the developer opts files be part of the library or not.
If they are not going to be part of the library but only the standard CLI tool (my preference), they need to go into a different section in CMakeLists.txt and Makefile.am.
If they are going to be part of the library, public function declarations ought to be in libavrdude.h rather than in a separate header file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good for the developer opts files to be part of the library.

@mcuee
Copy link
Collaborator

mcuee commented Jul 23, 2022

Yes, you need to somehow protect the wildcards for the part name to be evaluated by the shell that you use. Maybe leaving out the space does the trick in some of the shells: -p*/c

Thanks. Indeed the trick works (both for MSYS2 shell and PowerShell under Windows Terminal).

$ ./avrdude_partdesc.exe -p*/c | head
.cmderr AT90S4414       flash-read_hi   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-read_lo   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_hi  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_lo  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       eeprom-read     bit 16 outside addressable space should be x or 0 but is a8
.cmderr AT90S4414       eeprom-write    bit 16 outside addressable space should be x or 0 but is a8

.cmderr ATmega64        eeprom-read     bit 19 outside addressable space should be x or 0 but is a11
.cmderr ATmega64        eeprom-write    bit 19 outside addressable space should be x or 0 but is a11

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_partdesc.exe -p*/c | head
.cmderr AT90S4414       flash-read_hi   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-read_lo   bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_hi  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       flash-write_lo  bit 19 outside addressable space should be x or 0 but is a11
.cmderr AT90S4414       eeprom-read     bit 16 outside addressable space should be x or 0 but is a8
.cmderr AT90S4414       eeprom-write    bit 16 outside addressable space should be x or 0 but is a8

.cmderr ATmega64        eeprom-read     bit 19 outside addressable space should be x or 0 but is a11
.cmderr ATmega64        eeprom-write    bit 19 outside addressable space should be x or 0 but is a11

@stefanrueger
Copy link
Collaborator Author

I wonder whether we want the developer opts files be part of the library or not

Great shout: I hadn't thought of that. Generally this is CLI stuff, but some of the functions I have initially written for this section later turned out to be useful for parsing in config_gram.y, so could move to libavrdude.

Some, eg, avr_set_addr_mem() might even play a bigger role overall: I suggest to replacing avr_set_addr() with avr_set_addr_mem() in the near future to enable simplification of avrdude.conf. See extended commit message 572849e. I am convinced that this move will harden AVRDUDE against SPI opcode specification error in avrdude.conf as it selects the required address bits algorithmically whilst maintaining the flexibility to set the position of the address bits with the original SPI command syntax. @dl8dtl please specifically review this routine (commit 572849e) thoroughly, so we have confidence this plan will work.

My proposal is to move the following functions to avrpart.c and declare them in libavrdude.h (principally because they are generally useful in a library sense and/or are are actually used):

  • char cmdbitchar(CMDBIT cb);
  • char *cmdbitstr(CMDBIT cb);
  • const char *opcodename(int opnum);
  • char *opcode2str(OPCODE *op, int opnum, int detailed);
  • int intlog2(unsigned int n);
  • int avr_set_addr_mem(AVRMEM *mem, int opnum, unsigned char *cmd, unsigned long addr);
  • int part_match(const char *pattern, const char *string);

I am at sixes and sevens over the following one

  • int opcodecmp(OPCODE *op1, OPCODE *op2, int opnum);

and am tempted to leave that one in developer_opts.c for the time being and make it static. I no longer recall why I made it global, possibly because I thought that could be used for parsing.

@dl8dtl OK?

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Jul 26, 2022

@mcuee, @dl8dtl Following your reviews (thanks!) I have now

  • Improved the help message -p/h for developer option -p
  • Moved some useful CMDBIT/part functions from developer_opts.c to avrpart.c
  • Declared these CMDBIT/part functions in libavrdude.h
  • Moved developer_opts* file names from library section to main section for c/make

I also added a parent_id to parts (and programmers), so that -p*/s now can give a truly compact version of the current avrdude.conf part descriptions that in turn can be understood by AVRDUDE. Just try the following

# Create a hex dump of the raw *internal* representation of all parts
avrdude -p*/r >/tmp/avrdude.conf.raw

# Output the definition of all parts in avrdude.conf syntax
avrdude -p*/s >/tmp/avrdude.conf.parts

# Now edit the system avrdude.conf file so that the section below 
# PART DEFINITIONS gets replaced by /tmp/avrdude.conf.parts
# My system avrdude.conf file shrunk from 19354 lines to 14823 lines

# Below proves this change of the avrdude.conf file has left the internal representation invariant
avrdude -p*/r | diff - /tmp/avrdude.conf.raw  && echo OK
OK

This is the true significance of these developer options -p*/? for parts.

I now consider this PR ready to be merged. What d'ya say?

@stefanrueger
Copy link
Collaborator Author

There is one thing, though: As the output of avrdude -p*/s might actually be used in one of the next avrdude.conf files for real, please have a look at whether you like the formatting of that. It is better we fix a recommended formatting now rather than later. See also comment #934 (comment)

@mcuee
Copy link
Collaborator

mcuee commented Jul 27, 2022

@stefanrueger and @dl8dtl

The new avrdude.conf from this pull request is not backwards compatible. Shall we bump the version to 8.0 (and not 7.1 as planned)?

C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git.exe -C .\avrdude_partdesc.conf -c arduino -p m328p -P COM5
avrdude_git.exe: error reading system wide configuration file ".\avrdude_partdesc.conf"

Or shall we merge the pull request without the change to avrdude.conf and leave that a bit later? In that case, we can still release 7.1 version first. The avrdude executable generated from this pull request is still backwards compatible with prior avrdude.conf file.

C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_partdesc.exe -C .\avrdude.conf -c arduino -qp m328p -P COM5

avrdude_partdesc.exe: AVR device initialized and ready to accept instructions
avrdude_partdesc.exe: Device signature = 0x1e950f (probably m328p)

avrdude_partdesc.exe done.  Thank you.

@MCUdude
Copy link
Collaborator

MCUdude commented Jul 27, 2022

The new avrdude.conf from this pull request is not backwards compatible. Shall we bump the version to 8.0 (and not 7.1 as planned)?

Or shall we merge the pull request without the change to avrdude.conf and leave that a bit later? In that case, we can still release 7.1 version first. The avrdude executable generated from this pull request is still backwards compatible with prior avrdude.conf file.

I'd vote for the latter. Keep avrdude.conf compatible with 7.0 and later bump the version number to 7.1

@mcuee
Copy link
Collaborator

mcuee commented Jul 27, 2022

There is one thing, though: As the output of avrdude -p*/s might actually be used in one of the next avrdude.conf files for real, please have a look at whether you like the formatting of that. It is better we fix a recommended formatting now rather than later. See also comment #934 (comment)

Just to help a bit here. Here are the original and formatted avrdude.conf (from git and from this pull request).

avrdude_conf.zip

It seems to be fine to me. One minor thing is that I like to have one empty line between two parts.

@mcuee
Copy link
Collaborator

mcuee commented Jul 27, 2022

@stefanrueger
The output of avrdude -p*/s does not include the programmer parts. How do you combine the two?

@stefanrueger
Copy link
Collaborator Author

The new avrdude.conf from this pull request is not backwards compatible.

This is an extension to avrdude.conf, which are standard routine (think adding hvudpi variables etc). These happen all the time. The new avrdude.conf is never backward compatible with old versions of AVRDUDE (because old AVRDUDE versions croak with later introduced, ie, unknown variables).

The only incompatibility here is that this PR makes AVRDUDE extend memories rather than zap them. Zapping memories has been considered an error, so the single one incompatibility is desired. Other than that the changes to the grammar are backward compatible. You can always use an old avrdude.conf file and it works with the new AVRDUDE version.

@stefanrueger
Copy link
Collaborator Author

For formatting look at the output of avrdude -p m328/s

#------------------------------------------------------------
# ATmega328
#------------------------------------------------------------

part
    desc                = "ATmega328";
    id                  = "m328";
    stk500_devcode      = 0x86;
    chip_erase_delay    = 9000;
    pagel               = 0xd7;
    bs2                 = 0xc2;
    signature           = 0x1e 0x95 0x14;
    has_debugwire       = yes;
    timeout             = 200;
    stabdelay           = 100;
    cmdexedelay         = 25;
    synchloops          = 32;
    pollindex           = 3;
    pollvalue           = 0x53;
    predelay            = 1;
    postdelay           = 1;
    pollmethod          = 1;
    pp_controlstack     =
        0x0e, 0x1e, 0x0f, 0x1f, 0x2e, 0x3e, 0x2f, 0x3f, 
        0x4e, 0x5e, 0x4f, 0x5f, 0x6e, 0x7e, 0x6f, 0x7f, 
        0x66, 0x76, 0x67, 0x77, 0x6a, 0x7a, 0x6b, 0x7b, 
        0xbe, 0xfd, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00;
    flash_instr         = 0xb6, 0x01, 0x11;
    eeprom_instr        =
        0xbd, 0xf2, 0xbd, 0xe1, 0xbb, 0xcf, 0xb4, 0x00, 
        0xbe, 0x01, 0xb6, 0x01, 0xbc, 0x00, 0xbb, 0xbf, 
        0x99, 0xf9, 0xbb, 0xaf;
    hventerstabdelay    = 100;
    latchcycles         = 5;
    togglevtg           = 1;
    poweroffdelay       = 15;
    resetdelayms        = 1;
    hvleavestabdelay    = 15;
    resetdelay          = 15;
    chiperasepolltimeout = 10;
    programfusepolltimeout = 5;
    programlockpolltimeout = 5;
    ocdrev              = 1;
    chip_erase          = "1010.1100--100x.xxxx--xxxx.xxxx--xxxx.xxxx";
    pgm_enable          = "1010.1100--0101.0011--xxxx.xxxx--xxxx.xxxx";

    memory "eeprom"
        size            = 1024;
        page_size       = 4;
        min_write_delay = 3600;
        max_write_delay = 3600;
        readback        = 0xff 0xff;
        mode            = 65;
        delay           = 20;
        blocksize       = 4;
        readsize        = 256;
        read            = "1010.0000--000x.xxaa--aaaa.aaaa--oooo.oooo";
        write           = "1100.0000--000x.xxaa--aaaa.aaaa--iiii.iiii";
        loadpage_lo     = "1100.0001--0000.0000--0000.00aa--iiii.iiii";
        writepage       = "1100.0010--00xx.xxaa--aaaa.aa00--xxxx.xxxx";
    ;

    memory "flash"
        paged           = yes;
        size            = 0x8000;
        page_size       = 128;
        num_pages       = 256;
        min_write_delay = 4500;
        max_write_delay = 4500;
        readback        = 0xff 0xff;
        mode            = 65;
        delay           = 6;
        blocksize       = 128;
        readsize        = 256;
        read_lo         = "0010.0000--00aa.aaaa--aaaa.aaaa--oooo.oooo";
        read_hi         = "0010.1000--00aa.aaaa--aaaa.aaaa--oooo.oooo";
        loadpage_lo     = "0100.0000--000x.xxxx--xxaa.aaaa--iiii.iiii";
        loadpage_hi     = "0100.1000--000x.xxxx--xxaa.aaaa--iiii.iiii";
        writepage       = "0100.1100--00aa.aaaa--aaxx.xxxx--xxxx.xxxx";
    ;

    memory "lfuse"
        size            = 1;
        min_write_delay = 4500;
        max_write_delay = 4500;
        read            = "0101.0000--0000.0000--xxxx.xxxx--oooo.oooo";
        write           = "1010.1100--1010.0000--xxxx.xxxx--iiii.iiii";
    ;

    memory "hfuse"
        size            = 1;
        min_write_delay = 4500;
        max_write_delay = 4500;
        read            = "0101.1000--0000.1000--xxxx.xxxx--oooo.oooo";
        write           = "1010.1100--1010.1000--xxxx.xxxx--iiii.iiii";
    ;

    memory "efuse"
        size            = 1;
        min_write_delay = 4500;
        max_write_delay = 4500;
        read            = "0101.0000--0000.1000--xxxx.xxxx--oooo.oooo";
        write           = "1010.1100--1010.0100--xxxx.xxxx--xxxx.xiii";
    ;

    memory "lock"
        size            = 1;
        min_write_delay = 4500;
        max_write_delay = 4500;
        read            = "0101.1000--0000.0000--xxxx.xxxx--oooo.oooo";
        write           = "1010.1100--111x.xxxx--xxxx.xxxx--11ii.iiii";
    ;

    memory "signature"
        size            = 3;
        read            = "0011.0000--000x.xxxx--xxxx.xxaa--oooo.oooo";
    ;

    memory "calibration"
        size            = 1;
        read            = "0011.1000--000x.xxxx--0000.0000--oooo.oooo";
    ;
;

@stefanrueger
Copy link
Collaborator Author

The output of avrdude -p*/s does not include the programmer parts. How do you combine the two?

Correct. For testing, I edited /usr/local/etc/avrdude.conf to remove the section below PART DEFINITIONS and include the output of avrdude -p*/s in its place. Alternatively, you can make the same change in src/avrdude.conf.in and install.

@mcuee
Copy link
Collaborator

mcuee commented Jul 27, 2022

The new avrdude.conf from this pull request is not backwards compatible.

This is an extension to avrdude.conf, which are standard routine (think adding hvudpi variables etc). These happen all the time. The new avrdude.conf is never backward compatible with old versions of AVRDUDE (because old AVRDUDE versions croak with later introduced, ie, unknown variables).

The only incompatibility here is that this PR makes AVRDUDE extend memories rather than zap them. Zapping memories has been considered an error, so the single one incompatibility is desired. Other than that the changes to the grammar are backward compatible. You can always use an old avrdude.conf file and it works with the new AVRDUDE version.

No issue with the incompatibility. I am just saying it is better to up-rev avrdude to 8.0 in that case for next release as I consider it to be a major upgrade in that case. If it is named 7.1 then I kind of expect that the conf file for 7.1 version would be compatible with 7.0 version. But maybe that is a wrong expectation (as I can see 5.xx version exist for many years).

@MCUdude
Copy link
Collaborator

MCUdude commented Jul 27, 2022

The new avrdude.conf from this pull request is not backwards compatible. Shall we bump the version to 8.0 (and not 7.1 as planned)?
Or shall we merge the pull request without the change to avrdude.conf and leave that a bit later? In that case, we can still release 7.1 version first. The avrdude executable generated from this pull request is still backwards compatible with prior avrdude.conf file.

I'd vote for the latter. Keep avrdude.conf compatible with 7.0 and later bump the version number to 7.1

Sorry, I thought "old" Avrdude 6.3 and 7.0 conf files would work with this PR. A new Avrdude version should support older conf files, but older Avrdude versions would obviously not support support new features added to Avrdude.conf.

@dl8dtl would be the one to decide a version number for the next release, but I have no issues with this PR being merged 👍

@stefanrueger
Copy link
Collaborator Author

A new Avrdude version should support older conf files

And this PR does! Old .conf files work.

The syntax is backwards compatible, as is the associated semantics albeit with a single exception: Opening a memory in a part now inherits/extends the properties rather than zap its contents. The previous behaviour was not well documented and less well known amongst users, which has led to a few errors as both users and maintainers seem to have expected inheritance. This PR brings expectation, documentation and reality in line.

I wouldn't worry about version numbering at this point owing to that technicality. My recommendation is to merge the PR, so the new behaviour is available asap.

The main goal of this PR is to provide memory inheritance and the tools (developer option -p */? ) to then restructure avrdude.conf to build up ISP parts with good use of memory inheritance, making it more systematic, smaller and punchier.

@mcuee
Copy link
Collaborator

mcuee commented Jul 28, 2022

I wouldn't worry about version numbering at this point owing to that technicality. My recommendation is to merge the PR, so the new behaviour is available asap.

@stefanrueger

I agree to merge the pull request.

Sorry for the versioning noise. I think what you have stated is correct.
New avdude + new avrdude.conf --> OK
New avdude + old avrdude.conf --> OK
Old avdude + old avrdude.conf --> OK
Old avdude + new avrdude.conf --> NOT OK

@stefanrueger stefanrueger merged commit 7f63632 into avrdudes:main Aug 2, 2022
@stefanrueger stefanrueger deleted the partdesc branch August 2, 2022 17:28
@MCUdude
Copy link
Collaborator

MCUdude commented Aug 10, 2022

@stefanrueger I just realised that it has problems with the AVR-Dx/Ex series. Several memories are printed twice:

$ ./avrdude -p avr128da48/S
#------------------------------------------------------------
# AVR128DA48
#------------------------------------------------------------

part parent ".avrdx"
    desc                = "AVR128DA48";
    id                  = "avr128da48";
    hvupdi_variant      = 1;
    signature           = 0x1e 0x97 0x08;
    has_updi            = yes;
    nvm_base            = 0x1000;
    ocd_base            = 0x0f80;

    memory "eeprom"
        size            = 512;
        offset          = 0x1400;
        readsize        = 256;
    ;

    memory "flash"
        size            = 0x20000;
        page_size       = 512;
        offset          = 0x800000;
        readsize        = 256;
    ;

    memory "fuse0"
        size            = 1;
        offset          = 0x1050;
        readsize        = 1;
    ;

    memory "wdtcfg"
        alias "fuse0";
    ;

    memory "fuse0"
        size            = 1;
        offset          = 0x1050;
        readsize        = 1;
    ;

    memory "wdtcfg"
        alias "fuse0";
    ;

    memory "fuse1"
        size            = 1;
        offset          = 0x1051;
        readsize        = 1;
    ;

    memory "bodcfg"
        alias "fuse1";
    ;

    memory "fuse1"
        size            = 1;
        offset          = 0x1051;
        readsize        = 1;
    ;

    memory "bodcfg"
        alias "fuse1";
    ;

    memory "fuse2"
        size            = 1;
        offset          = 0x1052;
        readsize        = 1;
    ;

    memory "osccfg"
        alias "fuse2";
    ;

    memory "fuse2"
        size            = 1;
        offset          = 0x1052;
        readsize        = 1;
    ;

    memory "osccfg"
        alias "fuse2";
    ;

    memory "fuse4"
        size            = 1;
        offset          = 0x1054;
        readsize        = 1;
    ;

    memory "tcd0cfg"
        alias "fuse4";
    ;

    memory "fuse4"
        size            = 1;
        offset          = 0x1054;
        readsize        = 1;
    ;

    memory "tcd0cfg"
        alias "fuse4";
    ;

    memory "fuse5"
        size            = 1;
        offset          = 0x1055;
        readsize        = 1;
    ;

    memory "syscfg0"
        alias "fuse5";
    ;

    memory "fuse5"
        size            = 1;
        offset          = 0x1055;
        readsize        = 1;
    ;

    memory "syscfg0"
        alias "fuse5";
    ;

    memory "fuse6"
        size            = 1;
        offset          = 0x1056;
        readsize        = 1;
    ;

    memory "syscfg1"
        alias "fuse6";
    ;

    memory "fuse6"
        size            = 1;
        offset          = 0x1056;
        readsize        = 1;
    ;

    memory "syscfg1"
        alias "fuse6";
    ;

    memory "fuse7"
        size            = 1;
        offset          = 0x1057;
        readsize        = 1;
    ;

    memory "codesize"
        alias "fuse7";
    ;

    memory "append"
        alias "fuse7";
    ;

    memory "fuse7"
        size            = 1;
        offset          = 0x1057;
        readsize        = 1;
    ;

    memory "codesize"
        alias "fuse7";
    ;

    memory "append"
        alias "fuse7";
    ;

    memory "fuse7"
        size            = 1;
        offset          = 0x1057;
        readsize        = 1;
    ;

    memory "codesize"
        alias "fuse7";
    ;

    memory "append"
        alias "fuse7";
    ;

    memory "fuse8"
        size            = 1;
        offset          = 0x1058;
        readsize        = 1;
    ;

    memory "bootsize"
        alias "fuse8";
    ;

    memory "bootend"
        alias "fuse8";
    ;

    memory "fuse8"
        size            = 1;
        offset          = 0x1058;
        readsize        = 1;
    ;

    memory "bootsize"
        alias "fuse8";
    ;

    memory "bootend"
        alias "fuse8";
    ;

    memory "fuse8"
        size            = 1;
        offset          = 0x1058;
        readsize        = 1;
    ;

    memory "bootsize"
        alias "fuse8";
    ;

    memory "bootend"
        alias "fuse8";
    ;

    memory "fuses"
        size            = 9;
        page_size       = 16;
        offset          = 0x1050;
        readsize        = 16;
    ;

    memory "lock"
        size            = 4;
        offset          = 0x1040;
        readsize        = 4;
    ;

    memory "tempsense"
        size            = 2;
        offset          = 0x1104;
        readsize        = 1;
    ;

    memory "signature"
        size            = 3;
        offset          = 0x1100;
        readsize        = 3;
    ;

    memory "prodsig"
        size            = 125;
        page_size       = 125;
        offset          = 0x1103;
        readsize        = 125;
    ;

    memory "sernum"
        size            = 16;
        offset          = 0x1110;
        readsize        = 1;
    ;

    memory "userrow"
        size            = 32;
        page_size       = 32;
        offset          = 0x1080;
        readsize        = 32;
    ;

    memory "usersig"
        alias "userrow";
    ;

    memory "userrow"
        size            = 32;
        page_size       = 32;
        offset          = 0x1080;
        readsize        = 32;
    ;

    memory "usersig"
        alias "userrow";
    ;

    memory "data"
        offset          = 0x1000000;
    ;
;

@stefanrueger
Copy link
Collaborator Author

Brilliant find @MCUdude! I corrected this on the current PR #1059: the loops to print memories in a certain order wrongly used locate_mem() rather than locate_mem_noalias().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants