Refactor spi modules to improve structure #467

bradleyharden · 2021-07-05T20:41:48Z

Make several improvements to the sercom::v2::spi module:

Introduce a Registers type that acts as a task-focused API for the
registers, as opposed to the register-focused API of the PAC. This
abstraction also serves to remove interior mutability of the PAC
struct and lets us implement Sync for Registers
Add a Capability trait and three corresponding types: Rx, Tx and
Duplex. Add a Capability type parameter to the Spi struct and
use it to differentiate the various embedded HAL trait
implementations. This is a better solution than the previous approach,
which was a set of marker traits implemented on Pads types.
Combine the thumbv6m and thumbv7em modules to reuse more code. The
major differences between the two chips come in the Pads type, and
the Length and CharSize types. Introduce a Size trait that
essentially acts a trait alias for either Length or CharSize.
Split up the modules and use conditional imports to handle everything
correctly for the three different chips.

Because the spi module is no longer split between the two
chip-specific modules, also consolidate the impl_pad modules as well.

Finally, improve the documentation for the spi module itself, as well
as for the embedded HAL trait implementations.

hal/src/sercom/v2/pad/impl_pad_thumbv7em.rs

hal/src/sercom/v2/spi.rs

jbeaurivage · 2021-07-28T15:00:30Z

hal/src/sercom/v2/spi.rs

+    /// Change the transaction [`Length`]
+    ///
+    /// Changing the transaction [`Length`] while is enabled is permissible but
+    /// `unsafe`. If you have sent or received *any* bytes at the current
+    /// [`Length`], you **must** wait for a TXC flag before changing to a new
+    /// [`Length`].
+    #[inline]
+    #[cfg(feature = "min-samd51g")]
+    pub unsafe fn length<L: Length>(self) -> Spi<Config<C::Pads, C::OpMode, L>, A>
+    where
+        Config<C::Pads, C::OpMode, L>: ValidConfig,
+    {
+        Spi {
+            config: self.config.into().length(),
+            capability: self.capability,
+        }
+    }


Is it really unsafe? In a previous discussion we agreed that unsafe should, as much as possible, be used only for things that may incur memory unsafety/UB. I guess it could be argued that this is "memory unsafe" as the data received is not what was expected. I'm not sure.

Yes, I think you're right. It can put you in a sort of dead lock, or it can give you incorrect values, but it can't corrupt memory. Maybe it could in combination with DMA, but I think that's a separate issue.

I also removed unsafe from set_dyn_length for the same reason. Now only getting a reference to the underlying Sercom type and directly reading and writing the DATA register are unsafe. I think those are closer to memory unsafety.

hal/src/sercom/v2/spi/impl_ehal_thumbv6m.rs

hal/src/sercom/v2/spi/impl_ehal_thumbv7em.rs

hal/src/sercom/v2/spi/pads_thumbv6m.rs

jbeaurivage · 2021-07-28T15:14:41Z

hal/src/sercom/v2/spi/pads_thumbv7em.rs

+/// Map an [`OptionalPadNum`] to its corresponding `DIPO` value
+pub trait Dipo: OptionalPadNum {
+    const DIPO: Option<u8>;
+}
+
+impl Dipo for NoneT {
+    const DIPO: Option<u8> = None;
+}
+
+impl Dipo for Pad0 {
+    const DIPO: Option<u8> = Some(0);
+}
+impl Dipo for Pad1 {
+    const DIPO: Option<u8> = Some(1);
+}
+impl Dipo for Pad2 {
+    const DIPO: Option<u8> = Some(2);
+}
+impl Dipo for Pad3 {
+    const DIPO: Option<u8> = Some(3);
+}


This is new. Is there a downside to just setting DIPO/DOPO to a nonsensical value instead? (asking for uart-v2).

