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
Add reclock functionality for SPI #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thank you for this PR! I left a few comments
src/spi.rs
Outdated
@@ -114,6 +114,20 @@ pub struct Spi<SPI, PINS> { | |||
pins: PINS, | |||
} | |||
|
|||
fn compute_clock_variant(clocks: Hertz, freq: Hertz) -> u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use the type-safe alternative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename to
fn compute_clock_variant(clocks: Hertz, freq: Hertz) -> u8 { | |
fn compute_baud_rate(clocks: Hertz, freq: Hertz) -> u8 { |
pub fn reclock<F>(&mut self, freq: F, clocks: Clocks) | ||
where F: Into<Hertz> | ||
{ | ||
self.spi.cr1.modify(|_, w| w.spe().disabled()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any side effects changing the frequency, or is it sufficient to disable and enable it right after it? Any delay needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I didn't find any documentation about this.
All I can say is that I tested this with an SD card using the embedded-sdmmc library. I initialize the card at 400KHz, then I switch to 16Mhz and data is red without a problem. I'm reading mp3s from the SD-card and playing them (after decoding) at the same time. Would there be a problem I think I'd notice it. If I leave the card at 400Khz the playing doesn't go well. I tried this also by initializing the card directly at 8 Mhz, and I would get random things and strange crashes in the sdmmc library. I also believe this depends on the device at the other end. With SD cards this works, with other devices it might not.
@@ -200,6 +204,17 @@ macro_rules! hal { | |||
pub fn free(self) -> ($SPIX, (SCK, MISO, MOSI)) { | |||
(self.spi, self.pins) | |||
} | |||
|
|||
pub fn reclock<F>(&mut self, freq: F, clocks: Clocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a doc-comment.
@Sh3Rm4n Thank you for your review. I believe I addressed all your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for this! And sorry for the long delay.
If you want to, you can add a changelog entry :)
@Sh3Rm4n thank you for your review. I don't have the rights to merge the PR. Could you do it for me? |
CHANGELOG.md
Outdated
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
- Support for safe one-shot DMA transfers ([#86](https://github.com/stm32-rs/stm32f3xx-hal/pull/86)) | |||
- DMA support for serial reception and transmission ([#86](https://github.com/stm32-rs/stm32f3xx-hal/pull/86)) | |||
- ADC support for `stm32f303` devices ([#47](https://github.com/stm32-rs/stm32f3xx-hal/pull/47)) | |||
- SPI support for reclock after initialization ([#98](https://github.com/stm32-rs/stm32f3xx-hal/pull/98)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the wrong place, as 0.5.0 is already released. You'd have to move it under Unreleased
:)
Some devices (eg SD cards) require the initialization to be done a at a lower frequency and allow the frequency to be later increased. Thus a re-clock functionality has been added. The tm4c123x-hal also does this.