Fix various cfg-related crimes #1774

cbiffle · 2024-05-06T21:18:03Z

The nightly toolchain started doing checking for cfgs on Sunday, so I bumped our toolchain revision locally to see what it'd find.

In short, it got big mad.

This PR fixes All The Things. The commits provide detailed descriptions, but here's a summary:

Boards (as in target_board) now have to be defined somewhere. I've added a boards directory containing (currently empty) config files per board. Any reference to a target_board without a corresponding file will now be treated as a typo.
Renamed some boards that only mentioned their processor type and not the board itself.
Removed vestiges of the psc-a and sidecar-a boards, which saw firmware support ended a while ago, but were never fully removed from the code.
Removed the final bits of STM32H7B3 devel board support, something we haven't used in ~4 years at this point.
Removed dead code implementing a traptrace feature in Sidecar, which isn't actually in the Cargo.toml and thus can't be built.
Fixed stm32xx-i2c, which was attempting to key off of target_board without actually exposing the cfg in its build script. Forgetting to do this is now a compiler warning.
Fixed jefe, which was attempting to key off of armv8m without actually exposing the cfg in its build script. Also now a warning.
Fixed build/i2c to use its own features instead of generating code that tried to match on features in its clients, which was working by accident.

In order to formally move us to a toolchain that prevents this from regressing, I had to do some small unrelated fixes, namely:

Stop using the core::uXX::MAX constants, which are deprecated now.
Bump a couple deps because their derive macros produced code the new compiler dislikes.
Add a missing syn feature in derive-idol-err, which had apparently been picking it up from some dependency.

These were named after the generic processor model, which isn't right.

This code can't be built, because we have no board support for PSC A anymore.

All of this code was statically unreachable and hasn't been built in... years, at this point. If we want it we can find it in version control.

There's no way to turn this on, so it definitely hasn't been tested in $PERIOD.

The trailing compile_error! will do fine, and this prevents it from matching on something that rustc thinks is a typo'd board name.

Jefe had a check that attempted to avoid building dump support on LPC55, but that check didn't work, because it was checking for the presence of one of our custom cfgs...but not doing the invocation in its build script that actually generates that cfg. rustc will start checking such things soon, which is how I found this.

This crate was generating code that required the _client crate_ to have certain features defined, which is almost certainly a mistake, since this crate has the same set of features. Changed to key off features in the code generator instead.

As of this commit, the following two classes of mistakes will be caught when using a recent rustc: 1. Detecting the armv6m, armv7m, or armv8m features without a build script that actually exposes them. 2. Misspelling a target_board.

We were apparently picking up this feature from a dep, which changed when I attempted a cargo update.

mkeeter · 2024-05-07T14:20:10Z

build/util/src/lib.rs

+
+    let values = boards.join(",");
+
+    println!("cargo:rustc-check-cfg=cfg(target_board, values({values}))");


it took me a while to figure out what was going on here, but it's amazing

mkeeter · 2024-05-07T14:30:54Z

drv/stm32xx-sys-api/src/h7.rs

-    #[cfg(feature = "h7b3")]
-    OctoSpi1 = periph(Group::Ahb3, 14), // B3 only
-    #[cfg(any(feature = "h743", feature = "h747", feature = "h753"))]
+    #[cfg(any(feature = "h743", feature = "h753"))]


Is any(feature = "h743", feature = "h753") guaranteed to be true? Now that we've removed h7b3 support, h743 and h753 are the only remaining h7* features, so I think we can remove this condition (and others below).

mkeeter · 2024-05-07T14:33:54Z

drv/stm32xx-sys-api/src/h7.rs

@@ -158,12 +151,9 @@ pub enum Peripheral {
    Swp = periph(Group::Apb1H, 2),
    Crsen = periph(Group::Apb1H, 1),

-    #[cfg(feature = "h7b3")]
-    Dfsdm1 = periph(Group::Apb2, 30), // B3 differs from 43/47
-
    Hrtim = periph(Group::Apb2, 29), // 43/47 only


I'm confused by the comments in this file – there are many lines which was 43/47 only but aren't gated on h743.

(we could also probably remove the references to the B3 and 47 more generally, since they're both unsupported)

mkeeter · 2024-05-07T14:35:26Z

README.mkdn

- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g031.toml` - stm32g031-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g070.toml` - stm32g070-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1-nucleo


Naming nit: should nucleo be at the start or end? We have both stm32g0b1-nucleo and nucleo-ih743zi2 board names.

mkeeter · 2024-05-07T14:37:00Z

task/hiffy/src/main.rs

@@ -71,7 +71,6 @@ cfg_if::cfg_if! {
        target_board = "gimlet-d",
        target_board = "gimlet-e",
        target_board = "gimlet-f",
-        target_board = "sidecar-a",


mkeeter · 2024-05-07T14:37:34Z

task/power/src/main.rs

-        target_board = "psc-b",
-        target_board = "psc-c"
-    ),
+    any(target_board = "psc-b", target_board = "psc-c"),
    path = "bsp/psc_abc.rs"


Nit: rename to psc_bc.rs now that rev A is gone?

mkeeter · 2024-05-07T14:38:50Z

Thanks for doing all of this cleanup!

With the toolchain bump, do you see any changes to stackmargin for our in-product images?

hawkw · 2024-05-07T15:19:10Z

boards/README.mkdn

hawkw · 2024-05-07T15:21:18Z

build/util/src/lib.rs

+            let Ok(dirent) = dirent else {
+                continue;
+            };


nit: should we maybe print a warning on weird/bad dir entries?

aapoalas

Few comments passing by

aapoalas · 2024-05-07T18:44:46Z

build/util/src/lib.rs

+        );
+    }
+    boards.sort();
+    boards.dedup();


