Cleanup IOMUXC feature #75

mciantyre · 2020-07-18T20:49:40Z

The PR addresses the TODOs described in #73, updates documentation, adds tests, and describes how users should migrate their code.

The only new feature is that there's a convenience function for erasing all pad types. It turns the struct of strongly-typed pads into arrays of type-erased pads, with arrays named by their grouping (like ad_b0). Here's a comparison of how one access the same physical pad using the strongly-typed interface, and the type-erased interface:

let pads = unsafe { imxrt106x_iomuxc::Pads::new() };
use_pad(&pads.ad_b0.p13);

let erased_pads = pads.erase();
use_erased_pad(&erased_pads.ad_b0[13]);

Closes #26.

Reference documentation in sub-modules.

The empty enum cannot be made into an instance. This should indicate that, even though they're public, they're only type-level identifiers.

- remove ConfigBuilder; just use Config - add a mask that specifies which fields were changing - add documentation and examples - add a few unit tests

These tests are visually larger. I moved them into their own files so they're easier to study. This also gives us the opportunity to do some more advanced failure analysis, like a formal diff between the two files (today's error messages are hard to parse...).

Generate a top-level erase() method to perform cascading pad type erasure.

Rather than having a separate crate per chip implementation, I'm proposing that we have a single, imxrt-iomuxc crate. We expose chip-specific pads behind feature flags. I know this is the opposite of what I originally proposed, but hear me out. We won't require a feature flag, but a feature flags will be needed to access processor-specific pads. The interfaces are the only thing that we expose if we don't use a feature flag, so that's the same as using the imxrt-iomuxc crate by itself. Adding more and more feature flags is the same as depending on more and more crates. If we expand that through a user's dependency graph, it ends up being equivalent. But, managing a single crate with multiple feature flags is easier than managing multiple crates that effectively represent features. We will still compile all of the tests without feature flags. And, we can document everything by specifying '--all-features'. Finally, we can expose a simpler interface without the "if it's not documented, it's not public" clauses.

Check the imxrt-iomuxc crate separately from the HAL

mciantyre · 2020-07-19T13:53:13Z

In #72, we proposed that we should have an interface crate, imxrt-iomuxc, and then maintain processor-specific crates, like imxrt106x-iomuxc. The latest batch of commits re-thinks this approach. We now have a single imxrt-iomuxc crate that uses feature-flags to include processor-specific pads. I think it will be a simpler to maintain, and it doesn't impact our plans for a split HAL. We discuss the equivalence and trade-offs in 6ff4795.

This PR includes the refactor and IOMUXC consolidation.

Remove documentation notes about "crate family" or implementation crates. Fix some 'public but internal' visibility, and justify other #[doc(hidden)] attributes.

teburd · 2020-07-21T15:47:04Z

CONTRIBUTING.md

+The `imxrt-iomuxc` crate family does not require any feature flags, and it will build for your host. Consider using `--package` flags to build and test the crate family in one command:
+
+```
+cargo build --package=imxrt-iomuxc --package=imxrt106x-iomuxc --package=imxrt-iomuxc-build


Is this right now, since you've removed the imxrt106x-iomuxc crate?

teburd · 2020-07-21T15:48:55Z

imxrt-hal/CHANGELOG.md

+
+    ```rust
+    let mut uart = uarts.uart2.init(
+        peripherals.iomuxc.ad_b1.p02,


This is a really nice API now, I really like the way this call looks with the changes made here

teburd · 2020-07-21T15:49:48Z

imxrt-hal/CHANGELOG.md

+    should change to
+
+    ```rust
+    type HardwareFlag = hal::gpio::GPIO<hal::iomuxc::ad_b1::AD_B1_10, hal::gpio::Output>;


And again here, the resulting type and API is much nicer now

Not sure what I was thinking when putting together the previous GPIO types 😛they were gnarly! There's still more gnarly HAL code I need to rethink...

teburd · 2020-07-21T15:50:39Z

imxrt-hal/CHANGELOG.md

+    type HardwareFlag = hal::gpio::GPIO<hal::iomuxc::ad_b1::AD_B1_10, hal::gpio::Output>;
+    ```
+
+    The new GPIO types expose a no-return `toggle()` method, which shadows an embedded_hal


I suppose there's a reason we don't need to return here, its because its effectively infallible correct? Very nice if so

teburd · 2020-07-21T15:52:39Z

imxrt-iomuxc/README.md

+
+```toml
+[dependencies]
+imxrt106x-iomuxc = { version = "0.1", features = ["imxrt102x", "imxrt106x"] }


I believe this is now imxrt-ioxmuc correct?

teburd · 2020-07-21T15:53:09Z

imxrt-iomuxc/README.md

+
+```toml
+[dependencies]
+imxrt106x-iomuxc = { version = "0.1", features = ["imxrt106x"] }


I believe this should be imxrt-iomuxc

teburd · 2020-07-21T15:56:08Z

imxrt-iomuxc/src/i2c.rs


 /// Tag that indicates the SCL signal
-pub struct SCL;
+pub enum SCL {}


why the type change here?

It's one of a few ways to express a zero-sized type. But, this ZST can never (accidentally) be an object, unlike the current approach.

pub struct SCL; // 1

could be instantiated by crate users. Not a big deal, but it's not something they should do.

pub struct SCL(()); // 2

cannot be instantiated by crate users, since they can't see the inner unit. But, us imxrt-iomuxc maintainers can still create an instance of it inside our crate, then pass an instance of it through our API. Again, not something we should do, but we're not preventing it.

pub enum SCL {} // 3

cannot be instantiated outside our crate, or inside our crate, since there's no variants. It's the strongest signal of "don't make an instance," since it's intended be a type-level tag.

The only difference I can note is how rustdoc documents these type-level tags. They're either grouped as enums or structs. I've no preference here. If we prefer organizing type tags as structs, I'll recommend that we go to option // 2 to prevent users from creating needless SCL objects.

This makes sense to me, thanks for explaining. Enums seem fine to me

teburd · 2020-07-21T15:59:20Z

imxrt-iomuxc/src/config.rs

 #[repr(u32)]
 pub enum Hysteresis {
-    Enabled = 1 << 16,
-    Disabled = 0 << 16,
+    Enabled = 1 << HYSTERESIS_SHIFT,


Thank you for defining these consts, makes it easier to read

mciantyre added 8 commits July 17, 2020 20:29

Remove old HAL crate-level documentation

bfac5ef

Reference documentation in sub-modules.

iomuxc: use empty enums instead of structs

86d330c

The empty enum cannot be made into an instance. This should indicate that, even though they're public, they're only type-level identifiers.

Support modifications to pad configurations

88c735b

- remove ConfigBuilder; just use Config - add a mask that specifies which fields were changing - add documentation and examples - add a few unit tests

iomuxc: ensure pads are Send and not Sync

df6e75f

Update HAL CHANGELOG with IOMUXC migration tips

a510149

Update project README and CONTRIBUTING docs

906895b

Erase all processor pads from one function

b573249

Generate a top-level erase() method to perform cascading pad type erasure.

mciantyre requested a review from teburd July 18, 2020 20:49

mciantyre added 4 commits July 19, 2020 09:34

Update code generation tests for module support

1f2c62a

iomuxc: update README to justify consolidation

6ff4795

iomuxc: separate CI job from HAL

345272d

Check the imxrt-iomuxc crate separately from the HAL

iomuxc: remove hints of implementation crates

250848d

Remove documentation notes about "crate family" or implementation crates. Fix some 'public but internal' visibility, and justify other #[doc(hidden)] attributes.

mciantyre marked this pull request as ready for review July 19, 2020 18:37

teburd approved these changes Jul 21, 2020

View reviewed changes

mciantyre merged commit 250848d into master Jul 21, 2020

mciantyre deleted the iomuxc-cleanup branch July 21, 2020 23:44

mciantyre mentioned this pull request Aug 2, 2020

Support a safer IOMUXC interface #77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup IOMUXC feature #75

Cleanup IOMUXC feature #75

mciantyre commented Jul 18, 2020

mciantyre commented Jul 19, 2020

teburd Jul 21, 2020

teburd Jul 21, 2020

teburd Jul 21, 2020

mciantyre Jul 21, 2020

teburd Jul 21, 2020

teburd Jul 21, 2020

teburd Jul 21, 2020

teburd Jul 21, 2020

mciantyre Jul 21, 2020

teburd Jul 22, 2020

teburd Jul 21, 2020

Cleanup IOMUXC feature #75

Cleanup IOMUXC feature #75

Conversation

mciantyre commented Jul 18, 2020

mciantyre commented Jul 19, 2020

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