`UsartSpi` Support #562

CoolSlimbo · 2024-06-23T05:02:40Z

PR regarding issue #561.

This is just so I have the PR made for now, work still needs to be made on it.

So far, USART0 is implemented for most of the atmega mc's. However, they are not tested, as I am lacking something to test it with.

Needs to be tested. Will test via Arudino UNO when I move them ig

…ny SPI modules to test on...

stappersg · 2024-06-23T06:06:15Z

mcu/atmega-hal/src/spi.rs

@@ -31,6 +31,66 @@ avr_hal_generic::impl_spi! {
    cs: port::PB0,
 }

+#[cfg(any(


// Devices where UART port can be a SPI port

stappersg · 2024-06-23T06:29:36Z

Idea, maybe a wild idea: The new code in one #[cfg(any( ))] block?

Anyway: Success with testing.

CoolSlimbo · 2024-06-23T06:32:16Z

Wait. It actually worked on hardware? Part of me is surprised lol

I'll probably move it into its own module, but can't use one config, because they align to different ports between devices.

CoolSlimbo · 2024-06-23T07:34:37Z

Currently added in (I think) at minimum all the USART0 implimentations. But there's still a large portion to go, and finding the XCK pin requires getting the datasheet for each.

avr-hal-generic/src/usart_spi.rs

…rom here baby)

…cludes rejig of pin layouts between boards.

CoolSlimbo · 2024-06-24T10:09:41Z

Implements USART, correctly, on all devices. Should past CI successfully.

An example could be in order, however, I am lacking any SPI slaves, so I cannot test this myself.

Rahix · 2024-06-27T16:27:26Z

An example could be in order, however, I am lacking any SPI slaves, so I cannot test this myself.

SPI can be tested very easily by looping back MOSI to MISO (so TX to RX in this case). We also have this in our real SPI example, maybe it makes sense to add an example that does the same for UsartSpi?

… doesn't have the right pins. Will fix that soon. If someone else can test would be great.

Rahix · 2024-07-07T21:38:00Z

Hey, there seem to be some compiler errors, as seen in CI. Can you take a look?

armandas · 2024-07-24T09:15:13Z

I went on and tested the atmega2560-usart_spi-feedback example. I tested USART1, USART2 and USART3 and they all work. USART0 is connected to USB, so I did not test that.

My diff to get it to compile:

diff --git a/examples/atmega2560/src/bin/atmega2560-usart_spi-feedback.rs b/examples/atmega2560/src/bin/atmega2560-usart_spi-feedback.rs
index 3a2d145..bce2ad4 100644
--- a/examples/atmega2560/src/bin/atmega2560-usart_spi-feedback.rs
+++ b/examples/atmega2560/src/bin/atmega2560-usart_spi-feedback.rs
@@ -36,12 +36,12 @@ fn main() -> ! {
 
     // Create SPI interface.
     let (mut spi, _) = usart_spi::Usart1Spi::new(
-        dp.SPI,
+        dp.USART1,
         pins.pd5.into_output(),
         pins.pd3.into_output(),
         pins.pd2.into_pull_up_input(),
         pins.pd4.into_output().downgrade(),
-        spi::Settings::default(),
+        atmega_hal::spi::Settings::default(),
     );
 
     loop {

Other configs, for reference:

USART2:

    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(),
    );

USART3:

    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(),
    );

(Tested)[Rahix#562 (comment)]. Introduces points for other examples commented in.

avr-hal-generic/src/usart_spi.rs

armandas · 2024-07-25T04:18:07Z

@Rahix I was wondering if it would make sense to re-export configuration structs and enums defined in spi from usart_spi?

Rahix · 2024-07-25T20:31:12Z

I was wondering if it would make sense to re-export configuration structs and enums defined in spi from usart_spi?

To be honest, I would prefer not to add such re-exports. The more paths there are to reach an item the more convoluted user code will get.

Rahix · 2024-07-25T20:35:06Z

avr-hal-generic/src/usart_spi.rs

+                    use $crate::hal::spi;
+
+                    // Setup control registers
+                    // We start by setting the UBBRn to 0


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?

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)

Rahix · 2024-07-25T20:39:22Z

avr-hal-generic/src/usart_spi.rs

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