It's been several months, but I think I remember why I did this. I have Dipo and Dopo traits, but I also have a DipoDopo trait. The latter trait is responsible for taking two Option<u8> values and resolving them to valid pairs of DIPO and DOPO. I didn't want to accidentally introduce a loopback scenario, so I needed to make sure the two values never collide, even when one of the pins is NoneT. For SAMD2x chips, that came in the form of expanding the macro. For SAMD5x chips, I did this.

I think I remember concluding that UART doesn't have to worry about loopback in the same way SPI does.

bradleyharden · 2021-09-08T02:59:36Z

@jacobrosenthal, I refactored this to get rid of its dependence on the pygamer port. I also fixed DMA in light of the UART merge. I still need to go through your comments.

Also see #467

bradleyharden · 2021-10-17T05:27:45Z

@jbeaurivage, see the latest commit to address your comments

Make several improvements to the `sercom::v2::spi` module: - Introduce a `Registers` type that acts as a task-focused API for the registers, as opposed to the register-focused API of the PAC. This abstraction also serves to remove interior mutability of the PAC struct and lets us implement `Sync` for `Registers` - Add a `Capability` trait and three corresponding types: `Rx`, `Tx` and `Duplex`. Add a `Capability` type parameter to the `Spi` struct and use it to differentiate the various embedded HAL trait implementations. This is a better solution than the previous approach, which was a set of marker traits implemented on `Pads` types. - Combine the `thumbv6m` and `thumbv7em` modules to reuse more code. The major differences between the two chips come in the `Pads` type, and the `Length` and `CharSize` types. Introduce a `Size` trait that essentially acts a trait alias for either `Length` or `CharSize`. Split up the modules and use conditional imports to handle everything correctly for the three different chips. Because the `spi` module is no longer split between the two chip-specific modules, also consolidate the `impl_pad` modules as well. Finally, improve the documentation for the `spi` module itself, as well as for the embedded HAL trait implementations.

bradleyharden · 2021-10-17T05:50:40Z

I had to make some changes to metro_m4's Cargo.toml and run cargo update to get it to compile. Not sure what happened there.

bradleyharden · 2021-10-18T16:37:07Z

Whoops, I already know this is going to fail. I forgot to update the metro_m4 for the new Capability feature.

jbeaurivage · 2021-10-19T16:43:15Z

@bradleyharden, if you rebase to master and update the HAL/BSP changelogs, I'll be happy to merge.

bradleyharden · 2021-10-19T18:43:36Z

@jbeaurivage, done

Also see atsamd-rs#467

* Refactor `spi` modules to improve structure Make several improvements to the `sercom::v2::spi` module: - Introduce a `Registers` type that acts as a task-focused API for the registers, as opposed to the register-focused API of the PAC. This abstraction also serves to remove interior mutability of the PAC struct and lets us implement `Sync` for `Registers` - Add a `Capability` trait and three corresponding types: `Rx`, `Tx` and `Duplex`. Add a `Capability` type parameter to the `Spi` struct and use it to differentiate the various embedded HAL trait implementations. This is a better solution than the previous approach, which was a set of marker traits implemented on `Pads` types. - Combine the `thumbv6m` and `thumbv7em` modules to reuse more code. The major differences between the two chips come in the `Pads` type, and the `Length` and `CharSize` types. Introduce a `Size` trait that essentially acts a trait alias for either `Length` or `CharSize`. Split up the modules and use conditional imports to handle everything correctly for the three different chips. Because the `spi` module is no longer split between the two chip-specific modules, also consolidate the `impl_pad` modules as well. Finally, improve the documentation for the `spi` module itself, as well as for the embedded HAL trait implementations.

bradleyharden changed the title ~~Refactor v2 spi~~ Refactor spi modules to improve structure Jul 5, 2021

bradleyharden force-pushed the refactor_v2_spi branch 2 times, most recently from 3558a40 to 81176e2 Compare July 5, 2021 20:59

bradleyharden force-pushed the refactor_v2_spi branch from 81176e2 to 1f9a6cb Compare July 25, 2021 02:13

