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

Update embassy-stm32 after stm32-metapac timer_v2 #2499

Merged
merged 10 commits into from
Feb 9, 2024

Conversation

eZioPan
Copy link
Contributor

@eZioPan eZioPan commented Jan 30, 2024

Summary

  • Keep BASIC of TIM PAC as BasicInstance
  • Merge 1CH, 2CH, GP16 of TIM PAC into CaptureComapre16bitInstance
  • Keep GP32 of TIM PAC as CaptureComapre32bitInstance
  • Merge 1CH_CMP, 2CH_CMP, ADV of TIM PAC into ComplementaryCaptureCompare16bitInstace
  • Remove GeneralPurpose16bitInstance, GeneralPurpose32bitInstance and AdvancedControlInstance, since we merge different kinds of PAC TIMs, they are not accurate under new PAC.
  • pin_trait! has been redesign to match TimerInstance more precise.

Such that this PR didn't do much modify to non-sealed trait.

Since PAC has 10 different blocks, and HAL only got 4 traits. The "sealed" part is a little bit thicker than original.

  • All methods other than Channel-count related is put into "smallest" sealed::TimerInstance

    • sealed::Core is about PSC, CNT, ARR and Interrupt
    • sealed::BasicNoCr2 add extra methods to enable DMA. This one is for Timer other than 1CH and 2CH. 1CH and 2CH don't have DMA request.
    • sealed::GeneralPurpose1ChannelInstance (1CH of PAC) has all methods that all Channel shared.
    • sealed::GeneralPurpose1ChannelComplementaryInstance (1CH_CMP of PAC) has all methods that all Complementary Channel shared.
  • Methods for Channel count related is put into "kind-largest" sealed::TimerInstance

    • sealed::GeneralPurpose16bitInstance has all Capture/Compare Channel-count related methods
    • sealed::AdvancedControlInstance has all Complementary Capture/Compare Channel-count related methods
  • To merge 10 TIM of PAC into 4 TimerInstance of HAL:
    All merges are done by kinda-unsafe-promotion

    • 1CH and 2CH are forced implement "sealed::GP16Instance"
    • 1CH_CMP and 2CH_CMP are forced implement "sealed::ADVInstance"

    mostly just read/write to empty fields of same register.


This PR is based on my fork of stm32-data-generated for embassy CI pick timer_v2 PAC

DO NOT MERGE BEFORE

MERGED.

NEED A

  • Rebase

AFTER embassy-rs/stm32-data#364 MERGED, TO REMOVE MY FORK FROM CODEBASE


This PR should pass CI, but there still need review for functionality check.
Especially for CaptureCompare16bitInstance and ComplementaryCaptureCompare16bitInstance trait design.

@eZioPan eZioPan force-pushed the update-metapac8 branch 4 times, most recently from 8acc9a4 to a0b9b45 Compare January 30, 2024 12:41
@eZioPan eZioPan marked this pull request as ready for review January 30, 2024 13:06
@eZioPan
Copy link
Contributor Author

eZioPan commented Jan 31, 2024

Update 2024/Feb/2:

restore original public API of timer, remove macro inside "sealed".

This could make incorrect Timer-Channel mapping delay to a runtime panic, but more easier to write a high-level driver.


Update 2024/Feb/1:

Try use macro to add common method inside each "sealed" traits.

TIM inheritance change

Core -------------------------> 1CH -------------------------> 1CH_CMP
  |                              |                              ^   |
  +--> Basic_NoCr2 --> Basic     +--> 2CH --> GP16 --> GP32     |   +--> 2CH_CMP --> ADV
           |             |             |      ^  |              |           ^         ^
           |             |             +------|--|--------------|-----------+         |
           |             +--------------------+  +--------------|-----------|---------+
           |             |                                      |           |
           |             +--------------------------------------|-----------+
           +----------------------------------------------------+

But this require user use ugly

seal::SomeTimerInstance::some_method(self.inner.deref(), ...)

or

seal::SomeTimerInstance::some_method(self.inner.deref_mut(), ...)

to resolve method name collision.


First, let me show my appreciate to anyone review this PR! Thank you for your time and effort!

I'm not sure what to do with this situation:

Main branch has single sealed::CaptureCompare16bitInstance ("sealed::CC16") trait, and it's a sub trait of sealed::GeneralPurpose16bitInstance ("sealed::GP16"), then all ChannelxPin and ChannelxComplementaryPin is attach to "sealed::CC16".
But in this PR, we will have 8 different TIMs and 2 virtual TIMs to match ST's RMs and make rust trait system work with them.

Here is the TIM inheritance

                                    CaptureCompare32bitInstance
        CaptureCompare16bitInstance                      |     ComplementaryCaptureCompare16bitInstance
                                 v                       |        v
