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

Use page erase for UPDI programming #1837

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

stefanrueger
Copy link
Collaborator

This PR makes AVRDUDE use page erase for UPDI parts.

Traditionally, AVRDUDE has used page erase for XMEGAs in favour of chip erase unless the user decided to go for -e:

avrdude/src/main.c

Lines 1684 to 1687 in 42c984c

if ((p->prog_modes & PM_PDI) && pgm->page_erase && lsize(updates) > 0) {
pmsg_info("Note: programmer supports page erase for Xmega devices.\n");
imsg_info("Each page will be erased before programming it, but no chip erase is performed.\n");
imsg_info("To disable page erases, specify the -D option; for a chip-erase, use the -e option.\n");

The main change is to use (PM_PDI | PM_UPDI) instead of PM_PDI the remainder being documentation and clarification. A secondary change is to switch off trailing 0xff optimisation when page erase is used over chip erase, the reason being that in the rare but possible case of an application having 0xff data storage at its end that must not be cut off in absence of a CE. (Not doing so has been a bug for XMEGA programming.)

Apart from the fact that it makes sense to program UPDI flash using page erase, it greatly simplifies the treatment of the new bootrow memory (AVR-DU and AVR-EB parts) that needs erasing before writing. Whilst @dbuchwald has offered a PR where bootrow is page erased before writing (thanks!), the same work would need to be done for all other UPDI programmers:

$ avrdude -p 64du28 -c? |& grep -v via.boot | grep -v serialupdi

Valid programmers for part AVR64DU28 are:
  atmelice_updi      = Atmel-ICE (ARM/AVR) via UPDI
  dryrun             = Emulates programming without a programmer via UPDI
  jtag2updi          = JTAGv2 to UPDI bridge via UPDI
  nanoevery          = JTAGv2 to UPDI bridge via UPDI
  jtag3updi          = Atmel AVR JTAGICE3 via UPDI
  pickit4_updi       = MPLAB(R) PICkit 4 via UPDI
  pkobn_updi         = Curiosity nano (nEDBG) via UPDI
  powerdebugger_updi = Atmel PowerDebugger (ARM/AVR) via UPDI
  snap_updi          = MPLAB(R) SNAP via UPDI
  xplainedmini_updi  = Atmel AVR XplainedMini via UPDI
  xplainedpro_updi   = Atmel AVR XplainedPro via UPDI

In absence of -e the new method leaves the contents of not programmed UPDI flash pages as before. However, the user will be informed that no CE happens:

$ avrdude -c serialupdi -p 64du28 -U flash:w:'"@ @ @"':m 
avrdude: AVR device initialized and ready to accept instructions
avrdude: device signature = 0x1e9622 (probably 64du28)
avrdude: NOT erasing chip as page erase will be used for new flash/bootrow contents
avrdude: reading 6 bytes for flash from input file "@ @ @"
         in 1 section [0, 5]: 1 page and 506 pad bytes
         writing 6 bytes to flash ...
         writing | ################################################## | 100% 0.20 s 
         reading | ################################################## | 100% 0.07 s 
         6 bytes of flash verified

avrdude done.  Thank you.

@stefanrueger stefanrueger added bug Something isn't working enhancement New feature or request labels Jul 6, 2024
@dbuchwald
Copy link
Contributor

I think this is great idea to do it like that, just wanted to make sure - for the bootrow memory to work it still needs #1834 right?
Also I'm curious about bootrow support in the other UPDI programmers - are you sure they also suffer from the double-write issue? My concern is that this change might introduce some kind of regression to other UPDI implementations, and as such should be tested against each of them...

@dl8dtl
Copy link
Contributor

dl8dtl commented Jul 6, 2024

As these new AVRx[DE] devices are architecturally Xmega devices, it seems valid and useful to me to treat them the same wherever it makes sense.

@stefanrueger
Copy link
Collaborator Author

for the bootrow memory to work it still needs #1834 right?

Yes, #1834 or #1835. Both should work with this PR. I take your preference.

bootrow support in the other UPDI programmers - are you sure they also suffer from the double-write issue?

Not at all: I have not looked into the code (and don't have these programmers). Like serialupdi they might not work with bootrow (why would they? it's a new memory that only recently saw the documentation published). But seeing that bootrow can be folded into flash treatment as PR #1834 does I imagine it's kinda relatively easy to fix. I wouldn't be in a position to do a more complex PR for other programmers in the same way as PR #1835 did.

[PR #1837] might introduce some kind of regression to other UPDI implementations, and as such should be tested

I agree. From a theoretical point of view I can see how the suggested high-level use of page erase should work. Currently we don't seem to have the capacity to test within the core maintenance team (holidays, time pressures etc), so my strategy is to merge, so we widen the pool of testers before v 8.0 to those who like the bleeding edge versions.

@dbuchwald
Copy link
Contributor

dbuchwald commented Jul 7, 2024

Yes, #1834 or #1835. Both should work with this PR. I take your preference.

That's a tough choice to make. #1835 is a bit more "correct" as in it introduces new memory as a separate flow (which should be the case), but it adds extra page erase operation which in this situation doesn't seem to make sense anymore.

I will implement another PR, similar to #1835 but without these additional page erase calls. This will ensure that we have separate flow for bootrow memory (just in case, for future use), but functionally it will be identical to #1834 - it will treat bootrow just like flash, no added page erase.

Would that be OK?

@stefanrueger
Copy link
Collaborator Author

implement another PR, similar to #1835 but without these additional page erase calls

@dbuchwald Sounds like the perfect plan! Thank you

@stefanrueger stefanrueger merged commit 883a614 into avrdudes:main Jul 7, 2024
13 checks passed
@stefanrueger stefanrueger deleted the updi-page-erase branch July 7, 2024 13:40
@dbuchwald
Copy link
Contributor

@stefanrueger done. Instead of creating new branch and a new PR I just changed the one used in #1835 - feel free to give it a try and if you like how it works, you can go ahead and merge it.

I must say, I absolutely love this cooperation, every time I get an e-mail about new feature request or bug report I feel this excitement that got me into software engineering years ago. I really wish I could have such a great team in my daily work :)

@stefanrueger
Copy link
Collaborator Author

Thanks for the PR and for the kind words! Yes, AVRDUDE is a very neat project, and the best is that we get to collaborate with brilliant people. Thanks for being on the team @dbuchwald - without your code AVRDUDE wouldn't be what it is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants