Add I2S/SAI pad info #3

lkolbly · 2021-04-03T20:04:48Z

This adds traits and impls for the SAI peripherals. (also, it updates the Cargo.toml to point at this repo)

This is very 106x-specific, should I wrap it in a cfg(feature = "imxrt106x")? That's the only feature in the crate (on the v0.1 branch), so I'm not sure. The 1010 I believe does have some (2?) SAI peripherals, and the pads are almost certainly not the same.

SAI3 overlaps on several pins with SAI1, so adding it complains about re-implementing a few traits. There may be a clever way around this (in particular, the traits could be split into separate e.g. RxSyncPin and RxBclkPin traits), I can do that if we want but I don't feel a pressing need to do so right now. Also the daisy names are different for some reason.

Testing-wise, I've only tested this with SAI1 in one-bit TX mode, with a specific set of pins. I tried to copy the table as closely as possible.

teburd · 2021-04-05T14:08:39Z

Really happy to see this happening!

teburd

Looks pretty good generally, I need to look closer at the SAI pads and alts from the reference doc

Cargo.toml

daisy.py

lkolbly · 2021-04-05T14:16:20Z

I need to look closer at the SAI pads and alts from the reference doc

Please do and take your time, the 3s and the 8s blend together on my screen. :D

mciantyre

Great start! A few questions and requests.

Can we target the main branch for this PR? We don't have to add 1010 SAI pins right now, but we should design knowing that those pins will eventually be implemented. I'll take care of backporting to the 0.1 branch.

SAI3 overlaps on several pins with SAI1, so adding it complains about re-implementing a few traits.

The ADC pins had the same issue. Check out the adc module to see how we solved it: the Pin trait is generic over an ADC module. Instead of using custom types for SAI[1, 2, 3] (analogous to ADC1 and ADC2), let's try using typenum types to express the SAI instance. If my thinking is correct, it might work nicely with changes we're considering in the RAL.

There may be a clever way around this (in particular, the traits could be split into separate e.g. RxSyncPin and RxBclkPin traits)

At the moment, I lean towards a single Pin trait to maintain consistency with the rest of the crate. But if we find that multiple traits is cleaner or easier for the user, I'm happy to support it.

Auto-generating the SAI implementations in build.rs diverges with the rest of the pin implementations. I love how the tables are easy to read and review, but I'm not sure it warrants the complexity of build-time generated code. Could we compromise with declarative macros to implement the pins? If auto-generated code is the way to go, let's move the types and routines into the imxrt-iomuxc-build crate, which will make it easier to test (now or later).

I took your branch and started playing around with these ideas. Check out the sai-i2s branch if you want to see what it looks like.

mciantyre · 2021-04-05T15:50:13Z

src/sai.rs

+pub trait TxDataPin: super::IOMUX {
+    const ALT: u32;
+    type Module: super::consts::Unsigned;
+    type Index: super::consts::Unsigned;
+}
+
+pub trait RxDataPin: super::IOMUX {
+    const ALT: u32;
+    type Module: super::consts::Unsigned;
+    type Index: super::consts::Unsigned;
+}


Do RxDataPin and TxDataPin need an associated daisy register? I see constants like DAISY_SAI1_RX_DATA3_SD_B1_02, which seem to imply additional pin setup.

(Since RX_DATA_03 and TX_DATA_01 are multiplexed on the same pad, I'm guessing that the daisy register applies to both, despite only seeing "RX_DATA" in the name. Let me know if this isn't right!)

Some signals, like SAI1's TX_DATA_00, don't have daisy registers. Consider using an Option if that's the case.

I'm guessing that the daisy register applies to both

I don't think so, since I think the daisy registers only apply to inputs so they wouldn't apply to TX.

RxDataPin does have daisy registers though, good catch. I think it has one for every pin.

mciantyre · 2021-04-05T15:50:43Z

src/sai.rs