nitpick: Duplicates would probably be a mistake of some sort, maybe worth a warning print as well?

aapoalas · 2024-05-07T18:51:06Z

drv/stm32xx-sys-api/src/h7.rs

    Hrtim = periph(Group::Apb2, 29), // 43/47 only

-    #[cfg(any(feature = "h743", feature = "h747", feature = "h757"))]
+    #[cfg(feature = "h743")]
    Dfsdm1 = periph(Group::Apb2, 28), // 43/47 differ from B3

    Sai3 = periph(Group::Apb2, 24), // 43/47 only


question: There's a bunch of "B3 only" commented lines below: Are those now entirely unused?

cbiffle · 2024-05-07T19:35:55Z

With the toolchain bump, do you see any changes to stackmargin for our in-product images?

I have not yet been able to test this. I'd be willing to remove the toolchain bump from this PR, but the changes may regress and I'd like to opt out of refixing them.

cbiffle force-pushed the cbiffle/cfg-check branch from ffe5ebd to af8d2f6 Compare May 6, 2024 21:21

Rename STM32G0 Nucleo boards to have "nucleo" in their name.

e28de34

These were named after the generic processor model, which isn't right.

cbiffle force-pushed the cbiffle/cfg-check branch 2 times, most recently from 09a9197 to 013ffeb Compare May 6, 2024 21:58

cbiffle added 4 commits May 6, 2024 15:12

Remove conditional code for unsupported psc-a rev.

5727bc5

This code can't be built, because we have no board support for PSC A anymore.

Remove final vestiges of STM32H7B3 support.

e15f4cd

All of this code was statically unreachable and hasn't been built in... years, at this point. If we want it we can find it in version control.

hiffy: remove dead sidecar-a code.

e666e57

sidecar: remove apparently dead traptrace feature.

1c4d93e

There's no way to turn this on, so it definitely hasn't been tested in $PERIOD.

cbiffle force-pushed the cbiffle/cfg-check branch 2 times, most recently from a212a56 to 1d3d2ad Compare May 6, 2024 22:16

cbiffle changed the title ~~Cbiffle/cfg check~~ Fix various cfg-related crimes May 6, 2024

cbiffle added 8 commits May 6, 2024 15:33

gimlet: drop cfg for gimlet-a

df762c8

The trailing compile_error! will do fine, and this prevents it from matching on something that rustc thinks is a typo'd board name.

stm32xx-i2c: expose target_board so cfgs work

c4806ac

build/i2c: use your own darn features

5ae8599

This crate was generating code that required the _client crate_ to have certain features defined, which is almost certainly a mistake, since this crate has the same set of features. Changed to key off features in the code generator instead.

Emit check-cfg lines for custom cfgs.

51851b5

As of this commit, the following two classes of mistakes will be caught when using a recent rustc: 1. Detecting the armv6m, armv7m, or armv8m features without a build script that actually exposes them. 2. Misspelling a target_board.

stop using deprecated core::xxx::MAX constants.

55d4ec6

derive-idol-error: add missing syn feature

4b45ae6

We were apparently picking up this feature from a dep, which changed when I attempted a cargo update.

Bump zeroize to fix "non-local impl" warning.

a992d63

cbiffle force-pushed the cbiffle/cfg-check branch from 1d3d2ad to 868fcf3 Compare May 6, 2024 22:33

Bump toolchain to 2024-05-05

52287f2

cbiffle force-pushed the cbiffle/cfg-check branch from 868fcf3 to 52287f2 Compare May 6, 2024 22:35

cbiffle marked this pull request as ready for review May 6, 2024 22:36

cbiffle requested review from flihp and labbott as code owners May 6, 2024 22:36

cbiffle requested a review from mkeeter May 6, 2024 22:36

mkeeter reviewed May 7, 2024

View reviewed changes

hawkw reviewed May 7, 2024

View reviewed changes

aapoalas reviewed May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various cfg-related crimes #1774

Fix various cfg-related crimes #1774

cbiffle commented May 6, 2024 •

edited

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter commented May 7, 2024

hawkw May 7, 2024

hawkw May 7, 2024

aapoalas left a comment

aapoalas May 7, 2024

aapoalas May 7, 2024

cbiffle commented May 7, 2024


		let values = boards.join(",");

		println!("cargo:rustc-check-cfg=cfg(target_board, values({values}))");

Fix various cfg-related crimes #1774

Are you sure you want to change the base?

Fix various cfg-related crimes #1774

Conversation

cbiffle commented May 6, 2024 • edited

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

mkeeter commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapoalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiffle commented May 7, 2024

cbiffle commented May 6, 2024 •

edited