Provide cached byte-wise read/write API #1106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- int avr_read_byte_cached(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, unsigned char *value); - int avr_write_byte_cached(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, unsigned char data); - int avr_flush_cache(const PROGRAMMER *pgm, const AVRPART *p); avr_read_byte_cached() - Uses a cache if paged routines available and if memory is EEPROM or flash - Otherwise fall back to pgm->read_byte() - Out of memory addr: writes the current (page) cache and pretends reading a 0 correctly - Out of cache addr: writes the current (page) cache, then reads cache page for addr, and reads the correct byte - Cache is automagically created and initialised if needed avr_write_byte_cached() - Uses a cache if paged routines available and if memory is EEPROM or flash - Otherwise fall back to pgm->write_byte() - Out of memory addr: write current (page) cache and pretend writing correctly - Out of cache addr: write current (page) Cache, then fills cache to write to addr - Cache is automagically created and initialised if needed avr_flush_cache() - User should call this at the end to finally write all caches to device and free them
|
Cool, I've give it a try tomorrow! |
MCUdude
reviewed
Oct 2, 2022
src/Makefile.am
Outdated
| @@ -92,6 +92,7 @@ libavrdude_a_SOURCES = \ | |||
| avr.c \ | |||
| avr910.c \ | |||
| avr910.h \ | |||
| avrcache.c \ | |||
There was a problem hiding this comment.
A tiny formatting error here. Too many spaces used as indent.
There was a problem hiding this comment.
:) The old tab/space mixup. I had not anticipated this list be written with tabs.
|
I just gave the PR a try using a USBtinyISP and an ATmega2560. It appears to work just fine, but to my surprise, there's no delay when turning 0's into 1's! I'm not getting the |
It's still all in the cache. At this stage AVRDUDE does not yet want to write the flash page. Once that happens ( |
|
... ahhh and, of course, what also happened is that because you were in a single session the change from h to H and back to h all happened in the cache, not on the device flash, so when you finally quit the last written string was written onto the pristinely erased device, so possible w/out erasing it anew. My examples were different avrdude sessions that, at their respective ends, forced avrdude to synchronise with the device |
Maybe this info should be printed when using terminal mode to write to flash-like memories? I tend to just hit |
|
Initial test with usbasp seems to be good. The current git main will fail but the this PR works fine. |
|
The cache mechanism may need some explanation though. Not so sure why the cache message only shows after the quit command. |
- Now cached write only ever occurs when leaving the terminal - Added documentation about this new terminal feature - Added avr_chip_erase_cached() to set cache after CE - Added avr_reset_cache() to reset cache - Added an abort command for the terminal to reset the cache
|
@mcuee and @MCUdude: Thanks for comments. Following you comments I improved the cache API for terminal read/writes of flash/EEPROM
Please check different programmers and different MCU families (PDI, UPDI, ISP, ...). My standard bash test script is The penultimate command takes a looooong time (6 minutes) b/c a 1 bit needs be written that was 0 before. Writing this bit requires reading all of flash (and EEPROM), then a chip erase followed by a complete rewrite of the device. Should work for bootloaders, too, eg, I suspect terminal write might also work for jtag writes of EEPROM (which are said to only be able to write 0). Please review and test. |
ctrl+d is better (EOF in Un*x terminals) |
|
It seems to work fine for the following simple test cases using usbasp. I will try JTAG1 with EEPROM and bootloader later as well. |
|
@mcuee Thanks for testing. This is brilliant. I've just tested the arduino bootloader with and the result So probably good to merge unless there are further comments. |
|
I'd still like to test a little more on various hardware (JTAG, PDI, UPDI etc.) before merging. But if writing the cache to flash may take some time, how about a progress bar that's visible in non-quell mode? |
@stefanrueger Indeed it seems to work, tested with JTAG1 and ATmega32A. So I think this PR will fix #1072 and #1054. Ref: #1072 (enhancement) and #1054 (bug)
|
|
More complicated test case with JTAG1 and EEPROM. When I saw the "chip erase needed" information, I was worried that the Flash would get erazed. I was relieved that the Flash still works and not get erased at all. |
This was referenced Oct 4, 2022
|
@stefanrueger |
If the JTAGmkI officially supports a part (ATmega32 for instance), it also supports die-shrink variants like ATMega32A as well. |
|
JTAG1 is also okay with Flash. And it is good after the last operation the application still works. |
|
As I mentioned earlier, a progress bar when writing the cache in non-quell mode would be nice. I'm also getting a funny warning when using the avrispmkii; |
Also improves avr_erase_chip_cached() to preset and reset cache as the case may be.
Good point. I noted that I forgot to treat the offsets for apptable and boot for the XMEGAs. That's now done. |
|
Here's some output from various programmers and targets. I'm using the following commands: pkobn_updi + ATmega3209pkobn_updi + AVR64DD32xplainedmini_updi + ATmega4808serialupdi + ATmega4808jtag2updi + ATmega4808 (writing cache seemed quicker that other UPDI programmers)avrispmkii + ATxmega128A3U (was very quick writing the cache)dragon_jtag + ATxmega256A3BU (was very quick writing the cache)dragon_isp + ATmega8535stk600 + ATmega2560 (writing cache slow as heck!)xboot/avr109/butterfly + ATmega2560 (just for fun really)usbasp + ATmega1281usbtiny + ATmega128 |
Hilarious, indeed. This PR is one of the early adopters of
Yes, long waits are much neater with progress bars: @MCUdude and @mcuee: Many thanks for sterling and systematic testing. By and large the idea seems to work well, and it's good to see the terminal mode can now actually write to flash on the majority of devices. I am particularly pleased with jtag EEPROM writes now working in the terminal, too. And yes, programming one bit on the ATmega2560 can be slooooow: I don't have ATxmega devices, so cannot test the different flash type memories such as |
Any insights why this combo doesn't work in terminal mode? Does it work with |
I expect all xmegas and the newer UPDI interfaces to be vvv quick owing to page erase. No need to download the whole chip, erase it and upload it again... |
|
Thanks for the progress bar @stefanrueger! STK600 output below. The STK600 was way faster once I added We should probably look into the Lines 1690 to 1694 in 3b8f41c I tried to add |
Seems to work just fine! avrispmkii + ATxmega128A3U bootavrispmkii + ATxmega128A3U applicationavrispmkii + ATxmega128A3U apptable |
Glad to hear. But how can avrdude read 256k via 115200 baud in 12.8 s? It takes a minimum of 23 s at that baud rate... Or is the baudrate something nominal and the real comms speed much higher? Or does the STK600 protocol have some compression over the comms line? And why is the USBtiny so slow, indeed? An effective throughput below 9600 baud...
And when you write "Hello, apptable\n" to apptable 0 and read the corresponding address from flash, still there? And vice versa? |
The first command sets
You can try to add |
@MCUdude Ref: my test result under Linux, not as fast as your STK600 of 12.8s but still quite fast (14.52s). |
This may be because the avrispmkii uses a USB chip (pdiusbd12), while the STK600 uses an AVR, AT90USB1287, with native USB support. |
USBtiny is slow, adding Please refer to my test result here. BTW, I have consolidated some benchmark results here. |
- Added the syntax report_progress(1, -1, NULL) to abort a report neatly on detection of errors. For example aborting in the middle looks like Writing | #########################------------------------- | 50% 0.73s - In terminal, made a careful distinction between reporting Caching (for eeprom/flash) and Writing (other mem types) - Also did slight code refactoring
Now, I am happy with the current state of this PR. I could suppress the funny warnings for not implemented page erase functions, but I think that serves as useful reminder to us(*) to have a close look at proper implementation of page erase for UPDI etc. (*) us means in this context anyone but me ;) I don't have the hardware to test the page erase functionality. Will do a squash merge soon unless I hear otherwise. |
It appears to me that it's impossible to perform a page erase over ISP, is this correct? At least, it seems like no ISP programmer implementation supports page erase over ISP. As for UPDI, page erase should be implemented for JTAG3/EDBG and serialupdi. I'll look into the JTAG3 part and see if I can figure it out for UPDI targets. Maybe @dbuchwald can look into the SerialUPDI page erase functionality, now that Avrdude can utilize this in a useful way? |
Correct. Though bootloaders on parts with ISP interface can (and do) erase a page using SPM. I still plan to furnish my (overdue) urclock programmer with (optional) page erase functionality if the boodloader signals that it provides that separately. Normally, however, bootloaders would just erase a page before they write it, that is, for them memory does not look like NOR-memory that cannot set a bit once cleared. This PR just checks the existence of pgm->page_erase, and if that exists then it's called and if furthermore that works out it will be used: https://github.com/stefanrueger/avrdude/blob/8be06f4cb342a5558f15c017ae258453d334af2b/src/avrcache.c#L345-L355 |
|
The other aspect of page_erase is that it's a programmer function that I take to be applicable to all eeprom and flash like memories, just like paged_load and paged_write. So, any implementation ideally caters for all these, though the caller should be prepared to see page_erase return -1. |
Since BTW I think I figured how to get flash page erase working on UPDI targets using a JTAG3 compatible programmer. I'll submit a PR or open an issue regarding this tomorrow. Without jtag3 UPDI page erase fixWith jtag3 UPDI page erase fix |
Yes, the programmer should not say this. It should rather not offer pgm->page_erase, ie, set it to NULL. It's one of those functions that are not mandatory in the programmer and which the caller is obliged to check for existence first. This is outside this PR, though.
Vvv cool. Cannot wait to see the PR! |
MCUdude
added a commit
to MCUdude/avrdude
that referenced
this pull request
Oct 5, 2022
Very handy to have now that avrdudes#1106 is merged
I've pushed the fix here. It's just a few code lines, but it takes time to track something like this down. Now all we need is page erase functionality for SerialUPDI! |
Brill - vvv cool to see page erase coming along! |
I think it is good to create a PR with your fix without wating for serialupidi, as your fix already works for JTAG3 based updi programmers. |
This was referenced Oct 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supposed to alleviate Issue #1020.
Provides an API for cached byte-wise access
int avr_read_byte_cached(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, unsigned char *value);int avr_write_byte_cached(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, unsigned char data);int avr_flush_cache(const PROGRAMMER *pgm, const AVRPART *p);avr_read_byte_cached()avr_write_byte_cached()avr_flush_cache()The terminal calls
pgm->read_byte_cached(),pgm->read_write_cached()andpgm->flush_cache(); these are initialised withavr_read_byte_cached(),avr_write_byte_cached()andavr_flush_cache(). This indirection is useful b/c this cache is implemented with vvv generic paged load/write calls; individual programmers might have a much slicker cache and can overload correspondingly.Writing the current cache page is a bit tricky. Normally one can use a small cache and write to the device using paged write for those pages that were changed. However, some memories look like NOR memories, ie, they can only write 0 bits, not 1 bits. When this is detected, either page erase is deployed (typically PDI/UPDI parts only), or if that is not available, both EEPROM and flash caches are expanded to cover the full device. This takes some time, of course. When finally at the end the cache is synchronised to the device, again, this takes some time, as a full chip erase is needed followed by writing back both EEPROM and flash memories.
Give it a spin!
Here my simple test (note the time taken for changing the character H to h in flash):