Introduce memory types in lieu of string comparisons with memory names #1538
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
The name memtype can mean different things in the AVRDUDE src. Mostly it means the name string of the memory, eg, "flash" or "eeprom"; sometimes it is a character designating a memory, eg, 'E' for "eeprom"; and yet at other times it means a programmer-defined memory type, eg, MTYPE_BOOT_FLASH. The occurrences of memtype in the code have now neen renamed appropriately.
|
Thanks for the PR! I'll read through it carefully and see if I can spot something that seems odd. However, what would have to be someday next week, because I'm away (without my computer) this weekend. |
|
I will carry out some simple tests as part of my tests toward 7.3 release. |
|
My first test shows the results are positive: no behavior differences between this PR and git main. I am tesing the MinGW build sleep issue of serialupdi and this PR has the same results as git main:
|
|
Second simple tests using Edit: need to redo the test with the correct avrdude.conf file. |
|
Edit: need to redo the test with the correct avrdude.conf file. |
|
serialupdi is also okay. git main. |
|
jtag2updi using Arduino Nano Every (offical version from Arduino) -- this PR behaves the same as git main. Note: ignore the timeout as this is a known issue for Windows terminal mode using MinGW and GNU Readline. This PR. git main |
|
Tested jtag2updi again with Nano 4808 and it behaves the same as git main as well. |
MCUdude
reviewed
Oct 29, 2023
src/jtag3.c
Outdated
| u32_to_b4(xd.nvm_fuse_offset, m->offset & ~15); | ||
| } else if (str_starts(m->desc, "lock")) { | ||
| } else if (mem_is_a_fuse(m) && !fuseinit++) { // Any fuse is OK | ||
| u32_to_b4(xd.nvm_fuse_offset, m->offset & ~15); |
There was a problem hiding this comment.
Indent accidentally added?
MCUdude
approved these changes
Oct 29, 2023
There was a problem hiding this comment.
I've read through the entire PR, and I've not seen a single error.
I think this is safe to merge as is, but I'd like to double-check that the way stk500v2.c deals with fuse memory offsets works as intended.
Excellent work @stefanrueger!
|
Simple test using AVR ISPmkII clone and ATmega2560 -- looks okay. |
|
Simple test using the wiring bootloader and ATmega2560 -- results are good as well. I have to use pipes to avoid time out in terminal mode. |
Indeed! This PR should be good to go from my side. |
Rather than finding memories by name this finds memories by the correct type further separating memory names from their functionality. Also fixes a few unchecked avr_locate_mem() calls that could lead to segmentation faults.
|
@stefanrueger your last commit looks good as well. I'll do a little more testing this evening just to be sure. |
|
Sanity check with latest commit: no regression compared to git main when it comes to using old config file. |
Closed
|
It seems to work with xmega as well. Tested with ATxmega32A4 and AVRISP mkII clone. git main |
|
@MCUdude Clearly, reading |
|
Git main: PR #1538: |
|
Sorry, but I don't have a 2-series I can test with. But here are the results for AVR64DD32 and AVR64EA48. AVR64DD32Git main: PR #1538: AVR64EA48Git main: PR #1538: |
|
So, we cannot reproduce the type of failure I saw in @askn37's results. One last try: What about |
|
Git main: PR #1538: |
|
OK, brilliant. So all seems good. Thank you. Might be different FW in @askn37's programmer. |
|
No regression for AVR64EA48 with onboard |
I have the 2-series and it is okay with serialupdi. Need to test jtag2updi later this week. |
|
Testcase JTAGICEmkII FWV=7
|
MCUdude
reviewed
Nov 2, 2023
... so that the fuses access is byte-wise not by paged_load/wite
|
I think this PR is almost ready to merge. @mcuee I would like to see how communication traces in windows look like. Could you test the traces for
Then I'd like to merge. |
|
Yes I will carry out some testing today. |
Done.
Sorry I can not test this one. I have also provided the traces for the other two under Windows.
|
@stefanrueger |
|
This is a proposed patch for issue#1534. The data section start address 0x10000000 required by the PM_PDI chip is not required by the PM_UPDI chip, so modify the corresponding code in jtagmkII_read_byte. Additionally, printf will also be fixed since the display will be distorted if read_chip_rev receives a negative number. Reference: This is in Japanese, but it is a memo for patch production. |
This will require a separate PR. I have no doubt this helps the updi4avr firmware, but we would have to check whether other firmware, in particular jtag2updi is (negatively) affected by this. Also, similar code |
|
| read_chip_rev receives a negative number @askn37 Good catch. I have reviewed |
|
@mcuee Thanks for testing the ser send/recv comms traces. |
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.
Fixes #1330
This PR introduces an integer memory component
typethat encodes the memory type and some of its attributes in bitfields allowing us to separate the name of a memory from its function. This means that instead of a string compare with the memory name, AVRDUDE now bases its programming strategy on testing bits in the type integer.Nothing in the functionality has changed, and actually the type bits are still determined by the memory name, albeit once when parsing the
avrdude.conffile rather than 391 times in 30 source files. There is now only a small step left of externalising thetypeinavrdude.confand fully separating name from function but this is not part of this PR.This is one of the unthankful PRs that do not improve functionality but that attempt to bring clarity to the role and properties of the memories that AVRDUDE deals with. As such this needs a careful review (@MCUdude?) and good testing (@mcuee?) as virtually all programmers will have seen a code change.
The first thing to look at is how memory types are modelled in avr.c. The other, perhaps, how the new type is accessed in
libavrdude.hmacros.Most of the string comparisons could be replaced automatically with bit tests, but the fuses were dealt with mostly by hand.