From f6acd98d7a6b04a588230b9dd0771339394e88fb Mon Sep 17 00:00:00 2001 From: Jesse Braham Date: Fri, 24 May 2024 09:27:59 -0700 Subject: [PATCH 1/2] Add a subcommand to lint all packages using `clippy`, use in CI --- .github/workflows/ci.yml | 36 +++------------ xtask/src/lib.rs | 2 +- xtask/src/main.rs | 94 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a42a50ed71c..f1f4387004c 100644 --- a/.github/workflows/ci.yml +++ b/.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 @@ -218,47 +216,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: - - name: clippy (esp-hal, esp32) - 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 + # Lint all packages: + - run: cargo xtask lint-packages rustfmt: runs-on: ubuntu-latest diff --git a/xtask/src/lib.rs b/xtask/src/lib.rs index 72af6bc0d2d..005bdbb8560 100644 --- a/xtask/src/lib.rs +++ b/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, diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 9d3b46fb19f..99b0c99d514 100644 --- a/xtask/src/main.rs +++ b/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::>(); + 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 = Vec::new(); for elf in fs::read_dir(&args.path)? { From 6c80205874f5b3231b5d5ddf718efad2031ac3e3 Mon Sep 17 00:00:00 2001 From: Jesse Braham Date: Fri, 24 May 2024 09:31:21 -0700 Subject: [PATCH 2/2] Address `clippy` warnings --- esp-backtrace/src/lib.rs | 11 +++++++---- esp-backtrace/src/riscv.rs | 1 + esp-build/src/lib.rs | 4 ++-- esp-ieee802154/src/hal.rs | 6 +++--- esp-ieee802154/src/lib.rs | 12 +++++++----- esp-ieee802154/src/pib.rs | 2 +- esp-ieee802154/src/raw.rs | 4 ++-- esp-metadata/src/lib.rs | 16 ++++++++-------- 8 files changed, 31 insertions(+), 25 deletions(-) diff --git a/esp-backtrace/src/lib.rs b/esp-backtrace/src/lib.rs index 2608c1bc4b9..64b4f9c64ab 100644 --- a/esp-backtrace/src/lib.rs +++ b/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" { diff --git a/esp-backtrace/src/riscv.rs b/esp-backtrace/src/riscv.rs index 99db0995c3f..bc73cbed772 100644 --- a/esp-backtrace/src/riscv.rs +++ b/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 diff --git a/esp-build/src/lib.rs b/esp-build/src/lib.rs index 9999ee38b64..3de49b6ac3d 100644 --- a/esp-build/src/lib.rs +++ b/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) -> Vec<(&LitStr, &LitStr)> { +fn unique_pairs(features: &[LitStr]) -> Vec<(&LitStr, &LitStr)> { let mut pairs = Vec::new(); let mut i = 0; diff --git a/esp-ieee802154/src/hal.rs b/esp-ieee802154/src/hal.rs index afe86da8809..c43cb379adf 100644 --- a/esp-ieee802154/src/hal.rs +++ b/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); diff --git a/esp-ieee802154/src/lib.rs b/esp-ieee802154/src/lib.rs index 1832679ee4a..b8375d46560 100644 --- a/esp-ieee802154/src/lib.rs +++ b/esp-ieee802154/src/lib.rs @@ -112,8 +112,8 @@ impl<'a> Ieee802154<'a> { Self { _align: 0, transmit_buffer: [0u8; FRAME_SIZE], - _phantom1: PhantomData::default(), - //_phantom2: PhantomData::default(), + _phantom1: PhantomData, + //_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 static RX_AVAILABLE_CALLBACK: Mutex>> = Mutex::new(RefCell::new(None)); +#[allow(clippy::type_complexity)] static TX_DONE_CALLBACK_FN: Mutex>> = Mutex::new(RefCell::new(None)); +#[allow(clippy::type_complexity)] static RX_AVAILABLE_CALLBACK_FN: Mutex>> = Mutex::new(RefCell::new(None)); fn tx_done() { diff --git a/esp-ieee802154/src/pib.rs b/esp-ieee802154/src/pib.rs index 77aadd767fc..e1ef5292d2b 100644 --- a/esp-ieee802154/src/pib.rs +++ b/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()); } } } diff --git a/esp-ieee802154/src/raw.rs b/esp-ieee802154/src/raw.rs index 834a793d741..6cbd647c4d2 100644 --- a/esp-ieee802154/src/raw.rs +++ b/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(); diff --git a/esp-metadata/src/lib.rs b/esp-metadata/src/lib.rs index aab172f54ab..44db919ad4b 100644 --- a/esp-metadata/src/lib.rs +++ b/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 ESP32C2_TOML: &'static str = include_str!("../devices/esp32c2.toml"); -const ESP32C3_TOML: &'static str = include_str!("../devices/esp32c3.toml"); -const ESP32C6_TOML: &'static str = include_str!("../devices/esp32c6.toml"); -const ESP32H2_TOML: &'static str = include_str!("../devices/esp32h2.toml"); -const ESP32P4_TOML: &'static str = include_str!("../devices/esp32p4.toml"); -const ESP32S2_TOML: &'static str = include_str!("../devices/esp32s2.toml"); -const ESP32S3_TOML: &'static str = include_str!("../devices/esp32s3.toml"); +const ESP32_TOML: &str = include_str!("../devices/esp32.toml"); +const ESP32C2_TOML: &str = include_str!("../devices/esp32c2.toml"); +const ESP32C3_TOML: &str = include_str!("../devices/esp32c3.toml"); +const ESP32C6_TOML: &str = include_str!("../devices/esp32c6.toml"); +const ESP32H2_TOML: &str = include_str!("../devices/esp32h2.toml"); +const ESP32P4_TOML: &str = include_str!("../devices/esp32p4.toml"); +const ESP32S2_TOML: &str = include_str!("../devices/esp32s2.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();