Add a lint-packages subcommand to xtask, use in CI workflow (#1594)

* Add a subcommand to lint all packages using `clippy`, use in CI

* Address `clippy` warnings

.github/workflows/ci.yml

@@ -8,8 +8,6 @@
 # 1a.) If the device has a low-power core (which is supported in
 #      `esp-lp-hal`), then update the `if` condition to build prerequisites.
 # 2.) In the 'msrv-riscv' job, add checks as needed for the new chip.
-# 3.) In the 'clippy-riscv' job, add checks as needed for the new chip.
-# 3.) In the 'rustfmt' job, add checks as needed for the new chip.
 
 name: CI
 
@@ -224,47 +222,23 @@ jobs:
           cargo xtask build-package --toolchain=esp --features=esp32s3 --target=xtensa-esp32s3-none-elf esp-hal
 
   # --------------------------------------------------------------------------
-  # Lint
+  # Lint & Format
 
   clippy:
     runs-on: ubuntu-latest
 
     steps:
       - uses: actions/checkout@v4
+      # We use the 'esp' toolchain for *all* targets, in order to get a
+      # semi-stable and consistent set of lints for all targets:
       - uses: esp-rs/xtensa-toolchain@v1.5
         with:
           default: true
           ldproxy: false
       - uses: Swatinem/rust-cache@v2
 
-      ## esp-hal:
+      # Lint all packages:
-      - name: clippy (esp-hal, esp32)
+      - run: cargo xtask lint-packages
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32 --target=xtensa-esp32-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32c2)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32c2 --target=riscv32imc-unknown-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32c3)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32c3 --target=riscv32imc-unknown-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32c6)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32c6 --target=riscv32imac-unknown-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32h2)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32h2 --target=riscv32imac-unknown-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32s2)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32s2 --target=xtensa-esp32s2-none-elf -- -D warnings
-      - name: clippy (esp-hal, esp32s3)
-        run: cd esp-hal && cargo clippy -Zbuild-std=core --features=esp32s3 --target=xtensa-esp32s3-none-elf -- -D warnings
-      ## esp-hal-smartled:
-      - name: clippy (esp-hal-smartled)
-        run: cd esp-hal-smartled && cargo clippy -Zbuild-std=core --features=esp32c6 --target=riscv32imac-unknown-none-elf -- -D warnings
-      ## esp-lp-hal:
-      - name: clippy (esp-lp-hal, esp32c6)
-        run: cd esp-lp-hal && cargo clippy -Zbuild-std=core --features=esp32c6 --target=riscv32imac-unknown-none-elf -- -D warnings
-      - name: clippy (esp-lp-hal, esp32s2)
-        run: cd esp-lp-hal && cargo clippy -Zbuild-std=core --features=esp32s2 --target=riscv32imc-unknown-none-elf -- -D warnings
-      - name: clippy (esp-lp-hal, esp32s3)
-        run: cd esp-lp-hal && cargo clippy -Zbuild-std=core --features=esp32s3 --target=riscv32imc-unknown-none-elf -- -D warnings
-      # esp-riscv-rt:
-      - name: clippy (esp-riscv-rt)
-        run: cd esp-riscv-rt && cargo clippy -Zbuild-std=core --target=riscv32imc-unknown-none-elf -- -D warnings
 
   rustfmt:
     runs-on: ubuntu-latest

esp-backtrace/src/lib.rs

@@ -1,9 +1,9 @@
-#![no_std]
+#![allow(rustdoc::bare_urls, unused_macros)]
+#![cfg_attr(nightly, feature(panic_info_message))]
 #![cfg_attr(target_arch = "xtensa", feature(asm_experimental_arch))]
-#![allow(rustdoc::bare_urls)]
 #![doc = include_str!("../README.md")]
 #![doc(html_logo_url = "https://avatars.githubusercontent.com/u/46717278")]
-#![cfg_attr(nightly, feature(panic_info_message))]
+#![no_std]
 
 #[cfg(feature = "defmt")]
 use defmt as _;
@@ -288,8 +288,11 @@ fn is_valid_ram_address(address: u32) -> bool {
 ))]
 #[allow(unused)]
 fn halt() -> ! {
-    loop {}
+    loop {
+        continue;
+    }
 }
+
 #[cfg(feature = "custom-halt")]
 fn halt() -> ! {
     extern "Rust" {

esp-backtrace/src/riscv.rs

@@ -7,6 +7,7 @@ use crate::MAX_BACKTRACE_ADDRESSES;
 // we get better results (especially if the caller was the last function in the
 // calling function) if we report the address of the JALR itself
 // even if it was a C.JALR we should get good results using RA - 4
+#[allow(unused)]
 pub(super) const RA_OFFSET: usize = 4;
 
 /// Registers saved in trap handler

esp-build/src/lib.rs

@@ -146,7 +146,7 @@ ERROR: expected exactly one enabled feature from feature group:
 fn do_alert(color: Color, input: TokenStream) -> TokenStream {
     let message = parse_macro_input!(input as LitStr).value();
 
-    let ref mut stderr = StandardStream::stderr(ColorChoice::Auto);
+    let stderr = &mut StandardStream::stderr(ColorChoice::Auto);
     let color_spec = ColorSpec::new().set_fg(Some(color)).clone();
 
     let mut has_nonspace = false;
@@ -196,7 +196,7 @@ fn split_heading(s: &str) -> (Option<&str>, &str) {
     }
 }
 
-fn unique_pairs(features: &Vec<LitStr>) -> Vec<(&LitStr, &LitStr)> {
+fn unique_pairs(features: &[LitStr]) -> Vec<(&LitStr, &LitStr)> {
     let mut pairs = Vec::new();
 
     let mut i = 0;

esp-ieee802154/src/hal.rs

@@ -277,7 +277,7 @@ pub(crate) fn set_multipan_enable_mask(mask: u8) {
 #[inline(always)]
 pub(crate) fn set_multipan_panid(index: MultipanIndex, panid: u16) {
     unsafe {
-        let pan_id = (&*IEEE802154::PTR)
+        let pan_id = (*IEEE802154::PTR)
             .inf0_pan_id()
             .as_ptr()
             .offset(4 * index as isize);
@@ -288,7 +288,7 @@ pub(crate) fn set_multipan_panid(index: MultipanIndex, panid: u16) {
 #[inline(always)]
 pub(crate) fn set_multipan_short_addr(index: MultipanIndex, value: u16) {
     unsafe {
-        let short_addr = (&*IEEE802154::PTR)
+        let short_addr = (*IEEE802154::PTR)
             .inf0_short_addr()
             .as_ptr()
             .offset(4 * index as isize);
@@ -299,7 +299,7 @@ pub(crate) fn set_multipan_short_addr(index: MultipanIndex, value: u16) {
 #[inline(always)]
 pub(crate) fn set_multipan_ext_addr(index: MultipanIndex, ext_addr: *const u8) {
     unsafe {
-        let mut ext_addr_ptr = (&*IEEE802154::PTR)
+        let mut ext_addr_ptr = (*IEEE802154::PTR)
             .inf0_extend_addr0()
             .as_ptr()
             .offset(4 * index as isize);

esp-ieee802154/src/lib.rs

@@ -112,8 +112,8 @@ impl<'a> Ieee802154<'a> {
         Self {
             _align: 0,
             transmit_buffer: [0u8; FRAME_SIZE],
-            _phantom1: PhantomData::default(),
+            _phantom1: PhantomData,
-            //_phantom2: PhantomData::default(),
+            //_phantom2: PhantomData,
         }
     }
 
@@ -206,7 +206,7 @@ impl<'a> Ieee802154<'a> {
             .unwrap();
         self.transmit_buffer[0] = (offset - 1) as u8;
 
-        ieee802154_transmit(self.transmit_buffer.as_ptr() as *const u8, false); // what about CCA?
+        ieee802154_transmit(self.transmit_buffer.as_ptr(), false); // what about CCA?
 
         Ok(())
     }
@@ -216,7 +216,7 @@ impl<'a> Ieee802154<'a> {
         self.transmit_buffer[1..][..frame.len()].copy_from_slice(frame);
         self.transmit_buffer[0] = frame.len() as u8;
 
-        ieee802154_transmit(self.transmit_buffer.as_ptr() as *const u8, false); // what about CCA?
+        ieee802154_transmit(self.transmit_buffer.as_ptr(), false); // what about CCA?
 
         Ok(())
     }
@@ -266,7 +266,7 @@ impl<'a> Ieee802154<'a> {
     pub fn set_rx_available_callback_fn(&mut self, callback: fn()) {
         critical_section::with(|cs| {
             let mut rx_available_callback_fn = RX_AVAILABLE_CALLBACK_FN.borrow_ref_mut(cs);
-            rx_available_callback_fn.replace(unsafe { core::mem::transmute(callback) });
+            rx_available_callback_fn.replace(callback);
         });
     }
 
@@ -304,8 +304,10 @@ static TX_DONE_CALLBACK: Mutex<RefCell<Option<&'static mut (dyn FnMut() + Send)>
 static RX_AVAILABLE_CALLBACK: Mutex<RefCell<Option<&'static mut (dyn FnMut() + Send)>>> =
     Mutex::new(RefCell::new(None));
 
+#[allow(clippy::type_complexity)]
 static TX_DONE_CALLBACK_FN: Mutex<RefCell<Option<fn()>>> = Mutex::new(RefCell::new(None));
 
+#[allow(clippy::type_complexity)]
 static RX_AVAILABLE_CALLBACK_FN: Mutex<RefCell<Option<fn()>>> = Mutex::new(RefCell::new(None));
 
 fn tx_done() {

esp-ieee802154/src/pib.rs

@@ -222,7 +222,7 @@ fn ieee802154_set_multipan_hal(pib: &Pib) {
         if (pib.multipan_mask & (1 << index)) != 0 {
             set_multipan_panid(index.into(), pib.panid[index]);
             set_multipan_short_addr(index.into(), pib.short_addr[index]);
-            set_multipan_ext_addr(index.into(), pib.ext_addr[index].as_ptr() as *const u8);
+            set_multipan_ext_addr(index.into(), pib.ext_addr[index].as_ptr());
         }
     }
 }

esp-ieee802154/src/raw.rs

@@ -254,7 +254,7 @@ fn stop_current_operation() {
 
 fn set_next_rx_buffer() {
     unsafe {
-        set_rx_addr(RX_BUFFER.as_mut_ptr() as *mut u8);
+        set_rx_addr(RX_BUFFER.as_mut_ptr());
     }
 }
 
@@ -329,7 +329,7 @@ fn ieee802154_sec_update() {
 
 fn next_operation() {
     let previous_operation = critical_section::with(|cs| {
-        let state = STATE.borrow_ref(cs).clone();
+        let state = *STATE.borrow_ref(cs);
 
         if ieee802154_pib_get_rx_when_idle() {
             enable_rx();

esp-metadata/src/lib.rs

@@ -1,13 +1,13 @@
 //! Metadata for Espressif devices, primarily intended for use in build scripts.
 
-const ESP32_TOML: &'static str = include_str!("../devices/esp32.toml");
+const ESP32_TOML: &str = include_str!("../devices/esp32.toml");
-const ESP32C2_TOML: &'static str = include_str!("../devices/esp32c2.toml");
+const ESP32C2_TOML: &str = include_str!("../devices/esp32c2.toml");
-const ESP32C3_TOML: &'static str = include_str!("../devices/esp32c3.toml");
+const ESP32C3_TOML: &str = include_str!("../devices/esp32c3.toml");
-const ESP32C6_TOML: &'static str = include_str!("../devices/esp32c6.toml");
+const ESP32C6_TOML: &str = include_str!("../devices/esp32c6.toml");
-const ESP32H2_TOML: &'static str = include_str!("../devices/esp32h2.toml");
+const ESP32H2_TOML: &str = include_str!("../devices/esp32h2.toml");
-const ESP32P4_TOML: &'static str = include_str!("../devices/esp32p4.toml");
+const ESP32P4_TOML: &str = include_str!("../devices/esp32p4.toml");
-const ESP32S2_TOML: &'static str = include_str!("../devices/esp32s2.toml");
+const ESP32S2_TOML: &str = include_str!("../devices/esp32s2.toml");
-const ESP32S3_TOML: &'static str = include_str!("../devices/esp32s3.toml");
+const ESP32S3_TOML: &str = include_str!("../devices/esp32s3.toml");
 
 lazy_static::lazy_static! {
     static ref ESP32_CFG: Config = basic_toml::from_str(ESP32_TOML).unwrap();

xtask/src/lib.rs

@@ -15,7 +15,7 @@ use self::cargo::CargoArgsBuilder;
 
 pub mod cargo;
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Display, EnumIter, ValueEnum)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Display, EnumIter, ValueEnum)]
 #[strum(serialize_all = "kebab-case")]
 pub enum Package {
     EspAlloc,

xtask/src/main.rs

@@ -34,6 +34,8 @@ enum Cli {
     FmtPackages(FmtPackagesArgs),
     /// Generate the eFuse fields source file from a CSV.
     GenerateEfuseFields(GenerateEfuseFieldsArgs),
+    /// Lint all packages in the workspace with clippy
+    LintPackages(LintPackagesArgs),
     /// Run the given example for the specified chip.
     RunExample(ExampleArgs),
     /// Run all applicable tests or the specified test for a specified chip.
@@ -122,6 +124,9 @@ struct GenerateEfuseFieldsArgs {
     chip: Chip,
 }
 
+#[derive(Debug, Args)]
+struct LintPackagesArgs {}
+
 #[derive(Debug, Args)]
 struct RunElfArgs {
     /// Which chip to run the tests for.
@@ -150,6 +155,7 @@ fn main() -> Result<()> {
         Cli::BumpVersion(args) => bump_version(&workspace, args),
         Cli::FmtPackages(args) => fmt_packages(&workspace, args),
         Cli::GenerateEfuseFields(args) => generate_efuse_src(&workspace, args),
+        Cli::LintPackages(args) => lint_packages(&workspace, args),
         Cli::RunElfs(args) => run_elfs(args),
         Cli::RunExample(args) => examples(&workspace, args, CargoAction::Run),
         Cli::RunTests(args) => tests(&workspace, args, CargoAction::Run),
@@ -424,6 +430,94 @@ fn fmt_packages(workspace: &Path, args: FmtPackagesArgs) -> Result<()> {
     Ok(())
 }
 
+fn lint_packages(workspace: &Path, _args: LintPackagesArgs) -> Result<()> {
+    let mut packages = Package::iter().collect::<Vec<_>>();
+    packages.sort();
+
+    for package in packages {
+        let path = workspace.join(package.to_string());
+
+        // Unfortunately each package has its own unique requirements for
+        // building, so we need to handle each individually (though there
+        // is *some* overlap)
+
+        match package {
+            Package::EspBacktrace => lint_package(
+                &path,
+                &[
+                    "-Zbuild-std=core",
+                    "--no-default-features",
+                    "--target=riscv32imc-unknown-none-elf",
+                    "--features=esp32c6,defmt",
+                ],
+            )?,
+
+            Package::EspHal => {
+                // Since different files/modules can be included/excluded
+                // depending on the target, we must lint *all* targets:
+                for chip in Chip::iter() {
+                    lint_package(
+                        &path,
+                        &[
+                            "-Zbuild-std=core",
+                            &format!("--target={}", chip.target()),
+                            &format!("--features={chip}"),
+                        ],
+                    )?;
+                }
+            }
+
+            Package::EspHalProcmacros | Package::EspRiscvRt => lint_package(
+                &path,
+                &["-Zbuild-std=core", "--target=riscv32imc-unknown-none-elf"],
+            )?,
+
+            Package::EspHalSmartled | Package::EspIeee802154 | Package::EspLpHal => lint_package(
+                &path,
+                &[
+                    "-Zbuild-std=core",
+                    "--target=riscv32imac-unknown-none-elf",
+                    "--features=esp32c6",
+                ],
+            )?,
+
+            Package::EspPrintln => lint_package(
+                &path,
+                &[
+                    "-Zbuild-std=core",
+                    "--target=riscv32imc-unknown-none-elf",
+                    "--features=esp32c6",
+                ],
+            )?,
+
+            // We will *not* check the following packages with `clippy`; this
+            // may or may not change in the future:
+            Package::Examples | Package::HilTest => {}
+
+            // By default, no `clippy` arguments are required:
+            _ => lint_package(&path, &[])?,
+        }
+    }
+
+    Ok(())
+}
+
+fn lint_package(path: &Path, args: &[&str]) -> Result<()> {
+    log::info!("Linting package: {}", path.display());
+
+    let mut builder = CargoArgsBuilder::default()
+        .toolchain("esp")
+        .subcommand("clippy"); // TODO: Is this still actually required?
+
+    for arg in args {
+        builder = builder.arg(arg.to_string());
+    }
+
+    let cargo_args = builder.arg("--").arg("-D").arg("warnings").build();
+
+    xtask::cargo::run(&cargo_args, &path)
+}
+
 fn run_elfs(args: RunElfArgs) -> Result<()> {
     let mut failed: Vec<String> = Vec::new();
     for elf in fs::read_dir(&args.path)? {