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

UsartSpi Support #562

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions avr-hal-generic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod port;
pub mod simple_pwm;
pub mod spi;
pub mod usart;
pub mod usart_spi;
pub mod wdt;

/// Prelude containing all HAL traits
Expand Down
132 changes: 132 additions & 0 deletions avr-hal-generic/src/usart_spi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
//! MSPIM Implimentation
use crate::{port::PinOps, spi};

// This module just impliments a macro for SpiOps, since underlyingly, the Spi type can still be used since it just needs SpiOps

/// Dummy Pin for MPSPIM
pub struct UsartSPIDummyPin;

impl PinOps for UsartSPIDummyPin {
type Dynamic = todo!();

fn into_dynamic(self) -> Self::Dynamic {
todo!()
}
Comment on lines +10 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this in port.rs, gave it a try and at least it built. What do you think?

Suggested change
type Dynamic = todo!();
fn into_dynamic(self) -> Self::Dynamic {
todo!()
}
type Dynamic = Self;
fn into_dynamic(self) -> Self::Dynamic {
self
}

Copy link
Owner

Choose a reason for hiding this comment

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

From my side this is fine. This pin-type is non-functional anyway - it just exists to bypass API problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it turns out I could not build examples, as I did not know how to instantiate UsartSPIDummyPin.

I was able to hack the examples into compiling by implementing a separate new for USART SPI, though it doesn't feel great.

impl<H, USART, SCLKPIN, MOSIPIN, MISOPIN, UsartSPIDummyPin> Spi<H, USART, SCLKPIN, MOSIPIN, MISOPIN, UsartSPIDummyPin>
where
    USART: SpiOps<H, SCLKPIN, MOSIPIN, MISOPIN, UsartSPIDummyPin>,
    SCLKPIN: port::PinOps,
    MOSIPIN: port::PinOps,
    MISOPIN: port::PinOps,
{
    /// Instantiate an SPI with the registers, SCLK/MOSI/MISO/CS pins, and settings,
    /// with the internal pull-up enabled on the MISO pin.
    ///
    /// The pins are not actually used directly, but they are moved into the struct in
    /// order to enforce that they are in the correct mode, and cannot be used by anyone
    /// else while SPI is active.  CS is placed into a `ChipSelectPin` instance and given
    /// back so that its output state can be changed as needed.
    pub fn new_from_usart(
        p: USART,
        sclk: port::Pin<port::mode::Output, SCLKPIN>,
        mosi: port::Pin<port::mode::Output, MOSIPIN>,
        miso: port::Pin<port::mode::Input<port::mode::PullUp>, MISOPIN>,
        settings: Settings,
    ) -> Self {
        let mut spi = Self {
            p,
            sclk,
            mosi,
            miso: miso.forget_imode(),
            write_in_progress: false,
            _cs: PhantomData,
            _h: PhantomData,
        };
        spi.p.raw_setup(&settings);
        spi
    }
}

To be honest, Spi taking a CS pin only makes sense for Spi slaves, as master could possibly have 0-N CS pins.
@Rahix is slave mode even supported? How would you feel about changing the API? Maybe split new into new_master and new_slave?

Copy link
Owner

Choose a reason for hiding this comment

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

Spi only takes the CS pin to enforce a hardware restriction, it actually doesn't do anything with the CS pin related to "CS" operation at all. This is an unfortunate side-effect of the AVR SPI peripheral design. You can read up more on this in #27.

Actually, I had the same thought as you here: We need the Dummy Pin to make the type-system happy, but this should be hidden from the user if at all possible. The easiest way to hide it is by adding a new constructor, like the one you showcased here. I think that's the best option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reference, I did not know about this behaviour 🤦‍♂️

My remaining issue with the separate constructor is how to enforce that the user passes actual USART peripheral? Because, as it stands, the following compiles...

let mut spi = spi::Spi::new_from_usart(
    dp.SPI,
    pins.pb1.into_output(),
    pins.pb2.into_output(),
    pins.pb3.into_pull_up_input(),
    spi::Settings::default(),
);

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, can we add a trait bound on UsartOps maybe? If not, I'd introduce a new marker trait UsartSpiDevice that is implemented alongside SpiOps for USART peripherals. We can then add a trait bound on this marker trait to the new constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a trait bound on UsartOps seems to do it. I'll test on hardware and, if everything goes well, make a PR as you advised.


unsafe fn out_set(&mut self) {}

unsafe fn out_clear(&mut self) {}

unsafe fn out_toggle(&mut self) {}

unsafe fn out_get(&self) -> bool {
false
}

unsafe fn in_get(&self) -> bool {
true
}

unsafe fn make_output(&mut self) {}

unsafe fn make_input(&mut self, pull_up: bool) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unsafe fn make_input(&mut self, pull_up: bool) {}
unsafe fn make_input(&mut self, _pull_up: bool) {}

}

