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

Ignore target memories not present in avrdude.conf #1036

Closed
wants to merge 1 commit into from

Conversation

MCUdude
Copy link
Collaborator

@MCUdude MCUdude commented Jul 19, 2022

I'm just throwing this one in to check if there's any interest. Please close if permissive "features" like this are out of the question.

This is basically a more generic approach to #981.
Instead of terminating, it will now ignore a memory passed to Avrdude using -U, but not present in avrdude.conf.
The changes in main.c is there to prevent Avrdude from both trying to write and then verify a memory not present, which would lead to duplicated warning messages

Without this PR:

$ ./avrdude -cusbtiny -patmega2560 -Unotpresent:w:0x00:m -Uefuse:w:0xfd:m

avrdude: AVR device initialized and ready to accept instructions

Reading | ################################################## | 100% 0.01s

avrdude: Device signature = 0x1e9801 (probably m2560)
"notpresent" memory type not defined for part "ATmega2560"

avrdude done.  Thank you.

With this PR:

$ ./avrdude -cusbtiny -patmega2560 -Unotpresent:w:0x00:m -Uefuse:w:0xfd:m

avrdude: AVR device initialized and ready to accept instructions

Reading | ################################################## | 100% 0.01s

avrdude: Device signature = 0x1e9801 (probably m2560)
avrdude: warning: "notpresent" memory type not defined for part "ATmega2560"

avrdude: reading input file "0xfd"
avrdude: writing efuse (1 bytes):

Writing | ################################################## | 100% 0.00s

avrdude: 1 bytes of efuse written
avrdude: verifying efuse memory against 0xfd:

Reading | ################################################## | 100% 0.00s

avrdude: 1 bytes of efuse verified

avrdude done.  Thank you.

@mcuee mcuee added the enhancement New feature or request label Jul 19, 2022
@mcuee
Copy link
Collaborator

mcuee commented Jul 20, 2022

I think this is useful.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1036.exe -c usbasp -qp m8a  -D
 -U efuse:r:-:h -U lfuse:r:-:h -U hfuse:r:-:h

avrdude_pr1036.exe: AVR device initialized and ready to accept instructions
avrdude_pr1036.exe: Device signature = 0x1e9307 (probably m8a)
avrdude_pr1036.exe: warning: "efuse" memory type not defined for part "ATmega8A"

avrdude_pr1036.exe: reading lfuse memory:
avrdude_pr1036.exe: writing output file "<stdout>"
0xef
avrdude_pr1036.exe: reading hfuse memory:
avrdude_pr1036.exe: writing output file "<stdout>"
0xd9

avrdude_pr1036.exe done.  Thank you.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git.exe -c usbasp -qp m8a  -D 
-U efuse:r:-:h -U lfuse:r:-:h -U hfuse:r:-:h

avrdude_git.exe: AVR device initialized and ready to accept instructions
avrdude_git.exe: Device signature = 0x1e9307 (probably m8a)
"efuse" memory type not defined for part "ATmega8A"

avrdude_git.exe done.  Thank you.

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 21, 2022

The only thing I wonder is whether it makes sense to have an additional option for this.

Otherwise it's fine.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Jul 23, 2022

@MCUdude I like permissive behaviour and support this PR, in principle. I don't "get" the change in main.c, and I would leave main.c unchanged. I think I know what's intended: unless -V is given, any -U mem:w:... will create an implicit -U mem:v:... automatically, which is inserted in the list just behind the :w item, and the change in main.c seems to rely on -V having been absent and that the next thingy has been automatically inserted, but I don't see how one can rely on this.

I have in mind (should create an issue lest I forget) to propose a change of how verification is done. This automatic insertion of mem:v:... into the update chain breaks when the input file is stdin (because the file is reopened and read twice, which obvs fails for stdin). Instead I propose using a global variable (verify_all_writes or some such) that the write routine can check so it can carry out the verify with a cached (still present) read file. So, I hope that this double warning will go away on its own once I've come round to propose a solution for hardening AVRDUDE for stdin.

AVRDUDE has a great overall structure of encapsulation and modularisation, but this comes at a price where the individual units often don't know what the overall task is (like verify/do not verify). Having spotted similar problems in other parts (switch off trailing-0xff optimisation, do not complain that verification fails when the memory cannot be read owing to a such configured bootloader, etc) I would encourage all user options to be global. Global variables have been vilified in the 70s and 80s but really, they can be quite helpful.

[edit: should read in the 80s and 90s, not 70s and 80s]

@stefanrueger
Copy link
Collaborator

The only thing I wonder is whether it makes sense to have an additional option for this.

Good question. So, If I mistype a memory, say a fuse, could a situation arise where being permissive damages the part (or my expectation)?

@stefanrueger
Copy link
Collaborator

I would leave main.c unchanged

And here is another reason why: -V -U notexisting:w:0x12:m does not even warn once, I think. Also note that prev on the first run of the loop will contain the last used upd value from some other part of main (so to all intents and purposes is undefined).

@MCUdude
Copy link
Collaborator Author

MCUdude commented Jul 23, 2022

I don't "get" the change in main.c, and I would leave main.c unchanged

The idea was to prevent the warning message to be printed twice when trying to write to an invalid memory space. Is there a different and hopefully better way we can handle this?

(Without changes to main.c)

$ ./avrdude -cusbtiny -patmega2560 -Unotpresent:w:0x00:m -Uefuse:w:0xfd:m

avrdude: AVR device initialized and ready to accept instructions

Reading | ################################################## | 100% 0.00s

avrdude: Device signature = 0x1e9801 (probably m2560)
avrdude: warning: "notpresent" memory type not defined for part "ATmega2560"

avrdude: warning: "notpresent" memory type not defined for part "ATmega2560"

avrdude: reading input file "0xfd"
avrdude: writing efuse (1 bytes):

Writing | ################################################## | 100% 0.00s

avrdude: 1 bytes of efuse written
avrdude: verifying efuse memory against 0xfd:

Reading | ################################################## | 100% 0.00s

avrdude: 1 bytes of efuse verified

avrdude done.  Thank you.

@stefanrueger
Copy link
Collaborator

Is there a different and hopefully better way we can handle this?

Yes, do nothing here and wait until Issue #1044 is solved (or solve that first).

@MCUdude
Copy link
Collaborator Author

MCUdude commented Jul 25, 2022

Is there a different and hopefully better way we can handle this?

Yes, do nothing here and wait until Issue #1044 is solved (or solve that first).

I agree that #1044 should be solved first

@stefanrueger
Copy link
Collaborator

Assuming PR #1051 is accepted, one could change this PR to make do_op() return LIBAVRDUDE_SOFTFAIL if the memory does not exist. The caller main() might then check the return of do_op() against LIBAVRDUDE_GENERAL_FAILURE and only exit if that was returned (ie, continue on success and softfail). This would implement the desired behaviour.

The double warning is no longer an issue should PR #1051 be accepted.

@MCUdude
Copy link
Collaborator Author

MCUdude commented Aug 3, 2022

I'm closing this in favor of #1053, which provides a more elegant fix than this PR.

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