Core -------------------------> 1CH ---------------------|---> 1CH_CMP
  |                              |                       v      ^   |
  +--> Basic_NoCr2 --> Basic     +--> 2CH --> GP16 --> GP32     |   +--> 2CH_CMP --> ADV
           |             |             |      ^  |              |           ^         ^
           |             |             +------|--|--------------|-----------+         |
           |             +--------------------+  +--------------|-----------|---------+
           |             |                                      |           |
           |             +--------------------------------------|-----------+
           +----------------------------------------------------+

Strickly speaking, 1CH / 2CH aren't "2CH is 1CH with one more channel", neither "1CH and 2CH are GP16 without some channels". Beside channel-related count, there are some other fields and/or registers addition. So we can't simply merge them into one TIM.

My current implementation is make "trait sealed::CC16 :sealed::1CH", so that every pin_trait!() can compiled without error.

-    pub trait CaptureCompare16bitInstance: GeneralPurpose16bitInstance {
+    pub trait CaptureCompare16bitInstance: GeneralPurpose1ChannelInstance {

But this cause Pins other than Channel1Pin is not usable: "sealed::1CH" can only control one channel.

In my opinion, we will need to copy every method that has a Channel argument in "sealed::CC16" to each of "sealed::1CH", "sealed::2CH" and "sealed::GP16", then remove "sealed::CC16", and make pin_trait!() use "sealed::1CH", "sealed::2CH" and "sealed::GP16" directly, so that every ChannelxPin is match the first trait it fit into.

- pin_trait!(Channel1Pin, CaptureCompare16bitInstance);
+ pin_trait!(Channel1Pin, GeneralPurpose1ChannelInstance);
- pin_trait!(Channel2Pin, CaptureCompare16bitInstance);
+ pin_trait!(Channel1Pin, GeneralPurpose2ChannelInstance);
...

But this requires a lot of "just modify regs_xxx() and copy paste others" in sealed traits, and could render this piece of code sort of "unmaintainable".(For current impl, "simple modify and copy paste" is true. But we cannot assume there won't be someone make use of other additional fields/registers in future.)

And the same thing could also apply between 1CH_CMP/2CH_CMP/ADV and ChannelxComplementaryPin.

I got no idea on this situation.

Any ideas are welcomed! Thanks in advance!

@eZioPan eZioPan force-pushed the update-metapac8 branch 4 times, most recently from 0245421 to 751185a Compare February 1, 2024 10:11
@eZioPan eZioPan marked this pull request as draft February 1, 2024 10:30
@eZioPan
Copy link
Contributor Author

eZioPan commented Feb 1, 2024

STM32L0 is special, External Trigger is on TIM_2CH

@eZioPan eZioPan force-pushed the update-metapac8 branch 4 times, most recently from dac1038 to 68f24a2 Compare February 1, 2024 15:08
@eZioPan eZioPan marked this pull request as ready for review February 1, 2024 15:19
@eZioPan eZioPan force-pushed the update-metapac8 branch 5 times, most recently from 7ec042d to 9847770 Compare February 2, 2024 10:08
@eZioPan eZioPan marked this pull request as draft February 2, 2024 10:16
@eZioPan eZioPan force-pushed the update-metapac8 branch 6 times, most recently from 2c6520a to 566b493 Compare February 2, 2024 10:48
@eZioPan eZioPan marked this pull request as ready for review February 2, 2024 16:04
@eZioPan eZioPan force-pushed the update-metapac8 branch 2 times, most recently from 1607155 to 9e3608c Compare February 3, 2024 08:22
@Dirbaio Dirbaio added the trusted label Feb 5, 2024
@Dirbaio
Copy link
Member

Dirbaio commented Feb 5, 2024

bender run

@eZioPan
Copy link
Contributor Author

eZioPan commented Feb 5, 2024

It seems ok on HIL?

@Dirbaio
Copy link
Member

Dirbaio commented Feb 9, 2024

thank you! 🚀

@Dirbaio Dirbaio enabled auto-merge February 9, 2024 23:04
@Dirbaio
Copy link
Member

Dirbaio commented Feb 9, 2024

bender run

@Dirbaio Dirbaio disabled auto-merge February 9, 2024 23:23
@Dirbaio Dirbaio merged commit e7d0b8d into embassy-rs:main Feb 9, 2024
5 of 6 checks passed
@eZioPan eZioPan deleted the update-metapac8 branch February 10, 2024 04:59
jr-oss added a commit to jr-oss/embassy that referenced this pull request Mar 7, 2024
PR embassy-rs#2499 implemented timer hierarchy, but removed enable_outputs()
from trait CaptureCompare16bitInstance and from SimplePwm.

This functions is required for advanced timers to set bit BDTR.MOE
and to enable the output signal.
jr-oss added a commit to jr-oss/embassy that referenced this pull request Mar 8, 2024
PR embassy-rs#2499 implemented timer hierarchy, but removed enable_outputs()
from trait CaptureCompare16bitInstance and from SimplePwm.

This functions is required for advanced timers to set bit BDTR.MOE
and to enable the output signal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants