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

stm32/timer: add bare type-erased timer driver (nogeneric timers part 0) #3041

Closed
wants to merge 3 commits into from

Conversation

honzasp
Copy link
Contributor

@honzasp honzasp commented Jun 4, 2024

This PR introduces a bare timer driver bare::AnyTimer, which erases the TIMx peripheral type and provides bare access to the timer registers. To support different timer types, it is parameterized in the register type (such as TimGp16 or TimAdv).

This is intended to be an MVP for type-erased timer drivers. While bare::AnyTimer can already be useful on its own to applications that need just register-level access to the timers, it is also intended to be used as a basic building block for removing generics from higher-level timer drivers in Embassy.

Marked as draft because it depends on #3031 and because I also plan to add DMA support into the PR.

@honzasp honzasp marked this pull request as draft June 5, 2024 19:28
@honzasp
Copy link
Contributor Author

honzasp commented Jun 6, 2024

I added DMA support and rebased on top of latest changes to #3031.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 16, 2024

  • why add a new AnyTimer driver, instead of refactoring the existing low_level driver to remove generics? I'd prefer to not have two drivers that do the same thing.
  • We shouldn't make ChannelAndRequest public.
  • I'm not sure about using the PAC structs as typestate markers. The PAC is not a public dependency, users have to enable Cargo feature unstable-pac to get the embassy_stm32::pac reexport. So these are not types the user can name. I'd declare our own typestate marker as enum{}.

@honzasp
Copy link
Contributor Author

honzasp commented Jun 17, 2024

  • why add a new AnyTimer driver, instead of refactoring the existing low_level driver to remove generics? I'd prefer to not have two drivers that do the same thing.

The reasons were:

  1. I wanted to start with the smallest PR that supports timers without generics, and build on from here, so I decided not to touch any existing code.
  2. The idea was that the bare timer driver provides direct access to registers and implements the necessary interaction with RCC, and nothing else. The low level timer is a bit higher level than that, because it provides additional methods that wrap the register access.
  • We shouldn't make ChannelAndRequest public.

Could you please elaborate on why we shouldn't make this type public? The reason why I exposed it is that for timers, people will need to write their own drivers and handle DMA themselves. For example, I'm using the update DMA in my driver for DShot (a protocol for talking to an ESC, electronic speed controller for brushless motors).

Of course, people can use the dma::Transfer API and store the DMA channel and DMA request separately in their drivers, but the ChannelAndRequest API is very convenient and arguably safer, because it binds the channel and the request together. It feels like this struct corresponds to a meaningful concept ("DMA lane"? "DMA line"?) that we could expose as part of the DMA API.

Also, we already expose everything that ChannelAndRequest uses, it doesn't give people access to anything new.

  • I'm not sure about using the PAC structs as typestate markers. The PAC is not a public dependency, users have to enable Cargo feature unstable-pac to get the embassy_stm32::pac reexport. So these are not types the user can name. I'd declare our own typestate marker as enum{}.

I agree; I went for the PAC structs for the first PR, but I wanted to switch to our own typestate markers in a follow-up PR.


In fact, I have that follow-up PR mostly fleshed out, it might be ready this week. It attempts to solve the timer taxonomy problem: how to make sure that a timer driver uses only features supported by the timer peripheral, while making the driver usable with as many timers as possible?

My original plan was to merge this PR first and follow it with that "timer taxonomy" PR, but at this point it no longer makes sense; once that PR is ready, I think we should just close this PR.

@honzasp
Copy link
Contributor Author

honzasp commented Jun 27, 2024

Closed in favor of #3123. That PR still exposes dma::ChannelAndRequest and keeps the raw timer and low-level timer drivers separate, but I can of course change that if my reasons for that are not convincing :)

@honzasp honzasp closed this Jun 27, 2024
@honzasp honzasp deleted the any-timer branch June 27, 2024 05:35
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.

2 participants