Fix System Pin Feature Gate #38

martinmortsell · 2022-10-07T14:35:47Z

This pull request addresses #37 .

Instead of allowing usage of system pins (which fails) without the reconfigurable-system-pins feature set, not having the feature set now removes the pins completely, and any peripheral that directly depend on them.

It would be nice to achieve compilation failures that are more descriptive than

error[E0609]: no field `pb4` on type `BankB`
  --> examples/blinky.rs:62:25
   | 
62 |         let pb4 = bankb.pb4.into_output(true);
   |                         ^^^ help: a field with a similar name exists: `pb0`

but I have no idea of how to achieve that.

tmplt · 2022-10-12T07:33:38Z

CHANGELOG.md

@@ -19,6 +19,7 @@
 ### Fixed

 - HAL build when targeting `sams70n19b`.
+- reconfigurable-system-pins feature.


I suggest adding some details.

tmplt · 2022-10-12T07:34:00Z

hal/src/serial/twi.rs

@@ -41,11 +41,13 @@ twi.write_read(0x0, &[0b1000_0000], &mut buffer).unwrap();
 use crate::clocks::{Clock, Hertz, HostClock, PeripheralIdentifier};
 use crate::ehal::blocking;
 use crate::generics;
+#[cfg(feature = "reconfigurable-system-pins")]
+use crate::pac::TWIHS1;


Why is this gated?

Nevermind. It's because the peripheral requires the system pins.

tmplt · 2022-10-12T07:34:27Z

hal/src/serial/usart/mod.rs

@@ -83,15 +83,17 @@ let _ = spi.read().unwrap();
 pub use super::uart::{ChannelMode, ParityMode, UartConfiguration};
 use crate::clocks::{Clock, Hertz, HostClock, Pck, Pck4, PeripheralIdentifier};
 use crate::generics::{self, Token};
+#[cfg(feature = "reconfigurable-system-pins")]
+use crate::pac::USART1;


tmplt · 2022-10-12T07:41:34Z

Looks logical from a cursory review. I'd double-check if we somehow break API here and lose any read-only features. Add some details to the CHANGELOG.md and see the commit history on how to format these; I'd like to keep them uniform. These changes can be squashed into a single commit.

Adding documentation that these peripherals are unavailable without the feature would also be good.

Twi and Usart Peripheral now both mention System Pins.

martinmortsell · 2022-10-13T15:36:47Z

Should the CI be updated to run both with and without the reconfigurable-system-pins feature?

tmplt · 2022-10-13T15:51:32Z

Yes; a feature matrix may be applicable.

michalfita · 2022-10-19T14:55:48Z

@martinmortsell Please fix issues (merge back development shall help) and we can merge this and release 0.4.1.

martinmortsell · 2022-10-19T15:34:38Z

My attempt to add a featrues matrix to the CI seems to have been unsuccesful, reattempting later.

martinmortsell · 2022-10-20T06:59:40Z

And there you have it.

martinmortsell · 2022-10-20T07:05:50Z

.github/workflows/build.yml

@@ -12,7 +12,7 @@ jobs:
      - id: boards
        run: echo "::set-output name=board_matrix::$(ls ./boards --indicator-style=none | jq -ncR '[inputs]')"
      - id: features
-        run: echo "::set-output name=feature_matrix::["reconfigurable-system-pins","usart-spi-host-without-select"]"
+        run: echo "::set-output name=feature_matrix::[\"reconfigurable-system-pins\",\"usart-spi-host-without-select\"]"


Having the features hard coded like this isn't pretty, but I have no idea how to parse the actual list of features and not catch the internal-only features.

There is probably a better way to check different feature permutations (and maybe streamline the CI further), but we can delay that until we have more feature gates. Ideally they will be de-coupled, so we could just check with --all-features, but I don't know if we can separate the PACs from this.

There a cargo extensions crate called cargo-hack that can do feature permutations.

Raised #42 about this.

tmplt

Fixup the changelog and clean up the commit history and I'll approve. Thanks!

tmplt · 2022-10-21T14:10:34Z

CHANGELOG.md

@@ -24,6 +24,8 @@
 ### Fixed

 - HAL build when targeting `sams70n19b`.
+- The special-on-reset pins `PB4/5/6/7/12` are now completely unavailable, if the `reconfigurable-system-pins` feature is not enabled.
+- `Usart<Usart1>` and `Twi<TwiHS1>` are now only available if the `reconfigurable-system-pins` feature is enabled, as they cannot obtain a valid pin configuration without it.


