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

armandas · 2024-07-25T00:38:36Z

avr-hal-generic/src/usart_spi.rs

+                    // Set the baudrate of the UBRRn, idk what it should be set to, so for now, it'll be set to 0
+                    self.[<ubrr $n>].write(|w| unsafe{w.bits(0)});


UBRR sets the SPI clock frequency, so it should look something like this? I tested and it works, but I'm not sure if this is the best way to write the register in Rust.

Suggested change

// Set the baudrate of the UBRRn, idk what it should be set to, so for now, it'll be set to 0

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

// 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),

}

});

If you are certain on the numbers, you can make use of the commit 65b304e ("generic: spi: Assign discriminant values to clock rate enum") that I just pushed:

self.[<ubrr $n].write(|w| { 2u8.pow(settings.clock as u32) - 1 });

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.

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.

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

Rahix reviewed Jul 25, 2024

View reviewed changes

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 Jul 25, 2024

Rahix Jul 25, 2024

armandas commented Jul 25, 2024

Rahix commented Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

Rahix Jul 25, 2024

Rahix Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

Rahix Jul 25, 2024

CoolSlimbo Jul 25, 2024

		// Set the baudrate of the UBRRn, idk what it should be set to, so for now, it'll be set to 0
		self.[<ubrr $n>].write(\|w\| unsafe{w.bits(0)});

-                    // Set the baudrate of the UBRRn, idk what it should be set to, so for now, it'll be set to 0
-                    self.[<ubrr $n>].write(|w| unsafe{w.bits(0)});
+                    // 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),
+                        }
+                    });

	/// 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();
	}

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

`UsartSpi` Support #562

`UsartSpi` Support #562

armandas commented Jul 24, 2024 •

edited

Loading