Stm32f4xx: Dma2 & USART1 + Stm32f429idiscovery BSP #2936
Conversation
Furthoremore did some exploratory work on USART1
By adding the missing interrupt handlers.
|
I haven't had time to look at this in-depth, but this does seem like a nice contribution that we would be happy to take upstream. To answer one of your questions:
Looking at the changes, it seems you just mean that the APIs for interacting with UART etc. on the STM32f4 have changed? If so, that is fine. Changing a HIL would be an issue, but that does not seem to be happening here -- if all of the changes are contained within the stm32 chip/board that is fine. |
Yes exactly, all the changes are within the chip/board crates. This PR doesn't change any Tock user-land APIs. But now that I know I'm on the right track, I'll get to cleaning up, so this PR is in a mergeable state. |
Instead of providing a Dma1Clock and a Dma2Clock, we can use a shared class for both DMA's, as they share the exact same code.
Awesome! In order for CI to pass, this PR will need to make the relevant adjustments to upstream (stm) boards such that they build with the new USART API. Let me know if you have trouble with that |
|
Alright, will do. Adjusting the other BSPs shouldn't be difficult. I'm currently doing a bit more cleanup to the DMA module, to reduce code duplication between Dma1 & Dma2 as much as possible. Will probably upload that change next week. |
To reduce the code duplication between Dma1&Dma2, the Stream class is moved into the shared Dma module. Dma1 and Dma2 then implement the new StreamServer trait, that allows them to serve their own Streams. A further refactoring is achieved by adding a StreamPeripheral trait, which is implmeneted in Dma1Peripheral & Dma2Peripheral and returns all the constants that might be different between DMA peripherals. This way the dma::Stream never has to match over the peripheral, but only ever over to stream to access the correct registers. The data to write into the registers will be provided by the Peripheral. This should also make adding new peripherals cleaner, as the Stream doesn't need to be touched, but only the Peripheral enum.
Not all boards will use the new Dma features, but they must still be adapted to fit the new API.
|
I have done quite a few more changes to reduce the code duplication between Dma1 & Dma2. Most notably, I've extracted a StreamPeripheral interface that both Dma1Peripheral and Dma2Peripheral need to implement.
If you like the new changes, can you please approve the ci workflows, so that I can check whether everything still works? |
LeonMatthesKDAB
changed the title
chips/stm32f4xx/src/dma/dma2.rs
Outdated
| pub struct Dma2<'a> { | ||
| registers: StaticRef<DmaRegisters>, | ||
| clock: DmaClock<'a>, | ||
| } |
There was a problem hiding this comment.
Can you elaborate a bit on the decision to make Dma1 and Dma2 different types, rather than having a single Dma struct with multiple instantiations? I understand this design is necessary for StreamServer to have Peripheral as an associated type rather than a generic type, but it does lead to some code duplication between Dma1 and Dma2. Does using a generic parameter for StreamServer cause issues with propagation of generic parameters throughout the code base?
There was a problem hiding this comment.
I did that mostly because I wanted the streams from the different Dma's to be of different types. I realize now that parameterizing the Stream over the Peripheral would also have worked.
Another benefit of the current approach is that it is impossible to use a Dma with the wrong Peripheral type.
Because the Stream is associated with the Dma on a type-level basis.
If the Stream were made generic over the Peripheral, i.e. Stream<Dma1Peripheral>, one might accidentally create such a stream for the wrong Dma instance.
It's definitely a tradeoff between having code duplication vs. more checks at compile time.
With the current implementation the Dma is propagated to the types of Usart & SPI, so usart1 is a Usart<Dma2>.
In theory, we could simply have a single DmaPeripheral enum, that contains all peripherals, with a single, non-generic Stream struct implementation.
This way we wouldn't have any code duplication and both Usart and Spi would no longer have to be generic over the Dma or its peripheral.
The disadvantage then would be, that one could create a Usart instance for usart1 and accidentally pass it a stream of dma1, instead of dma2. Currently, this is impossible, as they are of different types.
We could have these checks at runtime, with the Peripheral reporting which Dma it belongs to and the system panicking if it's set up incorrectly, like I have added for checking whether the peripheral is added to the right stream.
So we basically have three options here:
- Keep Stream<DmaX>
- Change to Stream<DmaXPeripheral>
- Remove Generics from Stream & add runtime checks
In order of most code duplication & compile-time safety to least.
It's probably best if you decide what fits best with the current design-philosophy of Tock OS :)
There was a problem hiding this comment.
Static checks are good, and the code duplication here looks manageable (i.e. it's pretty formulaic and unlikely to need to change much if at all).
I think it makes sense to combine these into a single dma.rs file, and move this thread to a comment in the source explaining what static properties this structure is designed to enforce.
There was a problem hiding this comment.
+1 to Brad's suggestion; thanks for explaining the reasoning here!
There was a problem hiding this comment.
馃憤
I'll move everything into the shared dma module then and add a comment.
chips/stm32f4xx/src/dma/mod.rs
Outdated
| @@ -1,17 +1,18 @@ | |||
| use core::fmt::Debug; | |||
There was a problem hiding this comment.
Can we name this something more descriptive than "mod"? I'm not sure what this file contains that is not in the other dma files.
There was a problem hiding this comment.
Recombined the dma subfolder into a dma.rs file
The dma1 and dma2 submodules were too small to justify their existence - therefore it makes more sense to combine their code into a single dma module.
|
@bradjc Do you mind approving the github workflow runs? |
Started |
|
@bradjc It's been nearly a month since the "last call" label has been added. When can this PR be merged? |
|
bors r+ |
Pull Request Overview
This PR originates from my need to get Tock OS running on a Stm32f429i discovery board.
The board layout was copied & adapted from the Nucleo_f429zi code.
The ST-Link connector that this board uses to connect to the PC is only connected to USART1, which must be configured with the DMA2 controller.
This PR therefore adds the ability for Tock OS to enable the DMA2 & USART1 on the Stm32f4xx platforms.
Even though the Stm32f429iDISC BSP should probably not be added to Tock OS, adding the DMA2 and USART1 is surely useful.
Testing Strategy
This pull request was tested by connecting to the the STM32F429IDiscovery board via it's ST-Link interface.
UART output this way is working for both in- and output. The Tock OS console is accessible that way.
I'd be happy to receive suggestions on how to automate testing for this 馃
TODO
dmamodule (e.g. DmaClock)use super::*indma1.rsanddma2.rsHelp Wanted
As this introduces a new dma peripheral, as well as a new USART stream, it breaks the internal Kernel API in quite a few places.
Is this approach viable, or should complete API backward-compatibility be the target? This doesn't change any User-Space APIs though.
I've kept Dma1&2 as separate types for now, so that Dma2 peripherals can not accidentally be passed into the Dma1 or vice-versa.
That does however mean, that the code for Usart, as well as all BSPs based on the Stm32f4xx boards need to be adapted to deal with this. If someone knows a way to implement this backwards-compatible, that would be great!
Currently, I've added a generic DmaPeripheral as well as Stream enum types that simply wrap either a Dma1Peripheral/Dma2Peripheral or Stream respectively, so that i.e. Usart can deal with either.
Feedback on this approach is much appreciated. I can also see something based on traits and dyn references to be viable, but I'm unsure if this is suitable in the embedded context of Tock OS.
Documentation Updated
chipandboardspecific code, no additional changes indocshould be required.Formatting
make prepush.