Move these under Changed.

tmplt · 2022-10-21T14:16:26Z

.github/workflows/build.yml

@@ -12,7 +12,7 @@ jobs:
      - id: boards
        run: echo "::set-output name=board_matrix::$(ls ./boards --indicator-style=none | jq -ncR '[inputs]')"
      - id: features
-        run: echo "::set-output name=feature_matrix::["reconfigurable-system-pins","usart-spi-host-without-select"]"
+        run: echo "::set-output name=feature_matrix::[\"reconfigurable-system-pins\",\"usart-spi-host-without-select\"]"


There is probably a better way to check different feature permutations (and maybe streamline the CI further), but we can delay that until we have more feature gates. Ideally they will be de-coupled, so we could just check with --all-features, but I don't know if we can separate the PACs from this.

michalfita · 2022-10-23T18:45:11Z

@martinmortsell I can't push to this branch myself. You have to not only move the section to ## Changed but up to # Unreleased, as v0.4.0 has sailed already.

tmplt · 2022-10-24T14:12:24Z

Changes LGTM. Squash into a single commit to keep the history tidy.

martinmortsell · 2022-10-24T14:14:16Z

Changes LGTM. Squash into a single commit to keep the history tidy.

But is the squash not done as part of the merge?

tmplt · 2022-10-24T14:19:15Z

But is the squash not done as part of the merge?

Yes, but I do not have merging rights, so I can only state my wishes.

martinmortsell · 2022-10-24T14:20:13Z

Ok, fair.

martinmortsell added 2 commits October 7, 2022 16:25

Properly Feature Gate Usage of System Pins

028882a

RustFmt and Changelog

ebe7736

tmplt requested changes Oct 12, 2022

View reviewed changes

Update Documentation, Clarify CHANGELOG

9db3119

Twi and Usart Peripheral now both mention System Pins.

martinmortsell mentioned this pull request Oct 13, 2022

CI is Broken #40

Closed

martinmortsell added 2 commits October 19, 2022 17:02

Merge branch 'development' into fix/reconfigurable-system-pins

99d2191

Add Feature Matrix To CI

bb35b27

martinmortsell added 3 commits October 20, 2022 08:14

Reattempt Add feature Matrix To CI

878c615

Fix Quotes Around Features

abbb0c0

Fix Feature Matrix Generation

dc6a27c

martinmortsell commented Oct 20, 2022

View reviewed changes

tmplt requested changes Oct 21, 2022

View reviewed changes

michalfita mentioned this pull request Oct 23, 2022

Introduce feature permutations in CI #42

Open

Fix Changelog

b364e40

michalfita merged commit 809ac70 into atsams-rs:development Oct 24, 2022

michalfita mentioned this pull request Oct 24, 2022

Configuring System IO Pins Fails Silently When Feature Is Not Used #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix System Pin Feature Gate #38

Fix System Pin Feature Gate #38

martinmortsell commented Oct 7, 2022 •

edited

Loading

tmplt Oct 12, 2022

tmplt Oct 12, 2022

tmplt Oct 12, 2022

tmplt Oct 12, 2022

tmplt commented Oct 12, 2022

martinmortsell commented Oct 13, 2022

tmplt commented Oct 13, 2022

michalfita commented Oct 19, 2022

martinmortsell commented Oct 19, 2022

martinmortsell commented Oct 20, 2022

martinmortsell Oct 20, 2022

tmplt Oct 21, 2022

michalfita Oct 23, 2022

tmplt left a comment

tmplt Oct 21, 2022

tmplt Oct 21, 2022

michalfita commented Oct 23, 2022

tmplt commented Oct 24, 2022

martinmortsell commented Oct 24, 2022

tmplt commented Oct 24, 2022

martinmortsell commented Oct 24, 2022

Fix System Pin Feature Gate #38

Fix System Pin Feature Gate #38

Conversation

martinmortsell commented Oct 7, 2022 • edited Loading

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

tmplt commented Oct 12, 2022

martinmortsell commented Oct 13, 2022

tmplt commented Oct 13, 2022

michalfita commented Oct 19, 2022

martinmortsell commented Oct 19, 2022

martinmortsell commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmplt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalfita commented Oct 23, 2022

tmplt commented Oct 24, 2022

martinmortsell commented Oct 24, 2022

tmplt commented Oct 24, 2022

martinmortsell commented Oct 24, 2022

martinmortsell commented Oct 7, 2022 •

edited

Loading