bradleyharden mentioned this pull request Jul 25, 2021

UART v2 #443

Merged

6 tasks

bradleyharden force-pushed the refactor_v2_spi branch 2 times, most recently from 8fc3a97 to 3d9294b Compare July 25, 2021 15:57

jbeaurivage reviewed Jul 28, 2021

View reviewed changes

bradleyharden force-pushed the refactor_v2_spi branch from 3d9294b to ccbde91 Compare September 8, 2021 02:57

bradleyharden mentioned this pull request Sep 8, 2021

Port pygamer to v2 #455

Closed

bradleyharden mentioned this pull request Oct 5, 2021

Add undocumented IOSet #506

Merged

3 tasks

bradleyharden pushed a commit that referenced this pull request Oct 10, 2021

Add undocumented IOSet (#506)

17afec4

Also see #467

bradleyharden force-pushed the refactor_v2_spi branch from ccbde91 to 79d68ef Compare October 17, 2021 05:27

bradleyharden marked this pull request as ready for review October 17, 2021 05:28

bradleyharden added 2 commits October 17, 2021 01:49

Address comments from @jbeaurivage

c7126b1

bradleyharden force-pushed the refactor_v2_spi branch from 79d68ef to c7126b1 Compare October 17, 2021 05:49

Merge branch 'master' into refactor_v2_spi

501d462

bradleyharden mentioned this pull request Oct 18, 2021

Make SPI Error support defmt::Format #522

Closed

3 tasks

bradleyharden added 3 commits October 18, 2021 12:45

Add missing spi::Capability for metro_m4

a2de3fe

Fix placement of AtomicSize constraint

c9c5d97

Merge branch 'master' into refactor_v2_spi

d930050

jbeaurivage approved these changes Oct 19, 2021

View reviewed changes

bradleyharden added 2 commits October 19, 2021 14:11

Merge branch 'master' into refactor_v2_spi

0f8d0af

Update changelogs

d62e3ca

Merge branch 'master' into refactor_v2_spi

5167bca

jbeaurivage merged commit 635c583 into atsamd-rs:master Oct 19, 2021

kaizensparc pushed a commit to kaizensparc/atsamd that referenced this pull request Dec 24, 2021

Add undocumented IOSet (atsamd-rs#506)

2c0c5df

Also see atsamd-rs#467

bradleyharden deleted the refactor_v2_spi branch January 30, 2022 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor spi modules to improve structure #467

Refactor spi modules to improve structure #467

bradleyharden commented Jul 5, 2021 •

edited

Loading

jbeaurivage Jul 28, 2021

bradleyharden Oct 17, 2021

bradleyharden Oct 17, 2021

jbeaurivage Jul 28, 2021

bradleyharden Oct 17, 2021

bradleyharden commented Sep 8, 2021

bradleyharden commented Oct 17, 2021

bradleyharden commented Oct 17, 2021

bradleyharden commented Oct 18, 2021

jbeaurivage commented Oct 19, 2021

bradleyharden commented Oct 19, 2021

Refactor spi modules to improve structure #467

Refactor spi modules to improve structure #467

Conversation

bradleyharden commented Jul 5, 2021 • edited Loading

jbeaurivage Jul 28, 2021

Choose a reason for hiding this comment

bradleyharden Oct 17, 2021

Choose a reason for hiding this comment

bradleyharden Oct 17, 2021

Choose a reason for hiding this comment

jbeaurivage Jul 28, 2021

Choose a reason for hiding this comment

bradleyharden Oct 17, 2021

Choose a reason for hiding this comment

bradleyharden commented Sep 8, 2021

bradleyharden commented Oct 17, 2021

bradleyharden commented Oct 17, 2021

bradleyharden commented Oct 18, 2021

jbeaurivage commented Oct 19, 2021

bradleyharden commented Oct 19, 2021

bradleyharden commented Jul 5, 2021 •

edited

Loading