pub type UsartSpi<H, USART, SCLKPIN, MOSIPIN, MISOPIN> =
spi::Spi<H, USART, SCLKPIN, MOSIPIN, MISOPIN, UsartSPIDummyPin>;

// Impliment SpiOps trait for USART
#[macro_export]
macro_rules! add_usart_spi {
(
hal: $HAL:ty,
peripheral: $USART_SPI:ty,
register_suffix: $n:expr,
sclk: $sclkpin:ty,
mosi: $mosipin:ty,
miso: $misopin:ty,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
miso: $misopin:ty,
miso: $misopin:ty,
cs: $cspin:ty,

) => {
$crate::paste::paste! {
// This is quite a messy way to get the doc string working properly... but it works!
#[doc = concat!("**Clock:** `", stringify!($sclkpin), "`<br>**MOSI:** `", stringify!($mosipin), "`<br> **MISO:** `", stringify!($misopin), "`<br> **CS:** `", stringify!($cspin), "`")]
pub type [<Usart $n Spi>] = avr_hal_generic::usart_spi::UsartSpi<$HAL, $USART_SPI, $sclkpin, $mosipin, $misopin>;

impl $crate::spi::SpiOps<$HAL, $sclkpin, $mosipin, $misopin, $cspin> for $USART_SPI {
CoolSlimbo marked this conversation as resolved.
Show resolved Hide resolved
fn raw_setup(&mut self, settings: &crate::spi::Settings) {
use $crate::hal::spi;

// Setup control registers
// We start by setting the UBBRn to 0
Copy link
Owner

Choose a reason for hiding this comment

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

Don't comment what code is doing, that should be obvious from reading the line below. Please comment why it is doing this — what's the purpose of setting UBBRn to 0 here?

Copy link
Author

Choose a reason for hiding this comment

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

Unsure of the purpose.

But code examples in the data sheet all do so, explicitly setting it to 0, before setting the baudrate (which I will update the calculation for)

self.[<ubrr $n>].write(|w| unsafe {w.bits(0)});

// We have to translate the character size register into the 2 bits which are the MSB/LSB and the phase
// 5 Bit Char = MSB and 1st
// 6 Bit Char = MSB and 2nd
// 7 Bit Char = LSB and 1st
// 8 Bit Char = LSB and 2nd
self.[<ucsr $n c>].write(|w| {
w.[<umsel $n>]().spi_master();

match settings.data_order {
crate::spi::DataOrder::MostSignificantFirst => match settings.mode.phase {
spi::Phase::CaptureOnFirstTransition => w.[<ucsz $n>]().chr5(),
spi::Phase::CaptureOnSecondTransition => w.[<ucsz $n>]().chr6(),
},
crate::spi::DataOrder::LeastSignificantFirst => match settings.mode.phase {
spi::Phase::CaptureOnFirstTransition => w.[<ucsz $n>]().chr7(),
spi::Phase::CaptureOnSecondTransition => w.[<ucsz $n>]().chr8(),
},
};

match settings.mode.polarity {
spi::Polarity::IdleLow => w.[<ucpol $n>]().clear_bit(),
spi::Polarity::IdleHigh => w.[<ucpol $n>]().set_bit(),
}
});

// Enable receiver and transmitter, and also the rec interrupt.
self.[<ucsr $n b>].write(|w| w
.[<txen $n>]().set_bit()
.[<rxen $n>]().set_bit()
);

// Set the clock divider for SPI clock.
self.[<ubrr $n>].write(|w| {
match settings.clock {
crate::spi::SerialClockRate::OscfOver2 => w.bits(0),
crate::spi::SerialClockRate::OscfOver4 => w.bits(1),
crate::spi::SerialClockRate::OscfOver8 => w.bits(3),
crate::spi::SerialClockRate::OscfOver16 => w.bits(7),
crate::spi::SerialClockRate::OscfOver32 => w.bits(15),
crate::spi::SerialClockRate::OscfOver64 => w.bits(31),
crate::spi::SerialClockRate::OscfOver128 => w.bits(63),
}
});
}

fn raw_release(&mut self) {
self.[<ucsr $n c>].write(|w| w.[<umsel $n>]().usart_async());
self.[<ucsr $n b>].reset();
}

fn raw_check_iflag(&self) -> bool {
self.[<ucsr $n a>].read().[<rxc $n>]().bit_is_set()
}

fn raw_read(&self) -> u8 {
self.[<udr $n>].read().bits()
}

fn raw_write(&mut self, byte: u8) {
self.[<udr $n>].write(|w| unsafe { w.bits(byte) });
}

fn raw_transaction(&mut self, byte: u8) -> u8 {
self.raw_write(byte);
while !self.raw_check_iflag() {}
self.raw_read()
}
}
}
};
}
78 changes: 78 additions & 0 deletions examples/atmega2560/src/bin/atmega2560-usart_spi-feedback.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
//! This example demonstrates how to set up a SPI interface and communicate
//! over it. The physical hardware configuration consists of connecting a
//! jumper directly from pin `PB2` to pin `PB3`.
//!
//! Run the program using `cargo run`.
//! You should see it output the line `data: 42` repeatedly.
//! If the output you see is `data: 255`, you may need to check your jumper.

#![no_std]
#![no_main]

use atmega_hal::delay::Delay;
use atmega_hal::usart::{Baudrate, Usart};
use atmega_hal::usart_spi;
use embedded_hal::delay::DelayNs;
use embedded_hal::spi::SpiBus;
use panic_halt as _;

// Define core clock. This can be used in the rest of the project.
type CoreClock = atmega_hal::clock::MHz16;

#[avr_device::entry]
fn main() -> ! {
let dp = atmega_hal::Peripherals::take().unwrap();
let pins = atmega_hal::pins!(dp);

let mut delay = Delay::<crate::CoreClock>::new();

// set up serial interface for text output
let mut serial = Usart::new(
dp.USART0,
pins.pe0,
pins.pe1.into_output(),
Baudrate::<crate::CoreClock>::new(57600),
);

// Create SPI interface.
let (mut spi, _) = usart_spi::Usart1Spi::new(
dp.USART1,
pins.pd5.into_output(),
pins.pd3.into_output(),
pins.pd2.into_pull_up_input(),
pins.pd4.into_output().downgrade(),
atmega_hal::spi::Settings::default(),
);

/* Other SPI examples for other USART's
let (mut spi, _) = usart_spi::Usart2Spi::new(
dp.USART2,
pins.ph2.into_output(),
pins.ph1.into_output(),
pins.ph0.into_pull_up_input(),
pins.pd4.into_output().downgrade(),
atmega_hal::spi::Settings::default(),
);

let (mut spi, _) = usart_spi::Usart3Spi::new(
dp.USART3,
pins.pj2.into_output(),
pins.pj1.into_output(),
pins.pj0.into_pull_up_input(),
pins.pd4.into_output().downgrade(),
atmega_hal::spi::Settings::default(),
);
*/

loop {
// Send a byte
let data_out: [u8; 1] = [42];
let mut data_in: [u8; 1] = [0];
// Send a byte
// Because MISO is connected to MOSI, the read data should be the same
spi.transfer(&mut data_in, &data_out).unwrap();

ufmt::uwriteln!(&mut serial, "data: {}\r", data_in[0]).unwrap();
delay.delay_ms(1000);
}
}
3 changes: 3 additions & 0 deletions mcu/atmega-hal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ pub mod eeprom;
#[cfg(feature = "device-selected")]
pub use eeprom::Eeprom;

#[cfg(feature = "device-selected")]
pub mod usart_spi;

pub struct Atmega;

#[cfg(any(feature = "atmega48p", feature = "atmega168", feature = "atmega328p"))]
Expand Down
6 changes: 4 additions & 2 deletions mcu/atmega-hal/src/usart.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ pub type UsartReader<USART, RX, TX, CLOCK> =
feature = "atmega328p",
feature = "atmega328pb",
feature = "atmega1284p",
feature = "atmega164pa"
feature = "atmega164pa",
feature = "atmega48p"
))]
pub type Usart0<CLOCK> = Usart<
crate::pac::USART0,
Expand All @@ -27,7 +28,8 @@ pub type Usart0<CLOCK> = Usart<
feature = "atmega328p",
feature = "atmega328pb",
feature = "atmega1284p",
feature = "atmega164pa"
feature = "atmega164pa",
feature = "atmega48p"
))]
avr_hal_generic::impl_usart_traditional! {
hal: crate::Atmega,
Expand Down
104 changes: 104 additions & 0 deletions mcu/atmega-hal/src/usart_spi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
//! USART MSPIM implimentations
//!
//! The following list details how many USARTs and if the USARTs support MSPIM for each board choosable.
//!
//! | Board | USARTs | SPI |
//! |-------|--------|-----|
//! | `atmega48p` | 1 | Yes |
//! | `atmega164pa`| 2 | Yes |
//! | `atmega168` | 1 | Yes |
//! | `atmega328p` | 1 | Yes |
//! | `atmega328pb` | 1 | Yes |
//! | `atmega32a` | 1 | No |
//! | `atmega32u4` | 1 | Yes |
//! | `atmega2560` | 4 | Yes |
//! | `atmega128a` | 2 | No |
//! | `atmega1280` | 4 | Yes |
//! | `atmega1284p` | 2 | Yes |
//! | `atmega8` | 1 | No |

// Supress warning because it doesn't recognise us using it in macros properly.
#[allow(unused_imports)]
use crate::port;

#[cfg(any(feature = "atmega1280", feature = "atmega2560"))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART0,
register_suffix: 0,
sclk: port::PE2,
mosi: port::PE1,
miso: port::PE0,
cs: port::Dynamic,
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, this is quite a hack. We really don't need the cs dance for MSPIM so it would be nice to somehow wrangle the SPI module to not require a CS pin for MSPIM...

Copy link
Author

Choose a reason for hiding this comment

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

This was done because I impliment SpiOps to cheap out on work, and not rewrite everything.

I can either reimplement a new spiops, or, force the use of a DummyPin internally (so users don’t have to worry about it) via the DummyPin crate (I believe that’s its name)

Copy link
Owner

Choose a reason for hiding this comment

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

I would be fine with defining a UsartSpiDummyCs pin type that needs to be passed to Spi::new() to sidestep this. Pulling in an external crate for this is not a good idea, I think.

Copy link
Author

Choose a reason for hiding this comment

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

An alternative to the Dummy CS Pin (and what I believe my original thought process was), is how it is, where it's dynamic, meaning the CS can be handled by the driver in some which way, by manual toggle instead of SPI handling it.

}

#[cfg(any(feature = "atmega1280", feature = "atmega2560"))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART1,
register_suffix: 1,
sclk: port::PD5,
mosi: port::PD3,
miso: port::PD2,
cs: port::Dynamic,
}

#[cfg(any(feature = "atmega1280", feature = "atmega2560"))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART2,
register_suffix: 2,
sclk: port::PH2,
mosi: port::PH1,
miso: port::PH0,
cs: port::Dynamic,
}

#[cfg(any(feature = "atmega1280", feature = "atmega2560"))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART3,
register_suffix: 3,
sclk: port::PJ2,
mosi: port::PJ1,
miso: port::PJ0,
cs: port::Dynamic,
}

#[cfg(any(
feature = "atmega168",
feature = "atmega328p",
feature = "atmega328pb",
feature = "atmega48p"
))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART0,
register_suffix: 0,
sclk: port::PD4,
mosi: port::PD1,
miso: port::PD0,
cs: port::Dynamic,
}

#[cfg(any(feature = "atmega1284p", feature = "atmega164pa",))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART0,
register_suffix: 0,
sclk: port::PB0,
mosi: port::PD1,
miso: port::PD0,
cs: port::Dynamic,
}

#[cfg(any(feature = "atmega1284p", feature = "atmega164pa",))]
avr_hal_generic::add_usart_spi! {
hal: crate::Atmega,
peripheral: crate::pac::USART1,
register_suffix: 1,
sclk: port::PD4,
mosi: port::PD3,
miso: port::PD2,
cs: port::Dynamic,
}
Loading