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

Remove pgm->page_erase() function when unable to erase page #1376

Merged
merged 4 commits into from
Jun 4, 2023

Conversation

stefanrueger
Copy link
Collaborator

Fixes #1267

AVRDUDE treats pgm->page_erase() as an optional function that should only be set if the programmer/part actually can erase pages. jtagmkII, jtag3 and stk500v2 programmers offer page_erase() functions that, in some circumstances, cannot erase pages. This PR attempts to correct this behaviour.

@mcuee mcuee added the enhancement New feature or request label Jun 2, 2023
@stefanrueger stefanrueger changed the title Page erase Remove pgm->page_erase() function when unable to erase page Jun 2, 2023
@stefanrueger stefanrueger added bug Something isn't working and removed enhancement New feature or request labels Jun 2, 2023
@mcuee
Copy link
Collaborator

mcuee commented Jun 3, 2023

@stefanrueger

Just wondering how to test this. I have the STK500V2 based programmer. Supposedly #1266 has already silent the warnings.

@mcuee
Copy link
Collaborator

mcuee commented Jun 3, 2023

I figured out a way to test this PR.
Test results are positive with STK500v2 (and should be similar for AVRISP mkII).

  1. git main without PR Ensure full words are loaded, low-byte first, for ISP programming  #1265 (not exactly revert but similar to revert): there is an error message
PS C:\work\avr\avrdude_test\avrdude_main> git diff
diff --git a/src/avrcache.c b/src/avrcache.c
index da40c7a7..0ddfa1ad 100644
--- a/src/avrcache.c
+++ b/src/avrcache.c
@@ -333,10 +333,10 @@ static int guessBootStart(const PROGRAMMER *pgm, const AVRPART *p) {

 // Page erase but without error messages if it does not work
 static int silent_page_erase(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, unsigned int a) {
-  int bakverb = verbose;
-  verbose = -123;
+//  int bakverb = verbose;
+//  verbose = -123;
   int ret = pgm->page_erase? pgm->page_erase(pgm, p, m, a): -1;
-  verbose = bakverb;
+//  verbose = bakverb;

   return ret;
 }

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git_no_pr1266 -c stk500v2 -P COM3 -p m2560 -t
avrdude_git_no_pr1266: AVR device initialized and ready to accept instructions
avrdude_git_no_pr1266: device signature = 0x1e9801 (probably m2560)
avrdude> dump flash 0x400 0x20
Reading | ################################################## | 100% 0.17 s
00400  00 00 00 00 ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
00410  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
avrdude> write flash 0x400 0xaa 0x55 0xaa 0x55 0xaa 0x55
Caching | ################################################## | 100% 0.02 s
avrdude> flush
avrdude_git_no_pr1266: synching cache to device ...
avrdude_git_no_pr1266 error: this function must never be called
Reading | ################################################## | 100% 93.42 s
Erasing | ################################################## | 100% 0.06 s
Writing | ################################################## | 100% 5.13 s
avrdude> dump flash 0x400 0x20
Reading | ################################################## | 100% 0.08 s
00400  aa 55 aa 55 aa 55 ff ff  ff ff ff ff ff ff ff ff  |.U.U.U..........|
00410  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  |................|
avrdude> quit

avrdude_git_no_pr1266 done.  Thank you.
  1. git main: error message was suppressed by PR Ensure full words are loaded, low-byte first, for ISP programming  #1265
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git -c stk500v2 -P COM3 -p m2560 -t
avrdude_git: AVR device initialized and ready to accept instructions
avrdude_git: device signature = 0x1e9801 (probably m2560)
avrdude> write flash 0x400 0x55 0xaa 0x55 0xaa 0x55 0xaa
Caching | ################################################## | 100% 0.11 s
avrdude> flush
avrdude_git: synching cache to device ...
Reading | ################################################## | 100% 93.39 s
Erasing | ################################################## | 100% 0.06 s
Writing | ################################################## | 100% 5.08 s
avrdude> quit

avrdude_git done.  Thank you.
  1. This PR
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1376 -c stk500v2 -P COM3 -p m2560 -t
avrdude_pr1376: AVR device initialized and ready to accept instructions
avrdude_pr1376: device signature = 0x1e9801 (probably m2560)
avrdude> dump flash 0x400 0x10
Reading | ################################################## | 100% 0.14 s
00400  55 aa 55 aa 55 aa ff ff  ff ff ff ff ff ff ff ff  |U.U.U...........|
avrdude> write flash 0x400 0xaa 0x55 0xaa 0x55 0xaa 0x55
Caching | ################################################## | 100% 0.02 s
avrdude> flush
avrdude_pr1376: synching cache to device ...
Reading | ################################################## | 100% 93.29 s
Erasing | ################################################## | 100% 0.07 s
Writing | ################################################## | 100% 5.13 s
avrdude> quit

avrdude_pr1376 done.  Thank you.
  1. This PR without PR Ensure full words are loaded, low-byte first, for ISP programming  #1265 (not exactly revert but similar to revert):
PS C:\work\avr\avrdude_test\avrdude_sr> git diff
diff --git a/src/avrcache.c b/src/avrcache.c
index b0cb51da..b64cd896 100644
--- a/src/avrcache.c
+++ b/src/avrcache.c
@@ -333,10 +333,10 @@ static int guessBootStart(const PROGRAMMER *pgm, const AVRPART *p) {

 // Page erase but without error messages if it does not work
 static int silent_page_erase(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, unsigned int a) {
-  int bakverb = verbose;
-  verbose = -123;
+//  int bakverb = verbose;
+//  verbose = -123;
   int ret = pgm->page_erase? pgm->page_erase(pgm, p, m, a): -1;
-  verbose = bakverb;
+//  verbose = bakverb;

   return ret;
 }

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1376_mod -c stk500v2 -P COM3 -p m2560 -t
avrdude_pr1376_mod: AVR device initialized and ready to accept instructions
avrdude_pr1376_mod: device signature = 0x1e9801 (probably m2560)
avrdude> dump flash 0x400 0x10
Reading | ################################################## | 100% 0.13 s
00400  aa 55 aa 55 aa 55 ff ff  ff ff ff ff ff ff ff ff  |.U.U.U..........|
avrdude> write flash 0x400 0x55 0xaa 0x55 0xaa 0x55 0xaa
Caching | ################################################## | 100% 0.02 s
avrdude> flush
avrdude_pr1376_mod: synching cache to device ...
Reading | ################################################## | 100% 93.42 s
Erasing | ################################################## | 100% 0.06 s
Writing | ################################################## | 100% 5.10 s
avrdude> dump flash 0x400 0x10
Reading | ################################################## | 100% 0.04 s
00400  55 aa 55 aa 55 aa ff ff  ff ff ff ff ff ff ff ff  |U.U.U...........|
avrdude> quit

avrdude_pr1376_mod done.  Thank you.

@mcuee
Copy link
Collaborator

mcuee commented Jun 3, 2023

Wiring bootloader testing results: using the MegaCore STK500v2 bootloader.

There seems to be an issue but not because of this PR. I will need to check again.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git -c wiring -P COM11 -p m2560 -t
avrdude_git: AVR device initialized and ready to accept instructions
avrdude_git: device signature = 0x1e9801 (probably m2560)
avrdude> dump flash 0x400 0x10
Reading | ################################################## | 100% 0.11 s
00400  00 00 00 00 00 00 ff ff  ff ff ff ff ff ff ff ff  |................|
avrdude> write flash 0x400 0xaa 0x55 0xaa 0x55 0xaa 0x55
Caching | ################################################## | 100% 0.02 s
avrdude> flush
avrdude_git: synching cache to device ...
Reading | ################################################## | 100% 35.30 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_git error: command failed
avrdude_git error: chip erase failed
avrdude> quit
avrdude_git: synching cache to device ...
Reading | ################################################## | 100% 0.01 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_git error: command failed
avrdude_git error: chip erase failed

avrdude_git done.  Thank you.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_git_no_pr1266 -c wiring -P COM11 -p m2560 -t
avrdude_git_no_pr1266: AVR device initialized and ready to accept instructions
avrdude_git_no_pr1266: device signature = 0x1e9801 (probably m2560)
avrdude> write flash 0x400 0xaa 0x55 0xaa 0x55 0xaa 0x55
Caching | ################################################## | 100% 0.08 s
avrdude> flush
avrdude_git_no_pr1266: synching cache to device ...
avrdude_git_no_pr1266 error: this function must never be called
Reading | ################################################## | 100% 35.28 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_git_no_pr1266 error: command failed
avrdude_git_no_pr1266 error: chip erase failed
avrdude> quit
avrdude_git_no_pr1266: synching cache to device ...
avrdude_git_no_pr1266 error: this function must never be called
Reading | ################################################## | 100% 0.00 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_git_no_pr1266 error: command failed
avrdude_git_no_pr1266 error: chip erase failed

avrdude_git_no_pr1266 done.  Thank you.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1376 -c wiring -P COM11 -p m2560 -t
avrdude_pr1376: AVR device initialized and ready to accept instructions
avrdude_pr1376: device signature = 0x1e9801 (probably m2560)
avrdude> dump flash 0x400 0x10
Reading | ################################################## | 100% 0.11 s
00400  00 00 00 00 00 00 ff ff  ff ff ff ff ff ff ff ff  |................|
avrdude> write flash 0x400 0xaa 0x55 0xaa 0x55 0xaa 0x55
Caching | ################################################## | 100% 0.02 s
avrdude> flush
avrdude_pr1376: synching cache to device ...
Reading | ################################################## | 100% 35.35 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_pr1376 error: command failed
avrdude_pr1376 error: chip erase failed
avrdude> flush
avrdude_pr1376: synching cache to device ...
Reading | ################################################## | 100% 0.01 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_pr1376 error: command failed
avrdude_pr1376 error: chip erase failed
avrdude> quit
avrdude_pr1376: synching cache to device ...
Reading | ################################################## | 100% 0.01 s
Erasing | -------------------------------------------------- | 0% 0.01 s
avrdude_pr1376 error: command failed
avrdude_pr1376 error: chip erase failed

avrdude_pr1376 done.  Thank you.

@stefanrueger
Copy link
Collaborator Author

@mcuee: well done for testing this. You can also see the difference to git main in the terminal: The jtag2, jtagmkII and stk500v2 programmer variants should not offer the pgerase command on parts or programmer variants that cannot do a page erase. In git main, when page erase is offered for a classic part in an stk500v2 programmer, the page erase should fail with an unusual(!) message "this function must never be called".

@mcuee
Copy link
Collaborator

mcuee commented Jun 4, 2023

I think this PR is safe to merge now. I will check the Wiring bootloader issue later. In any case, I think wiring bootloader is not popular (only used by Arduino Mega2560 board) and the Arduino bootloader FW is not that good (very old optiboot version and also old STK500v2 bootloader with EEPROM bug not fixed).

@stefanrueger stefanrueger merged commit 8947683 into avrdudes:main Jun 4, 2023
@stefanrueger stefanrueger deleted the page_erase branch June 4, 2023 21:06
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.

To smoke out all the programmers that advertise their page_erase() but then fail to provide it
2 participants