Don't enable the interrupt by default - it should always be explicitly enabled by the user through a method call, see e.g.

avr-hal/avr-hal-generic/src/usart.rs

Lines 300 to 308 in 65b304e

/// Enable the interrupt for [`Event`].

pub fn listen(&mut self, event: Event) {

self.p.raw_interrupt(event, true);

}

/// Disable the interrupt for [`Event`].

pub fn unlisten(&mut self, event: Event) {

self.p.raw_interrupt(event, false);

}

You don't need to implement this here, though — it should be part of the spi module.

Could you elaborate on what you mean by this?

I understand why to remove it (idk why it was there to begin with), but do you want me to add methods to change the interrupts? Or something other, because I don't understand "it should be part of the spi module"

Rahix · 2024-07-25T20:46:23Z

avr-hal-generic/src/usart_spi.rs

+                fn raw_release(&mut self) {
+                    // Probably a better way to "release" the SPI interface, but from the datasheet, this is what they suggest, so ig it works
+                    self.[<ucsr $n c>].write(|w| w.[<umsel $n>]().usart_async());
+                }


Similar to the USART code, you also need to disable the USART peripheral here:

avr-hal/avr-hal-generic/src/usart.rs

Lines 506 to 510 in 65b304e

fn raw_deinit(&mut self) {

// Wait for any ongoing transfer to finish.

$crate::nb::block!(self.raw_flush()).ok();

self.[<ucsr $n b>].reset();

}

(The UCSRnB reset is the important part).

With that included, you can drop your comment.

Rahix · 2024-07-25T20:51:07Z

mcu/atmega-hal/src/usart_spi.rs

+    sclk: port::PE2,
+    mosi: port::PE1,
+    miso: port::PE0,
+    cs: port::Dynamic,


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...

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)

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.

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.

Co-authored-by: Armandas Jarušauskas <[email protected]>

Add reset to USART.

armandas

I messed with the code for a bit and got it to compile. Don't have a board on hand at the moment, but will try to run the code this week.

armandas · 2024-09-30T01:10:06Z

avr-hal-generic/src/usart_spi.rs

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


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

}

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

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?

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.

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(), );

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.

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.

armandas · 2024-09-30T01:10:27Z

avr-hal-generic/src/usart_spi.rs

+
+    unsafe fn make_output(&mut self) {}
+
+    unsafe fn make_input(&mut self, pull_up: bool) {}


Suggested change

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

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

armandas · 2024-09-30T01:10:52Z

avr-hal-generic/src/usart_spi.rs

+        register_suffix: $n:expr,
+        sclk: $sclkpin:ty,
+        mosi: $mosipin:ty,
+        miso: $misopin:ty,


Suggested change

miso: $misopin:ty,

miso: $misopin:ty,

cs: $cspin:ty,

Rahix · 2024-10-03T12:07:58Z

BTW, @armandas, if you want to pick up this work while @CoolSlimbo isn't around, feel free to open a new PR based on top of this one.

CoolSlimbo added 2 commits June 23, 2024 14:51

Inital implimentation. Adds in on USART0 only.

4a110c9

Needs to be tested. Will test via Arudino UNO when I move them ig

Implimented atmega328p support. Also untested, because I don't have a…

8ac3a86

…ny SPI modules to test on...

stappersg reviewed Jun 23, 2024

View reviewed changes

CoolSlimbo added 5 commits June 23, 2024 17:04

Reoragnise the usart_spi into its own module. + Docs

2cd0cbd

Rejig the docs.

82ad09a

Atmega1280 and 2560 UsartSpi Implimentations

ab5fad0

Add 48p Normal Usart, idk what it was missing

5debde9

Added some more.

a37cbc2

CoolSlimbo changed the title UsartSpi Support **UNTESTED** UsartSpi Support Jun 23, 2024

CoolSlimbo mentioned this pull request Jun 23, 2024

Utilising the atmega2560's "USART in SPI Mode" #561

Open

Rahix reviewed Jun 23, 2024

View reviewed changes

avr-hal-generic/src/usart_spi.rs Show resolved Hide resolved

Rahix linked an issue Jun 23, 2024 that may be closed by this pull request

Utilising the atmega2560's "USART in SPI Mode" #561

Open

Rahix added hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal labels Jun 23, 2024

CoolSlimbo added 4 commits June 23, 2024 18:59

Fixed issue regarding CI.

c5b076f

Mark bit writes as unsafe.

9ddb92b

Optimise my laziness (remove the inital implimentation, only macros f…

b88cf04

…rom here baby)

Finalise the remaining USART's (turns out I was right at the end). In…

d9bfd46

…cludes rejig of pin layouts between boards.

CoolSlimbo added 2 commits June 28, 2024 09:41

Added some docs to make it less confusing.

597b073

I haven't tested this because my atmega2560 is currently soldered and…

54990d0

… doesn't have the right pins. Will fix that soon. If someone else can test would be great.

Adjust example to properly run.

46f4183

(Tested)[Rahix#562 (comment)]. Introduces points for other examples commented in.

armandas reviewed Jul 25, 2024

View reviewed changes

avr-hal-generic/src/usart_spi.rs Outdated Show resolved Hide resolved

Rahix reviewed Jul 25, 2024

View reviewed changes

CoolSlimbo and others added 5 commits July 26, 2024 15:53

Merge branch 'Rahix:main' into main

893f775

Update avr-hal-generic/src/usart_spi.rs w/ armandas clock rates

8fa453f

Co-authored-by: Armandas Jarušauskas <[email protected]>

Add dummy pin (figure out how to dynamic it)

3593202

Add reset to USART.

Merge branch 'main' of https://github.com/CoolSlimbo/avr-hal

0f0c38c

Seriously... how do I do dynamic tho?

7b21e5c

armandas reviewed Sep 30, 2024

View reviewed changes

armandas added a commit to armandas/avr-hal that referenced this pull request Oct 5, 2024

Update comments to address feedback in Rahix#562

8c4ca6f

armandas added a commit to armandas/avr-hal that referenced this pull request Oct 5, 2024

Update comments to address feedback in Rahix#562

d4acdd6

armandas mentioned this pull request Oct 5, 2024

UsartSpi Support #588

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`UsartSpi` Support #562

`UsartSpi` Support #562

CoolSlimbo commented Jun 23, 2024

stappersg Jun 23, 2024

stappersg commented Jun 23, 2024

CoolSlimbo commented Jun 23, 2024

CoolSlimbo commented Jun 23, 2024

CoolSlimbo commented Jun 24, 2024

Rahix commented Jun 27, 2024

Rahix commented Jul 7, 2024

armandas commented Jul 24, 2024 •

edited

Loading

armandas commented Jul 25, 2024

Rahix commented Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 26, 2024

Rahix Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

armandas left a comment

armandas Sep 30, 2024

Rahix Sep 30, 2024

armandas Oct 2, 2024

Rahix Oct 2, 2024

armandas Oct 2, 2024

Rahix Oct 3, 2024

armandas Oct 4, 2024

armandas Sep 30, 2024

armandas Sep 30, 2024

Rahix commented Oct 3, 2024

	/// Enable the interrupt for [`Event`].
	pub fn listen(&mut self, event: Event) {
	self.p.raw_interrupt(event, true);
	}

	/// Disable the interrupt for [`Event`].
	pub fn unlisten(&mut self, event: Event) {
	self.p.raw_interrupt(event, false);
	}

	fn raw_deinit(&mut self) {
	// Wait for any ongoing transfer to finish.
	$crate::nb::block!(self.raw_flush()).ok();
	self.[<ucsr $n b>].reset();
	}


		unsafe fn make_output(&mut self) {}

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

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

UsartSpi Support #562

Are you sure you want to change the base?

UsartSpi Support #562

Conversation

CoolSlimbo commented Jun 23, 2024

Choose a reason for hiding this comment

stappersg commented Jun 23, 2024

CoolSlimbo commented Jun 23, 2024

CoolSlimbo commented Jun 23, 2024

CoolSlimbo commented Jun 24, 2024

Rahix commented Jun 27, 2024

Rahix commented Jul 7, 2024

armandas commented Jul 24, 2024 • edited Loading

armandas commented Jul 25, 2024

Rahix commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armandas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahix commented Oct 3, 2024

`UsartSpi` Support #562

`UsartSpi` Support #562

armandas commented Jul 24, 2024 •

edited

Loading