search

embassy-rs / stm32-data

75 stars 107 forks source link

fix(stm32h7): timer version assignements #511

Open elagil opened 2 months ago

elagil commented 2 months ago

Closes https://github.com/embassy-rs/stm32-data/issues/509

elagil commented 2 months ago

Apparently, multiple timer versions are not allowed at once. I am quite sure that the H7 parts do actually use timers of different versions.

For example, I checked the TISEL byte_offset field, which for the timers 16 and 17 on the H7 indicates timer_v1 (the offset is indeed 104, instead of 92 for timer_v2).

image

elagil commented 2 months ago

I made a "fix" to allow mismatching timer peripheral versions in the chip generator.

embassy-ci[bot] commented 2 months ago

diff: https://ci.embassy.dev/jobs/669427c30f2d/artifacts/diff.html

embassy-ci[bot] commented 2 months ago

diff: https://ci.embassy.dev/jobs/0c7da5a906a6/artifacts/diff.html

elagil commented 2 months ago

@Dirbaio I created a single file in the following way:

Is there a good way to test this?

elagil commented 2 months ago

As you said, embassy-stm32::timer does not yet support multiple driver versions at once. Only now did I understand the full extent of this issue. For example, set_trigger_source only accepts an input of type TriggerSource, but now, there would have to be an additional TriggerSourceV2 type for the v2 timers of the chip. Also, the low_level module has to use different register definitions, based on the timer.

@Dirbaio Do you have a suggestion for avoiding a lot of code duplication? We may just generate for either the v1 or the v2 timers. The others would not be usable in that case - not perfect, but at least 9 timers can be used correctly in case of v2 (vs. 8 v1 timers).

I adjusted this PR according to the last suggestion.

embassy-ci[bot] commented 2 months ago

diff: https://ci.embassy.dev/jobs/7796d35619e8/artifacts/diff.html

embassy-ci[bot] commented 2 months ago

diff: https://ci.embassy.dev/jobs/4aa9f61a0f18/artifacts/diff.html

elagil commented 2 months ago

Is this a good approach, or should we change it?

embassy-ci[bot] commented 1 month ago

diff: https://ci.embassy.dev/jobs/8af0a05a49a3/artifacts/diff.html

Dirbaio commented 1 month ago

@elagil if you look through the diff the CI bot posted, there's many timers where the registers field is gone. This will make those be unusable with the HAL. The reason for it is no longer matching any rule in perimap. Can you take a look?

elagil commented 1 month ago

@Dirbaio I know, the disabled timers are the v1 timers. This PR deliberately makes it so that only v2 timers are active.

Up to now, all timers (even v2) were used with the v1 register set. I chose to enable only v2 timers with this PR because there are more v2 than v1 timers, and they have more features. These now use the v2 register definitions, as they should.

Do you have an idea for solving this problem? v1 and v2 do not have compatible registers and cannot be merged into a single definition file. As you said, in embassy, we cannot use multiple driver versions at once. See also the comment above: https://github.com/embassy-rs/stm32-data/pull/511#issuecomment-2264943506

Dirbaio commented 1 month ago

ah sorry I should've read the thread instead of assuming my memory is in working order :sweat_smile:

There's people out there using a lot of timers, sometimes someone asks in Matrix how to free the timer used by the time driver because they're running out. I can imagine making a bunch of timers not have registers will be unpopular. I'd say allowing all timers to be usable is better than having 100% accuracy but only allowing using half the timers.

so yes, if some timers have some features and the others don't then we'll need two copies of fieldsets/enums and make the right timer block use the right ones...

The HAL will have to deal with this ... somehow. Perhaps it's okay for now to be slightly inaccurate, for example using the "more advanced" enums/fieldsets for all timers and hope users read the docs of which features are available on which timers.

elagil commented 1 month ago

The HAL will have to deal with this ... somehow.

Ok, I will think of something for handling two versions at once without excessive duplication. I need some of the advanced features of the v2 timers (e.g. USB SOF signal as trigger), so I can't just use v1 definitions.