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

Fix Issue #915 wrong or missing SPI commands in .conf #934

Closed
wants to merge 17 commits into from

Conversation

stefanrueger
Copy link
Collaborator

@stefanrueger stefanrueger commented Apr 11, 2022

Makes avrdude warn about SPI programming command definitions in .conf with too few bits or with incorrect address bits; two levels of warning exist:

  • Standard (no -v needed) if address bits miss or are wrong, and thus would make the SPI command fail
  • When -v is given also warns about superfluous address bits in the command that are unlikely to affect functionality

Ensures that a lone a in SPI programming commands uses the correct address bit number (depending on its position in the command). This includes load_ext_addr and considers the use of word addresses in flash memories and byte addresses everywhere else. The previous initialisation for lone a was not helpful, which may be the reason why it never could have been, and indeed never was, used in .conf files.

Also repairs the avrdude.conf.in file for the serious errors mentioned in Issue #915, most of which avrdude now warns about.

This PR addresses two types of human error that have kept appearing in the .conf files:

  1. Wrong assignment of address lines in a bits or missing address bits
  2. Opening a memory section to modify parameters in a parent inherited part description (in fact it zaps all parameters first)

The reason why avrdude -v warns about superfluous address bit errors in SPI commands is that these indicate copy/paste errors that warrant a further look at the description.

Limitations of the PR: The first type of human error is now thoroughly handled in that avrdude warns when it sees this behaviour, and that the current .conf file has been scrubbed of these errors. The second problem is only partially handled by this PR in that only some obvious errors affecting some 5 parts have been corrected in .conf.

@MCUdude
Copy link
Collaborator

MCUdude commented Apr 12, 2022

The formatting in avrdude.conf is a bit off in several places. Would be great if you could make sure the indentation level and spaces between numbers match the existing content or the content it replaces.

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Apr 12, 2022

Would be great if you could make sure the indentation level and spaces between numbers match ...

As first step, I think the PR should be reviewed. After all, it fixes serious SPI command problems of some 25% of SPI programming capable parts. Once that's done and people are happy with it then I am most happy to rewrite the formatting of the SPI programming commands.

BTW, what is the expected formatting? The current formatting of the 1371 commands in .conf is

  • Two lines with two strings (814 times 146 of which use a bits)
  • Four lines with four strings (517 times all of which using a bits)
  • Some of the 4-line descriptions indent the last 3 lines differently than the first line
  • Most are bit aligned/quite a few are not bit aligned or not aligned at all
  • Some come with nibble spacing/some without nibble spacing
  • Most start the command at position 9, but not all
  • Most of which align the = sign, but not all
  • Mixed use of styles happens within a description of a single memory of a single part
  • Some use initial spaces in the strings, some don't
  • Some use tabs, probably intended to be 8-spaces stops, some use 4-space tabs, some spaces

Don't just take my word for it, look at:

avrdude/src/avrdude.conf.in

Lines 8648 to 8661 in 60960ba

write = " 1 1 0 0 0 0 0 0",
" 0 0 0 x x x x a8",
" a7 a6 a5 a4 a3 a2 a1 a0",
" i i i i i i i i";
loadpage_lo = " 1 1 0 0 0 0 0 1",
" 0 0 0 0 0 0 0 0",
" 0 0 0 0 0 0 a1 a0",
" i i i i i i i i";
writepage = " 1 1 0 0 0 0 1 0",
" 0 0 x x x x x a8",
" a7 a6 a5 a4 a3 a2 0 0",
" x x x x x x x x";

or

avrdude/src/avrdude.conf.in

Lines 8848 to 8854 in 60960ba

write = "1 1 0 0 0 0 0 0 0 0 x x x x x a8",
"a8 a6 a5 a4 a3 a2 a1 a0 i i i i i i i i";
loadpage_lo = " 1 1 0 0 0 0 0 1",
" 0 0 0 0 0 0 0 0",
" 0 0 0 0 0 0 a1 a0",
" i i i i i i i i";

The venerable ATmega328 uses this style

avrdude/src/avrdude.conf.in

Lines 9596 to 9604 in 60960ba

write = " 1 1 0 0 0 0 0 0",
" 0 0 0 x x x a9 a8",
" a7 a6 a5 a4 a3 a2 a1 a0",
" i i i i i i i i";
loadpage_lo = " 1 1 0 0 0 0 0 1",
" 0 0 0 0 0 0 0 0",
" 0 0 0 0 0 0 a1 a0",
" i i i i i i i i";

@stefanrueger
Copy link
Collaborator Author

The vast majority (5382) of assignments happen at column position 25:

$ for i in {1..30}; do echo $(expand -t8 src/avrdude.conf.in | cut -c$i | grep = | wc -l) $i; done |\
  sort -nr | head -6
5382 25
886 19
719 22
577 26
410 9
292 15

How about I

  • expand all tabs in avrdude.conf.in to 8-space stops
  • rewrite all memory SPI commands into one of the following two formats
        write           = "1 0 1 0 1 1 0 0   1 0 1 0 0 0 0 0",
                          "x x x x x x x x   i i i i i i i i";

when no address bit is used in the command, and something like

        loadpage_hi     = "0 1 0 0 1 0 0 0",
                          "0 0 0 x x x x x x x a5 a4 a3 a2 a1 a0",
		          "i i i i i i i i";

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

when at least one address bit is used?

I'll make sure that after parsing the avrdude.conf file the internal parsed representation of the beautified avrdude.conf is identical to that of the previously used one.

Answers on a postcard, please.

@stefanrueger
Copy link
Collaborator Author

Unless there are more comments I consider this PR now finished

@MCUdude
Copy link
Collaborator

MCUdude commented Apr 14, 2022

Thank you for spending the time doing all this! I'm just a contributor that just hangs around here. @dl8dtl has to review this in detail.

@stefanrueger stefanrueger changed the title Fix Issue #915 Fix Issue #915 wrong or missing SPI commands in .conf Apr 16, 2022
@mcuee mcuee added the bug Something isn't working label Jun 20, 2022
@stefanrueger
Copy link
Collaborator Author

I am closing this PR, as avrdude.conf has been fixed and more general work is on the way to make AVRDUDE alert to users entering implausibe SPI commands or wrong address lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants