stm32-rs / stm32f7xx-hal

A Rust embedded-hal HAL for all MCUs in the STM32 F7 family
Apache License 2.0
117 stars 68 forks source link

Add QSPI driver and example #107

Closed bbrown1867 closed 3 years ago

bbrown1867 commented 3 years ago
bbrown1867 commented 3 years ago

@hannobraun @mvertescher any feedback on this driver?

bbrown1867 commented 3 years ago

Thank you for the PR, @bbrown1867!

I don't know anything about QSPI, nor do I have the hardware or time to test this. I did a quick review, and can't see anything wrong with it. Although it's a bit odd that the DMA methods are blocking (why use DMA then).

I left some comments with general nitpicks. Feel free to address those is a follow-up PR or not at all, as you prefer. I'm going to merge this now. It's been open long enough, I think.

Thanks for taking a look @hannobraun !

Yes this peripheral is a bit more niche so I didn't expect a lot of feedback on it.

The DMA methods are much faster than the polling ones, so even if blocking DMA could be beneficial. I agree that making the API non-blocking would be an improvement. I had tried to do this by returning the DMA handle after starting but had issues with exposing crate-private types publicly. Perhaps I need to make some kind of transfer wrapper type like spi.rs does. In general the DMA functions could probably be improved to be more generic too, so perhaps I will submit another PR in the future to address this.

hannobraun commented 3 years ago

Thanks for the explanation, @bbrown1867. Yes, DMA APIs can be tricky. I haven't worked on this HAL for a while, but I assume those methods are crate-private because they would allow the user to circumvent things that the API would like to guarantee. Providing a peripheral-specific wrapper is the way to go, I think.