Skip to content
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

fix(stm32h7): timer version assignements #511

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elagil
Copy link
Contributor

@elagil elagil commented Jul 26, 2024

Closes #509

@elagil
Copy link
Contributor Author

elagil commented Jul 26, 2024

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
Copy link
Contributor Author

elagil commented Jul 26, 2024

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

@embassy-ci
Copy link

embassy-ci bot commented Jul 26, 2024

@elagil elagil force-pushed the fix_stm32h7_timer_versions branch from 1236d4b to 1c76bdc Compare July 26, 2024 18:39
@embassy-ci
Copy link

embassy-ci bot commented Jul 26, 2024

@elagil
Copy link
Contributor Author

elagil commented Jul 26, 2024

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

  • Copy timer_v2.yaml to timer_h7.yaml.
  • Remove all definitions from timer_h7.yaml that are the exactly same in timer_v1.yaml.
  • Rename any remaining definitions (enums, fieldsets, blocks) to end in _V2, as well as their references.
  • Copy all definitions from timer_v1.yaml into timer_h7.yaml (to sensible locations)

Is there a good way to test this?

@elagil
Copy link
Contributor Author

elagil commented Aug 2, 2024

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.

@elagil elagil force-pushed the fix_stm32h7_timer_versions branch from a4cb325 to 60b204b Compare August 2, 2024 10:00
@embassy-ci
Copy link

embassy-ci bot commented Aug 2, 2024

@elagil elagil force-pushed the fix_stm32h7_timer_versions branch from 60b204b to 1061ea7 Compare August 2, 2024 10:07
@embassy-ci
Copy link

embassy-ci bot commented Aug 2, 2024

@elagil
Copy link
Contributor Author

elagil commented Aug 14, 2024

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

@embassy-ci
Copy link

embassy-ci bot commented Aug 18, 2024

@Dirbaio
Copy link
Member

Dirbaio commented Sep 10, 2024

@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
Copy link
Contributor Author

elagil commented Sep 10, 2024

@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: #511 (comment)

@Dirbaio
Copy link
Member

Dirbaio commented Sep 10, 2024

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

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
Copy link
Contributor Author

elagil commented Sep 11, 2024

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.

@elagil elagil marked this pull request as draft November 4, 2024 22:13
Disables v1 timers, until there is support for multiple
timer versions in embassy at the same time.
@embassy-ci
Copy link

embassy-ci bot commented Nov 5, 2024

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.

Incorrect timer versions on STM32H723
2 participants