atmega328p: Make PRR register writable

[gpgreen]

changed atmega328p to use patch and allow writing of PRR register
Fixed field names in PRR register to match datasheet

I am porting an existing C project to rust to learn how to do avr embedded programming. I needed access to the PRR register, which i did using unsafe pointer writes for now. I tried this fix to avr-device, and it seems to generate the rust code correctly, but when i try to use it in my project I get the following error:

warning: Linking globals named 'DEVICE_PERIPHERALS': symbol multiply defined!

error: failed to load bc of "avr_device-ee0b56ac06c1c352.avr_device.f0xkhm9n-cgu.7.rcgu.o":

[Rahix]

warning: Linking globals named 'DEVICE_PERIPHERALS': symbol multiply defined!

That means you're now including multiple versions of avr-device. You can use cargo tree to get a dependency graph and you'll probably see two versions of the crate in there. To fix this, you'll probably want to use a crate-patch instead of referencing your custom version of avr-device in [dependencies] somewhere. E.g.

[dependencies]
# ...
avr-device = "0.2.3"
# ...

[patch.crates-io]
avr-device = { path = "/path/to/your/modified/version" }

See The [patch] section for details.

[Rahix]

I do not think this is a common patch, the PRR register(s) seem to differ wildly between the different MCUs. Thus I would suggest moving the patch directly into this file (below the includes).

[Rahix]

Hmm, I am unsure if this is the right thing to do. Most other chips also have these fields named PRTWI/PRSPI so we'd have to fix them all. On the other hand I think the reason why they do not have the suffix number is that the registers for TWI/SPI also do not have suffix numbers in contrast to e.g. the timer/usart regs. In general it seems the consensus is to move away from the suffixed register and fields names as this also makes HAL code much easier to write. So I think I'd keep these field names here as they are as well...

[gpgreen]

If the crate docs don't match the datasheet, then you have to dive into the code to see whether it is a mistake or not. The generated docs don't have the same information as the code, ie what bit positions the field is covering. That's the first thing i had to do, ie check that PRTWI was really PRTWI0 as in the datasheet, I didn't notice it wasn't writable at first. From other work I know there is lots of mistakes in the microchip svd files. OTOH i understand about making HAL more common, except in that case it seems better to have a method on the peripheral to turn off its clock, and just hide the PRR register.

It makes sense to move out of the common file, i can do that.

Thanks for the tip on the compile problem. I'll give that a try when I have time. I'm still a newbie at rust.

[gpgreen]

I updated the change to have no common/cpu.yaml. I looked at some of the other registers and compared with the datasheet. Some have the numbers on the registers/fields and some don't. Very unsatisfying.

[Rahix]

Some have the numbers on the registers/fields and some don't. Very unsatisfying.

Yeah, it is a horrible mess and nothing is consistent anywhere... If you want to read some more discussions about how we should deal with this, here's a link: #55 (comment)

Honestly, I think trying to stay close to any kind of vendor scheme is close to impossible when the vendor doesn't even stay consistent with their own stuff. So let's just go with whatever is most ergonomic to use for us...

The generated docs don't have the same information as the code, ie what bit positions the field is covering.

Just FYI, this information is actually available - if you know where to look. The svd2rust API docs are unfortunately quite hard to decipher...

If you look at, for example, the write proxy for this field: https://docs.rs/avr-device/0.2.3/avr_device/atmega32u4/cpu/prr0/struct.W.html, the docs do have each bit position spelled out.

[gpgreen]

Ah ok. Thanks for the link to the discussion. Since i haven't tried doing a rust macro, the subtleties pass me by. :)
I ran into another weird thing. What do you think about this register:

https://docs.rs/avr-device/0.2.3/avr_device/atmega328p/exint/eimsk/struct.INT_R.html

There are 2 interrupts, but the field is 2 bits wide, instead of the "logical" 2 fields for each interrupt, INT0 and INT1. Is this inconsistent svd? It still works but if you were dealing with both interrupts it would be more complicated to turn one of them on/off instead of that very nice rust field abstraction.

[Rahix]

There are 2 interrupts, but the field is 2 bits wide, instead of the "logical" 2 fields for each interrupt, INT0 and INT1. Is this inconsistent svd? It still works but if you were dealing with both interrupts it would be more complicated to turn one of them on/off instead of that very nice rust field abstraction.

Looks like the vendor's chip description is just wrong here... I'll gladly take a patch that splits it into two fields! :)

BTW, Microchip/Atmel don't provide the SVDs for these chips. They provide "ATDF" files which we first convert to SVD using atdf2svd.

[gpgreen]

I was looking at the atdf file originally, used the incorrect term. I will submit a PR for the this issue also. Thanks for all the help.