-
Notifications
You must be signed in to change notification settings - Fork 139
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
Write flash rarely works in terminal mode for classic AVRs #1020
Comments
My proposal is to provide in
I have written a cache for the urclock SPM programmer draft that can be utilised here, see #940 (comment) Thoughts? |
Sorry for the short answer, but I'm replying on my phone. I agree with everything you say, and I'm a huge fan of the terminal mode. That's why I spend so much time improving the write command to support other data than bytes, which has turned out to be quite useful for testing purposes. So would we be able to modify a single byte write access? Perhaps by reading a flash page, modifying the content and writing it back again? |
Exactly! With a r/w cache (for speed and lest wearing out memory unnecessarily) and using the paged r/w functions of the programmer (for generality) |
Yes, I noted the addition of strings, etc. Vvv useful! |
Interesting side note: when I tried to update my STK500 using an AVRISPmkII (as the built-in avr910 loader failed), that doesn't work either – it's an AT90S8535 (where the byte write ought to work) but somehow, stk500v2 broke that. Maybe due to decomissioning the SPI_MULTI command. |
There are already some programmers implementing caches (I think JTAG programmers). Generalizing that might be a good move. |
Just some quick tests to confirm that there are problems with avrftdi (JTAGkey2), usbasp, usbtinyisp, ft245r (arduino-ft232r) and optiboot (stk500v1 bootloader).
But wiring bootloader (stk500v2) works fine.
PICkit 2 (with #1004 fix) also works fine.
|
@stefanrueger Your prediction is pretty accurate except PICKit2. |
@mcuee Many thanks for testing! And for developing a good test bench. I am convinced that this will improve the quality of AVRDUDE over time (actually, it already has!), and it gives us more confidence for merging PRs.
I still think terminal write won't work for PICKit2. Could you please try the test again after erasing flash. Explanation: before testing Maybe include into the test protocol the preparation of the MCU to defined state or alternating test strings between tests? |
@stefanrueger
|
And I verfied again that wiring bootloader works fine.
|
ATmega328PB should count as classic AVR. xplainedmini programer (built-in programmer for the Microchip ATmega328PB-XMINI) seems to work fine.
Then I tested two new UPDI AVRs and they work fine as well.
|
AVR109 bootloader for Arduino Leonardo does not seem to support flash write at all in terminal mode.
|
With ATmega4808, I am not able to write to flash in terminal mode using
With ATmega32,
|
@MCUdude BTW, it is very strange that |
@stefanrueger |
@MCUdude
Same for the the ATmega328PB with xplainedmini.
|
Same for an AVRISPMKII clone and a STK500v2 clone (just got them today, the STK500v2 clone is just the programming part and do not support high voltage isp).
|
Before starting on the flash write problem I checked a few edge cases of integer inputs for terminal write. I know from experience that parsing number strings in C is hard. Real hard (so I know where to look ;). Analysis of the few detected edge-case problems below. I have now read the source and know what to do to fix the problems. For the out-of-range problems it's best to make a distinction between unsigned and signed data types. Easy, I'll add the U suffix just like C. Unlike C I plan to treat hex constants without 'U' as unsigned, as users would not understand that 0xff as a signed data type would require two bytes (highest signed byte is 0x7f). Generally, I'd like to treat hex numbers "special" to "do the right thing", for example, making 0x0002 write 2 bytes, so that people can cut and paste tables. Also, I'd like to introduce double as new data type for terminal write (in anticipation of Jörg's promised avr-libc extension for 64-bit double @dl8dtl ;), and get strings and character constants parsed like in C. I have code that I wrote for another project, so that's straightforward. I expect a PR soon-ish for making term.c ready to deal with the write flash problem, but if anyone has a comment or different suggestion, do drop a line. Here my test analysis:
Above write in fill mode initialises 16 bytes with 0x55: cool feature! This is used in all tests to "clear the canvas" like so:
This write command writes one byte, 0xff, to EEPROM location 0x182. Works as expected! The following tests all clear the canvas, carry out one write and then show the read back. For clarity only the relevant write and the read-back will be shown, but each test was carried out as above.
Same as above.
Good. As expected.
Octal, perfect.
Strings. Cool.
Octal 01234567 is 0x53977, indeed. Might be neater if a number used the min space of 1, 2, 4, 8 bytes without warning?
Characters. Cool.
Mixed use. Very cool.
Fill mode repeats the string. How cool is that?
... and fills until it runs out of road. Very good.
Same if the last argument is a 4-byte long. Good.
Explicit size works fine.
Bug? What happened to the "repeat last item" fill? Not expecting that only 0x7f gets filled?
Bug? Why is -128L not a long?
Again, only low byte of the long is used for filling.
In signed interpretation this should write two bytes (-255 does not fit into one byte as signed), and it does. But why a warning saying one byte is being written?
Bug? Why is 0x0f written, not 0xff?
Good. As expected.
Good. As expected.
Bug? Why is 0x7fffFFFFffffFFFF written?
This is correct, as expected.
Bug? Again 0x7fffFFFFffffFFFF is written, but input completely different. There seems to be a problem when an 64-bit input has the high bit set.
Picking up on -0xff... tests. Seen this before.
4 bytes correctly written, but warning says 2 bytes.
8 bytes correctly written, but warning says 4 bytes.
OK, if 0xffffFFFFffffFFFF is interpreted as C-style unsigned number, then -0xffffFFFFffffFFFF should be 1. Interpreted as signed number this should be an "out of range" error. However, 0x8000000000000000 is written.
This is correct, but (as a suggestion for improvement) it might be neater if two bytes were written (think a table of hex words being cut and paste)
Correct, but might be neater if 4 bytes were written.
Correct, but might be neater if 8 bytes were written.
Correct.
Correct.
Input is 2^64-1, but 2^63-1 is written. Probably the same (undetected) overflow error as before.
Input is -2^64+1, but -2^63 is written. Probably the same (undetected) overflow error as before. |
Here the worked out proposal for data representation in terminal write:
|
Thanks for taking your time to look at some of the terminal write edge cases. It looks like I haven't tested all of them myself, so there are obviously a few bugs left. If you have a solution for this, it would be great with a separate PR for this. I'll admit that I've never worked with C strings like this before, so it was quite a challenge to get it (almost) right.
Please do!
I agree! When thinking about it, I would also expect 0x0002 to write two bytes.
Great! I never got time to look at this myself. Perhaps with suffix
Can you elaborate? Change how strings in double-quotes and characters in single-quotes are handled?
Looks great to me! Way better than a Norwegian like me could come up with. I'll assume this would be added to the TEXI documentation and perhaps the manpage? |
@MCUdude PR #1025 available. I take it you volunteer to review? ;) Thanks!
Sure, I'd like to avoid git conflicts, so prob after the PR queue has come down. I am confident @dl8dtl won't complain if we get those merged that are useful, fix problems and have been tested.
un-escaping backslash: For now double without suffix are the default, suffix 'F' makes them 32-bit float (as before). |
Just an update: PR #1025 has been merged. |
Finally I got avr109 bootloader working with Arduino Uno (ATmega328p, tested with avrdude and avrosp2). But it does not seem to support Flash writing in terminal mode even with all the bootloader options enabled. EEPROM support is okay. Ref: avrosp2: https://xdevs.com/doc/xDevs.com/M16/AVROSP2/
|
FYI as well.
Not so sure if the issue is only relevant to JTAG1 or it affects JTAG2 (JTAGICE mkii or AVR Dragon) and JTAG3 (JTAG ICE 3 and Atmel-ICE and Power Debugger) in JTAG mode.
JTAG2 (AVR Dragon in JTAG mode).
|
Writing zeros works, but writing 1's doesn't. Atmel ICE (JTAG) and ATmega128
Atmel ICE (JTAG) and AT90CAN128
|
https://avrdudes.github.io/avrdude/7.0/avrdude_39.html#Troubleshooting
Based on the test results, the solution may need some minor updates. Something like the following.
|
From the following datasheet, by right JTAG can write Flash in words (two bytes) or in Page, after chip erase (can not write 1's). https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-8151-8-bit-AVR-ATmega128A_Datasheet.pdf For JTAG 31.10.17. Programming the Flash
A more efficient data transfer can be achieved using the PROG_PAGELOAD instruction:
|
If we based on the above datasheet, Not able to write 1s is an AVR chip JTAG limitation. However, "Currently, jtagmkI and jtagmkII codes cannot write to the flash ROM one word at a time" is an avrdude limitation.
Again from datasheet. Related Links |
Reference post from @MCUdude here.
|
@stefanrueger has an interesting comment here. It is kind of the unltimate brutal force solution to work around the limitations of the programers. I think it is probably not worth doing though, in most cases (unless desperate).
The proposal in this issue is much better and feasible whenever possible. |
Turns out that this issue is not a bug, after all. A litte-noted line in the documentation says: [Terminal write] is not implemented for bank-addressed memories such as the flash memory of ATMega devices. |
So PR #1106 is a proposal for a shiny new feature ;) |
Closed with PR #1106 |
The reason for this is that terminal mode relies on the generic
avr_write_byte()
routineavrdude/src/term.c
Lines 569 to 570 in 64ee485
avr_write_byte_default()
ATtiny12
,ATtiny15
,AT90S1200
,AT90S4414
,AT90S2313
,AT90S2333
,AT90S2343
,AT90S4433
,AT90S4434
,AT90S8515
,AT90S8535
)term.c
reads back to check there will be a failureavrdude/src/term.c
Lines 581 to 586 in 64ee485
Affected programmers:
avrftdi.c
,buspirate.c
,ft245r.c
,linuxgpio.c
,linuxspi.c
,par.c
,pickit2.c
,serbb_posix.c
,serbb_win32.c
,stk500.c
,usbasp.c
,usbtiny.c
For those who prefer to test, run
avrdude -t
with your favourite AVR part and your favourite programmer:I have not looked at those programmers that specify their own write byte routine; these might work in terminal mode.
Of course, if any program should be able to modify a flash byte interactively, it should be AVRDUDE, not? Although AVRDUDE started out as a simple uploader/downloader 20 years ago, it's now considered the ultimate AVR programming tool these days.
The text was updated successfully, but these errors were encountered: