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

Spi api changes #469

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 107 additions & 29 deletions rp2040-hal/src/spi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,53 +78,131 @@ impl<S: State, D: SpiDevice, const DS: u8> Spi<S, D, DS> {
/// Set baudrate based on peripheral clock
///
/// Typically the peripheral clock is set to 125_000_000
///
/// Note that this takes ~100us on rp2040 at runtime. If that's too slow for you, see
/// [calc_spi_clock_divider_settings_for_baudrate]
pub fn set_baudrate<F: Into<HertzU32>, B: Into<HertzU32>>(
&mut self,
peri_frequency: F,
baudrate: B,
) -> HertzU32 {
let freq_in = peri_frequency.into().to_Hz();
let baudrate = baudrate.into().to_Hz();
let mut prescale: u8 = u8::MAX;
let mut postdiv: u8 = 0;

// Find smallest prescale value which puts output frequency in range of
// post-divide. Prescale is an even number from 2 to 254 inclusive.
for prescale_option in (2u32..=254).step_by(2) {
// We need to use an saturating_mul here because with a high baudrate certain invalid prescale
// values might not fit in u32. However we can be sure those values exeed the max sys_clk frequency
// So clamping a u32::MAX is fine here...
if freq_in < ((prescale_option + 2) * 256).saturating_mul(baudrate) {
prescale = prescale_option as u8;
break;
}
}

let settings = calc_spi_clock_divider_settings_for_baudrate(freq_in, baudrate);

// We might not find a prescale value that lowers the clock freq enough, so we leave it at max
debug_assert_ne!(prescale, u8::MAX);

// Find largest post-divide which makes output <= baudrate. Post-divide is
// an integer in the range 0 to 255 inclusive.
for postdiv_option in (1..=255u8).rev() {
if freq_in / (prescale as u32 * postdiv_option as u32) > baudrate {
postdiv = postdiv_option;
break;
}
}
debug_assert_ne!(settings.prescale, u8::MAX);

self.set_baudrate_from_settings(&settings);

// Return the frequency we were able to achieve
use fugit::RateExtU32;
(freq_in / (settings.prescale as u32 * (1 + settings.postdiv as u32))).Hz()
}

/// Set the baudrate using a previously calculated [SpiClockDividerSettings]
pub fn set_baudrate_from_settings(&mut self, settings: &SpiClockDividerSettings) {
self.device
.sspcpsr
.write(|w| unsafe { w.cpsdvsr().bits(prescale) });
.write(|w| unsafe { w.cpsdvsr().bits(settings.prescale) });
self.device
.sspcr0
.modify(|_, w| unsafe { w.scr().bits(postdiv) });
.modify(|_, w| unsafe { w.scr().bits(settings.postdiv) });
}

// Return the frequency we were able to achieve
use fugit::RateExtU32;
(freq_in / (prescale as u32 * (1 + postdiv as u32))).Hz()
/// Set the mode
///
/// Momentarily disables / enables the device so be careful of truncating ongoing transfers.
pub fn set_mode(&mut self, mode: Mode) {
// disable the device
self.device.sspcr1.modify(|_, w| w.sse().clear_bit());

// Set the polarity and phase
self.device.sspcr0.modify(|_, w| {
w.spo()
.bit(mode.polarity == Polarity::IdleHigh)
.sph()
.bit(mode.phase == Phase::CaptureOnSecondTransition)
});

// enable the device
self.device.sspcr1.modify(|_, w| w.sse().set_bit());
}

/// Wait until all operations have completed and the bus is idle.
pub fn flush(&self) -> Result<(), nb::Error<Infallible>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another remark: The future of nb is a little bit unclear. It might get replaced by an async API.
While I'd say that we should continue supporting the nb traits of embedded-hal for the foreseeable future, I wouldn't add methods using it to the the rp2040-hal impls.

So this method should have the same signature as https://docs.rs/embedded-hal/1.0.0-alpha.9/embedded_hal/spi/trait.SpiBusFlush.html#tymethod.flush, fn flush(&mut self) -> Result<(), Self::Error>.

Copy link
Contributor

@ptpaterson ptpaterson Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self::Error is currently being implemented as Infallable, so the proposed signature change would mean flush would have to be blocking, right?

    fn flush(&self) -> Result<(), Self::Error> {
        while self.device.sspsr.read().bsy().bit() {}
        Ok(())
    }

If that's the case, why not make the inherent impl return () and allow an eh alpha SpiBusFlush implementation to use that. See Fallability discussion in proposed migration guide.

    fn flush(&self) {
        while self.device.sspsr.read().bsy().bit() {}
    }

Or for that matter, if it is helpful to have an inherent non-blocking implementation (I honestly don't know if that's helpful) why not let the inherent impl be non-blocking (keep the current signature with Result<(), nb::Error<Infallible>>) to be consistent with the other inherent impls and the SpiBusFlush trait impl loop over it to make it blocking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized this PR is based on a relatively old commit -- this needs a rebase.

So if rebased, Spi<Enabled, D, DS> now has an is_busy method which can be used to implement flush. I think it makes sense to only implement flush for enabled SPI anyway.

impl<D: SpiDevice, const DS: u8> Spi<Enabled, D, DS> {
    /* ... */
    fn flush(&self) {
        while self.is_busy {}
    }
}

which would allow later:

    #[cfg(feature = "eh1_0_alpha")]
    impl<D: SpiDevice> eh1::SpiBusFlush for Spi<Enabled, D, $nr> {
        fn flush(&mut self) -> Result<(), Self::Error> {
            self.flush();
            Ok(())
        }
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if rebased, Spi<Enabled, D, DS> now has an is_busy method which can be used to implement flush.

Exactly, and is_busy is functionally equivalent to the non-blocking version of flush, so it's sufficient to have the blocking flush method.

if self.device.sspsr.read().bsy().bit() {
Err(nb::Error::WouldBlock)
} else {
Ok(())
}
}
}

/// Clock divider settings
pub struct SpiClockDividerSettings {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should most likely derive:

  • Copy, Clone
  • PartialEq, Eq
  • Debug
  • defmt::Format (when the feature is enabled)

/// The prescaler for writing to sspcpsr
pub prescale: u8,
/// The postdiv for writing to sspcr0
pub postdiv: u8,
}
Comment on lines +144 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making fields public is a strong commitment (see rust's future proofing guidelines).

Also, I'm not even sure allowing users to set them to arbitrary values is a good idea since they'd need to be validated before set in the registers (eg only even values are allowed in CPSDVSR).


/// Calculate the prescale and post divider settings for a required baudrate that can then be
/// passed to [Spi::set_baudrate_from_settings]
///
/// This calculation takes ~100us on rp2040 at runtime and is used by [set_baudrate] every time
/// it's called. That might not be acceptable in
/// situations where you need to change the baudrate often.
///
/// Note that this is a const function so you can use it in a static context if you don't change your
/// peripheral clock frequency at runtime.
///
/// If you do change your peripheral clock at runtime you can store the [SpiClockDividerSettings] and only re-calculate it
/// when the peripheral clock frequency changes.
pub const fn calc_spi_clock_divider_settings_for_baudrate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would make a nice inherent method to SpiClockDividerSettings like

impl SpiClockDividerSettings {
    pub const fn new(peri_frequency: HertzU32, baudrate: HertzU32) -> Self {
        todo!()
    }
}

peri_frequency_hz: u32,
baudrate_hz: u32,
) -> SpiClockDividerSettings {
let mut prescale: u8 = u8::MAX;
let mut postdiv: u8 = 0;

// Find smallest prescale value which puts output frequency in range of
// post-divide. Prescale is an even number from 2 to 254 inclusive.
let mut prescale_option: u32 = 0;
loop {
prescale_option += 2;
if prescale_option >= 254 {
break;
}

// We need to use a saturating_mul here because with a high baudrate certain invalid prescale
// values might not fit in u32. However we can be sure those values exeed the max sys_clk frequency
// So clamping a u32::MAX is fine here...
if peri_frequency_hz < ((prescale_option + 2) * 256).saturating_mul(baudrate_hz) {
prescale = prescale_option as u8;
break;
}
}

// Find largest post-divide which makes output <= baudrate. Post-divide is
// an integer in the range 0 to 255 inclusive.
let mut postdiv_option = 255u8;
loop {
if peri_frequency_hz / (prescale as u32 * postdiv_option as u32) > baudrate_hz {
postdiv = postdiv_option;
break;
}

postdiv_option -= 1;
if postdiv_option < 1 {
break;
}
}

SpiClockDividerSettings { prescale, postdiv }
}

impl<D: SpiDevice, const DS: u8> Spi<Disabled, D, DS> {
/// Create new spi device
pub fn new(device: D) -> Spi<Disabled, D, DS> {
Expand Down