No `ctc` function for `atmega328p`'s `WGM1_W`

[Cryptjar]

I'm a bit puzzled, why is there no ctc function on atmega328p::tc1::tccr1a::WGM1_W, as it is the case for WGM0_W and WGM2_W?

This inconsistency came up while I was working on https://github.com/Rahix/avr-hal/pull/257. Not having this or a comparable function makes it hard to use timers in a generic way.

[Rahix]

This is usually an inconsistency in the vendor-provided ATDF files. We have infrastructure here in avr-device to fix such things - check the existing patches in https://github.com/Rahix/avr-device/tree/main/patch. Docs for the patch format can be found here: https://github.com/stm32-rs/svdtools#device-and-peripheral-yaml-format

Whenever you encounter such inconsistencies, please feel free to send a PR here with a fix :)

[Cryptjar]

Hmm, I'm trying to wrap my head around this ATDF stuff. If I understand it correctly, the "vendor" only defines the WGMn fields, e.g. for TC0:

https://github.com/Rahix/avr-device/blob/4adeecdf0ff9648beefa4fff7fb8f183413dbe78/vendor/atmega328p.atdf#L847

and

https://github.com/Rahix/avr-device/blob/4adeecdf0ff9648beefa4fff7fb8f183413dbe78/vendor/atmega328p.atdf#L841

Then, the patch files define an enum on them (or seemingly only on WGM0):

https://github.com/Rahix/avr-device/blob/4adeecdf0ff9648beefa4fff7fb8f183413dbe78/patch/timer/dev/8bit.yaml#L17-L22

And from that enum the ctc function, seems to be generated.

However, this data sheet of the atmega328p defines in Table 14-8 on page 86 that CTC is 0b010 combining WGM02 with WGM0. So it seems to me that the ctc function only sets the lower two bits of that, and if it happens that WGM02 was previously set to 1, calling ctc would yield the reserved state 0b110. Which seems rather wrong to me.

According to the data sheet, it gets even more complex with the TC1 because there are four bits and two CTC modes.

Thus, it seems that it would be more appropriate to have a ctc that can actually access both TCCRnA and TCCRnB. However, it seems to me that this is quite atypical compared to the rest of the avr-device API, and I have no clue how to express this kind in the patching YAML files (if it is even possible).

[Rahix]

Ah, you got it and now I also remember: We didn't add enum-values for this timer precisely because of the split bit-field. There is no way to represent this in SVD so code unfortunately has to manually set the bit-values for these timers...

[Rahix]

Actually, this can be seen, for example, in the old PWM implementation. There, a setup function had to be defined manually for each timer. Compare

https://github.com/Rahix/avr-hal/blob/ce52c90d3950cd81090627e3ad8494a15e4c8f66/chips/atmega328p-hal/src/pwm.rs#L48

with

https://github.com/Rahix/avr-hal/blob/ce52c90d3950cd81090627e3ad8494a15e4c8f66/chips/atmega328p-hal/src/pwm.rs#L95-L97

I don't think there are really any other options than doing it this way :/ Maybe enough timers are similar enough to create some macro-magic to reduce code-duplication but then this would of course make it much harder to understand... So I'm not sure if that would be worth it.

[dalpil]

I guess one could also argue for the removal of the COM?? and WGM?patches altogether, since they omit some of the timer modes due to the split WGM bitfield (as pointed out by Cryptjar) and obscure the settings of the mode-dependant (normal/fast/phase-correct/et.c.) COM??-registers.

Setting COM?? to 0b01 doesn't always mean "match_toggle" for example, since it depends on what mode the timer is in (as pointed out by the patch itself).

That being said -- it sure is convenient to have human-readable settings for some of this stuff, but it seems to me like the abstractions necessary to cover all the different combinations of modes and settings in a user-friendly way is something that maybe belongs in avr-hal rather than in avr-device(?).

These are just speculations from my part, as I haven't really dug into avr-device that much, but it did cause a bit of confusion on my part whilst playing around with different PWM generation modes on the atmega328p and attiny85, and in the end I opted for the .bits() method for setting the registers, to avoid confusion.

[Rahix]

It's a difficult decision to make... I completely agree that a proper abstraction on top is needed and it should go into avr-hal, not here. Using .bits() is the right interface to use.

However, when looking at the original PWM abstraction, we did leverage the enumerated value names for some of the timers, so I think there is value in keeping them... I am not sure to what degree though, because as you mentioned, sometimes the existing ones are actually wrong, too...

[Cryptjar]

Ok, I added clearing the top bit for 8-bit timers:

https://github.com/Cryptjar/avr-hal/blob/dcfde7d59405f0acaaf1f945ad2bc8ef6a157333/avr-hal-generic/src/time.rs#L278-L288

and configured 16-bit timers by setting the raw bits:

https://github.com/Cryptjar/avr-hal/blob/dcfde7d59405f0acaaf1f945ad2bc8ef6a157333/avr-hal-generic/src/time.rs#L369-L379

Thus, I no longer consider this issue a blocker for https://github.com/Rahix/avr-hal/pull/257