+pub trait TxDataPin: super::IOMUX {
+    const ALT: u32;
+    type Module: super::consts::Unsigned;
+    type Index: super::consts::Unsigned;


Could we use a const INDEX: [u32|usize] here, instead of a type number? Comment also applies to RxDataPin::Index.

Hmm, I'm not sure you can. Ideally, I think the HAL implementation would look something like this:

pub fn build_1bit_tx<MCLK, BCLK, SYNC, DATA>( self, clock_config: ClockConfig, mut mclk: MCLK, mut bclk: BCLK, mut sync: SYNC, mut data: DATA, ) -> SAI<M> where MCLK: sai::Pin<Module = M, Signal = sai::MCLK2>, BCLK: sai::Pin<Module = M, Signal = sai::TX_BCLK>, SYNC: sai::Pin<Module = M, Signal = sai::TX_SYNC>, DATA: sai::TxDataPin<Module = M, Index = U0>,

And in particular, the data pin arguments should only accept specific indexes, and I can't get the type bound to work with associated constants.

Looks good to me! Yea, I hit a similar limitation when trying to move this entire crate to const generics. Let's stick with the typenums.

Type bounds are a very good reason to keep the typenums, I agree!

mciantyre · 2021-04-05T15:57:17Z

src/sai.rs

+pub trait Signal: private::Sealed {}
+
+/// A tag that indicates a SAI TX bit clock pad
+pub enum TX_BCLK {}


Let's start heeding these clippy naming warnings. I pushed a commit that addresses the CI failures. And I'm thinking about a large rename before the 0.2 release...

For the 0.1 release, it's OK with me to have inconsistent names, particularly if it will save us and others refactoring time in the future.

mciantyre · 2021-04-05T16:07:01Z

src/sai.rs

+pub fn prepare_rxdata<P: RxDataPin>(pin: &mut P) {
+    super::alternate(pin, P::ALT);
+    super::set_sion(pin);
+}


This discussion indicates that we might need to disable the drive strength configuration when using the SAI1 multi-channel pins. Have we seen that need in practice?

Hm, I have not tested SAI input (or even multi-line output), so I'm not really in a position to easily test it. It's not clear to me whether that behaviour would transfer over to the 106x series, since ostensibly the 1010 has no daisy chain registers so the configuration scheme could be different.

Let's keep this as-is. Once merged, we'll track our uncertainty / future testing with an issue.

mciantyre · 2021-04-05T16:47:27Z

build.rs

+    generate_sai(std::fs::File::create(out_dir.join("sai_pads.rs"))?)?;
+
    #[cfg(feature = "imxrt106x")]
    imxrt106x(fs::File::create(out_dir.join("imxrt106x.rs"))?)?;


Let's namespace this SAI code underneath the auto-generated 106x code, and behind the "imxrt106x" feature flag. It'll make porting to the main branch easier.

lkolbly · 2021-04-11T15:37:38Z

I took your branch and started playing around with these ideas. Check out the sai-i2s branch if you want to see what it looks like.

Awesome, that branch looks pretty reasonable to me! I agree that the macro is cleaner than a build script.

I think we will need associated types for the indexes though, rather than consts.

mciantyre · 2021-04-13T12:39:30Z

Agreed on keeping typenums for data line indices! Pushed a corresponding commit to the sai-i2s branch in this remote.

mciantyre

LGTM! Thanks for working through this. I'll leave this open for @BFrog to take a look. Unless we need more time to review, I'll merge after a week, and release 0.1.3 with these changes.

teburd

This looks pretty darn solid to me at this point, again I didn't look too deeply at the pad and iomux values, but more broadly at the macro, api, and such

Update Cargo.toml repo and daisy usage

c274e94

teburd reviewed Apr 5, 2021

View reviewed changes

Cargo.toml Show resolved Hide resolved

daisy.py Show resolved Hide resolved

mciantyre reviewed Apr 5, 2021

View reviewed changes

mciantyre mentioned this pull request Apr 13, 2021

ADC: favor typenum instances, associated constants #5

Merged

lkolbly force-pushed the sai branch from e728416 to 6116b98 Compare April 18, 2021 13:41

lkolbly changed the base branch from v0.1 to master April 18, 2021 13:41

Implement SAI pads

164bdba

lkolbly force-pushed the sai branch from e549fde to 164bdba Compare April 18, 2021 13:57

mciantyre approved these changes Apr 19, 2021

View reviewed changes

mciantyre added this to the 0.1.3 milestone Apr 19, 2021

teburd approved these changes Apr 21, 2021

View reviewed changes

mciantyre merged commit 164bdba into imxrt-rs:master Apr 24, 2021

mciantyre mentioned this pull request Apr 24, 2021

Preparing SAI pads #6

Open

mciantyre mentioned this pull request Aug 7, 2021

SAI/I2S Support imxrt-rs/imxrt-hal#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add I2S/SAI pad info #3

Add I2S/SAI pad info #3

lkolbly commented Apr 3, 2021

teburd commented Apr 5, 2021

teburd left a comment

lkolbly commented Apr 5, 2021

mciantyre left a comment

mciantyre Apr 5, 2021

lkolbly Apr 11, 2021

mciantyre Apr 5, 2021

lkolbly Apr 11, 2021

mciantyre Apr 13, 2021

teburd Apr 13, 2021

mciantyre Apr 5, 2021

mciantyre Apr 5, 2021

lkolbly Apr 11, 2021

mciantyre Apr 13, 2021

mciantyre Apr 5, 2021

lkolbly commented Apr 11, 2021

mciantyre commented Apr 13, 2021

mciantyre left a comment

teburd left a comment

Add I2S/SAI pad info #3

Add I2S/SAI pad info #3

Conversation

lkolbly commented Apr 3, 2021

teburd commented Apr 5, 2021

teburd left a comment

Choose a reason for hiding this comment

lkolbly commented Apr 5, 2021

mciantyre 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkolbly commented Apr 11, 2021

mciantyre commented Apr 13, 2021

mciantyre left a comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment