ATMega328P Support

[jonahbron]

Opening another issue here for stuff specifically related to this repo

Related to: Rahix/avr-hal#2

@Rahix In your first comment on that issue you said the first place to start is working on the chip patch. I see that there is already an atmega328p patch file, but it has fewer _includes. My guess is that it's incomplete in its current form.

Luckily, the ATmega328P is very similar to the already implemented ATmega32U4. This should mean that you can for the most part copy the patches from that chip.

That sounds good... the only issue is that I don't know where to find the differences between the 32u4 and the 328p. Is that info I can find in the data sheet? Or documented elsewhere on the Microchip website? Again please pardon my extreme ignorance here.

[jonahbron]

Reading through the atmega328p.adtf, the patch YAML files, and the ATMega328 datasheet, I'm beginning to see relationships between their contents. For example, I see that the YAML files seem to target XML elements in their adtf files by [name] attribute. And the info I see in the adft files seems to have corresponding details in the datasheet.

There are a few things that are still mysterious, principally some of the underscore patch actions in the patch file. The following are ones I see in some of the YAML files:

• _write_constraint - found here
• _replace_enum - found here

Neither of those are mentioned in the stm32-rs README. Where can I find documentation on their behavior? The _replace_enum command is particularly confusing because I see no enums at all at all the adtf's.

At a higher level, it seems to me like the objective here is to add patches such that when applied to the atdf file, has an output that matches what's described in the datasheet. Is that roughly accurate?

[Rahix]

My guess is that it's incomplete in its current form.

Yes

Is that info I can find in the data sheet? Or documented elsewhere on the Microchip website?

Unfortunately, I don't know of a document that outlines just the differences. I think, the best thing to do is looking at each patch for ATmega32U4 and checking the datasheet if it might apply. If it does, try including it and looking at the generated output (more specifically the rust documentation of the generated output).

I see that the YAML files seem to target XML elements in their adtf files by [name] attribute

Ah, that is not quite correct. The atdf files are first converted to svd using atdf2svd and the patches are then applied to the generated svd files. If you ran the generation once, you can find these svd files in svd/.

_write_constraint & _replace_enum

Those two were added by us, because upstream svdpatch.py does not support these kinds of modifications. Docs are in this project's README.

The _replace_enum command is particularly confusing because I see no enums at all at all the adtf's.

They exist in the atdf file as well, but they are called <value-groups> in there. If you take a look at the svd file, you will see <enumeratedValues>, which is what this modifier targets. These enums describe the values which you are allowed to write to a certain register and what each of these values means.

_replace_enum can do two things: Either it replaces an existing set of enumeratedValues with another one or it creates a new one if none exists so far.

At a higher level, it seems to me like the objective here is to add patches such that when applied to the atdf file, has an output that matches what's described in the datasheet. Is that roughly accurate?

Well, the patches are applied to the generated svd, not directly to the atdf. Apart from that, you are correct: The intention is to fix wrong/missing information before generating rust definitions.

---

That all said, I don't think it is necessary for you to dive deep and patch the whole thing, unless you really want to ... It is well enough if you take a look which of the common patches also apply to ATmega328P and maybe compare the generated output of the UART and I2C peripherals against the datasheet.

(For that, look at the final generated rust crate's documentation, not the svd files as these are quite convoluted)

[jonahbron]

@Rahix I've submitted a PR with the added patches. The only one left out is PLL which is apparently for USB which the 328p does not support.

and maybe compare the generated output of the UART and I2C peripherals against the datasheet.

I see no mention of either of those in the generated documentation. How would I perform that comparison?

[jonahbron]

Reading through your comment on the avr-hal repo again, I see you draw a relationship between I2C and TWI. I'll read through that and try to verify that things match. As for UART, would I look at USART to compare?

[Rahix]

TWI is the name Atmel calls I2C. It stands for "Two Wire Interface". Sorry, I should have made that more clear.
UART and USART are the same for all we care about. If you want to know the details, you can read the respective Wikipedia articles (UART vs USART).

[jonahbron]

@Rahix All of the registers for TWI look the same, however I'm unable to check the register address (?). The documentation lists the registers and their various bits, but don't actually declare their positions or masks. Is it necessary that I check that, and if so where would I find them?

Still reading through USART.

[Rahix]

The addresses are most likely correct, I don't think you need to double check. For now, you can just leave the stuff in this repo as-is, unless you encounter errors while working on the avr-hal crate.

For future reference: The register addresses are most easily visible in the "Register Summary" close to the end of the datasheet.

[jonahbron]

@Rahix Okay good to know, I'll leave them. I see them in the datasheet, I'm wondering where to find them in the Rust docs though.

[jonahbron]

@Rahix The registers and bit lists all look the same for USART as well, so all good. However there are a couple of sections where the descriptions from the datasheet didn't make it into the patches. If it's okay with you I'll open another PR with more of those descriptions added.

[jonahbron]

Oh I did find one difference actually. For the enumerated values on the UCSZ0 bits, the value for 0x111 (description: "9-bit") has been omitted. Should I add that? I've confirmed that value is present in the 32u4 too.

[Rahix]

Sounds good! If you can add those to the common patch, the other chips will benefit your work as well!

I'm wondering where to find them in the Rust docs though.

They are unfortunately not easily visible there because of the way svd works ...

[jonahbron]

@Rahix I've opened PR #13 which fixes a couple of small things. It's not much, partly because some of the descriptions I thought were missing I just didn't know how to find.

I've taken some other notes based on the datasheet as well that I'm less clear whether they need a solution, or even should have one.

Datasheet Section
10.9.2: Table 10-2: this table doesn't seem to have a corresponding enum in Rust.  Does it need one?
Tables 14-2, 14-3, 14-4: These three tables describe the purpose of the same bits under different configurations.  Is there a good way to represent all three conditions for the bit config?
Table 14-5, 14-6, 14-7: ditto
20.8.2: UCSR0A has reserved bits 0..4 exposed in Rust.  Is that bad?
20.8.3: UCSR0B has reserved bits 0..2 exposed in Rust.  Is that bad?
20.8.4: UCSR0C has reserved bits 3..5 exposed in Rust.  Is that bad?

[Rahix]

Thanks for the PR!

About your notes:

10.9.2: Table 10-2: this table doesn't seem to have a corresponding enum in Rust. Does it need one?

The bits in this table are separate, which means you unfortunately can't add an enum for them ...

Tables 14-2, 14-3, 14-4: These three tables describe the purpose of the same bits under different configurations. Is there a good way to represent all three conditions for the bit config?

I have already patched this enum to have useful names that are not specific to one of the three different modes. SVD does not have a way to model these kinds of constraint AFAIK.

20.8.2: UCSR0A has reserved bits 0..4 exposed in Rust. Is that bad?
20.8.3: UCSR0B has reserved bits 0..2 exposed in Rust. Is that bad?
20.8.4: UCSR0C has reserved bits 3..5 exposed in Rust. Is that bad?

Hmm, yes that should not be the case. What confused me a little is the fact that I seem to have a different version of the datasheet which has these bits documented. I'll take a closer look if I get some spare time. For now, it should be ok to just leave them in.

[Rahix]

I think the correct datasheet is this one. It has the bits in UCSR0? defined. The datasheet you linked above does not contain the ATmega328P in a 28 PDIP package which is what Arduino Uno uses. But apart from that I did not find much information why there are two different datasheets available.

Anyway, I don't think we need to be concerned about this and can safely assume those bits exist.

[jonahbron]

Great, thanks. I'll leave this issue open until I've been able to verify I can run code on my Uno and that there aren't any problems remaining here.

[jonahbron]

Confirmed that at least two examples work, I consider this issue completed. Thanks for the direction @Rahix !