Skip to content

Add some modern avr support #667

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add some modern avr support #667

wants to merge 10 commits into from

Conversation

tomtor
Copy link

@tomtor tomtor commented Jun 9, 2025

I added an mcu/avrmodern_hal with an implementation for usart and ports, for the attiny402 and attiny1614. Also some code for attiny3224 and avr128db28 (commented out because these are not in the current avr_device crate).

More can be added in the future.

Also added 2 examples and tested them on an attiny402.

It is a minimal implementation, but I think that it would be nice to have at least some code in this repository for the modern AVR range. The added examples/avrmodern/README.md has some generic hints on Rust for these devices (could also be added to the avr-device repository?)

Rahix added 6 commits May 5, 2025 17:51
Use latest avr-device as a git dependency for testing the upcoming
version.
This allows us to see which targets are currently failing the build.
As avr-device is upgrading to use svd2rust version 0.33.1, there are
some significant changes in the generated API.  We have to adapt the HAL
code to use the new API whereever relevant.

This commit was mostly generated using the following command, which adds
the parentheses behind each register access to change it from
struct-field access to method call.

	cargo build --message-format json 2>/dev/null \
	  | jq '.message.children[].spans[] | {file: .file_name, line: .line_start, col: (.text[0].highlight_start - 1), insert: .suggested_replacement}' 2>/dev/null \
	  | jq -r '"sed -ri '"'"'" + (.line | tostring) + "s/^(.{" + (.col | tostring) + "})/\\1" + .insert + "/'"'"' $(cd ../..; realpath " + .file + ")"' \
	  | sort | uniq | bash

Shell magic for the win :)

Beyond this, .bits() had to be converted to .set() where safe accesses
are performed.
avr-device no longer has a `critical-section-impl` feature.  Instead it
now has a `critical-section` feature that is enabled by default [1].  As
such, we can drop the re-exposed feature here.

[1]: Rahix/avr-device#195
In svd2rust 0.34.0, the `.write()` and `.modify()` register access
functions were changed to return the written register value.  As
avr-device is upgrading to a more recent svd2rust version, we need to
adjust accordingly.

Fixup the few places where the return value of `.write()` or `.modify()`
mattered due to expression form calls.
Previous versions of `avr-device` allowed accidentally mis-specifying
the MCU in the interrupt definition.  Now that this is caught, fix one
such mistake here.
@tomtor tomtor force-pushed the avrmodern branch 2 times, most recently from b1a8630 to 1c6065e Compare June 10, 2025 10:06
@tomtor
Copy link
Author

tomtor commented Jun 10, 2025

Fixed formatting and added some README text for AVR128DB28 flash mapping

@tomtor tomtor force-pushed the avrmodern branch 2 times, most recently from 26b4393 to 79fdcce Compare June 10, 2025 19:23
@nonik0
Copy link

nonik0 commented Jun 10, 2025

Hi @tomtor, it's exciting to see your contribution! I definitely would like to get AVR modern/xmega architecture into avr-hal as well. I just want to make sure you're also aware of #357 which achieves a lot of the similar functionality, although it looks like you have USART, examples, and more included--in addition to the port impl. Could you take a look at the existing PR and see if you could rebase off that since it's been through several rounds of Rahix review?

I have done some minor work off #357 in the service of my ATtiny1614-based project, adding a working EEPROM implementation (thought currently stuck with an assembly impl, can't get to function with svd2rust yet). I would like to add ADC functionality into avr-hal as well but I found the complexity around fitting the differences of the xmega architecture in idiomatically with the current design of avr-hal's ADC traits a bit foreboding for my nascent Rusty mind. Currently just using svd2rust register access directly in my project for ADC stuff.

Also as an FYI, there's imminent code churn event coming with the next version of avr-device/svd2rust. See #656. I am guessing we are really close to a "PR freeze" to avoid a bunch of churn for @Rahix to get the necessary updates in, so it might be useful to preemptively rebase your PR to the avr-device-new branch.

@tomtor
Copy link
Author

tomtor commented Jun 11, 2025

@nonik0 Hi, I looked for an existing pull request, but obviously missed yours. :/

There is a lot of overlap, but I think mine is indeed a bit more complete, as you already mentioned with the uart and examples. It would be good and fun to work on this together.

I see that your PR has avr_spec which is not in the current main. Is it in avr_device_new?

Rebasing on the new branch is a good idea, i will have a look at that.
Do you have suggestions of what to copy from your PR to mine or vice versa? We should close one, after we have merged our PRs. I don't really care which one we pick.

How would you suggest we proceed? I think one of us should send PRs to the other to sync.

@nonik0
Copy link

nonik0 commented Jun 12, 2025

@tomtor No worries. Yes, the avr_device_new will remove avr_spec files. Sorry, I forgot to link my current branch that's based off #357. Here's a summary of what I have done in that branch (not too much):

  • added support for attiny404/804/1604, mostly @agausmann work, I just added more boilerplate for the others
  • rebased to avr_device_new, absorbing svd2rust changes... mostly (I was just looking and saw one macro still had a chunk of code commented out)
  • added EEPROM support, using modified asm code from MegaTinyCore
  • messy, unfinished ADC code - definitely can remove anything here

Also, a little bit more on my EEPROM impl--last time I had checked the asm in the impl was not compiling with the new build setup (with target=avr-none) in avr_device_new. So as a workaround I was still including the avr-spec file as a target in my project to get the asm to compile. I tried to do the EEPROM impl without asm and only using rust2svd register access but have been unsuccessful after several attempts (compiled code too slow I figure). However, after syncing my fork, it looks like I can now compile the asm without the target avr-spec file. But looks like there are optimizations really needed as just changing the target back also added over 1.1KB to my bin size, presumably due to less optimal compilation/instruction use, I see this showing up in cargo bloat:

File  .text    Size             Crate Name
0.0%   4.3%    614B compiler_builtins compiler_builtins::mem::memmove
0.0%   2.3%    334B compiler_builtins compiler_builtins::mem::memcpy

Anyway, since you're actively working on this, I am happy to proceed with whatever is easiest for you to do for merging the work because ideally I just want to get some level of support in avr-hal first, and then supplement from there. My initial thoughts would be to merge with my existing branch since the design/naming/structure that @agausmann has already done has been reviewed by Rahix and is more up-to-date with the current state of the repo. I am also not sure how to proceed with the EEPROM impl given the above. Let me know if you have any thoughts, I believe the asm should also be the same for the XMEGA 0 and 1 series so should work for you with minimal adjustment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants