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

avrftdi.c: paged write can fail at addr 0 #1073

Closed
nemuisan opened this issue Aug 20, 2022 · 19 comments · Fixed by #1074
Closed

avrftdi.c: paged write can fail at addr 0 #1073

nemuisan opened this issue Aug 20, 2022 · 19 comments · Fixed by #1074
Labels
bug Something isn't working

Comments

@nemuisan
Copy link

I found possibly bug in avrftdi.c.
It impacts when do sector writes less than sector-size binary.
(e.g. write 202bytes into atmega1284p(256 sector size)

I suggest patch to fix this problem.
It prevents that invalid poll_index value if "addr(unsigned int)" is "0".

@@ -1059,7 +1059,7 @@
 	/* find a poll byte. We cannot poll a value of 0xff, so look
 	 * for a value != 0xff
 	 */
-	for(poll_index = addr+len-1; poll_index > addr-1; poll_index--)
+	for(poll_index = addr+len-1; poll_index-1 > addr; poll_index--)
 		if(m->buf[poll_index] != 0xff)
 			break;
 

I met same problem for 9 yeas ago,using atmega1284p and JTAGKey2 till now.
#303

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2022

Thanks for the report. Sorry I closed #303 based on my tests but indeed I did not test 202 bytes binary. I will test this out with ATmega2560.

You can actually re-open #303 but no problem to open this new one as the patch here seems to be much simpler than the old patches in #303.

@mcuee mcuee added the bug Something isn't working label Aug 20, 2022
@stefanrueger
Copy link
Collaborator

sector writes less than sector-size binary

The paged read write/routines (which in this case call the function you try to fix) are not meant to be called with less than page size length. And I don't think AVRDUDE ever calls them with anything different from mem->page_size or mem->readsize/blocksize. Could you give an example hex file and AVRDUDE command that fails, so we can reproduce.

The suggested change pretends there was a non-0xff byte in the buffer when there might be none. Not sure this is a cure.

@nemuisan
Copy link
Author

Dear mcuee and stefanrueger,

I attached patched/no-patched log with avrdude commands(verbose level was -v -v -v).
and 202byte minimal atmega1284p avr-gcc project .

Please comfirm this.

no_patched.log
patched.log
atmega1284p_minimal_test.zip

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2022

Indeed I can reproduce with the 202B test hex file in the zip file.
atmega1284p_minimal_test.zip

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -p m2560 -U .\test_202B.hex

avrdude.exe: AVR device initialized and ready to accept instructions

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

avrdude.exe: Device signature = 0x1e9801 (probably m2560)
avrdude.exe: NOTE: "flash" memory has been specified, an erase cycle will be performed
             To disable this feature, specify the -D option.
avrdude.exe: erasing chip
avrdude.exe: reading input file .\test_202B.hex for flash
avrdude.exe: writing 202 bytes flash ...

Writing | ################################################## | 100% 0.01s

avrdude.exe: 202 bytes of flash written
avrdude.exe: verifying flash memory against .\test_202B.hex

Reading | ################################################## | 100% 0.06s

avrdude.exe: verification error, first mismatch at byte 0x0000
             0xff != 0x0c
avrdude.exe: verification error; content mismatch

avrdude.exe done.  Thank you.

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2022

Then I tried the patch mentione in #303 but it does not seem to help.
file #29223: avrftdi_sync.patch

PS C:\work\avr\avrdude_test\avrdude_main> git diff
diff --git a/src/avrftdi.c b/src/avrftdi.c
index 315f23e..352a4ab 100644
--- a/src/avrftdi.c
+++ b/src/avrftdi.c
@@ -471,6 +471,35 @@ static inline int avrftdi_transmit(PROGRAMMER * pgm, unsigned char mode, const u
                return avrftdi_transmit_mpsse(pdata, mode, buf, data, buf_size);
 }

+static int avrftdi_sync(avrftdi_t* pdata)
+{
+       unsigned char buf[1];
+       const unsigned char cmd[] = { GET_BITS_LOW, SEND_IMMEDIATE };
+       E(ftdi_write_data(pdata->ftdic, cmd, sizeof(cmd)) != sizeof(cmd), pdata->ftdic);
+
+       int num = 0;
+       do
+       {
+               int n = ftdi_read_data(pdata->ftdic, buf, sizeof(buf));
+               if(n > 0)
+                       num += n;
+               E(n < 0, pdata->ftdic);
+       } while(num < 1);
+
+       /*
+        * TODO: error handling when more bytes are read than anticipated. This would
+        * indicate we lost synchronization with the FTDI device. Not sure what the
+        * proper way of handling this would be. Probably restart programming from
+        * scratch or abort. If avrdude would be capable we could re-init FTDI and
+        * start re-programming from the last page (omitting chip erase, but erasing
+        * the current page, because it might contain garbage).
+        */
+       if(num > 1)
+               log_warn("Read %d extra bytes\n", num-1);
+
+       return 0;
+}
+
 static int write_flush(avrftdi_t* pdata)
 {
        unsigned char buf[6];
@@ -500,24 +529,8 @@ static int write_flush(avrftdi_t* pdata)
         * Use read pin status command as sync.
         */
        //E(ftdi_usb_purge_buffers(pdata->ftdic), pdata->ftdic);
-
-       unsigned char cmd[] = { GET_BITS_LOW, SEND_IMMEDIATE };
-       E(ftdi_write_data(pdata->ftdic, cmd, sizeof(cmd)) != sizeof(cmd), pdata->ftdic);
-
-       int num = 0;
-       do
-       {
-               int n = ftdi_read_data(pdata->ftdic, buf, sizeof(buf));
-               if(n > 0)
-                       num += n;
-               E(n < 0, pdata->ftdic);
-       } while(num < 1);

-       if(num > 1)
-               log_warn("Read %d extra bytes\n", num-1);
-
-       return 0;
-
+       return avrftdi_sync(pdata);
 }

 static int avrftdi_check_pins_bb(PROGRAMMER * pgm, bool output)
@@ -1091,6 +1104,7 @@ static int avrftdi_flash_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m,
        {
                log_warn("Skipping empty page (containing only 0xff bytes)\n");
                /* TODO sync write */
+               avrftdi_sync(to_pdata(pgm));
                /* sleep */
                usleep((m->max_write_delay));
        }

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_issue303.exe -c jtagkey -p m2560 -U .\test_202B.hex

avrdude_issue303.exe: AVR device initialized and ready to accept instructions

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

avrdude_issue303.exe: Device signature = 0x1e9801 (probably m2560)
avrdude_issue303.exe: NOTE: "flash" memory has been specified, an erase cycle will be performed
                      To disable this feature, specify the -D option.
avrdude_issue303.exe: erasing chip
avrdude_issue303.exe: reading input file .\test_202B.hex for flash
avrdude_issue303.exe: writing 202 bytes flash ...

Writing | ################################################## | 100% 0.01s

avrdude_issue303.exe: 202 bytes of flash written
avrdude_issue303.exe: verifying flash memory against .\test_202B.hex

Reading | ################################################## | 100% 0.06s

avrdude_issue303.exe: verification error, first mismatch at byte 0x0000
                      0xff != 0x0c
avrdude_issue303.exe: verification error; content mismatch

avrdude_issue303.exe done.  Thank you.

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2022

Then I tried @nemuisan's patch and it does seem to sort out the issue.

But if I read @stefanrueger's reply correctly, there may be other issue in the code so the fix may not be complete.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_issue1073.exe -c jtagkey -p m2560 -U .\test_202B.hex

avrdude_issue1073.exe: AVR device initialized and ready to accept instructions

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

avrdude_issue1073.exe: Device signature = 0x1e9801 (probably m2560)
avrdude_issue1073.exe: NOTE: "flash" memory has been specified, an erase cycle will be performed
                       To disable this feature, specify the -D option.
avrdude_issue1073.exe: erasing chip
avrdude_issue1073.exe: reading input file .\test_202B.hex for flash
avrdude_issue1073.exe: writing 202 bytes flash ...

Writing | ################################################## | 100% 0.06s

avrdude_issue1073.exe: 202 bytes of flash written
avrdude_issue1073.exe: verifying flash memory against .\test_202B.hex

Reading | ################################################## | 100% 0.06s

avrdude_issue1073.exe: 202 bytes of flash verified

avrdude_issue1073.exe done.  Thank you.

@nemuisan
Copy link
Author

#303 patch was too old and not suitable for newest dude commit.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Aug 20, 2022

The problem is that the address and loop variables are unsigned. Downward loops with unsigned are tricky, and the code got it wrong. So when addr is 0, the following loop is not carried out

avrdude/src/avrftdi.c

Lines 1062 to 1064 in 38aa131

for(poll_index = addr+len-1; poll_index > addr-1; poll_index--)
if(m->buf[poll_index] != 0xff)
break;
because addr-1 ticks over to the largest unsigned int (and nothing is larger than that).

The following would do the job

  for(poll_index = addr+len-1; (int) poll_index >= (int) addr; poll_index--)

That is not sufficient, though. The if after the loop is clearly intended to check whether the break; in the loop succeeded. It needs changing to something to the effect of

if((int) poll_index >= (int) addr && m->buf[poll_index] != 0xff)

@stefanrueger
Copy link
Collaborator

Well, actually it is sufficient to test

if((int) poll_index >= (int) addr)

@mcuee
Copy link
Collaborator

mcuee commented Aug 20, 2022

Well, actually it is sufficient to test

if((int) poll_index >= (int) addr)

@stefanrueger
Yes this works. Thanks.

 MINGW64 /c/work/avr/avrdude_test/avrdude_main
$ git diff
diff --git a/src/avrftdi.c b/src/avrftdi.c
index 315f23e..3f756d7 100644
--- a/src/avrftdi.c
+++ b/src/avrftdi.c
@@ -1059,7 +1059,8 @@ static int avrftdi_flash_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m,
        /* find a poll byte. We cannot poll a value of 0xff, so look
         * for a value != 0xff
         */
-       for(poll_index = addr+len-1; poll_index > addr-1; poll_index--)
+//     for(poll_index = addr+len-1; poll_index > addr-1; poll_index--)
+       for(poll_index = addr+len-1; (int) poll_index >= (int) addr; poll_index--)
                if(m->buf[poll_index] != 0xff)
                        break;

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_issue1073v1.exe -c jtagkey -p m2560 -U .\test_202B.hex

avrdude_issue1073v1.exe: AVR device initialized and ready to accept instructions

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

avrdude_issue1073v1.exe: Device signature = 0x1e9801 (probably m2560)
avrdude_issue1073v1.exe: NOTE: "flash" memory has been specified, an erase cycle will be performed
                         To disable this feature, specify the -D option.
avrdude_issue1073v1.exe: erasing chip
avrdude_issue1073v1.exe: reading input file .\test_202B.hex for flash
avrdude_issue1073v1.exe: writing 202 bytes flash ...

Writing | ################################################## | 100% 0.06s

avrdude_issue1073v1.exe: 202 bytes of flash written
avrdude_issue1073v1.exe: verifying flash memory against .\test_202B.hex

Reading | ################################################## | 100% 0.06s

avrdude_issue1073v1.exe: 202 bytes of flash verified

avrdude_issue1073v1.exe done.  Thank you.

@nemuisan Please help to test as well. Do you agree with @stefanrueger's patch?

@stefanrueger stefanrueger changed the title avrftdi.c: program fails when write less than sector-size binary. avrftdi.c: paged write can fail at addr 0 Aug 20, 2022
@stefanrueger
Copy link
Collaborator

See PR #1074

@nemuisan your patch was almost correct, the loop condition should have been poll_index+1 > addr which is equivalent to (int) poll_index >= (int) addr used in the PR (a matter of taste). In any case, it needs a second change in the if after the loop to detect empty pages (all 0xff). The comments in the else branch of the loop don't bode well, though. @mcuee could you create a test .hex file that starts with at least one page (say 256 bytes) of 0xff followed by non-0xff content and test that, please?

@mcuee
Copy link
Collaborator

mcuee commented Aug 21, 2022

@stefanrueger
My srecord skills are limited. Hopefully the following is correct.

MINGW64 /c/work/avr/avrdude_test/avrdude_bin
$ srec_cat test_202B.hex -intel -crop 0x0000 0x7FFF -offset 0x0200 -o test_202B_offset_0x200.hex -intel

$ cat test_202B_offset_0x200.hex
:020000040000FA
:200200000C9446000C9450000C9450000C9450000C9450000C9450000C9450000C94500068
:200220000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450003E
:200240000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450001E
:200260000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C945000FE
:200280000C9450000C9450000C94500011241FBECFEFD0E4DEBFCDBF0E9452000C946300EA
:2002A0000C9400002E98269A48EC50E020E488E893E1FA013197F1F701970097D1F785B1F4
:0A02C000822785B9F4CFF894FFCF30
:00000001FF

$ cat test_1073.hex
:20000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00
:20002000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE0
:20004000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFC0
:20006000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFA0
:20008000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF80
:2000A000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF60
:2000C000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF40
:2000E000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF20
:20010000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
:20012000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFDF
:20014000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFBF
:20016000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF9F
:20018000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF7F
:2001A000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5F
:2001C000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF3F
:2001E000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF1F
:200200000C9446000C9450000C9450000C9450000C9450000C9450000C9450000C94500068
:200220000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450003E
:200240000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450001E
:200260000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C945000FE
:200280000C9450000C9450000C94500011241FBECFEFD0E4DEBFCDBF0E9452000C946300EA
:2002A0000C9400002E98269A48EC50E020E488E893E1FA013197F1F701970097D1F785B1F4
:0A02C000822785B9F4CFF894FFCF30
:00000001FF

Interestingly the current git only has issues with the test_202B.hex file but not the other two. PR #1074 is good for all three test cases.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -qqp m2560 -U .\test_202B.hex && echo OK
avrdude.exe: verification error, first mismatch at byte 0x0000
             0xff != 0x0c
avrdude.exe: verification error; content mismatch
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -qqp m2560 -U .\test_1073.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -qqp m2560 
-U .\test_202B_offset_0x200.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560 -U .\test_202B.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560 -U .\test_1073.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560 
-U .\test_202B_offset_0x200.hex && echo OK
OK

No regression with old test file as well.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560 
-U .\blink-mega2560+lext-test.hex && echo OK
OK

@mcuee
Copy link
Collaborator

mcuee commented Aug 21, 2022

Same if I use offset of 0x100.

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -qqp m2560
 -U .\test_202B_offset_0x100.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -c jtagkey -qqp m2560
 -U .\test_1073_2.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560
 -U .\test_202B_offset_0x100.hex && echo OK
OK
PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1 -c jtagkey -qqp m2560
 -U .\test_1073_2.hex && echo OK
OK

PS C:\work\avr\avrdude_test\avrdude_bin> cat .\test_202B_offset_0x100.hex
:020000040000FA
:200100000C9446000C9450000C9450000C9450000C9450000C9450000C9450000C94500069
:200120000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450003F
:200140000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450001F
:200160000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C945000FF
:200180000C9450000C9450000C94500011241FBECFEFD0E4DEBFCDBF0E9452000C946300EB
:2001A0000C9400002E98269A48EC50E020E488E893E1FA013197F1F701970097D1F785B1F5
:0A01C000822785B9F4CFF894FFCF31
:00000001FF
PS C:\work\avr\avrdude_test\avrdude_bin> cat .\test_1073_2.hex
:20000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00
:20002000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE0
:20004000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFC0
:20006000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFA0
:20008000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF80
:2000A000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF60
:2000C000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF40
:2000E000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF20
:200100000C9446000C9450000C9450000C9450000C9450000C9450000C9450000C94500069
:200120000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450003F
:200140000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C9450001F
:200160000C9450000C9450000C9450000C9450000C9450000C9450000C9450000C945000FF
:200180000C9450000C9450000C94500011241FBECFEFD0E4DEBFCDBF0E9452000C946300EB
:2001A0000C9400002E98269A48EC50E020E488E893E1FA013197F1F701970097D1F785B1F5
:0A01C000822785B9F4CFF894FFCF31
:00000001FF

@mcuee
Copy link
Collaborator

mcuee commented Aug 21, 2022

@stefanrueger

Interestingly this fixed the remaining issue from #425. Thanks.
#425 (comment)
file #37526: ATTOBASICV234_m32-16MHZ-uart_btldr.hex

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude -qqp m32a -c jtagkey
 -U .\ATTOBASICV234_m32-16MHZ-uart_btldr.hex && echo OK
avrdude.exe: verification error, first mismatch at byte 0x0000
             0xff != 0x0c
avrdude.exe: verification error; content mismatch

PS C:\work\avr\avrdude_test\avrdude_bin> .\avrdude_pr1074v1.exe -qqp m32a -c jtagkey
 -U .\ATTOBASICV234_m32-16MHZ-uart_btldr.hex && echo OK
OK

@stefanrueger
Copy link
Collaborator

@mcuee Thank you for extensive tests. Glad to know empty (only 0xff) are not harmful in avrftdi.c despite the comments hinting there is still work to do in this case (but actually I don't see why that's needed). Good stuff.

FYI, the error occurred whenever the last byte in the first flash page was 0xff and the first page had (at least) one non-0xff byte elsewhere. In this case the first page would not be written. AVRDUDE fills unset bytes in a page with 0xff, so it can write full flash pages. This means that input flash files shorter than a page trigger that problem, too, which @nemuisan found out (thanks for reporting!).

poll_index+1 > addr which is equivalent to (int) poll_index >= (int) addr used in the PR

This is not necessarily true. When I wrote this I had not considered that casts of unsigned variables (poll_index and addr) to int are implementation-dependent. Both gcc and clang document that the cast just reinterprets the bit pattern (in which case above should be equivalent for the ranges of addr and poll_index that will occur). In theory, a compiler might decide to saturate the cast for those unsigned numbers that are larger than the largest int meaning those unsigned values that are outside the int range get changed to the largest integer (eg, -1U which is 0xFFFFffffU would turn into MAX_INT, ie, 0x7FFFfff). gcc and clang however have documented that they implement the int cast so that (int) -1U becomes -1.

Anyway, I since changed the wrong loop condition poll_index > addr-1 to the correct loop condition poll_index+1 > addr which works when addr is 0 and does not need to rely on compiler implementation of the cast.

@stefanrueger
Copy link
Collaborator

Interestingly this fixed the remaining issue from #425.

Not sure about this. The test input file contains a 0xff at byte 128 (counting from 1), ie, the last page of the input file, which is why this particular error has been triggered.

$ avrdude -pm32a/At | grep flash.page_size
.ptmm	ATmega32A	flash	page_size	128

$ srec_cat ATTOBASICV234_m32-16MHZ-uart_btldr -i -output - -c_array | tr \\n " " | cut -d, -f128
 0xFF

@mcuee
Copy link
Collaborator

mcuee commented Aug 22, 2022

Anyway, I since changed the wrong loop condition poll_index > addr-1 to the correct loop condition poll_index+1 > addr which works when addr is 0 and does not need to rely on compiler implementation of the cast.

This is nice. Learned some about compiler implementation. Thanks.

@mcuee
Copy link
Collaborator

mcuee commented Aug 22, 2022

Interestingly this fixed the remaining issue from #425.

Not sure about this. The test input file contains a 0xff at byte 128 (counting from 1), ie, the last page of the input file, which is why this particular error has been triggered.

$ avrdude -pm32a/At | grep flash.page_size
.ptmm	ATmega32A	flash	page_size	128

$ srec_cat ATTOBASICV234_m32-16MHZ-uart_btldr -i -output - -c_array | tr \\n " " | cut -d, -f128
 0xFF

Thanks for pointing out the root cause. Yes I mean to say one of the remaining thing for issue #425 is to check why avrftdi still has issues with the test hex file since it should have fixed the issue caused by the mega commit which still affect the stk500v2 based programmers in the current git main.


Ref:

Just a summary of issue #425.
The root cause of the issue is the mega commit d742827 on 15 Seot 2011.

The mega commit affects multiple programmers but some of them (like jtagmki, usbasp and usbtiny) have since fixed the issue. It does not seem to affect avr109 bootloader and STK500v1.

  1. To be tested: avr910 programmer (Done on 21-August-2022, no issue)
  2. Confirmed and need to be fixed: stk500v2, jtagmkii (AVR Dragon and AVR JTAGICE mkii)
  3. For avrftdi, the particular issue may have been fixed but there is still another issue not related to this issue. (Tested on 21-August 2022, Correct flash paged write for avrftdi.c #1074 will fix this remaining issue)

@nemuisan
Copy link
Author

nemuisan commented Aug 22, 2022

Dear stefanrueger,

I tested your fix and got works fine.

and continue to evaluate next patch #1074

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 a pull request may close this issue.

3 participants