diff --git a/.cargo/config b/.cargo/config deleted file mode 100644 index 35049cbcb..000000000 --- a/.cargo/config +++ /dev/null @@ -1,2 +0,0 @@ -[alias] -xtask = "run --package xtask --" diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 000000000..0e8ce01fb --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,10 @@ +[alias] +xtask = "run --package xtask --" + +[build] +# The purpose of this flag is to block crates using version_detect from "detecting" +# features that are no longer supported by the toolchain, because despite its name, +# version_detect is basically "if nightly { return true; }". This setting gets +# overridden within xtask for Hubris programs, so this only affects host tools like +# xtask. +rustflags = ["-Zallow-features=proc_macro_diagnostic,asm_const,naked_functions,used_with_arg"] diff --git a/.github/workflows/build-one.yml b/.github/workflows/build-one.yml index 8f8b22b27..895de6393 100644 --- a/.github/workflows/build-one.yml +++ b/.github/workflows/build-one.yml @@ -79,10 +79,14 @@ jobs: target/release/humility -a target/${{ inputs.app_name }}/dist/build-${{ inputs.app_name }}-image-$image.zip manifest; \ done - - name: Clippy - if: inputs.os == 'ubuntu-latest' - run: | - cargo xtask clippy ${{ inputs.app_toml}} -- --deny warnings + # TODO: Clippy temporarily disabled 2024-04 while people clean up + # the new Clippy warnings resulting from the last toolchain + # upgrade. If you're reading this, try turning it back on and see + # if we're good yet. + #- name: Clippy + # if: inputs.os == 'ubuntu-latest' + # run: | + # cargo xtask clippy ${{ inputs.app_toml}} -- --deny warnings # upload the output of our build - name: Upload build archive diff --git a/Cargo.lock b/Cargo.lock index e77dbf8fc..689853903 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -243,6 +243,7 @@ dependencies = [ "convert_case", "indexmap 1.9.1", "multimap", + "rangemap", "serde", ] @@ -1676,8 +1677,6 @@ dependencies = [ "build-spi", "build-util", "counters", - "derive-idol-err", - "gateway-messages", "hubpack", "idol", "idol-runtime", @@ -2018,6 +2017,7 @@ dependencies = [ "build-stm32xx-sys", "build-util", "cfg-if", + "counters", "drv-stm32xx-gpio-common", "drv-stm32xx-sys-api", "drv-stm32xx-uid", @@ -2044,6 +2044,7 @@ dependencies = [ "idol", "idol-runtime", "num-traits", + "serde", "userlib", "zerocopy 0.6.6", ] @@ -2652,7 +2653,7 @@ checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" [[package]] name = "idol" version = "0.4.0" -source = "git+https://github.com/oxidecomputer/idolatry.git#070c4dc890f8c3dc26fcadd227316ee2b771e138" +source = "git+https://github.com/oxidecomputer/idolatry.git#b529f7b82face38cfc3851a641277fb7dfbedc4b" dependencies = [ "indexmap 1.9.1", "once_cell", @@ -2669,7 +2670,7 @@ dependencies = [ [[package]] name = "idol-runtime" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/idolatry.git#070c4dc890f8c3dc26fcadd227316ee2b771e138" +source = "git+https://github.com/oxidecomputer/idolatry.git#b529f7b82face38cfc3851a641277fb7dfbedc4b" dependencies = [ "counters", "userlib", @@ -5450,7 +5451,6 @@ name = "vsc-err" version = "0.1.0" dependencies = [ "counters", - "drv-spi-api", "idol-runtime", ] diff --git a/Cargo.toml b/Cargo.toml index 187428781..f8b52c3a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,3 @@ -cargo-features = ["resolver", "named-profiles"] - [workspace] members = [ "build/*", diff --git a/app/demo-stm32g0-nucleo/app-g070.toml b/app/demo-stm32g0-nucleo/app-g070.toml index 8918b93ad..b4e1f04ff 100644 --- a/app/demo-stm32g0-nucleo/app-g070.toml +++ b/app/demo-stm32g0-nucleo/app-g070.toml @@ -7,7 +7,7 @@ stacksize = 944 [kernel] name = "demo-stm32g0-nucleo" -requires = {flash = 19040, ram = 1632} +requires = {flash = 19264, ram = 1632} features = ["g070"] stacksize = 640 diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index b77041786..c8b9c9c70 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -6,7 +6,7 @@ stacksize = 896 [kernel] name = "demo-stm32h7-nucleo" -requires = {flash = 24000, ram = 5120} +requires = {flash = 24736, ram = 5120} features = ["h743", "dump"] [tasks.jefe] @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h743", "exti"] +features = ["h743", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true @@ -107,7 +107,7 @@ priority = 3 start = true task-slots = ["sys", "user_leds"] notifications = ["button"] -config = { led = 1, rising = false, falling = true } +config = { led = 1, edge = "Edge::Rising" } [tasks.udpecho] name = "task-udpecho" diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index 8c2f9ea51..efa28c158 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -6,7 +6,7 @@ stacksize = 896 [kernel] name = "demo-stm32h7-nucleo" -requires = {flash = 24544, ram = 5120} +requires = {flash = 24736, ram = 5120} features = ["h753", "dump"] [tasks.jefe] @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true @@ -116,7 +116,7 @@ priority = 3 start = true task-slots = ["sys", "user_leds"] notifications = ["button"] -config = { led = 1, rising = false, falling = true } +config = { led = 1, edge = "Edge::Rising" } [tasks.udpecho] name = "task-udpecho" diff --git a/app/donglet/app-g031-i2c.toml b/app/donglet/app-g031-i2c.toml index e9ce2aa03..218a6c2b3 100644 --- a/app/donglet/app-g031-i2c.toml +++ b/app/donglet/app-g031-i2c.toml @@ -6,7 +6,7 @@ board = "donglet-g031" [kernel] name = "app-donglet" -requires = {flash = 18752, ram = 1616} +requires = {flash = 18944, ram = 1616} features = ["g031"] stacksize = 936 diff --git a/app/donglet/app-g031.toml b/app/donglet/app-g031.toml index ed9d541d4..57c8dab43 100644 --- a/app/donglet/app-g031.toml +++ b/app/donglet/app-g031.toml @@ -6,7 +6,7 @@ board = "donglet-g031" [kernel] name = "app-donglet" -requires = {flash = 18944, ram = 1820} +requires = {flash = 19168, ram = 1820} features = ["g031"] stacksize = 936 diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 5e38e7124..df6658690 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -50,7 +50,7 @@ notifications = ["eth-irq", "mdio-timer-irq", "wake-timer", "jefe-state-change"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true @@ -214,7 +214,6 @@ priority = 4 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.host_sp_comms] name = "task-host-sp-comms" diff --git a/app/gimletlet/app.toml b/app/gimletlet/app.toml index 78a26012a..40559d81b 100644 --- a/app/gimletlet/app.toml +++ b/app/gimletlet/app.toml @@ -60,7 +60,6 @@ priority = 5 max-sizes = {flash = 16384, ram = 2048 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.dump_agent] name = "task-dump-agent" diff --git a/app/oxide-rot-1/app.toml b/app/oxide-rot-1/app.toml index 9d20f495c..cf17974ad 100644 --- a/app/oxide-rot-1/app.toml +++ b/app/oxide-rot-1/app.toml @@ -68,7 +68,6 @@ task-slots = ["syscon_driver"] [tasks.sprot] name = "drv-lpc55-sprot-server" priority = 6 -max-sizes = {flash = 48608, ram = 32768} uses = ["flexcomm8", "bootrom"] features = ["spi0"] start = true diff --git a/app/psc/base.toml b/app/psc/base.toml index eaaf62c64..050c16fd0 100644 --- a/app/psc/base.toml +++ b/app/psc/base.toml @@ -34,7 +34,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] @@ -219,7 +219,6 @@ priority = 3 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.sensor_polling] name = "task-sensor-polling" diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index d92bcee19..98cb98c8b 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -6,7 +6,7 @@ fwid = true [kernel] name = "sidecar" -requires = {flash = 25792, ram = 6256} +requires = {flash = 25984, ram = 6256} features = ["dump"] [caboose] @@ -31,7 +31,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] @@ -184,7 +184,6 @@ priority = 4 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.ecp5_mainboard] name = "drv-fpga-server" diff --git a/build/i2c/Cargo.toml b/build/i2c/Cargo.toml index 941b80a74..7604d4b1c 100644 --- a/build/i2c/Cargo.toml +++ b/build/i2c/Cargo.toml @@ -12,6 +12,7 @@ convert_case = { workspace = true } indexmap = { workspace = true } multimap = { workspace = true } serde = { workspace = true } +rangemap.workspace = true [features] h743 = [] diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index 0e33f2449..944230168 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -6,6 +6,7 @@ use anyhow::{bail, Context, Result}; use convert_case::{Case, Casing}; use indexmap::IndexMap; use multimap::MultiMap; +use rangemap::RangeSet; use serde::Deserialize; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write; @@ -269,21 +270,6 @@ struct DeviceRefdesKey { kind: Sensor, } -#[derive(Debug, PartialEq, Eq, Hash)] -struct DeviceBusKey { - device: String, - bus: String, - kind: Sensor, -} - -#[derive(Debug, PartialEq, Eq, Hash)] -struct DeviceBusNameKey { - device: String, - bus: String, - name: String, - kind: Sensor, -} - #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeviceSensor { pub name: Option, @@ -573,7 +559,7 @@ impl ConfigGenerator { d.device, d.address ); } - (_, Some(bus)) if buses.get(bus).is_none() => { + (_, Some(bus)) if !buses.contains_key(bus) => { panic!( "device {} at address {:#x} specifies \ unknown bus \"{}\"", @@ -1016,20 +1002,9 @@ impl ConfigGenerator { let mut all: Vec<_> = by_controller.iter_all().collect(); all.sort(); - for (controller, indices) in all { - let mut s: Vec = - indices.iter().map(|f| format!("{}", f)).collect::<_>(); - - s.sort(); - - write!( - &mut self.output, - r##" - {} => Some(Controller::I2C{}),"##, - s.join("\n | "), - controller, - )?; - } + match_arms(&mut self.output, all, |c| { + format!("Some(Controller::I2C{c})") + })?; write!( &mut self.output, @@ -1052,20 +1027,7 @@ impl ConfigGenerator { let mut all: Vec<_> = by_port.iter_all().collect(); all.sort(); - for (port, indices) in all { - let mut s: Vec = - indices.iter().map(|f| format!("{}", f)).collect::<_>(); - - s.sort(); - - write!( - &mut self.output, - r##" - {} => Some(PortIndex({})),"##, - s.join("\n | "), - port, - )?; - } + match_arms(&mut self.output, all, |p| format!("Some(PortIndex({p}))"))?; write!( &mut self.output, @@ -1254,7 +1216,7 @@ impl ConfigGenerator { // returned by `device_descriptions()` below: if we change the ordering // here, it must be updated there as well. for (index, device) in self.devices.iter().enumerate() { - if drivers.get(&device.device).is_some() { + if drivers.contains(&device.device) { let driver = device.device.to_case(Case::UpperCamel); let out = self.generate_device(device, 24); @@ -1471,7 +1433,7 @@ impl ConfigGenerator { { writeln!( &mut self.output, - "\n #[allow(non_camel_case_types)] + "\n #[allow(non_camel_case_types, dead_code)] pub struct Sensors_{struct_name} {{", )?; let mut f = |name, count| match count { @@ -1770,3 +1732,33 @@ pub fn device_descriptions() -> impl Iterator { }, ) } + +fn match_arms<'a, C>( + mut out: impl Write, + source: impl IntoIterator)>, + fmt: impl Fn(&C) -> String, +) -> Result<()> +where + C: 'a, +{ + for (controller, indices) in source { + let indices = indices + .iter() + .map(|&i| i..i + 1) + .collect::>(); + let s = indices + .iter() + .map(|range| format!("{}..={}", range.start, range.end - 1)) + .collect::>() + .join("\n | "); + + let result = fmt(controller); + + write!( + &mut out, + r##" + {s} => {result},"##, + )?; + } + Ok(()) +} diff --git a/build/stm32xx-sys/src/lib.rs b/build/stm32xx-sys/src/lib.rs index de630eccc..b5498a091 100644 --- a/build/stm32xx-sys/src/lib.rs +++ b/build/stm32xx-sys/src/lib.rs @@ -81,19 +81,20 @@ impl SysConfig { &self, ) -> anyhow::Result { #[derive(Debug)] - struct DispatchEntry<'a> { - _name: &'a str, + struct DispatchEntry { port: Port, task: syn::Ident, note: syn::Ident, + name: syn::Ident, } const NUM_EXTI_IRQS: usize = 16; - let mut dispatch_table: [Option>; NUM_EXTI_IRQS] = + let mut dispatch_table: [Option; NUM_EXTI_IRQS] = Default::default(); + let mut has_any_notifications = false; for ( - _name, + name, &GpioIrqConfig { port, pin, @@ -106,13 +107,19 @@ impl SysConfig { anyhow::bail!("pin {pin} is already mapped to IRQ {curr:?}") } Some(slot) => { + has_any_notifications = true; let task = syn::parse_str(&owner.name)?; let note = quote::format_ident!( "{}_MASK", owner.notification.to_uppercase().replace('-', "_") ); + + let name = quote::format_ident!( + "{}", + name.replace('-', "_") + ); *slot = Some(DispatchEntry { - _name, + name, port, task, note, @@ -126,22 +133,49 @@ impl SysConfig { let dispatches = dispatch_table.iter().map(|slot| match slot { Some(DispatchEntry { - port, task, note, .. + name, + port, + task, + note, + .. }) => quote! { - Some(( - #port, - userlib::TaskId::for_index_and_gen( + Some(ExtiDispatch { + port: #port, + task: userlib::TaskId::for_index_and_gen( hubris_num_tasks::Task::#task as usize, userlib::Generation::ZERO, ), - crate::notifications::#task::#note, - )) + mask: crate::notifications::#task::#note, + name: ExtiIrq::#name, + }) }, None => quote! { None }, }); + let counter_type = if has_any_notifications { + let irq_names = dispatch_table + .iter() + .filter_map(|slot| Some(&slot.as_ref()?.name)); + quote! { + #[derive(Copy, Clone, PartialEq, Eq, counters::Count)] + #[allow(nonstandard_style)] + pub(crate) enum ExtiIrq { + #( #irq_names ),* + } + } + } else { + // If there are no EXTI notifications enabled, just use `()` as the counter + // type, as it does implement `counters::Count`, but has no values, so + // we don't get a "matching on an uninhabited type" error. + quote! { + pub(crate) type ExtiIrq = (); + } + }; + Ok(quote! { - pub(crate) const EXTI_DISPATCH_TABLE: [Option<(Port, TaskId, u32)>; #NUM_EXTI_IRQS] = [ + #counter_type + + pub(crate) const EXTI_DISPATCH_TABLE: [Option; #NUM_EXTI_IRQS] = [ #( #dispatches ),* ]; }) diff --git a/build/util/src/lib.rs b/build/util/src/lib.rs index 0fea1a1ec..c4f062675 100644 --- a/build/util/src/lib.rs +++ b/build/util/src/lib.rs @@ -262,7 +262,7 @@ pub fn build_notifications() -> Result<()> { ); } if full_task_config.name == "task-jefe" - && full_task_config.notifications.get(0).cloned() + && full_task_config.notifications.first().cloned() != Some("fault".to_string()) { bail!("`jefe` must have \"fault\" as its first notification"); diff --git a/build/xtask/src/caboose_pos.rs b/build/xtask/src/caboose_pos.rs index 836547b76..bbaf5c0a6 100644 --- a/build/xtask/src/caboose_pos.rs +++ b/build/xtask/src/caboose_pos.rs @@ -52,9 +52,9 @@ pub fn get_caboose_pos_table_entry( ) -> Result> { // If the section isn't present, then we're not reading the caboose position // from this task. - let Some(caboose_pos_table_section) = elf::get_section_by_name( - elf, CABOOSE_POS_TABLE_SECTION - ) else { + let Some(caboose_pos_table_section) = + elf::get_section_by_name(elf, CABOOSE_POS_TABLE_SECTION) + else { return Ok(None); }; diff --git a/build/xtask/src/config.rs b/build/xtask/src/config.rs index e5e4d0ecf..bca20d971 100644 --- a/build/xtask/src/config.rs +++ b/build/xtask/src/config.rs @@ -33,8 +33,6 @@ struct RawConfig { #[serde(default)] image_names: Vec, #[serde(default)] - external_images: Vec, - #[serde(default)] signing: Option, stacksize: Option, kernel: Kernel, @@ -56,7 +54,6 @@ pub struct Config { pub version: u32, pub fwid: bool, pub image_names: Vec, - pub external_images: Vec, pub signing: Option, pub stacksize: Option, pub kernel: Kernel, @@ -176,7 +173,6 @@ impl Config { target: toml.target, board: toml.board, image_names: img_names, - external_images: toml.external_images, chip: toml.chip, epoch: toml.epoch, version: toml.version, diff --git a/build/xtask/src/dist.rs b/build/xtask/src/dist.rs index 0a4b813b6..b26da2fcb 100644 --- a/build/xtask/src/dist.rs +++ b/build/xtask/src/dist.rs @@ -375,8 +375,7 @@ pub fn package( // Build a set of requests for the memory allocator let mut task_reqs = HashMap::new(); for (t, sz) in task_sizes { - // XXX disabled because leases can't span regions - let _n = sz.len() + let n = sz.len() + cfg .toml .extern_regions_for(t, &cfg.toml.image_names[0]) @@ -394,7 +393,7 @@ pub fn package( t, TaskRequest { memory: sz, - spare_regions: 0, + spare_regions: 7 - n, }, ); } diff --git a/build/xtask/src/lsp.rs b/build/xtask/src/lsp.rs index 4c8ea8a8e..43bc0455e 100644 --- a/build/xtask/src/lsp.rs +++ b/build/xtask/src/lsp.rs @@ -114,7 +114,9 @@ impl PackageGraph { while let Some((pkg_name, feat)) = todo.pop() { // Anything not in `packages` is something from outside the // workspace, so we don't care about it. - let Some(pkg) = self.0.get(&pkg_name) else { continue }; + let Some(pkg) = self.0.get(&pkg_name) else { + continue; + }; // If we've never seen this package before, then insert all of // its non-optional dependencies with their features. @@ -277,7 +279,7 @@ fn check_task( extra_env: build_cfg.env, hash: "".to_owned(), build_override_command, - app: app_name.clone().to_owned(), + app: app_name.to_owned(), task: task_name.to_owned(), }; diff --git a/build/xtask/src/sizes.rs b/build/xtask/src/sizes.rs index 6970c74db..d28f31835 100644 --- a/build/xtask/src/sizes.rs +++ b/build/xtask/src/sizes.rs @@ -118,7 +118,7 @@ pub fn run( #[derive(Copy, Clone, Debug)] enum Recommended { - FixedSize(u32), + FixedSize, MaxSize(u32), } #[derive(Clone, Debug)] @@ -183,7 +183,7 @@ fn build_memory_map<'a>( .get(mem_name.to_owned()) .cloned() .map(match name { - "kernel" => Recommended::FixedSize, + "kernel" => |_| Recommended::FixedSize, _ => Recommended::MaxSize, }), }, @@ -265,7 +265,7 @@ fn print_task_table( match chunk.recommended { None => print!("(auto)"), Some(Recommended::MaxSize(m)) => print!("{}", m), - Some(Recommended::FixedSize(_)) => print!("(fixed)"), + Some(Recommended::FixedSize) => print!("(fixed)"), } println!(); } @@ -359,7 +359,7 @@ fn print_memory_map( match chunk.recommended { None => print!("(auto)"), Some(Recommended::MaxSize(m)) => print!("{}", m), - Some(Recommended::FixedSize(_)) => print!("(fixed)"), + Some(Recommended::FixedSize) => print!("(fixed)"), } } println!(); diff --git a/drv/gimlet-seq-server/build.rs b/drv/gimlet-seq-server/build.rs index 210947845..2f15ecc2d 100644 --- a/drv/gimlet-seq-server/build.rs +++ b/drv/gimlet-seq-server/build.rs @@ -5,7 +5,7 @@ use build_fpga_regmap::fpga_regs; use serde::Deserialize; use sha2::Digest; -use std::{convert::TryInto, fs, io::Write, path::PathBuf}; +use std::{fs, io::Write, path::PathBuf}; #[derive(Deserialize)] #[serde(deny_unknown_fields)] diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs index 980c23d65..c0ba2d543 100644 --- a/drv/gimlet-seq-server/src/main.rs +++ b/drv/gimlet-seq-server/src/main.rs @@ -153,10 +153,7 @@ fn main() -> ! { // irrelevant. But, `rustc` doesn't realize that this should // never return, we'll stick it in a `loop` anyway so the main // function can return `!` - // - // We don't care if this returns an error, because we're just - // doing it to die as politely as possible. - let _ = sys_recv_closed(&mut [], 0, TaskId::KERNEL); + sys_recv_notification(0); } } } diff --git a/drv/i2c-devices/src/at24csw080.rs b/drv/i2c-devices/src/at24csw080.rs index 710ff0864..c8adb1ca9 100644 --- a/drv/i2c-devices/src/at24csw080.rs +++ b/drv/i2c-devices/src/at24csw080.rs @@ -5,7 +5,6 @@ //! Driver for the AT24CSW080/4 I2C EEPROM use crate::Validate; -use core::convert::TryInto; use drv_i2c_api::*; use userlib::{hl::sleep_for, FromPrimitive, ToPrimitive}; use zerocopy::{AsBytes, FromBytes}; diff --git a/drv/i2c-devices/src/ds2482.rs b/drv/i2c-devices/src/ds2482.rs index fbd80cc54..225cac913 100644 --- a/drv/i2c-devices/src/ds2482.rs +++ b/drv/i2c-devices/src/ds2482.rs @@ -201,6 +201,8 @@ impl Ds2482 { pub fn search(&mut self) -> Result, Error> { let device = &self.device; + // TODO: lint is buggy in 2024-04-04 toolchain, retest later. + #[allow(clippy::manual_unwrap_or_default)] let branches = match self.branches { Some(branches) => { if branches.0 == 0 { diff --git a/drv/i2c-devices/src/max31790.rs b/drv/i2c-devices/src/max31790.rs index d3966e637..5eaffe4c5 100644 --- a/drv/i2c-devices/src/max31790.rs +++ b/drv/i2c-devices/src/max31790.rs @@ -6,7 +6,6 @@ use crate::Validate; use bitfield::bitfield; -use core::convert::TryFrom; use drv_i2c_api::*; use ringbuf::*; use userlib::units::*; diff --git a/drv/ksz8463/src/lib.rs b/drv/ksz8463/src/lib.rs index ae47b6eea..02bfef88f 100644 --- a/drv/ksz8463/src/lib.rs +++ b/drv/ksz8463/src/lib.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. #![no_std] -use drv_spi_api::{SpiDevice, SpiError, SpiServer}; +use drv_spi_api::{SpiDevice, SpiServer}; use ringbuf::*; use userlib::hl::sleep_for; @@ -14,19 +14,13 @@ pub use registers::{MIBCounter, Register}; #[derive(Copy, Clone, Eq, PartialEq, counters::Count)] pub enum Error { - SpiError(#[count(children)] SpiError), + SpiServerDied, WrongChipId(u16), } -impl From for Error { - fn from(s: SpiError) -> Self { - Self::SpiError(s) - } -} - impl From for Error { fn from(_: idol_runtime::ServerDeath) -> Self { - Self::SpiError(SpiError::TaskRestarted) + Self::SpiServerDied } } diff --git a/drv/lpc55-sha256/src/lib.rs b/drv/lpc55-sha256/src/lib.rs index 68df54cd5..b66773216 100644 --- a/drv/lpc55-sha256/src/lib.rs +++ b/drv/lpc55-sha256/src/lib.rs @@ -55,7 +55,7 @@ //! (This restriction would be straightforward to lift if required.) use core::num::Wrapping; -use userlib::{sys_irq_control, sys_recv_closed, TaskId}; +use userlib::{sys_irq_control, sys_recv_notification}; // These constants describe intrinsic properties of the SHA256 algorithm and // should not be changed. @@ -201,11 +201,7 @@ impl<'a> Hasher<'a> { // Wait for it! sys_irq_control(self.notification_mask, true); - let _ = sys_recv_closed( - &mut [], - self.notification_mask, - TaskId::KERNEL, - ); + sys_recv_notification(self.notification_mask); // Turn it back off lest it spam us in the future. self.engine.intenclr.write(|w| w.digest().set_bit()); @@ -235,11 +231,7 @@ impl<'a> Hasher<'a> { // Wait for it! sys_irq_control(self.notification_mask, true); - let _ = sys_recv_closed( - &mut [], - self.notification_mask, - TaskId::KERNEL, - ); + sys_recv_notification(self.notification_mask); // Turn it back off lest it spam us in the future. self.engine.intenclr.write(|w| w.waiting().set_bit()); diff --git a/drv/lpc55-spi-server/src/main.rs b/drv/lpc55-spi-server/src/main.rs index d57372f9e..7a30e6527 100644 --- a/drv/lpc55-spi-server/src/main.rs +++ b/drv/lpc55-spi-server/src/main.rs @@ -83,11 +83,7 @@ fn main() -> ! { let mut rx_done = false; loop { - if sys_recv_closed(&mut [], notifications::SPI_IRQ_MASK, TaskId::KERNEL) - .is_err() - { - panic!() - } + sys_recv_notification(notifications::SPI_IRQ_MASK); ringbuf_entry!(Trace::Irq); diff --git a/drv/lpc55-sprot-server/src/main.rs b/drv/lpc55-sprot-server/src/main.rs index e3a623bb1..7c6046392 100644 --- a/drv/lpc55-sprot-server/src/main.rs +++ b/drv/lpc55-sprot-server/src/main.rs @@ -57,7 +57,7 @@ use drv_sprot_api::{ use lpc55_pac as device; use ringbuf::{ringbuf, ringbuf_entry}; use userlib::{ - sys_irq_control, sys_recv_closed, task_slot, TaskId, UnwrapLite, + sys_irq_control, sys_recv_notification, task_slot, TaskId, UnwrapLite, }; mod handler; @@ -216,12 +216,7 @@ impl Io { loop { sys_irq_control(notifications::SPI_IRQ_MASK, true); - sys_recv_closed( - &mut [], - notifications::SPI_IRQ_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SPI_IRQ_MASK); // Is CSn asserted by the SP? let intstat = self.spi.intstat(); diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 06aafa80c..f0c497876 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -639,12 +639,7 @@ fn indirect_flash_read_words( flash.enable_interrupt_sources(); sys_irq_control(notifications::FLASH_IRQ_MASK, true); - // RECV from the kernel cannot produce an error, so ignore it. - let _ = sys_recv_closed( - &mut [], - notifications::FLASH_IRQ_MASK, - TaskId::KERNEL, - ); + sys_recv_notification(notifications::FLASH_IRQ_MASK); flash.disable_interrupt_sources(); } } @@ -773,9 +768,7 @@ fn do_block_write( fn wait_for_flash_interrupt() { sys_irq_control(notifications::FLASH_IRQ_MASK, true); - // RECV from the kernel cannot produce an error, so ignore it. - let _ = - sys_recv_closed(&mut [], notifications::FLASH_IRQ_MASK, TaskId::KERNEL); + sys_recv_notification(notifications::FLASH_IRQ_MASK); } fn same_image(which: UpdateTarget) -> bool { diff --git a/drv/psc-seq-server/src/main.rs b/drv/psc-seq-server/src/main.rs index cff7bcf88..59d6e8abf 100644 --- a/drv/psc-seq-server/src/main.rs +++ b/drv/psc-seq-server/src/main.rs @@ -45,6 +45,6 @@ fn main() -> ! { // We have nothing else to do, so sleep forever via waiting for a message // from the kernel that won't arrive. loop { - _ = sys_recv_closed(&mut [], 0, TaskId::KERNEL); + sys_recv_notification(0); } } diff --git a/drv/sidecar-front-io/src/transceivers.rs b/drv/sidecar-front-io/src/transceivers.rs index 692545ac7..678d090d3 100644 --- a/drv/sidecar-front-io/src/transceivers.rs +++ b/drv/sidecar-front-io/src/transceivers.rs @@ -848,7 +848,9 @@ impl Transceivers { FpgaController::Left => ldata, FpgaController::Right => rdata, }; - let Some(local_data) = local_data else { continue }; + let Some(local_data) = local_data else { + continue; + }; // loop through the 8 different fields we need to map for (word, out) in local_data.iter().zip(status_masks.iter_mut()) { diff --git a/drv/sidecar-mainboard-controller/src/tofino2.rs b/drv/sidecar-mainboard-controller/src/tofino2.rs index f40779bf9..3a1dc57d0 100644 --- a/drv/sidecar-mainboard-controller/src/tofino2.rs +++ b/drv/sidecar-mainboard-controller/src/tofino2.rs @@ -4,7 +4,6 @@ use crate::{Addr, MainboardController, Reg}; use bitfield::bitfield; -use core::convert::Into; use derive_more::{From, Into}; use drv_fpga_api::{FpgaError, FpgaUserDesign, WriteOp}; use drv_fpga_user_api::power_rail::*; diff --git a/drv/sidecar-seq-server/src/tofino.rs b/drv/sidecar-seq-server/src/tofino.rs index 5510343e4..9eb3910e4 100644 --- a/drv/sidecar-seq-server/src/tofino.rs +++ b/drv/sidecar-seq-server/src/tofino.rs @@ -4,7 +4,6 @@ use crate::*; use drv_i2c_devices::raa229618::Raa229618; -use drv_sidecar_mainboard_controller::tofino2::{DebugPort, Sequencer}; pub(crate) struct Tofino { pub policy: TofinoSequencerPolicy, diff --git a/drv/spi-api/Cargo.toml b/drv/spi-api/Cargo.toml index da063a452..12d8d6a1c 100644 --- a/drv/spi-api/Cargo.toml +++ b/drv/spi-api/Cargo.toml @@ -7,7 +7,6 @@ edition = "2021" [dependencies] -gateway-messages.workspace = true hubpack.workspace = true idol-runtime.workspace = true num-traits.workspace = true @@ -15,7 +14,6 @@ serde.workspace = true zerocopy.workspace = true counters = { path = "../../lib/counters" } -derive-idol-err.path = "../../lib/derive-idol-err" userlib.path = "../../sys/userlib" # This section is here to discourage RLS/rust-analyzer from doing test builds, diff --git a/drv/spi-api/src/lib.rs b/drv/spi-api/src/lib.rs index 5ac81bd48..ff957adc7 100644 --- a/drv/spi-api/src/lib.rs +++ b/drv/spi-api/src/lib.rs @@ -6,52 +6,9 @@ #![no_std] -// For Idol-generated code that fully qualifies error type names. -use crate as drv_spi_api; -use derive_idol_err::IdolError; -use gateway_messages::SpiError as GwSpiError; -use hubpack::SerializedSize; -use serde::{Deserialize, Serialize}; +use idol_runtime::ServerDeath; use userlib::*; -#[derive( - Copy, - Clone, - Debug, - FromPrimitive, - Eq, - PartialEq, - IdolError, - SerializedSize, - Serialize, - Deserialize, - counters::Count, -)] -#[repr(u32)] -pub enum SpiError { - /// Transfer size is 0 or exceeds maximum - BadTransferSize = 1, - - /// Server restarted - #[idol(server_death)] - TaskRestarted = 4, -} - -impl From for SpiError { - fn from(_: idol_runtime::ServerDeath) -> Self { - SpiError::TaskRestarted - } -} - -impl From for GwSpiError { - fn from(value: SpiError) -> Self { - match value { - SpiError::BadTransferSize => Self::BadTransferSize, - SpiError::TaskRestarted => Self::TaskRestarted, - } - } -} - #[derive( Copy, Clone, Debug, Eq, PartialEq, zerocopy::AsBytes, FromPrimitive, )] @@ -81,11 +38,15 @@ pub trait SpiServer { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError>; + ) -> Result<(), ServerDeath>; - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError>; + fn write(&self, device_index: u8, src: &[u8]) -> Result<(), ServerDeath>; - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError>; + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), ServerDeath>; /// Variant of `lock` that returns a resource management object that, when /// dropped, will issue `release`. This makes it much easier to do fallible @@ -96,7 +57,7 @@ pub trait SpiServer { &self, device_index: u8, assert_cs: CsState, - ) -> Result, idol_runtime::ServerDeath> + ) -> Result, ServerDeath> where Self: Sized, { @@ -119,9 +80,9 @@ pub trait SpiServer { &self, device_index: u8, cs_state: CsState, - ) -> Result<(), idol_runtime::ServerDeath>; + ) -> Result<(), ServerDeath>; - fn release(&self) -> Result<(), idol_runtime::ServerDeath>; + fn release(&self) -> Result<(), ServerDeath>; } impl SpiServer for Spi { @@ -130,14 +91,18 @@ impl SpiServer for Spi { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError> { + ) -> Result<(), ServerDeath> { Spi::exchange(self, device_index, src, dest) } - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError> { + fn write(&self, device_index: u8, src: &[u8]) -> Result<(), ServerDeath> { Spi::write(self, device_index, src) } - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError> { + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), ServerDeath> { Spi::read(self, device_index, dest) } @@ -145,11 +110,11 @@ impl SpiServer for Spi { &self, device_index: u8, cs_state: CsState, - ) -> Result<(), idol_runtime::ServerDeath> { + ) -> Result<(), ServerDeath> { Spi::lock(self, device_index, cs_state) } - fn release(&self) -> Result<(), idol_runtime::ServerDeath> { + fn release(&self) -> Result<(), ServerDeath> { Spi::release(self) } } @@ -184,7 +149,7 @@ impl SpiDevice { &self, source: &[u8], sink: &mut [u8], - ) -> Result<(), SpiError> { + ) -> Result<(), ServerDeath> { self.server.exchange(self.device_index, source, sink) } @@ -192,7 +157,7 @@ impl SpiDevice { /// /// If the controller is not locked, this will assert CS before driving the /// clock and release it after. - pub fn write(&self, source: &[u8]) -> Result<(), SpiError> { + pub fn write(&self, source: &[u8]) -> Result<(), ServerDeath> { self.server.write(self.device_index, source) } @@ -200,7 +165,7 @@ impl SpiDevice { /// /// If the controller is not locked, this will assert CS before driving the /// clock and release it after. - pub fn read(&self, dest: &mut [u8]) -> Result<(), SpiError> { + pub fn read(&self, dest: &mut [u8]) -> Result<(), ServerDeath> { self.server.read(self.device_index, dest) } @@ -223,17 +188,14 @@ impl SpiDevice { /// /// If your task tries to lock two different `SpiDevice`s at once, the /// second one to attempt will get `BadDevice`. - pub fn lock( - &self, - assert_cs: CsState, - ) -> Result<(), idol_runtime::ServerDeath> { + pub fn lock(&self, assert_cs: CsState) -> Result<(), ServerDeath> { self.server.lock(self.device_index, assert_cs) } /// Releases a previous lock on the SPI controller (by your task). /// /// This will also deassert CS, if you had overridden it. - pub fn release(&self) -> Result<(), idol_runtime::ServerDeath> { + pub fn release(&self) -> Result<(), ServerDeath> { self.server.release() } @@ -245,7 +207,7 @@ impl SpiDevice { pub fn lock_auto( &self, assert_cs: CsState, - ) -> Result, idol_runtime::ServerDeath> { + ) -> Result, ServerDeath> { self.server.lock_auto(self.device_index, assert_cs) } } diff --git a/drv/stm32h7-eth/src/lib.rs b/drv/stm32h7-eth/src/lib.rs index 2079d09d5..615dff53d 100644 --- a/drv/stm32h7-eth/src/lib.rs +++ b/drv/stm32h7-eth/src/lib.rs @@ -12,7 +12,6 @@ #![no_std] -use core::convert::TryFrom; use core::sync::atomic::{self, Ordering}; #[cfg(feature = "h743")] @@ -435,11 +434,7 @@ impl Ethernet { // has disabled itself before proceeding. loop { userlib::sys_irq_control(self.mdio_timer_irq_mask, true); - let _ = userlib::sys_recv_closed( - &mut [], - self.mdio_timer_irq_mask, - userlib::TaskId::KERNEL, - ); + userlib::sys_recv_notification(self.mdio_timer_irq_mask); if !self.mdio_timer.cr1.read().cen().bit() { break; } diff --git a/drv/stm32h7-hash/src/lib.rs b/drv/stm32h7-hash/src/lib.rs index 652b25baa..c67d13683 100644 --- a/drv/stm32h7-hash/src/lib.rs +++ b/drv/stm32h7-hash/src/lib.rs @@ -29,7 +29,6 @@ use drv_hash_api::HashError; #[cfg(feature = "h753")] use stm32h7::stm32h753 as device; -use core::convert::TryInto; use core::mem::size_of; use userlib::*; use zerocopy::AsBytes; @@ -311,8 +310,7 @@ impl Hash { hl::sleep_for(1); } } - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); if self.reg.sr.read().dcis().bit() { break; } diff --git a/drv/stm32h7-qspi/src/lib.rs b/drv/stm32h7-qspi/src/lib.rs index efaaa468d..92253a4e6 100644 --- a/drv/stm32h7-qspi/src/lib.rs +++ b/drv/stm32h7-qspi/src/lib.rs @@ -15,7 +15,7 @@ use stm32h7::stm32h743 as device; use stm32h7::stm32h753 as device; use drv_qspi_api::Command; -use userlib::{sys_irq_control, sys_recv_closed, TaskId}; +use userlib::{sys_irq_control, sys_recv_notification}; use zerocopy::AsBytes; const FIFO_SIZE: usize = 32; @@ -198,9 +198,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = - sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); if self.reg.sr.read().ftf().bit() { break; } @@ -218,8 +216,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); } self.reg.cr.modify(|_, w| w.tcie().clear_bit()); } @@ -284,9 +281,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = - sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); // Try the check again. We may retry the check on spurious // wakeups, but, spurious wakeups are expected to be pretty @@ -321,8 +316,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); } // Clean up by disabling our interrupt sources. diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index 58f6fc159..b2d5a8994 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -32,7 +32,7 @@ use drv_stm32h7_spi as spi_core; use drv_stm32xx_sys_api as sys_api; use sys_api::PinSet; -use core::{cell::Cell, convert::Infallible}; +use core::{cell::Cell, cmp, convert::Infallible}; //////////////////////////////////////////////////////////////////////////////// @@ -64,7 +64,18 @@ pub struct SpiServerCore { #[derive(Copy, Clone, PartialEq, counters::Count)] enum Trace { - Start(#[count(children)] SpiOperation, (u16, u16)), + Start { + #[count(children)] + op: SpiOperation, + src_total_len: u32, + dst_total_len: u32, + }, + Reload { + #[count(children)] + op: SpiOperation, + src_len: u16, + dst_len: u16, + }, Tx(u8), Rx(u8), WaitISR(u32), @@ -84,7 +95,7 @@ pub struct LockState { /// [`SpiServerCore::exchange`]. #[derive(Copy, Clone, Eq, PartialEq)] pub enum TransferError { - /// Transfer size is 0 or exceeds maximum + /// Transfer size is 0. BadTransferSize = 1, /// Attempt to operate device N when there is no device N, or an attempt to @@ -97,19 +108,6 @@ pub enum TransferError { #[derive(Copy, Clone, Eq, PartialEq)] pub struct LockError(()); -impl From for RequestError { - fn from(value: TransferError) -> Self { - match value { - TransferError::BadTransferSize => { - RequestError::Runtime(SpiError::BadTransferSize) - } - TransferError::BadDevice => { - RequestError::Fail(ClientError::BadMessageContents) - } - } - } -} - impl From for RequestError { fn from(_: LockError) -> RequestError { RequestError::Fail(ClientError::BadMessageContents) @@ -199,7 +197,7 @@ impl SpiServerCore { &self, device_index: u8, dest: BufWrite, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey::<&[u8], _>( SpiOperation::read, device_index, @@ -212,7 +210,7 @@ impl SpiServerCore { &self, device_index: u8, src: BufRead, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey::<_, &mut [u8]>( SpiOperation::write, device_index, @@ -226,7 +224,7 @@ impl SpiServerCore { device_index: u8, src: BufRead, dest: BufWrite, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey( SpiOperation::exchange, device_index, @@ -331,32 +329,28 @@ impl SpiServerCore { panic!(); } - // Get the required transfer lengths in the src and dest directions. - // - // Sizes that overflow a u16 are invalid and we reject them - let src_len: u16 = tx - .as_ref() - .map(|tx| tx.remaining_size()) - .unwrap_or(0) - .try_into() - .map_err(|_| TransferError::BadTransferSize)?; - let dest_len: u16 = rx - .as_ref() - .map(|rx| rx.remaining_size()) - .unwrap_or(0) - .try_into() - .map_err(|_| TransferError::BadTransferSize)?; - let overall_len = src_len.max(dest_len); + let mut src_total_len = + tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0) as u32; + let mut dst_total_len = + rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0) as u32; // Zero-byte SPI transactions don't make sense and we'll // decline them. - if overall_len == 0 { + if src_total_len + dst_total_len == 0 { + // TODO(eliza): perhaps we should just return `Ok` here, so that we + // can completely remove the notion of a "bad transfer size" error? return Err(TransferError::BadTransferSize); } + // Get the required transfer lengths in the src and dest directions. + // We have a reasonable-looking request containing reasonable-looking // lease(s). This is our commit point. - ringbuf_entry!(Trace::Start(op, (src_len, dest_len))); + ringbuf_entry!(Trace::Start { + op, + src_total_len, + dst_total_len, + }); // Switch the mux to the requested port. let current_mux_index = self.current_mux_index.get(); @@ -375,13 +369,15 @@ impl SpiServerCore { self.current_mux_index.set(device.mux_index); } + let mut overall_len = + cmp::max(src_total_len as u16, dst_total_len as u16); + // Subtract the first 16-bit transfer size from the total. + src_total_len = + src_total_len.saturating_sub((src_total_len as u16) as u32); + dst_total_len = + dst_total_len.saturating_sub((dst_total_len as u16) as u32); + // Make sure SPI is on. - // - // Due to driver limitations we will only move up to 64kiB - // per transaction. It would be worth lifting this - // limitation, maybe. Doing so would require managing data - // in 64kiB chunks (because the peripheral is 16-bit) and - // using the "reload" facility on the peripheral. self.spi.enable(overall_len, device.clock_divider); // Load transfer count and start the state machine. At this @@ -414,133 +410,172 @@ impl SpiServerCore { } } - // We use this to exert backpressure on the TX state machine as the RX - // FIFO fills. Its initial value is the configured FIFO size, because - // the FIFO size varies on SPI blocks on the H7; it would be nice if we - // could read the configured FIFO size out of the block, but that does - // not appear to be possible. - // - // See reference manual table 409 for details. - let mut tx_permits = FIFO_DEPTH; - - // Track number of bytes sent and received. Sent bytes will lead - // received bytes. Received bytes indicate overall progress and - // completion. - let mut tx_count = 0; - let mut rx_count = 0; - - // The end of the exchange is signaled by rx_count reaching the - // overall_len. This is true even if the caller's rx lease is shorter or - // missing, because we have to pull bytes from the FIFO to avoid overrun - // conditions. - while rx_count < overall_len { - // At the end of this loop we're going to sleep if there's no - // obvious work to be done. Sleeping is not free, so, we only do it - // if this flag is set. (It defaults to set, we'll clear it if work - // appears below.) - let mut should_sleep = true; - - // TX engine. We continue moving bytes while these three conditions - // hold: - // - More bytes need to be sent. - // - Permits are available. - // - The TX FIFO has space. - while tx_count < overall_len - && tx_permits > 0 - && self.spi.can_tx_frame() - { - // The next byte to TX will come from the caller, if we haven't - // run off the end of their lease, or the fixed padding byte if - // we have. - let byte = if let Some(txbuf) = &mut tx { - if let Some(b) = txbuf.read() { - b + // If the transfer size exceeds `u16::MAX`, we'll loop here multiple + // times, using the `TSER` register to load the next chunk. + loop { + // Are there more bytes to send/receive? If so, we'll need to set + // the `TSER` register to start the next 16-bit transfer when this + // one finishes. + if src_total_len > 0 || dst_total_len > 0 { + let src_chunk = src_total_len as u16; + let dst_chunk = dst_total_len as u16; + let reload_size = cmp::max(src_chunk, dst_chunk); + self.spi.enable_reload(reload_size); + + src_total_len = src_total_len.saturating_sub(src_chunk as u32); + dst_total_len -= dst_total_len.saturating_sub(dst_chunk as u32); + } + + // We use this to exert backpressure on the TX state machine as the RX + // FIFO fills. Its initial value is the configured FIFO size, because + // the FIFO size varies on SPI blocks on the H7; it would be nice if we + // could read the configured FIFO size out of the block, but that does + // not appear to be possible. + // + // See reference manual table 409 for details. + let mut tx_permits = FIFO_DEPTH; + + // Track number of bytes sent and received. Sent bytes will lead + // received bytes. Received bytes indicate overall progress and + // completion. + let mut tx_count = 0; + let mut rx_count = 0; + + // The end of the exchange is signaled by rx_count reaching the + // overall_len. This is true even if the caller's rx lease is shorter or + // missing, because we have to pull bytes from the FIFO to avoid overrun + // conditions. + while rx_count < overall_len { + // At the end of this loop we're going to sleep if there's no + // obvious work to be done. Sleeping is not free, so, we only do it + // if this flag is set. (It defaults to set, we'll clear it if work + // appears below.) + let mut should_sleep = true; + + // TX engine. We continue moving bytes while these three conditions + // hold: + // - More bytes need to be sent. + // - Permits are available. + // - The TX FIFO has space. + while tx_count < overall_len + && tx_permits > 0 + && self.spi.can_tx_frame() + { + // The next byte to TX will come from the caller, if we haven't + // run off the end of their lease, or the fixed padding byte if + // we have. + let byte = if let Some(txbuf) = &mut tx { + // TODO: lint is buggy in 2024-04-04 toolchain, retest later + #[allow(clippy::manual_unwrap_or_default)] + if let Some(b) = txbuf.read() { + b + } else { + // We've hit the end of the lease. Stop checking. + tx = None; + 0 + } } else { - // We've hit the end of the lease. Stop checking. - tx = None; 0 - } - } else { - 0 - }; - - ringbuf_entry!(Trace::Tx(byte)); - self.spi.send8(byte); - tx_count += 1; - - // Consume one TX permit to make sure we don't overrun the RX - // fifo. - tx_permits -= 1; - - if tx_permits == 0 || tx_count == overall_len { - // We're either done, or we need to idle until the RX engine - // catches up. Either way, stop generating interrupts. - self.spi.disable_can_tx_interrupt(); - } + }; - // We don't adjust should_sleep in the TX engine because, if we - // leave this loop, we've done all the TX work we can -- and - // we're about to check for RX work unconditionally below. So, - // from the perspective of the TX engine, should_sleep is always - // true at this point, and the RX engine gets to make the final - // decision. - } + ringbuf_entry!(Trace::Tx(byte)); + self.spi.send8(byte); + tx_count += 1; - // Drain bytes from the RX FIFO. - while self.spi.can_rx_byte() { - // We didn't check rx_count < overall_len above because, if we - // got to that point, it would mean the SPI hardware gave us - // more bytes than we sent. This would be bad. And so, we'll - // detect that condition aggressively: - if rx_count >= overall_len { - panic!(); + // Consume one TX permit to make sure we don't overrun the RX + // fifo. + tx_permits -= 1; + + if tx_permits == 0 || tx_count == overall_len { + // We're either done, or we need to idle until the RX engine + // catches up. Either way, stop generating interrupts. + self.spi.disable_can_tx_interrupt(); + } + + // We don't adjust should_sleep in the TX engine because, if we + // leave this loop, we've done all the TX work we can -- and + // we're about to check for RX work unconditionally below. So, + // from the perspective of the TX engine, should_sleep is always + // true at this point, and the RX engine gets to make the final + // decision. } - // Pull byte from RX FIFO. - let b = self.spi.recv8(); - ringbuf_entry!(Trace::Rx(b)); - rx_count += 1; + // Drain bytes from the RX FIFO. + while self.spi.can_rx_byte() { + // We didn't check rx_count < overall_len above because, if we + // got to that point, it would mean the SPI hardware gave us + // more bytes than we sent. This would be bad. And so, we'll + // detect that condition aggressively: + if rx_count >= overall_len { + panic!(); + } - // Allow another byte to be inserted in the TX FIFO. - tx_permits += 1; + // Pull byte from RX FIFO. + let b = self.spi.recv8(); + ringbuf_entry!(Trace::Rx(b)); + rx_count += 1; + + // Allow another byte to be inserted in the TX FIFO. + tx_permits += 1; + + // Deposit the byte if we're still within the bounds of the + // caller's incoming lease. + if let Some(rx_reader) = &mut rx { + if rx_reader.write(b).is_err() { + // We're off the end. Stop checking. + rx = None; + } + } - // Deposit the byte if we're still within the bounds of the - // caller's incoming lease. - if let Some(rx_reader) = &mut rx { - if rx_reader.write(b).is_err() { - // We're off the end. Stop checking. - rx = None; + // By releasing a TX permit, we might have unblocked the TX + // engine. We can detect this when tx_permits goes 0->1. If this + // occurs, we should turn its interrupt back on, but only if + // it's still working. + if tx_permits == 1 && tx_count < overall_len { + self.spi.enable_can_tx_interrupt(); } - } - // By releasing a TX permit, we might have unblocked the TX - // engine. We can detect this when tx_permits goes 0->1. If this - // occurs, we should turn its interrupt back on, but only if - // it's still working. - if tx_permits == 1 && tx_count < overall_len { - self.spi.enable_can_tx_interrupt(); + // We've done some work, which means some time has elapsed, + // which means it's possible that room in the TX FIFO has opened + // up. So, let's not sleep. + should_sleep = false; } - // We've done some work, which means some time has elapsed, - // which means it's possible that room in the TX FIFO has opened - // up. So, let's not sleep. - should_sleep = false; - } + if should_sleep { + ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); - if should_sleep { - ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); + if self.spi.check_overrun() { + panic!(); + } - if self.spi.check_overrun() { - panic!(); + // Allow the controller interrupt to post to our + // notification set. + sys_irq_control(self.irq_mask, true); + // Wait for our notification set to get, well, set. + sys_recv_notification(self.irq_mask); } + } - // Allow the controller interrupt to post to our - // notification set. - sys_irq_control(self.irq_mask, true); - // Wait for our notification set to get, well, set. We ignore - // the result of this because an error would mean the kernel - // violated the ABI, which we can't usefully respond to. - let _ = sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); + // If there's more work to do, the TSERF interrupt should have fired. + if self.spi.check_tserf() { + // Another chunk was reloaded; update the remaining transfer + // size, clear the `TSERF` interrupt, and continue. + let src_len = src_total_len as u16; + let dst_len = dst_total_len as u16; + overall_len = + cmp::max(src_total_len as u16, dst_total_len as u16); + + // Clear the IRQ + self.spi.clear_tserf(); + + ringbuf_entry!(Trace::Reload { + op, + src_len, + dst_len, + }); + } else { + // Otherwise, we're done. + break; } } @@ -731,36 +766,36 @@ impl SpiServer for SpiServerCore { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError> { - SpiServerCore::exchange(self, device_index, src, dest).map_err(|e| { - match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - } - }) + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::exchange(self, device_index, src, dest).unwrap_lite(); + Ok(()) } - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError> { - SpiServerCore::write(self, device_index, src).map_err(|e| match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - }) + fn write( + &self, + device_index: u8, + src: &[u8], + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::write(self, device_index, src).unwrap_lite(); + Ok(()) } - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError> { - SpiServerCore::read(self, device_index, dest).map_err(|e| match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - }) + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::read(self, device_index, dest).unwrap_lite(); + Ok(()) } fn lock( diff --git a/drv/stm32h7-spi-server/src/main.rs b/drv/stm32h7-spi-server/src/main.rs index 83a8df358..af62ec4e9 100644 --- a/drv/stm32h7-spi-server/src/main.rs +++ b/drv/stm32h7-spi-server/src/main.rs @@ -61,13 +61,13 @@ impl InOrderSpiImpl for ServerImpl { _: &RecvMessage, device_index: u8, dest: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .read::>( device_index, dest.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn write( @@ -75,13 +75,13 @@ impl InOrderSpiImpl for ServerImpl { _: &RecvMessage, device_index: u8, src: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .write::>( device_index, src.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn exchange( @@ -90,14 +90,14 @@ impl InOrderSpiImpl for ServerImpl { device_index: u8, src: LenLimit, 65535>, dest: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .exchange::, LeaseBufWriter<_, BUFSIZ>>( device_index, src.into_inner().into(), dest.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn lock( diff --git a/drv/stm32h7-spi/src/lib.rs b/drv/stm32h7-spi/src/lib.rs index 8a720bd42..fc3c83f28 100644 --- a/drv/stm32h7-spi/src/lib.rs +++ b/drv/stm32h7-spi/src/lib.rs @@ -117,6 +117,36 @@ impl Spi { self.reg.cr1.modify(|_, w| w.spe().set_bit()); } + /// Set an additional 16-bit number of bytes to transfer when the current + /// value in `TSIZE` has been transferred. + /// + /// Per the manual: + /// > When the number of data programmed into `TSIZE` is transacted and if + /// > `TSER` contains a non-zero value, the content of `TSER` is copied + /// > into `TSIZE`, and `TSER` value is cleared automatically. + /// > The transaction is then extended by a number of data corresponding to + /// > the value reloaded into `TSIZE``. The `EOT` event is not raised in + /// > this case as the transaction continues. + /// > --- RM0433 Rev 8, p. 2177 + pub fn enable_reload(&self, tser: u16) { + self.reg.cr2.modify(|r, w| { + // This is (probably) a PAC bug: the `stm32h7` PAC doesn't let us write + // to the TSER field in CR2, only read from it. But, the STM32 manual + // (RM0433) specifically describes writing to TSER in order to extend a + // transfer. So, let's do that... + // + // If the PAC exposed writing to TSER, this should just be + // self.reg.cr2.modify(|_, w| w.tser().bits(tser); + let bits = r.tsize().bits() as u32 | ((tser as u32) << 16); + unsafe { w.bits(bits) } + }); + // Let's receive an interrupt when the reloaded transfer begins. + // N.B. This is separate from `enable_transfer_interrupts` because we + // only care about receiving the TSERFIE interrupt when the TSER + // register is in use. + self.reg.ier.modify(|_, w| w.tserfie().set_bit()) + } + pub fn start(&self) { self.reg.cr1.modify(|_, w| w.cstart().set_bit()); // Clear EOT flag @@ -269,6 +299,16 @@ impl Spi { self.reg.ifcr.write(|w| w.eotc().set_bit()); } + /// Returns `true` if the `TSER` register has reloaded the next transfer + /// chunk size. + pub fn check_tserf(&self) -> bool { + self.reg.sr.read().tserf().is_loaded() + } + + pub fn clear_tserf(&self) { + self.reg.ifcr.write(|w| w.tserfc().set_bit()); + } + pub fn read_status(&self) -> u32 { self.reg.sr.read().bits() } diff --git a/drv/stm32h7-sprot-server/src/main.rs b/drv/stm32h7-sprot-server/src/main.rs index 2bbbf253a..5425be86a 100644 --- a/drv/stm32h7-sprot-server/src/main.rs +++ b/drv/stm32h7-sprot-server/src/main.rs @@ -7,7 +7,6 @@ #![deny(elided_lifetimes_in_paths)] use attest_api::{AttestError, HashAlgorithm, NONCE_MAX_SIZE, NONCE_MIN_SIZE}; -use core::convert::Into; use drv_lpc55_update_api::{ RotBootInfo, RotPage, SlotId, SwitchDuration, UpdateTarget, }; diff --git a/drv/stm32h7-update-server/src/main.rs b/drv/stm32h7-update-server/src/main.rs index bb1ff367e..a263a5c4a 100644 --- a/drv/stm32h7-update-server/src/main.rs +++ b/drv/stm32h7-update-server/src/main.rs @@ -264,12 +264,7 @@ impl<'a> ServerImpl<'a> { // Wait for EOP notification via interrupt. loop { - sys_recv_closed( - &mut [], - notifications::FLASH_IRQ_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::FLASH_IRQ_MASK); if self.flash.bank2().sr.read().eop().bit() { break; } else { diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index def47b7cb..a38be81a7 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -343,17 +343,17 @@ fn main() -> ! { wfi: |notification, timeout| { const TIMER_NOTIFICATION: u32 = 1 << 31; - let dead = sys_get_timer().now.checked_add(timeout.0).unwrap_lite(); + // If the driver passes in a timeout that is large enough that it + // would overflow the kernel's 64-bit timestamp space... well, we'll + // do the best we can without compiling in an unlikely panic. + let dead = sys_get_timer().now.saturating_add(timeout.0); + sys_set_timer(Some(dead), TIMER_NOTIFICATION); - let m = sys_recv_closed( - &mut [], - notification | TIMER_NOTIFICATION, - TaskId::KERNEL, - ) - .unwrap_lite(); + let notification = + sys_recv_notification(notification | TIMER_NOTIFICATION); - if m.operation == TIMER_NOTIFICATION { + if notification == TIMER_NOTIFICATION { I2cControlResult::TimedOut } else { sys_set_timer(None, TIMER_NOTIFICATION); @@ -451,7 +451,7 @@ fn main() -> ! { let mut nread = 0; - match controller.write_read( + let controller_result = controller.write_read( addr, winfo.len, |pos| wbuf.read_at(pos), @@ -470,7 +470,8 @@ fn main() -> ! { rbuf.write_at(pos, byte) }, &ctrl, - ) { + ); + match controller_result { Err(code) => { // // NoDevice errors aren't hugely interesting -- diff --git a/drv/stm32xx-i2c/src/ltc4306.rs b/drv/stm32xx-i2c/src/ltc4306.rs index e56fe3317..82f38b64f 100644 --- a/drv/stm32xx-i2c/src/ltc4306.rs +++ b/drv/stm32xx-i2c/src/ltc4306.rs @@ -93,7 +93,7 @@ fn read_reg_u8( let mut rval = 0u8; let wlen = 1; - match controller.write_read( + let controller_result = controller.write_read( mux.address, wlen, |_| Some(reg), @@ -103,7 +103,8 @@ fn read_reg_u8( Some(()) }, ctrl, - ) { + ); + match controller_result { Err(code) => Err(mux.error_code(code)), _ => Ok(rval), } diff --git a/drv/stm32xx-i2c/src/max7358.rs b/drv/stm32xx-i2c/src/max7358.rs index a0d60330c..7822342d2 100644 --- a/drv/stm32xx-i2c/src/max7358.rs +++ b/drv/stm32xx-i2c/src/max7358.rs @@ -76,7 +76,7 @@ fn read_regs( rbuf: &mut [u8], ctrl: &I2cControl, ) -> Result<(), ResponseCode> { - match controller.write_read( + let controller_result = controller.write_read( mux.address, 0, |_| Some(0), @@ -86,7 +86,8 @@ fn read_regs( Some(()) }, ctrl, - ) { + ); + match controller_result { Err(code) => Err(mux.error_code(code)), _ => { for (i, &byte) in rbuf.iter().enumerate() { diff --git a/drv/stm32xx-sys-api/Cargo.toml b/drv/stm32xx-sys-api/Cargo.toml index 916136495..d46974634 100644 --- a/drv/stm32xx-sys-api/Cargo.toml +++ b/drv/stm32xx-sys-api/Cargo.toml @@ -9,6 +9,7 @@ cfg-if.workspace = true idol-runtime.workspace = true num-traits.workspace = true zerocopy.workspace = true +serde.workspace = true counters = { path = "../../lib/counters" } derive-idol-err = { path = "../../lib/derive-idol-err" } diff --git a/drv/stm32xx-sys-api/src/h7.rs b/drv/stm32xx-sys-api/src/h7.rs index 57559dd25..a8da1b764 100644 --- a/drv/stm32xx-sys-api/src/h7.rs +++ b/drv/stm32xx-sys-api/src/h7.rs @@ -89,9 +89,9 @@ pub enum Peripheral { #[cfg(any(feature = "h753", feature = "h743"))] Rng = periph(Group::Ahb2, 6), - #[cfg(any(feature = "h753"))] + #[cfg(feature = "h753")] Hash = periph(Group::Ahb2, 5), - #[cfg(any(feature = "h753"))] + #[cfg(feature = "h753")] Crypt = periph(Group::Ahb2, 4), #[cfg(feature = "h7b3")] diff --git a/drv/stm32xx-sys-api/src/lib.rs b/drv/stm32xx-sys-api/src/lib.rs index b944ae14c..5931eb230 100644 --- a/drv/stm32xx-sys-api/src/lib.rs +++ b/drv/stm32xx-sys-api/src/lib.rs @@ -20,6 +20,7 @@ cfg_if::cfg_if! { use derive_idol_err::IdolError; use userlib::*; +use zerocopy::AsBytes; pub use drv_stm32xx_gpio_common::{ Alternate, Mode, OutputType, PinSet, Port, Pull, Speed, @@ -32,6 +33,44 @@ pub enum RccError { NoSuchPeripheral = 1, } +/// Configures edge sensitivity for a GPIO interrupt +#[derive( + Copy, Clone, FromPrimitive, PartialEq, Eq, AsBytes, serde::Deserialize, +)] +// NOTE: This `repr` attribute is *not* necessary for +// serialization/deserialization, but it is used to allow casting to `u8` in the +// `Edge::{is_rising, is_falling}` methods. The current implementation of those +// methods with bit-and tests generates substantially fewer instructions than +// using `matches!` (see: https://godbolt.org/z/j5fdPfz3c). +#[repr(u8)] +pub enum Edge { + /// The interrupt will trigger on the rising edge only. + Rising = 0b01, + /// The interrupt will trigger on the falling edge only. + Falling = 0b10, + /// The interrupt will trigger on both teh rising and falling edge. + Both = 0b11, +} + +/// Describes which operation is performed by the [`Sys::gpio_irq_control`] IPC. +#[derive( + Copy, Clone, FromPrimitive, PartialEq, Eq, AsBytes, serde::Deserialize, +)] +// repr attribute is required for the derived `AsBytes` implementation +#[repr(u8)] +pub enum IrqControl { + /// Disable any interrupts mapped to the provided notification mask. + Disable = 0, + /// Enable any interrupts mapped to the provided notification mask. + Enable, + /// Check if any interrupts mapped to the provided notification mask have + /// been triggered, *without* enabling or disabling the interrupt. + /// + /// If an interrupt is currently enabled, it will remain enabled, while if + /// it is currently disabled, it will remain disabled. + Check, +} + impl Sys { /// Requests that the clock to a peripheral be turned on. /// @@ -285,4 +324,46 @@ impl Sys { } } +impl Edge { + /// Returns `true` if this edge sensitivity should trigger on the rising + /// edge. + pub fn is_rising(&self) -> bool { + *self as u8 & Self::Rising as u8 != 0 + } + + /// Returns `true` if this edge sensitivity should trigger on the falling + /// edge. + pub fn is_falling(&self) -> bool { + *self as u8 & Self::Falling as u8 != 0 + } +} + +impl core::ops::BitOr for Edge { + type Output = Self; + + fn bitor(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (Edge::Rising, Edge::Rising) => Edge::Rising, + (Edge::Falling, Edge::Falling) => Edge::Falling, + _ => Edge::Both, + } + } +} + +impl From for IrqControl { + fn from(value: bool) -> Self { + if value { + IrqControl::Enable + } else { + IrqControl::Disable + } + } +} + +impl From> for IrqControl { + fn from(value: Option) -> Self { + value.map(Self::from).unwrap_or(Self::Check) + } +} + include!(concat!(env!("OUT_DIR"), "/client_stub.rs")); diff --git a/drv/stm32xx-sys/Cargo.toml b/drv/stm32xx-sys/Cargo.toml index 9ac31aed9..706b2c6fc 100644 --- a/drv/stm32xx-sys/Cargo.toml +++ b/drv/stm32xx-sys/Cargo.toml @@ -10,6 +10,7 @@ drv-stm32xx-uid = { path = "../../drv/stm32xx-uid" } hubris-num-tasks = { path = "../../sys/num-tasks", features = ["task-enum"], optional = true } task-jefe-api = { path="../../task/jefe-api" } userlib = { path = "../../sys/userlib" } +counters = { path = "../../lib/counters", optional = true } bitflags = { workspace = true } cfg-if = { workspace = true } @@ -34,14 +35,16 @@ g030 = ["family-stm32g0", "stm32g0/stm32g030", "drv-stm32xx-sys-api/g030", "drv- g031 = ["family-stm32g0", "stm32g0/stm32g031", "drv-stm32xx-sys-api/g031", "drv-stm32xx-gpio-common/model-stm32g031"] g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv-stm32xx-gpio-common/model-stm32g070"] g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"] + no-ipc-counters = ["idol/no-counters"] +no-panic = ["userlib/no-panic"] # Enable external interrupt controller support. -exti = ["dep:hubris-num-tasks"] +exti = ["dep:hubris-num-tasks", "dep:counters"] # Disables the Jefe dependency, for use in tests where the test-runner task is # used as supervisor, rather than Jefe. -# +# # TODO(eliza): eventually, it would be much better if tasks that depend on the # sys driver could run separately from the kernel tests, and use a real Jefe # rather than the test supervisor. But, for now, this makes it build. diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 526bf7a54..8e7849c9f 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -34,7 +34,8 @@ //! EXTI unit. From a user perspective, this works as follows: //! //! 1. `sys` gets configured in the `app.toml` to route interrupts on a specific -//! pin to a specific task, using a specific notification. +//! pin to a specific task, using a specific notification. See [the section +//! on EXTI configuration](#configuring-exti-notification) for details. //! 2. That task configures the pin using `sys.gpio_configure` and friends //! (probably as an input, possibly with a pull resistor). //! 3. That task _also_ configures the pin's edge sensitivity, using @@ -73,7 +74,7 @@ //! //! 4. Repeat. //! -//! ## Configuring EXTI GPIO notifications +//! ## Configuring EXTI notifications //! //! In order for a task to receive notifications about GPIO pin interrupts from //! `sys`, we must first add the following to the task's config section in @@ -152,6 +153,139 @@ //! # The name of the client task and the notification to post to it. //! owner = { name = "my-great-task", notification = "my-gpio-notification" } //! ``` +//! +//! ## Using EXTI notifications +//! +//! Once a task has been configured to receive notifications from `sys` for EXTI +//! GPIO interrupts, as discussed above, it can use the `sys` task's IPC +//! interface to configure and control the EXTI interrupts. +//! +//! First, the task must ensure that it includes the generated constants for +//! notifications. If the task does not already include this code, it must add a +//! call to `build_util::build_notifications()` in its `build.rs`, and include +//! the generated using: +//! +//! ```rust +//! include!(concat!(env!("OUT_DIR"), "/notifications.rs")); +//! ``` +//! +//! In order to receive notifications, the task must first configure the pin on +//! which it wishes to receive interrupts in input mode, using the +//! [`Sys::gpio_configure_input`] IPC interface. Optionally, if pull-up or +//! pull-down resistors are required, they may be configured by this IPC as +//! well. Then, the task must use the [`Sys::gpio_irq_configure`] IPC to +//! configure the edge sensitivity for the GPIO interrupts mapped to a +//! notification mask. Interrupts can trigger either on the rising edge, the +//! falling edge, or both. These configurations only need to be performed once, +//! unless the task wishes to change the pin's configuration at runtime. +//! +//! For example, if we wish to use the EXTI notification +//! `"my-gpio-notification"` for pin PC13, as shown in the above section, we +//! might add the following to our task's `main`: +//! +//! ```rust,no-run +//! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +//! use userlib::*; +//! +//! task_slot!(SYS, sys); +//! +//! #[export_name = "main"] +//! pub fn main() -> ! { +//! let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); +//! +//! // Configure the pin as an input, without pull-up or pull-down +//! // resistors: +//! sys.gpio_configure_input( +//! PinSet { +//! port: Port::C, +//! pin_mask: (1 << 13), +//! }, +//! Pull::None, +//! ); +//! +//! // Now, configure the pin to trigger interrupts on the rising edge: +//! sys.gpio_irq_configure( +//! notifications::MY_GPIO_NOTIFICATION_MASK, +//! Edge::Rising, // or `Edge::Falling`, `Edge::Both` +//! ); +//! +//! // Eventually we will do other things here +//! } +//!``` +//! +//! Once this is done, the task can enable a GPIO interrupt by calling the +//! [`Sys::gpio_irq_control`] IPC. This IPC takes two arguments: a mask of +//! notification bits to disable, and a mask of notification bits to enable. +//! Once the interrupt is enabled, the task will receive a notification when the +//! GPIO pin's level changes, based on the edge sensitivity configured above. +//! Once the interrupt has triggered a notification, it will be automatically +//! disabled until the task calls `gpio_irq_control` again to re-enable it. +//! +//! Thus, continuing from the above example, to wait for GPIO notifications in a +//! loop, we would write something like this: +//! +//! ```rust,no-run +//! # fn handle_interrupt() {} +//! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull, Edge, IrqControl}; +//! use userlib::*; +//! +//! task_slot!(SYS, sys); +//! +//! #[export_name = "main"] +//! pub fn main() -> ! { +//! let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); +//! +//! sys.gpio_configure_input( +//! PinSet { +//! port: Port::C, +//! pin_mask: (1 << 13), +//! }, +//! Pull::None, +//! ); +//! +//! sys.gpio_irq_configure( +//! notifications::MY_GPIO_NOTIFICATION_MASK, +//! Edge::Rising, // or `Edge::Falling`, `Edge::Both` +//! ); +//! +//! // First, enable the interrupt, so that we can receive our +//! // notification. We loop here to retry if the `sys` task has panicked. +//! while sys +//! .gpio_irq_control(notifications::MY_GPIO_NOTIFICATION_MASK, IrqControl::Enable) +//! .is_err() +//! {} +//! +//! // Wait to recieve notifications for our GPIO interrupt in a loop: +//! loop { +//! // Wait for a notification. +//! sys_recv_notification(notifications::MY_GPIO_NOTIFICATION_MASK); +//! +//! // Call `sys.gpio_irq_control()` to both re-enable the interrupt +//! // and ask the `sys` task to confirm for us that the interrupt has +//! // actually fired. +//! // +//! // If we did *not* want to re-enable the interrupt here, we could +//! // pass `IrqControl::Check` rather than `IrqControl::Enable`. +//! let fired = sys +//! .gpio_irq_control(notifications::MY_GPIO_NOTIFICATION_MASK, IrqControl::Enable) +//! // If the `sys` task panicked, just wait for another notification. +//! .unwrap_or(false) +//! if fired { +//! // If the sys task confirms that our interrupt has fired, do... +//! // whatever it is this task is supposed to do when that happens. +//! handle_interrupt(); +//! } +//! } +//! } +//!``` +//! +//! For a more complete example of using GPIO interrupts, see the +//! [`nucleo-user-button`] demo task, which toggles a user LED on an +//! STM32H7-NUCLEO dev board when the user button is pressed. +//! +//! [`nucleo-user-button`]: https://github.com/oxidecomputer/hubris/tree/master/task/nucleo-user-button #![no_std] #![no_main] @@ -187,7 +321,7 @@ cfg_if! { } use drv_stm32xx_gpio_common::{server::get_gpio_regs, Port}; -use drv_stm32xx_sys_api::{Group, RccError}; +use drv_stm32xx_sys_api::{Edge, Group, IrqControl, RccError}; use idol_runtime::{ClientError, NotificationHandler, RequestError}; #[cfg(not(feature = "test"))] use task_jefe_api::{Jefe, ResetReason}; @@ -263,11 +397,11 @@ fn main() -> ! { // API the way we use peripherals. let syscfg = unsafe { &*device::SYSCFG::ptr() }; - for (i, entry) in generated::EXTI_DISPATCH_TABLE.iter().enumerate() { + for (i, entry) in dispatch_table_iter() { // Process entries that are filled in... - if let &Some((port, _, _)) = entry { + if let &Some(ExtiDispatch { port, .. }) = entry { let register = i >> 2; - let slot = i % 2; + let slot = i & 0b11; // This is an array of 4-bit fields spread across 4 32-bit // registers. We're indexing them with i. There is really no @@ -392,6 +526,9 @@ fn main() -> ! { // which is an operation that can't actually be used to violate Rust // safety. exti: unsafe { &*device::EXTI::ptr() }, + + #[cfg(feature = "exti")] + exti_cpupr_2: 0, }; #[cfg(feature = "exti")] @@ -402,6 +539,9 @@ fn main() -> ! { } } +#[cfg(feature = "exti")] +counters::counters!(__EXTI_IRQ_COUNTERS, generated::ExtiIrq); + struct ServerImpl<'a> { rcc: &'a device::rcc::RegisterBlock, @@ -409,6 +549,15 @@ struct ServerImpl<'a> { /// pin change interrupts. #[cfg(feature = "exti")] exti: &'a device::exti::RegisterBlock, + + /// A bitfield tracking which EXTI interrupts have fired since the last time + /// their owners have called the `gpio_irq_control` IPC. This is necessary + /// as we must unset the sending bit in the *real* EXTI_CPUPR1 register on + /// receipt of an interrupt in order to receive another one, but we must + /// hang onto the pending state until the task that actually uses that + /// interrupt asks us for it. + #[cfg(feature = "exti")] + exti_cpupr_2: u16, } impl ServerImpl<'_> { @@ -515,60 +664,61 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { fn gpio_irq_control( &mut self, rm: &RecvMessage, - disable_mask: u32, - enable_mask: u32, - ) -> Result<(), RequestError> { + mask: u32, + op: IrqControl, + ) -> Result> { // We want to only include code for this if exti is requested. // Unfortunately the _operation_ is available unconditionally, but we'll // fault any clients who call it if it's unsupported (below). cfg_if! { if #[cfg(feature = "exti")] { - - // Keep track of which bits in the caller-provided masks - // actually matched things. - let mut used_bits = 0u32; - - for (i, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { - // Only use populated rows in the table - if let Some((_, tid, mask)) = entry { - // Ignore anything assigned to another task - if tid.index() == rm.sender.index() { - - // Apply disable first so that including the same - // thing in both masks is a no-op. - if mask & disable_mask != 0 { - // Record that these bits meant something. - used_bits |= mask; - - // Disable this source by _clearing_ the - // corresponding mask bit. - self.exti.cpuimr1.modify(|r, w| { - let new_value = r.bits() & !(1 << i); - // Safety: not actually unsafe, PAC didn't - // model this field right - unsafe { - w.bits(new_value) - } - }); - } - - if mask & enable_mask != 0 { - // Record that these bits meant something. - used_bits |= mask; - - // Enable this source by _setting_ the - // corresponding mask bit. - self.exti.cpuimr1.modify(|r, w| { - let new_value = r.bits() | (1 << i); - // Safety: not actually unsafe, PAC didn't - // model this field right - unsafe { - w.bits(new_value) - } - }); - } + // This mask will later be used for checking the stored + // interrupt pending state in `self.exti_cpupr_2` --- we'll put + // a 1 here for the index of every slot that's mapped to a + // notification in the enable/disable masks. + // + // We'll also use the presence of any bits in this mask in order + // to determine whether the caller actually provided masks that + // map to anything interesting, and reply-fault if they + // didn't. + let mut slot_mask = 0u16; + + for (i, _) in exti_dispatch_for(rm.sender, mask) { + // What bit do we touch for this entry? (Mask is to ensure + // that the compiler understands this shift cannot + // overflow.) + let bit = 1 << (i & 0xF); + + // Record that these bits meant something. + slot_mask |= bit; + + match op { + IrqControl::Enable => { + // Enable this source by _setting_ the + // corresponding mask bit. + self.exti.cpuimr1.modify(|r, w| { + let new_value = r.bits() | (bit as u32); + // Safety: not actually unsafe, PAC didn't + // model this field right + unsafe { w.bits(new_value) } + }); + }, + IrqControl::Disable => { + // Disable this source by _clearing_ the + // corresponding mask bit. + self.exti.cpuimr1.modify(|r, w| { + let new_value = r.bits() & !(bit as u32); + // Safety: not actually unsafe, PAC didn't + // model this field right + unsafe { + w.bits(new_value) + } + }); + }, + IrqControl::Check => { + // We are just checking if an IRQ has triggered, + // so don't actually mess with the source's mask + // register at all. } } } @@ -585,18 +735,22 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // work around the configuration system to achieve it -- we // don't want to add too much cost to the common case of "no // error." But we also don't want the error to go undetected. - if enable_mask & used_bits != enable_mask - || disable_mask & used_bits != disable_mask - { - Err(ClientError::BadMessageContents.fail()) - } else { - Ok(()) + if slot_mask == 0 { + return Err(ClientError::BadMessageContents.fail()); } + // Check if any interrupts are pending for the slots mapped to + // the caller's notification masks. + let pending = self.exti_cpupr_2 & slot_mask != 0; + // ...and clear those bits for the next interrupt. + self.exti_cpupr_2 &= !slot_mask; + + Ok(pending) + } else { // Suppress unused variable warnings (yay conditional // compilation) - let _ = (rm, enable_mask, disable_mask); + let _ = (rm, mask, op); // Fault any clients who try to use this in an image where it's // not included. @@ -609,8 +763,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { &mut self, rm: &RecvMessage, mask: u32, - rising: bool, - falling: bool, + edge: Edge, ) -> Result<(), RequestError> { // We want to only include code for this if exti is requested. // Unfortunately the _operation_ is available unconditionally, but we'll @@ -618,64 +771,42 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { cfg_if! { if #[cfg(feature = "exti")] { - // Guardrail: a call where both "rising" and "falling" are false - // will effectively disable the interrupt, in a _different_ way - // from gpio_irq_control. Currently we can't imagine a case - // where this is useful, but we _can_ imagine cases where - // tolerating it would hide bugs in code, and so we're making it - // an error. - // - // If you arrive here in the future wondering why this is being - // treated as an error, that's why. If you need this - // functionality, you can probably just remove this check and - // comment. - if !rising && !falling { - return Err(ClientError::BadMessageContents.fail()); - } - // Keep track of which bits in the caller-provided masks // actually matched things. let mut used_bits = 0u32; - for (i, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { - // Only use populated rows in the table - if let Some((_, tid, entry_mask)) = entry { - // Operate only on rows assigned to the sending task - // _and_ that are relevant based on the arguments. - if tid.index() == rm.sender.index() - && mask & entry_mask != 0 - { - used_bits |= mask; - - // Set or clear Rising Trigger Selection - // Register bit according to the rising flag - self.exti.rtsr1.modify(|r, w| { - let new_value = if rising { - r.bits() | (1 << i) - } else { - r.bits() & !(1 << i) - }; - unsafe { - w.bits(new_value) - } - }); - - // Set or clear Falling Trigger Selection - // Register bit according to the rising flag - self.exti.ftsr1.modify(|r, w| { - let new_value = if falling { - r.bits() | (1 << i) - } else { - r.bits() & !(1 << i) - }; - unsafe { - w.bits(new_value) - } - }); + for (i, entry) in exti_dispatch_for(rm.sender, mask) { + // (Mask is to ensure that the compiler understands this + // shift cannot overflow.) + let imask = 1 << (i & 0xF); + + used_bits |= entry.mask; + + // Set or clear Rising Trigger Selection + // Register bit according to the rising flag + self.exti.rtsr1.modify(|r, w| { + let new_value = if edge.is_rising() { + r.bits() | imask + } else { + r.bits() & !imask + }; + unsafe { + w.bits(new_value) } - } + }); + + // Set or clear Falling Trigger Selection + // Register bit according to the rising flag + self.exti.ftsr1.modify(|r, w| { + let new_value = if edge.is_falling() { + r.bits() | imask + } else { + r.bits() & !imask + }; + unsafe { + w.bits(new_value) + } + }); } // Check that all the set bits in the caller's provided masks @@ -699,7 +830,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { } else { // Suppress unused variable warnings (yay conditional // compilation) - let _ = (rm, mask, rising, falling); + let _ = (rm, mask, edge); // Fault any clients who try to use this in an image where it's // not included. @@ -709,6 +840,35 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { } } +#[cfg(feature = "exti")] +struct ExtiDispatch { + port: Port, + task: TaskId, + mask: u32, + name: generated::ExtiIrq, +} + +/// Iterates over the indices of EXTI sources mapped to the provided +/// notification `mask` for the task with ID `task`. +#[cfg(feature = "exti")] +fn exti_dispatch_for( + task: TaskId, + mask: u32, +) -> impl Iterator { + // This is semantically equivalent to iter.enumerate, but winds up handing + // the compiler very different code that avoids an otherwise-difficult panic + // site on an apparently-overflowing addition (that was not actually capable + // of overflowing). + dispatch_table_iter().filter_map(move |(i, entry)| { + let entry = entry.as_ref()?; + if task.index() == entry.task.index() && mask & entry.mask != 0 { + Some((i, entry)) + } else { + None + } + }) +} + impl NotificationHandler for ServerImpl<'_> { fn current_notification_mask(&self) -> u32 { cfg_if! { @@ -745,9 +905,13 @@ impl NotificationHandler for ServerImpl<'_> { let mut bits_to_acknowledge = 0u16; - for (pin_idx, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { + for pin_idx in 0..16 { + // TODO: this sure looks like it should be using + // iter.enumerate, doesn't it? Unfortunately that's not + // currently getting inlined by rustc, resulting in rather + // silly code containing panics. This is significantly + // smaller. + let entry = &generated::EXTI_DISPATCH_TABLE[pin_idx]; if pending_and_enabled & 1 << pin_idx != 0 { // A channel is pending! We need to handle this // basically like the kernel handles native hardware @@ -757,9 +921,11 @@ impl NotificationHandler for ServerImpl<'_> { // - Clear the pending bit (we have to do this // manually unlike native interrupts). - if let &Some((_, taskid, mask)) = entry { - let taskid = sys_refresh_task_id(taskid); - sys_post(taskid, mask); + if let &Some(ExtiDispatch { task, mask, name, .. }) = entry { + counters::count!(__EXTI_IRQ_COUNTERS, name); + + let task = sys_refresh_task_id(task); + sys_post(task, mask); } else { // spurious interrupt. // TODO: probably add this to a counter; it's @@ -772,6 +938,11 @@ impl NotificationHandler for ServerImpl<'_> { } if bits_to_acknowledge != 0 { + // Save pending bits so that when the tasks that own the + // interrupt(s) that fired call `Sys.gpio_irq_control` to + // check if their IRQs fired, we'll be able to tell them. + self.exti_cpupr_2 |= bits_to_acknowledge; + // Mask and unpend interrupts en masse to save like six // cycles because we'll totally notice in practice // @@ -1054,10 +1225,21 @@ cfg_if! { } } +#[cfg(feature = "exti")] +#[inline(always)] +fn dispatch_table_iter( +) -> impl Iterator)> { + // TODO: this sure looks like it should be using iter.enumerate, doesn't it? + // Unfortunately that's not currently getting inlined by rustc, resulting in + // rather silly code containing panics. This is significantly smaller. + (0..generated::EXTI_DISPATCH_TABLE.len()) + .zip(&generated::EXTI_DISPATCH_TABLE) +} + include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl { - use super::{Port, RccError}; + use super::{Edge, IrqControl, Port, RccError}; include!(concat!(env!("OUT_DIR"), "/server_stub.rs")); } diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 036a5200a..e40ee1c24 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -596,129 +596,122 @@ impl NotificationHandler for ServerImpl { #[export_name = "main"] fn main() -> ! { + // This is a temporary workaround that makes sure the FPGAs are up + // before we start doing things with them. A more sophisticated + // notification system will be put in place. + let seq = Sequencer::from(SEQ.get_task_id()); + + let transceivers = Transceivers::new(FRONT_IO.get_task_id()); + let leds = Leds::new( + &i2c_config::devices::pca9956b_front_leds_left(I2C.get_task_id()), + &i2c_config::devices::pca9956b_front_leds_right(I2C.get_task_id()), + ); + + let net = task_net_api::Net::from(NET.get_task_id()); + let thermal_api = Thermal::from(THERMAL.get_task_id()); + let sensor_api = Sensor::from(SENSOR.get_task_id()); + let (tx_data_buf, rx_data_buf) = claim_statics(); + let mut server = ServerImpl { + transceivers, + leds, + net, + modules_present: LogicalPortMask(0), + front_io_board_present: FrontIOStatus::NotReady, + led_error: Default::default(), + leds_initialized: false, + led_states: LedStates([LedState::Off; NUM_PORTS as usize]), + blink_on: false, + system_led_state: LedState::Off, + disabled: LogicalPortMask(0), + consecutive_nacks: [0; NUM_PORTS as usize], + thermal_api, + sensor_api, + thermal_models: [None; NUM_PORTS as usize], + }; + + // There are two timers, one for each communication bus: + #[derive(Copy, Clone, Enum)] + #[allow(clippy::upper_case_acronyms)] + enum Timers { + I2C, + SPI, + Blink, + } + let mut multitimer = Multitimer::::new(notifications::TIMER_BIT); + // Immediately fire each timer, then begin to service regularly + let now = sys_get_timer().now; + multitimer.set_timer( + Timers::I2C, + now, + Some(Repeat::AfterDeadline(I2C_INTERVAL)), + ); + multitimer.set_timer( + Timers::SPI, + now, + Some(Repeat::AfterDeadline(SPI_INTERVAL)), + ); + multitimer.set_timer( + Timers::Blink, + now, + Some(Repeat::AfterDeadline(BLINK_INTERVAL)), + ); + + let mut buffer = [0; idl::INCOMING_SIZE]; loop { - // This is a temporary workaround that makes sure the FPGAs are up - // before we start doing things with them. A more sophisticated - // notification system will be put in place. - let seq = Sequencer::from(SEQ.get_task_id()); - - let transceivers = Transceivers::new(FRONT_IO.get_task_id()); - let leds = Leds::new( - &i2c_config::devices::pca9956b_front_leds_left(I2C.get_task_id()), - &i2c_config::devices::pca9956b_front_leds_right(I2C.get_task_id()), - ); - - let net = task_net_api::Net::from(NET.get_task_id()); - let thermal_api = Thermal::from(THERMAL.get_task_id()); - let sensor_api = Sensor::from(SENSOR.get_task_id()); - let (tx_data_buf, rx_data_buf) = claim_statics(); - let mut server = ServerImpl { - transceivers, - leds, - net, - modules_present: LogicalPortMask(0), - front_io_board_present: FrontIOStatus::NotReady, - led_error: Default::default(), - leds_initialized: false, - led_states: LedStates([LedState::Off; NUM_PORTS as usize]), - blink_on: false, - system_led_state: LedState::Off, - disabled: LogicalPortMask(0), - consecutive_nacks: [0; NUM_PORTS as usize], - thermal_api, - sensor_api, - thermal_models: [None; NUM_PORTS as usize], - }; + if server.front_io_board_present == FrontIOStatus::NotReady { + server.front_io_board_present = match seq.front_io_board_ready() { + Ok(true) => { + ringbuf_entry!(Trace::FrontIOBoardReady(true)); + FrontIOStatus::Ready + } + Err(SeqError::NoFrontIOBoard) => { + ringbuf_entry!(Trace::FrontIOSeqErr( + SeqError::NoFrontIOBoard + )); + FrontIOStatus::NotPresent + } + _ => { + ringbuf_entry!(Trace::FrontIOBoardReady(false)); + FrontIOStatus::NotReady + } + }; - // There are two timers, one for each communication bus: - #[derive(Copy, Clone, Enum)] - #[allow(clippy::upper_case_acronyms)] - enum Timers { - I2C, - SPI, - Blink, - } - let mut multitimer = - Multitimer::::new(notifications::TIMER_BIT); - // Immediately fire each timer, then begin to service regularly - let now = sys_get_timer().now; - multitimer.set_timer( - Timers::I2C, - now, - Some(Repeat::AfterDeadline(I2C_INTERVAL)), - ); - multitimer.set_timer( - Timers::SPI, - now, - Some(Repeat::AfterDeadline(SPI_INTERVAL)), - ); - multitimer.set_timer( - Timers::Blink, - now, - Some(Repeat::AfterDeadline(BLINK_INTERVAL)), - ); - - let mut buffer = [0; idl::INCOMING_SIZE]; - loop { - if server.front_io_board_present == FrontIOStatus::NotReady { - server.front_io_board_present = match seq.front_io_board_ready() - { - Ok(true) => { - ringbuf_entry!(Trace::FrontIOBoardReady(true)); - FrontIOStatus::Ready - } - Err(SeqError::NoFrontIOBoard) => { - ringbuf_entry!(Trace::FrontIOSeqErr( - SeqError::NoFrontIOBoard - )); - FrontIOStatus::NotPresent - } - _ => { - ringbuf_entry!(Trace::FrontIOBoardReady(false)); - FrontIOStatus::NotReady + // If a board is present, attempt to initialize its + // LED drivers + if server.front_io_board_present == FrontIOStatus::Ready { + ringbuf_entry!(Trace::LEDInit); + match server.transceivers.enable_led_controllers() { + Ok(_) => server.led_init(), + Err(e) => { + ringbuf_entry!(Trace::LEDEnableError(e)) } }; - - // If a board is present, attempt to initialize its - // LED drivers - if server.front_io_board_present == FrontIOStatus::Ready { - ringbuf_entry!(Trace::LEDInit); - match server.transceivers.enable_led_controllers() { - Ok(_) => server.led_init(), - Err(e) => { - ringbuf_entry!(Trace::LEDEnableError(e)) - } - }; - } } + } - multitimer.poll_now(); - for t in multitimer.iter_fired() { - match t { - Timers::I2C => { - // Handle the Front IO status checking as part of this - // loop because the frequency is what we had before and - // the server itself has no knowledge of the sequencer. - server.handle_i2c_loop(); - } - Timers::SPI => { - if server.front_io_board_present == FrontIOStatus::Ready - { - server.handle_spi_loop(); - } - } - Timers::Blink => { - server.blink_on = !server.blink_on; + multitimer.poll_now(); + for t in multitimer.iter_fired() { + match t { + Timers::I2C => { + // Handle the Front IO status checking as part of this + // loop because the frequency is what we had before and + // the server itself has no knowledge of the sequencer. + server.handle_i2c_loop(); + } + Timers::SPI => { + if server.front_io_board_present == FrontIOStatus::Ready { + server.handle_spi_loop(); } } + Timers::Blink => { + server.blink_on = !server.blink_on; + } } - - server.check_net( - tx_data_buf.as_mut_slice(), - rx_data_buf.as_mut_slice(), - ); - idol_runtime::dispatch(&mut buffer, &mut server); } + + server + .check_net(tx_data_buf.as_mut_slice(), rx_data_buf.as_mut_slice()); + idol_runtime::dispatch(&mut buffer, &mut server); } } //////////////////////////////////////////////////////////////////////////////// diff --git a/drv/vsc-err/Cargo.toml b/drv/vsc-err/Cargo.toml index 366182a86..54444ddf3 100644 --- a/drv/vsc-err/Cargo.toml +++ b/drv/vsc-err/Cargo.toml @@ -5,7 +5,6 @@ edition = "2021" [dependencies] counters = { path = "../../lib/counters" } -drv-spi-api = { path = "../spi-api" } idol-runtime = { workspace = true } # This section is here to discourage RLS/rust-analyzer from doing test builds, diff --git a/drv/vsc-err/src/lib.rs b/drv/vsc-err/src/lib.rs index a20a73973..9c82b5ba0 100644 --- a/drv/vsc-err/src/lib.rs +++ b/drv/vsc-err/src/lib.rs @@ -9,12 +9,10 @@ #![no_std] -use drv_spi_api::SpiError; use idol_runtime::ServerDeath; #[derive(Copy, Clone, Eq, PartialEq, counters::Count)] pub enum VscError { - SpiError(#[count(children)] SpiError), ServerDied, /// Error code produced by a proxy device handling PHY register /// reads/writes. @@ -100,12 +98,6 @@ pub enum VscError { OutOfRange, } -impl From for VscError { - fn from(s: SpiError) -> Self { - Self::SpiError(s) - } -} - impl From for VscError { fn from(_s: ServerDeath) -> Self { Self::ServerDied diff --git a/drv/vsc85xx/src/vsc8562.rs b/drv/vsc85xx/src/vsc8562.rs index ab667c1cd..fe2104844 100644 --- a/drv/vsc85xx/src/vsc8562.rs +++ b/drv/vsc85xx/src/vsc8562.rs @@ -2,8 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use core::convert::TryInto; - use zerocopy::{AsBytes, FromBytes}; use crate::{Phy, PhyRw, Trace}; diff --git a/idl/attest.idol b/idl/attest.idol index bbfff102c..c4267a515 100644 --- a/idl/attest.idol +++ b/idl/attest.idol @@ -16,7 +16,7 @@ Interface( "cert": ( doc: "Get a cert from the RoT-R", args: { - "index" : "u32", + "index" : "u32", "offset" : "u32", }, leases: { @@ -71,6 +71,7 @@ Interface( ), "log_len": ( doc: "Get length of the serialized measurement log", + args: {}, reply: Result( ok: "u32", err: Complex("AttestError"), @@ -80,6 +81,7 @@ Interface( ), "attest": ( doc: "Get an attestation", + args: {}, leases: { "nonce": (type: "[u8]", read: true, max_len: Some(128)), "dest": (type: "[u8]", write: true), @@ -92,6 +94,7 @@ Interface( ), "attest_len": ( doc: "Get the length of an attestation", + args: {}, reply: Result( ok: "u32", err: Complex("AttestError"), diff --git a/idl/gimlet-seq.idol b/idl/gimlet-seq.idol index 321fc19dd..96201f2b7 100644 --- a/idl/gimlet-seq.idol +++ b/idl/gimlet-seq.idol @@ -5,6 +5,7 @@ Interface( ops: { "get_state": ( doc: "Return the power state", + args: {}, reply: Simple(( type: "drv_gimlet_state::PowerState", recv: FromPrimitive("u8"), diff --git a/idl/ignition.idol b/idl/ignition.idol index ca4e16c94..90b146bba 100644 --- a/idl/ignition.idol +++ b/idl/ignition.idol @@ -88,7 +88,7 @@ Interface( err: CLike("drv_ignition_api::IgnitionError"), ), ), - "link_events": ( + "link_events": ( doc: "Return all transceiver events for the given port", args: { "port": "u8", diff --git a/idl/lpc55-pins.idol b/idl/lpc55-pins.idol index 390587b32..69e5b66af 100644 --- a/idl/lpc55-pins.idol +++ b/idl/lpc55-pins.idol @@ -1,46 +1,45 @@ Interface( name: "Pins", ops: { - "iocon_configure_raw": ( - args : { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "conf": "u32", - }, - reply: Simple("()"), - idempotent: true, - ), - "set_dir": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "dir": ( type: "Direction", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "set_val": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "val": ( type : "Value", recv: FromPrimitive("u8")), - }, - reply: Simple("()"), - idempotent: true, - ), - "read_val": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - }, - reply: Simple((type : "Value", recv: FromPrimitive("u8"))), - idempotent: true, - ), - "toggle": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - }, - reply: Result( - ok: "()", - err: ServerDeath, - ) - ), - - } + "iocon_configure_raw": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "conf": "u32", + }, + reply: Simple("()"), + idempotent: true, + ), + "set_dir": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "dir": (type: "Direction", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "set_val": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "val": (type: "Value", recv: FromPrimitive("u8")), + }, + reply: Simple("()"), + idempotent: true, + ), + "read_val": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + }, + reply: Simple((type: "Value", recv: FromPrimitive("u8"))), + idempotent: true, + ), + "toggle": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + }, + reply: Result( + ok: "()", + err: ServerDeath, + ) + ), + } ) diff --git a/idl/lpc55-update.idol b/idl/lpc55-update.idol index 1bc217c76..4b2d9d60d 100644 --- a/idl/lpc55-update.idol +++ b/idl/lpc55-update.idol @@ -5,7 +5,7 @@ Interface( ops: { "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "usize", err: CLike("drv_update_api::UpdateError"), @@ -13,10 +13,10 @@ Interface( ), "prep_image_update": ( doc: "Do any necessary preparation for writing the image. This may include erasing flash and unlocking registers", - args : { + args: { "image_type": "UpdateTarget", }, - reply : Result( + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -25,9 +25,9 @@ Interface( "write_one_block": ( doc: "Write a single block of an update image to the designated location.", args: { - "block_num" : "usize", + "block_num": "usize", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(1024)), }, reply: Result ( @@ -37,31 +37,31 @@ Interface( ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "current_version": ( doc: "Get the current image version", - args : { }, - reply : Simple("ImageVersion"), + args: {}, + reply: Simple("ImageVersion"), idempotent: true, encoding: Hubpack ), "status": ( doc: "Get info about installed images (deprecated - use rot_boot_info)", - args: { }, - reply : Result( + args: {}, + reply: Result( ok: "stage0_handoff::RotBootState", err: Complex("HandoffDataLoadError"), ), @@ -70,8 +70,8 @@ Interface( ), "rot_boot_info": ( doc: "RoT Boot selection and preference info", - args: { }, - reply : Result( + args: {}, + reply: Result( ok: "RotBootInfo", err: CLike("drv_update_api::UpdateError") ), @@ -112,7 +112,7 @@ Interface( "slot": "SlotId", "duration": "SwitchDuration", }, - reply : Result( + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -121,7 +121,8 @@ Interface( ), "reset": ( doc: "Reset unless an update is in progress.", - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -131,9 +132,9 @@ Interface( "read_rot_page": ( doc: "Read CMPA/CFPA page", args: { - "page": "RotPage", - }, - leases : { + "page": "RotPage", + }, + leases: { "data": (type: "[u8]", write: true, max_len: Some(512)), }, reply: Result ( diff --git a/idl/sbrmi.idol b/idl/sbrmi.idol index 43ea7c093..74f16981f 100644 --- a/idl/sbrmi.idol +++ b/idl/sbrmi.idol @@ -3,27 +3,27 @@ Interface( name: "Sbrmi", ops: { - "nthreads": ( + "nthreads": ( reply: Result( ok: "u8", err: CLike("SbrmiError"), ), idempotent: true, - ), - "enabled": ( + ), + "enabled": ( reply: Result( ok: "[u8; 16]", err: CLike("SbrmiError"), ), idempotent: true, - ), - "alert": ( + ), + "alert": ( reply: Result( ok: "[u8; 16]", err: CLike("SbrmiError"), ), idempotent: true, - ), + ), "cpuid": ( args: { "thread": "u8", diff --git a/idl/sidecar-seq.idol b/idl/sidecar-seq.idol index e1ce53302..3e037f520 100644 --- a/idl/sidecar-seq.idol +++ b/idl/sidecar-seq.idol @@ -29,6 +29,7 @@ Interface( ), "tofino_seq_state": ( doc: "Return the Tofino sequencer state", + args: {}, reply: Result( ok: ( type: "TofinoSeqState", @@ -39,6 +40,7 @@ Interface( ), "tofino_seq_error": ( doc: "Return the Tofino sequencer error, if any", + args: {}, reply: Result( ok: ( type: "TofinoSeqError", diff --git a/idl/spi.idol b/idl/spi.idol index a601b66e1..5bdb4a345 100644 --- a/idl/spi.idol +++ b/idl/spi.idol @@ -14,7 +14,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "write": ( @@ -27,7 +27,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "exchange": ( @@ -41,7 +41,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "lock": ( diff --git a/idl/sprot.idol b/idl/sprot.idol index 5315b786b..822646b11 100644 --- a/idl/sprot.idol +++ b/idl/sprot.idol @@ -5,9 +5,9 @@ Interface( ops: { "status": ( doc: "Return status about the sprot protocol", - reply : Result( - ok: "SprotStatus", - err: Complex("SprotError"), + reply: Result( + ok: "SprotStatus", + err: Complex("SprotError"), ), encoding: Hubpack, idempotent: true, @@ -18,7 +18,7 @@ Interface( ok: "SprotIoStats", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "rot_state": ( @@ -39,18 +39,18 @@ Interface( ok: "PulseStatus", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, ), // The RoT update API is copy and pasted from idl/update.idol. "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "u32", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "prep_image_update": ( @@ -58,31 +58,31 @@ Interface( args: { "target": "UpdateTarget", }, - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "write_one_block": ( doc: "Write a single block of an update image to the designated location.", - args: { - "block_num" : "u32", + args: { + "block_num": "u32", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(512)), }, reply: Result ( ok: "()", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -90,8 +90,8 @@ Interface( ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -103,7 +103,7 @@ Interface( "slot": "SlotId", "duration": "SwitchDuration", }, - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -112,7 +112,7 @@ Interface( ), "reset": ( doc: "Reset", - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -124,7 +124,7 @@ Interface( args: { "address": "u32", }, - reply : Result( + reply: Result( ok: "()", err: Complex("DumpOrSprotError"), ), @@ -153,7 +153,7 @@ Interface( ok: "()", err: Complex("RawCabooseOrSprotError"), ), - leases : { + leases: { "out": (type: "[u8]", write: true), }, idempotent: true, @@ -180,7 +180,7 @@ Interface( "cert_len": ( doc: "Get length of a cert in the cert chain", args: { - "index" : "u32", + "index": "u32", }, reply: Result( ok: "u32", @@ -192,8 +192,8 @@ Interface( "cert": ( doc: "Get a cert from the alias cert chaing", args: { - "index" : "u32", - "offset" : "u32", + "index": "u32", + "offset": "u32", }, leases: { "dest": (type: "[u8]", write: true), @@ -237,7 +237,7 @@ Interface( "log": ( doc: "Get the measurement log", args: { - "offset" : "u32", + "offset": "u32", }, leases: { "dest": (type: "[u8]", write: true), @@ -259,12 +259,11 @@ Interface( ), "attest": ( doc: "Get an attestation", - args: {}, leases: { "nonce": (type: "[u8]", read: true, max_len: Some(128)), "dest": (type: "[u8]", write: true), }, - reply: Result( + reply: Result( ok: "()", err: Complex("AttestOrSprotError"), ), diff --git a/idl/stm32h7-update.idol b/idl/stm32h7-update.idol index 88f2cca66..013182741 100644 --- a/idl/stm32h7-update.idol +++ b/idl/stm32h7-update.idol @@ -5,7 +5,7 @@ Interface( ops: { "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "usize", err: CLike("drv_update_api::UpdateError"), @@ -13,7 +13,8 @@ Interface( ), "prep_image_update": ( doc: "Do any necessary preparation for writing the image. This may include erasing flash and unlocking registers", - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -21,9 +22,9 @@ Interface( "write_one_block": ( doc: "Write a single block of an update image to the designated location.", args: { - "block_num" : "usize", + "block_num": "usize", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(1024)), }, reply: Result ( @@ -33,24 +34,24 @@ Interface( ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "current_version": ( doc: "Get the current image version", - args : { }, - reply : Simple("ImageVersion"), + args: {}, + reply: Simple("ImageVersion"), idempotent: true, encoding: Hubpack ), diff --git a/idl/stm32xx-sys.idol b/idl/stm32xx-sys.idol index 16d240de3..d24cbbf29 100644 --- a/idl/stm32xx-sys.idol +++ b/idl/stm32xx-sys.idol @@ -99,31 +99,46 @@ Interface( // Configures some set of EXTI sources associated with the caller, // using the caller's notification bit space to name them. Any // sources included in the `mask` will be affected. - // - // If neither rising nor falling is true, the pin will not be - // capable of producing an interrupt, even if enabled. "gpio_irq_configure": ( args: { "mask": "u32", - "rising": "bool", - "falling": "bool", + "sensitivity": ( + type: "Edge", + recv: FromPrimitive("u8"), + ), }, reply: Simple("()"), idempotent: true, ), - // Changes a subset of EXTI sources mapped to the calling - // task, using the caller's notification bit space to - // name them. Any sources mapped to bits in the disable - // mask will be disabled; any sources mapped to bits in - // the enabled mask will be enabled. + // Performs an operation on a subset of EXTI sources mapped to the + // calling task, using the caller's notification bit space to name + // them, and returns whether any interrupts mapped to the provided + // notification bits have been triggered. + // + // Depending on the value of the `op` argument, this operation can + // either enable the intterrupts mapped to the notification mask + // (if `op` is `IrqControl::Enable`), disable those interrupts (if + // `op` is `IrqControl::Disable`), or do neither (if `op` is + // `IrqControl::Check`). Regardless of which operation is performed, + // this IPC will always return `true` if any interrupt in the + // provided notification mask has been triggered since the last time + // this IPC was called, and resets this status for the next call to + // this IPC. If an interrupt was enabled when this IPC is called with + // `IrqControl::Check` as the `op`, that interrupt will remain + // enabled, and if an interrupt was disabled, it will remain disabled. "gpio_irq_control": ( args: { - "disable_mask": "u32", - "enable_mask": "u32", + "mask": "u32", + "op": ( + type: "IrqControl", + recv: FromPrimitive("u8"), + ), }, - reply: Simple("()"), - idempotent: true, + reply: Result( + ok: "bool", + err: ServerDeath, + ), ), }, ) diff --git a/idl/syscon.idol b/idl/syscon.idol index 2c7619ac1..1b0437b39 100644 --- a/idl/syscon.idol +++ b/idl/syscon.idol @@ -1,38 +1,37 @@ Interface( name: "Syscon", ops: { - "enable_clock": ( - args : { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "disable_clock": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "enter_reset": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "leave_reset": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "chip_reset": ( - reply: Simple("()"), - idempotent: true, - ), - - } + "enable_clock": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "disable_clock": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "enter_reset": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "leave_reset": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "chip_reset": ( + reply: Simple("()"), + idempotent: true, + ), + } ) diff --git a/idl/validate.idol b/idl/validate.idol index 7c233765a..e1c69a1c3 100644 --- a/idl/validate.idol +++ b/idl/validate.idol @@ -8,7 +8,7 @@ Interface( "index": "u32", }, reply: Result( - ok: (type : "ValidateOk", recv: FromPrimitive("u8")), + ok: (type: "ValidateOk", recv: FromPrimitive("u8")), err: CLike("ValidateError"), ), idempotent: true, diff --git a/idl/vpd.idol b/idl/vpd.idol index 45c3010b1..2de32681c 100644 --- a/idl/vpd.idol +++ b/idl/vpd.idol @@ -54,7 +54,7 @@ Interface( err: CLike("VpdError"), ), ), - "num_vpd_devices": ( + "num_vpd_devices": ( doc: "Returns the total number of VPD devices in the system", args: {}, reply: Simple("usize"), diff --git a/lib/gnarle/src/lib.rs b/lib/gnarle/src/lib.rs index 76a5c46b3..dff3600ed 100644 --- a/lib/gnarle/src/lib.rs +++ b/lib/gnarle/src/lib.rs @@ -11,8 +11,6 @@ #![cfg_attr(not(feature = "std"), no_std)] -use core::convert::TryFrom; - /// Internal definition of how long the run count is. Tuning this might improve /// performance, though its current value seems optimal in practice. type RunType = u8; diff --git a/lib/stage0-handoff/src/lib.rs b/lib/stage0-handoff/src/lib.rs index 4d9a5d418..19afd239d 100644 --- a/lib/stage0-handoff/src/lib.rs +++ b/lib/stage0-handoff/src/lib.rs @@ -4,8 +4,6 @@ #![cfg_attr(not(test), no_std)] -use core::convert::From; -use core::marker::Sized; use core::ops::Range; use hubpack::SerializedSize; use serde::{Deserialize, Serialize}; diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 832531aca..38cefdfc1 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,5 +1,5 @@ [toolchain] -channel = "nightly-2022-11-01" +channel = "nightly-2024-04-04" targets = [ "thumbv6m-none-eabi", "thumbv7em-none-eabihf", "thumbv8m.main-none-eabihf" ] profile = "minimal" components = [ "rustfmt" ] diff --git a/sys/kern/src/fail.rs b/sys/kern/src/fail.rs index a27f762a5..444de6767 100644 --- a/sys/kern/src/fail.rs +++ b/sys/kern/src/fail.rs @@ -42,8 +42,14 @@ static mut KERNEL_EPITAPH: [u8; EPITAPH_LEN] = [0; EPITAPH_LEN]; #[cfg(not(feature = "nano"))] fn begin_epitaph() -> &'static mut [u8; EPITAPH_LEN] { // We'd love to use an AtomicBool here but we gotta support ARMv6M. - let previous_fail = - core::mem::replace(unsafe { &mut KERNEL_HAS_FAILED }, true); + // This could probably become SyncUnsafeCell in a future where it exists. + // + // Safety: we only access this function from this one site, and only zero or + // one times in practice -- and never from a context where concurrency or + // interrupts are enabled. + let previous_fail = unsafe { + core::ptr::replace(core::ptr::addr_of_mut!(KERNEL_HAS_FAILED), true) + }; if previous_fail { // Welp, you've called begin_epitaph twice, suggesting a recursive // panic. We can't very well panic in response to this since it'll just @@ -56,7 +62,7 @@ fn begin_epitaph() -> &'static mut [u8; EPITAPH_LEN] { // Safety: we can get a mutable reference to the epitaph because only one // execution of this function will successfully set that flag. - unsafe { &mut KERNEL_EPITAPH } + unsafe { &mut *core::ptr::addr_of_mut!(KERNEL_EPITAPH) } } #[cfg(not(feature = "nano"))] diff --git a/sys/kern/src/kipc.rs b/sys/kern/src/kipc.rs index 6dbd2227d..5e6f6fc50 100644 --- a/sys/kern/src/kipc.rs +++ b/sys/kern/src/kipc.rs @@ -10,7 +10,6 @@ use crate::arch; use crate::err::UserError; use crate::task::{current_id, ArchState, NextTask, Task}; use crate::umem::USlice; -use core::convert::TryFrom; use core::mem::size_of; /// Message dispatcher. diff --git a/sys/kern/src/startup.rs b/sys/kern/src/startup.rs index af5bc1bf2..1c6094805 100644 --- a/sys/kern/src/startup.rs +++ b/sys/kern/src/startup.rs @@ -5,7 +5,7 @@ //! Kernel startup. use crate::atomic::AtomicExt; -use crate::descs::RegionDesc; +use crate::descs::*; use crate::task::Task; use core::mem::MaybeUninit; use core::sync::atomic::{AtomicBool, Ordering}; @@ -51,7 +51,8 @@ pub unsafe fn start_kernel(tick_divisor: u32) -> ! { let task_descs = &HUBRIS_TASK_DESCS; // Safety: this reference will remain unique so long as the "only called // once per boot" contract on this function is upheld. - let task_table = unsafe { &mut HUBRIS_TASK_TABLE_SPACE }; + let task_table = + unsafe { &mut *core::ptr::addr_of_mut!(HUBRIS_TASK_TABLE_SPACE) }; // Initialize our RAM data structures. @@ -100,18 +101,27 @@ pub(crate) fn with_task_table(body: impl FnOnce(&mut [Task]) -> R) -> R { if TASK_TABLE_IN_USE.swap_polyfill(true, Ordering::Acquire) { panic!(); // recursive use of with_task_table } + // Safety: tbh it's never been clear to me why addr_of_mut is unsafe when it + // produces a raw pointer, but, see the Safety justification on the + // derefrence below for the full argument on why we can do this. + let task_table: *mut MaybeUninit<[Task; HUBRIS_TASK_COUNT]> = + unsafe { core::ptr::addr_of_mut!(HUBRIS_TASK_TABLE_SPACE) }; + // Pointer cast valid as MaybeUninit<[T; N]> and [MaybeUninit; N] have + // same in-memory representation. At the time of this writing + // MaybeUninit::transpose is not yet stable. + let task_table: *mut [MaybeUninit; HUBRIS_TASK_COUNT] = + task_table as _; + // This pointer cast is doing the equivalent of + // MaybeUninit::array_assume_init, which at the time of this writing is not + // stable. + let task_table: *mut [Task; HUBRIS_TASK_COUNT] = task_table as _; // Safety: we have observed `TASK_TABLE_IN_USE` being false, which means the // task table is initialized (note that at reset it starts out true) and // that we're not already within a call to with_task_table. Thus, we can // produce a reference to the task table without aliasing, and we can be // confident that the memory it's pointing to is initialized and shed the // MaybeUninit. - let task_table = unsafe { - &mut *(&mut HUBRIS_TASK_TABLE_SPACE - as *mut MaybeUninit<[Task; HUBRIS_TASK_COUNT]> - as *mut [MaybeUninit; HUBRIS_TASK_COUNT] - as *mut [Task; HUBRIS_TASK_COUNT]) - }; + let task_table = unsafe { &mut *task_table }; let r = body(task_table); @@ -120,5 +130,4 @@ pub(crate) fn with_task_table(body: impl FnOnce(&mut [Task]) -> R) -> R { r } -use crate::descs::*; include!(concat!(env!("OUT_DIR"), "/kconfig.rs")); diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index f61a1ca53..5137e5619 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -27,8 +27,6 @@ //! struct* type to make this easy and safe, e.g. `task.save().as_send_args()`. //! See the `task::ArchState` trait for details. -use core::convert::TryFrom; - use abi::{ FaultInfo, IrqStatus, LeaseAttributes, SchedState, Sysnum, TaskId, TaskState, ULease, UsageError, @@ -191,12 +189,35 @@ fn send(tasks: &mut [Task], caller: usize) -> Result { /// /// `caller` is a valid task index (i.e. not directly from user code). /// +/// # Returns +/// +/// If the operation found a sender and delivered a message, +/// `Ok(NextTask::Same)` to drop us right back into the caller. +/// +/// If the operation did not find a sender but was otherwise valid (i.e. needs +/// to block the sender), `Ok(something_else)` to context switch away from the +/// sender. +/// +/// This may also return `Ok(something_else)` to context switch to the +/// supervisor even if a message was successfully delivered, but only if at +/// least one blocked sender with invalid configuration was found and faulted +/// along the way. +/// +/// In terms of errors, +/// +/// `Err(UserError::Recoverable(..))` is only used to return Dead Codes, and +/// only in closed receive specifically. +/// +/// All other errors are `Err(UserError::Unrecoverable(_))` that result in a +/// fault delivered to the caller. This indicates that the location where the +/// caller requested to receive a delivered message isn't accessible or valid. +/// /// # Panics /// /// If `caller` is out of range for `tasks`. fn recv(tasks: &mut [Task], caller: usize) -> Result { - // We allow tasks to atomically replace their notification mask at each - // receive. We simultaneously find out if there are notifications pending. + // `take_notifications` interprets the new notification mask and finds out + // if notifications are pending. if let Some(firing) = tasks[caller].take_notifications() { // Pending! Deliver an artificial message from the kernel. tasks[caller].save_mut().set_recv_result( @@ -226,7 +247,12 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { // outcomes here. // First possibility: that task you're asking about is DEAD. + // + // N.B. this is actually the only point in the RECV implementation where + // the sender may receive an error code (as opposed to being faulted or + // just blocking waiting for a valid sender). let sender_idx = task::check_task_id_against_table(tasks, sender_id)?; + // Second possibility: task has a message for us. if tasks[sender_idx].state().is_sending_to(caller_id) { // Oh hello sender! @@ -238,15 +264,24 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { } Err(interact) => { // Delivery failed because of fault events in one or both - // tasks. We need to apply the fault status, and then if we - // didn't have to murder the caller, we'll retry receiving a - // message. + // tasks. `apply_to_src` extracts the src (sender) side and + // applies it directly to the task; if there was a problem + // on the dst (recv'r, us) side we ? it. It's a FaultInfo, + // so if dst screwed up the caller task will be faulted. + // + // If there was no problem, the caller will be informed that + // the task has died, and will generally opt to retry the + // recv as an open recv. + // + // The wake hint here may wake the supervisor before the + // caller regains control. let wake_hint = interact.apply_to_src(tasks, sender_idx)?; + // No fault in the caller at least, carry on. next_task = next_task.combine(wake_hint); } } } - // Third possibility: we need to block; fall through below. + // Third possibility: we need to block; fall through below. } else { // Open Receive @@ -269,20 +304,22 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { }) { // Oh hello sender! match deliver(tasks, sender, caller) { - Ok(_) => { + Ok(()) => { // Delivery succeeded! Sender is now blocked in reply. Go ahead // and let the caller resume. return Ok(next_task); } Err(interact) => { // Delivery failed because of fault events in one or both - // tasks. We need to apply the fault status, and then if we - // didn't have to murder the caller, we'll retry receiving a - // message. + // tasks. Because we're transferring a message from the + // sender to the recv'r (us, the caller) we use apply_to_src + // to potentially fault the sender, and then apply the dst + // side to the caller using ?. let wake_hint = interact.apply_to_src(tasks, sender)?; + // No fault in the caller, at least. This may wake the + // supervisor. next_task = next_task.combine(wake_hint); - // Okay, if we didn't just return, retry the search from a new - // position. + // Retry the search from our new position. last = sender; } } diff --git a/sys/kern/src/task.rs b/sys/kern/src/task.rs index 8cf0ac0c3..09caee26e 100644 --- a/sys/kern/src/task.rs +++ b/sys/kern/src/task.rs @@ -4,7 +4,6 @@ //! Implementation of tasks. -use core::convert::TryFrom; use core::ops::Range; use abi::{ diff --git a/sys/userlib/Cargo.toml b/sys/userlib/Cargo.toml index 062e30909..fa9a9f9ae 100644 --- a/sys/userlib/Cargo.toml +++ b/sys/userlib/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [features] default = ["critical-section"] panic-messages = [] +no-panic = [] critical-section = ["dep:critical-section"] [dependencies] diff --git a/sys/userlib/src/hl.rs b/sys/userlib/src/hl.rs index 00e7be98f..7fb7b5f6b 100644 --- a/sys/userlib/src/hl.rs +++ b/sys/userlib/src/hl.rs @@ -7,7 +7,7 @@ //! This is intended to provide a more ergonomic interface than the raw //! syscalls. -use abi::TaskId; +use abi::{Generation, TaskId}; use core::marker::PhantomData; use unwrap_lite::UnwrapLite; use zerocopy::{AsBytes, FromBytes, LayoutVerified}; @@ -147,8 +147,8 @@ where O: FromPrimitive, E: Into, { - let rm = - sys_recv(buffer, mask, source).map_err(|_| ClosedRecvError::Dead)?; + let rm = sys_recv(buffer, mask, source) + .map_err(|code| ClosedRecvError::Dead(Generation::from(code as u8)))?; let sender = rm.sender; if rm.sender == TaskId::KERNEL { notify(state, rm.operation); diff --git a/sys/userlib/src/kipc.rs b/sys/userlib/src/kipc.rs index 42e7c5d60..b153ca291 100644 --- a/sys/userlib/src/kipc.rs +++ b/sys/userlib/src/kipc.rs @@ -4,10 +4,10 @@ //! Operations implemented by IPC with the kernel task. -use crate::UnwrapLite; +use abi::{Kipcnum, TaskId}; use zerocopy::AsBytes; -use crate::*; +use crate::{sys_send, UnwrapLite}; pub fn read_task_status(task: usize) -> abi::TaskState { // Coerce `task` to a known size (Rust doesn't assume that usize == u32) diff --git a/sys/userlib/src/lib.rs b/sys/userlib/src/lib.rs index 8e27eba9c..4c7011a18 100644 --- a/sys/userlib/src/lib.rs +++ b/sys/userlib/src/lib.rs @@ -237,12 +237,14 @@ unsafe extern "C" fn sys_send_stub(_args: &mut SendArgs<'_>) -> RcLen { /// let it, but it always receives _something_. #[inline(always)] pub fn sys_recv_open(buffer: &mut [u8], notification_mask: u32) -> RecvMessage { - // The open-receive version of the syscall is defined as being unable to - // fail, and so we should always get a success here. (This is not using - // `unwrap` because that generates handling code with formatting.) match sys_recv(buffer, notification_mask, None) { Ok(rm) => rm, - Err(_) => panic!(), + Err(_) => { + // Safety: the open-receive version of the syscall is defined as + // being unable to fail in the kernel ABI, so this path can't happen + // modulo a kernel bug. + unsafe { core::hint::unreachable_unchecked() } + } } } @@ -259,20 +261,32 @@ pub fn sys_recv_open(buffer: &mut [u8], notification_mask: u32) -> RecvMessage { /// /// If `sender` is stale (i.e. refers to a deceased generation of the task) when /// you call this, or if `sender` is rebooted while you're blocked in this -/// operation, this will fail with `ClosedRecvError::Dead`. +/// operation, this will fail with `ClosedRecvError::Dead`, indicating the +/// `sender`'s new generation (not that a server generally cares). #[inline(always)] pub fn sys_recv_closed( buffer: &mut [u8], notification_mask: u32, sender: TaskId, ) -> Result { - sys_recv(buffer, notification_mask, Some(sender)) - .map_err(|_| ClosedRecvError::Dead) + sys_recv(buffer, notification_mask, Some(sender)).map_err(|code| { + // We're not using the extract_new_generation function here because + // that has a failure code path for cases where the code is not a + // dead code. In this case, sys_recv is defined as being _only_ + // capable of returning a dead code -- otherwise we have a serious + // kernel bug. So to avoid the introduction of a panic that can't + // trigger, we will do this manually: + ClosedRecvError::Dead(Generation::from(code as u8)) + }) } +/// Things that can go wrong (without faulting) during a closed receive +/// operation. #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub enum ClosedRecvError { - Dead, + /// The task you requested to receive from has restarted and the message may + /// never come. + Dead(Generation), } /// General version of RECV that lets you pick closed vs. open receive at @@ -287,8 +301,9 @@ pub fn sys_recv( ) -> Result { use core::mem::MaybeUninit; - // Flatten option into a packed u32. - let specific_sender = specific_sender + // Flatten option into a packed u32; in the C-compatible ABI we provide the + // task ID in the LSBs, and the "some" flag in the MSB. + let specific_sender_bits = specific_sender .map(|tid| (1u32 << 31) | u32::from(tid.0)) .unwrap_or(0); let mut out = MaybeUninit::::uninit(); @@ -297,7 +312,7 @@ pub fn sys_recv( buffer.as_mut_ptr(), buffer.len(), notification_mask, - specific_sender, + specific_sender_bits, out.as_mut_ptr(), ) }; @@ -319,6 +334,25 @@ pub fn sys_recv( } } +/// Convenience wrapper for `sys_recv` for the specific, but common, task of +/// listening for notifications. In this specific use, it has the advantage of +/// never panicking and not returning a `Result` that must be checked. +#[inline(always)] +pub fn sys_recv_notification(notification_mask: u32) -> u32 { + match sys_recv(&mut [], notification_mask, Some(TaskId::KERNEL)) { + Ok(rm) => { + // The notification bits come back from the kernel in the operation + // code field. + rm.operation + } + Err(_) => { + // Safety: Because we passed Some(TaskId::KERNEL), this is defined + // as not being able to happen. + unsafe { core::hint::unreachable_unchecked() } + } + } +} + pub struct RecvMessage { pub sender: TaskId, pub operation: u32, @@ -1288,6 +1322,15 @@ pub unsafe extern "C" fn _start() -> ! { } } +// Make the no-panic and panic-messages features mutually exclusive. +#[cfg(all(feature = "no-panic", feature = "panic-messages"))] +compile_error!( + "Both the userlib/panic-messages and userlib/no-panic feature flags \ + are set! This doesn't make a lot of sense and is probably not what \ + you wanted. (If you have a use case for this combination, update \ + this check in userlib.)" +); + /// Panic handler for user tasks with the `panic-messages` feature enabled. This /// handler will try its best to generate a panic message, up to a maximum /// buffer size (configured below). @@ -1296,7 +1339,7 @@ pub unsafe extern "C" fn _start() -> ! { /// task, to ensure that memory is available for the panic message, even if the /// resources have been trimmed aggressively using `xtask sizes` and `humility /// stackmargin`. -#[cfg(feature = "panic-messages")] +#[cfg(all(not(feature = "no-panic"), feature = "panic-messages"))] #[panic_handler] fn panic(info: &core::panic::PanicInfo<'_>) -> ! { // Implementation Note @@ -1418,7 +1461,7 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { // However, it is possible to produce an alias if the panic handler is // called reentrantly. This can only happen if the code in the panic handler // itself panics, which is what we're working very hard to prevent here. - let panic_buffer = unsafe { &mut PANIC_BUFFER }; + let panic_buffer = unsafe { &mut *core::ptr::addr_of_mut!(PANIC_BUFFER) }; // Whew! Time to write the darn message. // @@ -1443,12 +1486,26 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { /// Panic handler for tasks without the `panic-messages` feature enabled. This /// kills the task with a fixed message, `"PANIC"`. While this is less helpful /// than a proper panic message, the stack trace can still be informative. -#[cfg(not(feature = "panic-messages"))] +#[cfg(all(not(feature = "no-panic"), not(feature = "panic-messages")))] #[panic_handler] fn panic(_: &core::panic::PanicInfo<'_>) -> ! { sys_panic(b"PANIC") } +/// Panic handler for when panics are not permitted in a task. This is enabled +/// by the `no-panic` feature and causes a link error if a panic is introduced. +#[cfg(feature = "no-panic")] +#[panic_handler] +fn panic(_: &core::panic::PanicInfo<'_>) -> ! { + extern "C" { + fn you_have_introduced_a_panic_which_is_not_permitted() -> !; + } + + // Safety: this function does not exist, this code will not pass the linker + // and is thus not reachable. + unsafe { you_have_introduced_a_panic_which_is_not_permitted() } +} + #[inline(always)] pub fn sys_refresh_task_id(task_id: TaskId) -> TaskId { let tid = unsafe { sys_refresh_task_id_stub(task_id.0 as u32) }; diff --git a/sys/userlib/src/units.rs b/sys/userlib/src/units.rs index cf832313e..ed3aea16f 100644 --- a/sys/userlib/src/units.rs +++ b/sys/userlib/src/units.rs @@ -6,7 +6,6 @@ //! Tuple structs for units that are useful in the real world //! -use core::convert::TryFrom; use zerocopy::{AsBytes, FromBytes}; /// Degrees Celsius diff --git a/task/control-plane-agent/src/mgs_gimlet.rs b/task/control-plane-agent/src/mgs_gimlet.rs index a613d127b..ae03c797a 100644 --- a/task/control-plane-agent/src/mgs_gimlet.rs +++ b/task/control-plane-agent/src/mgs_gimlet.rs @@ -623,10 +623,10 @@ impl SpHandler for MgsHandler { .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), SpComponent::HOST_CPU_BOOT_FLASH => self .host_flash_update - .ingest_chunk(&chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + .ingest_chunk(&(), &chunk.id, chunk.offset, data), + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/mgs_psc.rs b/task/control-plane-agent/src/mgs_psc.rs index f74d835ab..ffb50bcf2 100644 --- a/task/control-plane-agent/src/mgs_psc.rs +++ b/task/control-plane-agent/src/mgs_psc.rs @@ -332,9 +332,9 @@ impl SpHandler for MgsHandler { SpComponent::SP_ITSELF | SpComponent::SP_AUX_FLASH => self .sp_update .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/mgs_sidecar.rs b/task/control-plane-agent/src/mgs_sidecar.rs index ea3b01175..edbdbdfd9 100644 --- a/task/control-plane-agent/src/mgs_sidecar.rs +++ b/task/control-plane-agent/src/mgs_sidecar.rs @@ -385,9 +385,9 @@ impl SpHandler for MgsHandler { SpComponent::SP_ITSELF | SpComponent::SP_AUX_FLASH => self .sp_update .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/update/host_flash.rs b/task/control-plane-agent/src/update/host_flash.rs index 3485d26de..3f011a044 100644 --- a/task/control-plane-agent/src/update/host_flash.rs +++ b/task/control-plane-agent/src/update/host_flash.rs @@ -84,6 +84,9 @@ static_assertions::const_assert!( impl ComponentUpdater for HostFlashUpdate { const BLOCK_SIZE: usize = PAGE_SIZE_BYTES; + type UpdatePrepare = ComponentUpdatePrepare; + type SubComponent = (); + fn prepare( &mut self, buffer: &'static UpdateBuffer, @@ -243,6 +246,7 @@ impl ComponentUpdater for HostFlashUpdate { fn ingest_chunk( &mut self, + _sub: &(), id: &UpdateId, offset: u32, mut data: &[u8], diff --git a/task/control-plane-agent/src/update/mod.rs b/task/control-plane-agent/src/update/mod.rs index a24abbbf2..650db2551 100644 --- a/task/control-plane-agent/src/update/mod.rs +++ b/task/control-plane-agent/src/update/mod.rs @@ -3,9 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::mgs_handler::UpdateBuffer; -use gateway_messages::{ - ComponentUpdatePrepare, SpError, UpdateId, UpdateStatus, -}; +use gateway_messages::{SpError, UpdateId, UpdateStatus}; #[cfg(feature = "gimlet")] pub(crate) mod host_flash; @@ -30,6 +28,15 @@ pub(crate) trait ComponentUpdater { /// mechanism wants as a single chunk. const BLOCK_SIZE: usize; + /// Record provided to the `prepare` operation. Generally + /// `ComponentUpdatePrepare`, and would default to that if associated type + /// defaults were stable at the time of this writing (2024-04), which they + /// are not. + type UpdatePrepare; + + /// Type used to specify sub-components within a component. + type SubComponent; + /// Attempt to start preparing for an update, using `buffer` as the backing /// store for incoming data. /// @@ -38,7 +45,7 @@ pub(crate) trait ComponentUpdater { fn prepare( &mut self, buffer: &'static UpdateBuffer, - update: ComponentUpdatePrepare, + update: Self::UpdatePrepare, ) -> Result<(), SpError>; /// Returns true if this task needs `step_preparation()` called. @@ -53,6 +60,7 @@ pub(crate) trait ComponentUpdater { /// Attempt to ingest a single update chunk from MGS. fn ingest_chunk( &mut self, + sub: &Self::SubComponent, id: &UpdateId, offset: u32, data: &[u8], diff --git a/task/control-plane-agent/src/update/rot.rs b/task/control-plane-agent/src/update/rot.rs index bc1ad91de..928301f31 100644 --- a/task/control-plane-agent/src/update/rot.rs +++ b/task/control-plane-agent/src/update/rot.rs @@ -51,6 +51,9 @@ enum State { impl ComponentUpdater for RotUpdate { const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; + type UpdatePrepare = ComponentUpdatePrepare; + type SubComponent = (); + fn prepare( &mut self, buffer: &'static UpdateBuffer, @@ -131,6 +134,7 @@ impl ComponentUpdater for RotUpdate { fn ingest_chunk( &mut self, + _sub: &(), id: &UpdateId, offset: u32, mut data: &[u8], diff --git a/task/control-plane-agent/src/update/sp.rs b/task/control-plane-agent/src/update/sp.rs index 570a02e93..f83a7551b 100644 --- a/task/control-plane-agent/src/update/sp.rs +++ b/task/control-plane-agent/src/update/sp.rs @@ -56,6 +56,7 @@ use crate::mgs_common::UPDATE_SERVER; use crate::mgs_handler::{BorrowedUpdateBuffer, UpdateBuffer}; +use crate::update::ComponentUpdater; use cfg_if::cfg_if; use core::ops::{Deref, DerefMut}; use drv_caboose::CabooseReader; @@ -98,12 +99,6 @@ pub(crate) struct SpUpdate { } impl SpUpdate { - #[cfg(feature = "auxflash")] - pub(crate) const BLOCK_SIZE: usize = - crate::usize_max(BLOCK_SIZE_BYTES, drv_auxflash_api::PAGE_SIZE_BYTES); - #[cfg(not(feature = "auxflash"))] - pub(crate) const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; - pub(crate) fn new() -> Self { Self { sp_task: Update::from(UPDATE_SERVER.get_task_id()), @@ -111,11 +106,22 @@ impl SpUpdate { current: None, } } +} - pub(crate) fn prepare( +impl ComponentUpdater for SpUpdate { + #[cfg(feature = "auxflash")] + const BLOCK_SIZE: usize = + crate::usize_max(BLOCK_SIZE_BYTES, drv_auxflash_api::PAGE_SIZE_BYTES); + #[cfg(not(feature = "auxflash"))] + const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; + + type UpdatePrepare = SpUpdatePrepare; + type SubComponent = SpComponent; + + fn prepare( &mut self, buffer: &'static UpdateBuffer, - update: SpUpdatePrepare, + update: Self::UpdatePrepare, ) -> Result<(), SpError> { // Do we have an update already in progress? match self.current.as_ref().map(|c| c.state()) { @@ -178,14 +184,14 @@ impl SpUpdate { Ok(()) } - pub(crate) fn is_preparing(&self) -> bool { + fn is_preparing(&self) -> bool { match self.current.as_ref().map(|c| c.state()) { Some(State::AuxFlash(s)) => s.is_preparing(), _ => false, } } - pub(crate) fn step_preparation(&mut self) { + fn step_preparation(&mut self) { // Do we have an update? let current = match self.current.as_mut() { Some(current) => current, @@ -231,7 +237,7 @@ impl SpUpdate { }); } - pub(crate) fn status(&self) -> UpdateStatus { + fn status(&self) -> UpdateStatus { let current = match self.current.as_ref() { Some(current) => current, None => return UpdateStatus::None, @@ -264,7 +270,7 @@ impl SpUpdate { } } - pub(crate) fn ingest_chunk( + fn ingest_chunk( &mut self, component: &SpComponent, id: &UpdateId, @@ -377,7 +383,7 @@ impl SpUpdate { }) } - pub(crate) fn abort(&mut self, id: &UpdateId) -> Result<(), SpError> { + fn abort(&mut self, id: &UpdateId) -> Result<(), SpError> { // Do we have an update in progress? If not, nothing to do. let current = match self.current.as_mut() { Some(current) => current, diff --git a/task/gimlet-inspector/src/main.rs b/task/gimlet-inspector/src/main.rs index fad973364..228a9132d 100644 --- a/task/gimlet-inspector/src/main.rs +++ b/task/gimlet-inspector/src/main.rs @@ -99,24 +99,14 @@ fn main() -> ! { // packets off our recv queue at the top of our main // loop. Err(SendError::QueueFull) => { - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } } } } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted; just retry. diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index ff35cbe9b..50eec44c7 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -166,6 +166,9 @@ fn main() -> ! { sleep_ms = 1; sleeps = 0; + // TODO without a safety comment explaining why these are safe, it is + // not clear if this is sound, do _not_ "fix" this by slapping on an + // addr_of_mut! without further analysis! let text = unsafe { &HIFFY_TEXT }; let data = unsafe { &HIFFY_DATA }; let rstack = unsafe { &mut HIFFY_RSTACK[0..] }; diff --git a/task/hiffy/src/stm32h7.rs b/task/hiffy/src/stm32h7.rs index 76397da84..007454c8d 100644 --- a/task/hiffy/src/stm32h7.rs +++ b/task/hiffy/src/stm32h7.rs @@ -47,7 +47,9 @@ enum Trace { ringbuf!(Trace, 64, Trace::None); -pub struct Buffer(u8); +// Field is only used in the debugger, appears dead to the compiler. +pub struct Buffer(#[allow(dead_code)] u8); + // // The order in this enum must match the order in the functions array that // is passed to execute. diff --git a/task/host-sp-comms/src/bsp/gimlet_bcde.rs b/task/host-sp-comms/src/bsp/gimlet_bcde.rs index 92d07cfda..c2c18718d 100644 --- a/task/host-sp-comms/src/bsp/gimlet_bcde.rs +++ b/task/host-sp-comms/src/bsp/gimlet_bcde.rs @@ -82,8 +82,9 @@ impl ServerImpl { let packrat = &self.packrat; let mut data = InventoryData::VpdIdentity(Default::default()); self.tx_buf.try_encode_inventory(sequence, b"U615/ID", || { - let InventoryData::VpdIdentity(identity) = &mut data - else { unreachable!(); }; + let InventoryData::VpdIdentity(identity) = &mut data else { + unreachable!(); + }; *identity = packrat .get_identity() .map_err(|_| InventoryDataResult::DeviceAbsent)? @@ -184,18 +185,21 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::bmr491::CommandCode; let InventoryData::Bmr491 { - mfr_id, - mfr_model, - mfr_revision, - mfr_location, - mfr_date, - mfr_serial, - mfr_firmware_data, - temp_sensor: _, - voltage_sensor: _, - current_sensor: _, - power_sensor: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_location, + mfr_date, + mfr_serial, + mfr_firmware_data, + temp_sensor: _, + voltage_sensor: _, + current_sensor: _, + power_sensor: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -234,15 +238,18 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::isl68224::CommandCode; let InventoryData::Isl68224 { - mfr_id, - mfr_model, - mfr_revision, - mfr_date, - ic_device_id, - ic_device_rev, - voltage_sensors: _, - current_sensors: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_date, + ic_device_id, + ic_device_rev, + voltage_sensors: _, + current_sensors: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -286,17 +293,20 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::raa229618::CommandCode; let InventoryData::Raa229618 { - mfr_id, - mfr_model, - mfr_revision, - mfr_date, - ic_device_id, - ic_device_rev, - temp_sensors: _, - power_sensors: _, - voltage_sensors: _, - current_sensors: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_date, + ic_device_id, + ic_device_rev, + temp_sensors: _, + power_sensors: _, + voltage_sensors: _, + current_sensors: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -351,7 +361,10 @@ impl ServerImpl { temp_sensor: _, voltage_sensor: _, current_sensor: _, - } = &mut data else { unreachable!() }; + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -403,7 +416,10 @@ impl ServerImpl { temp_sensor: _, voltage_sensor: _, current_sensor: _, - } = &mut data else { unreachable!() }; + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -447,8 +463,11 @@ impl ServerImpl { eeprom1, eeprom2, eeprom3, - temp_sensor: _ - } = &mut data else { unreachable!(); }; + temp_sensor: _, + } = &mut data + else { + unreachable!(); + }; *id = dev.read_reg(0x0Fu8)?; *eeprom1 = dev.read_reg(0x05u8)?; *eeprom2 = dev.read_reg(0x06u8)?; @@ -474,7 +493,10 @@ impl ServerImpl { minor_rel, hotfix_rel, product_id, - } = &mut data else { unreachable!(); }; + } = &mut data + else { + unreachable!(); + }; // This chip includes a separate register that controls the // upper address byte, i.e. a paged memory implementation. // We'll use `write_read_reg` to avoid the possibility of @@ -509,8 +531,9 @@ impl ServerImpl { let ksz8463 = ksz8463::Ksz8463::new(ksz8463_dev); let mut data = InventoryData::Ksz8463 { cider: 0 }; self.tx_buf.try_encode_inventory(sequence, b"U401", || { - let InventoryData::Ksz8463 { cider } = &mut data - else { unreachable!(); }; + let InventoryData::Ksz8463 { cider } = &mut data else { + unreachable!(); + }; *cider = ksz8463 .read(ksz8463::Register::CIDER) .map_err(|_| InventoryDataResult::DeviceFailed)?; @@ -600,8 +623,9 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { // TODO: does packrat index match PCA designator? if packrat.get_spd_present(index as usize) { - let InventoryData::DimmSpd { id, .. } = &mut data - else { unreachable!(); }; + let InventoryData::DimmSpd { id, .. } = &mut data else { + unreachable!(); + }; packrat.get_full_spd_data(index as usize, id); Ok(&data) } else { @@ -626,8 +650,9 @@ impl ServerImpl { *data = InventoryData::At24csw08xSerial([0u8; 16]); let dev = At24Csw080::new(f(I2C.get_task_id())); self.tx_buf.try_encode_inventory(sequence, name, || { - let InventoryData::At24csw08xSerial(id) = data - else { unreachable!(); }; + let InventoryData::At24csw08xSerial(id) = data else { + unreachable!(); + }; for (i, b) in id.iter_mut().enumerate() { *b = dev.read_security_register_byte(i as u8).map_err(|e| { match e { @@ -656,8 +681,9 @@ impl ServerImpl { let dev = f(I2C.get_task_id()); let mut data = InventoryData::VpdIdentity(Default::default()); self.tx_buf.try_encode_inventory(sequence, name, || { - let InventoryData::VpdIdentity(identity) = &mut data - else { unreachable!(); }; + let InventoryData::VpdIdentity(identity) = &mut data else { + unreachable!(); + }; *identity = read_one_barcode(dev, &[(*b"BARC", 0)])?.into(); Ok(&data) }) @@ -689,8 +715,11 @@ impl ServerImpl { let InventoryData::FanIdentity { identity, vpd_identity, - fans: [fan0, fan1, fan2] - } = &mut data else { unreachable!(); }; + fans: [fan0, fan1, fan2], + } = &mut data + else { + unreachable!(); + }; *identity = read_one_barcode(dev, &[(*b"BARC", 0)])?.into(); *vpd_identity = read_one_barcode(dev, &[(*b"SASY", 0), (*b"BARC", 0)])?.into(); diff --git a/task/net/src/bsp/gimlet_bcdef.rs b/task/net/src/bsp/gimlet_bcdef.rs index ad9a22c8f..6859b3e9b 100644 --- a/task/net/src/bsp/gimlet_bcdef.rs +++ b/task/net/src/bsp/gimlet_bcdef.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -72,12 +72,9 @@ impl crate::bsp_support::Bsp for BspImpl { None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } @@ -93,7 +90,6 @@ impl crate::bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10).and_pin(12)), slow_power_en: false, power_good: &[], // TODO - pll_lock: None, // TODO? ksz8463: Ksz8463::new(ksz8463_dev), diff --git a/task/net/src/bsp/gimletlet_mgmt.rs b/task/net/src/bsp/gimletlet_mgmt.rs index d2457804e..a6c6829ed 100644 --- a/task/net/src/bsp/gimletlet_mgmt.rs +++ b/task/net/src/bsp/gimletlet_mgmt.rs @@ -154,7 +154,6 @@ impl bsp_support::Bsp for BspImpl { power_en: None, slow_power_en: false, power_good: &[], - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::A.pin(9), diff --git a/task/net/src/bsp/psc_a.rs b/task/net/src/bsp/psc_a.rs index b8ad78eca..e020fcbb7 100644 --- a/task/net/src/bsp/psc_a.rs +++ b/task/net/src/bsp/psc_a.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive, TaskId}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -65,12 +65,9 @@ impl bsp_support::Bsp for BspImpl { Some(PowerState::Init) | None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } @@ -86,7 +83,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10).and_pin(12)), slow_power_en: true, power_good: &[], // TODO - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::C.pin(2), diff --git a/task/net/src/bsp/psc_bc.rs b/task/net/src/bsp/psc_bc.rs index ad56af1fb..aa91a4f99 100644 --- a/task/net/src/bsp/psc_bc.rs +++ b/task/net/src/bsp/psc_bc.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -70,12 +70,9 @@ impl bsp_support::Bsp for BspImpl { Some(PowerState::Init) | None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } @@ -90,7 +87,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10)), slow_power_en: false, power_good: &PG_PINS, - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::C.pin(2), // SP_TO_MGMT_SW_RESET_L diff --git a/task/net/src/bsp/sidecar_bcd.rs b/task/net/src/bsp/sidecar_bcd.rs index 39ffb5833..6d7be1813 100644 --- a/task/net/src/bsp/sidecar_bcd.rs +++ b/task/net/src/bsp/sidecar_bcd.rs @@ -75,7 +75,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(11)), slow_power_en: false, power_good: &[], // TODO - pll_lock: None, // TODO? ksz8463: Ksz8463::new(ksz8463_dev), // SP_TO_EPE_RESET_L diff --git a/task/net/src/mgmt.rs b/task/net/src/mgmt.rs index c3a3724ac..e8c221942 100644 --- a/task/net/src/mgmt.rs +++ b/task/net/src/mgmt.rs @@ -58,9 +58,6 @@ pub struct Config { /// Goes high once power is good pub power_good: &'static [sys_api::PinSet], - /// Goes high once the PLLs are locked - pub pll_lock: Option, - pub ksz8463: Ksz8463, pub ksz8463_nrst: sys_api::PinSet, pub ksz8463_rst_type: Ksz8463ResetSpeed, diff --git a/task/nucleo-user-button/src/main.rs b/task/nucleo-user-button/src/main.rs index a46efff19..ae3acf667 100644 --- a/task/nucleo-user-button/src/main.rs +++ b/task/nucleo-user-button/src/main.rs @@ -11,13 +11,13 @@ #![no_std] #![no_main] -use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +use drv_stm32xx_sys_api::{Edge, IrqControl, PinSet, Port, Pull}; use ringbuf::ringbuf_entry; use userlib::*; #[cfg(not(any( target_board = "nucleo-h753zi", - target_board = "nucleo-h743zi2" + target_board = "nucleo-h743zi2", )))] compile_error!( "the `nucleo-user-button` task is only supported on the Nucleo H753ZI and H743ZI2 boards" @@ -29,10 +29,8 @@ task_slot!(SYS, sys); task_config::optional_task_config! { /// The index of the user LED to toggle led: usize, - /// Whether to enable rising-edge interrupts - rising: bool, - /// Whether to enable falling-edge interrupts - falling: bool, + /// Edge sensitivity for the button interrupt + edge: Edge, } // In real life, we might not actually want to trace all of these events, but @@ -42,17 +40,18 @@ task_config::optional_task_config! { enum Trace { #[count(skip)] None, - /// We called the `Sys.gpio_irq_configure` IPC with these arguments. - GpioIrqConfigure { + GpioIrqConfigure { mask: u32, edge: Edge }, + + /// We called the `Sys.gpio_irq_control` IPC with these arguments, and it + /// returned whether the interrupt had fired or not. + GpioIrqControl { mask: u32, - rising: bool, - falling: bool, + op: IrqControl, + #[count(children)] + fired: bool, }, - /// We called the `Sys.gpio_irq_control` IPC with these arguments. - GpioIrqControl { enable_mask: u32, disable_mask: u32 }, - /// We received a notification with these bits set. Notification(u32), @@ -67,11 +66,10 @@ pub fn main() -> ! { let user_leds = drv_user_leds_api::UserLeds::from(USER_LEDS.get_task_id()); let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); - let (led, rising, falling) = if let Some(config) = TASK_CONFIG { - (config.led, config.rising, config.falling) - } else { - (0, false, true) - }; + let Config { led, edge } = TASK_CONFIG.unwrap_or(Config { + led: 0, + edge: Edge::Rising, + }); sys.gpio_configure_input( PinSet { @@ -83,41 +81,36 @@ pub fn main() -> ! { ringbuf_entry!(Trace::GpioIrqConfigure { mask: notifications::BUTTON_MASK, - rising, - falling, + edge, }); - sys.gpio_irq_configure(notifications::BUTTON_MASK, rising, falling); + sys.gpio_irq_configure(notifications::BUTTON_MASK, edge); loop { - // The first argument to `gpio_irq_control` is the mask of interrupts to - // enable, while the second is the mask to disable. So, enable the - // button notification. - let disable_mask = 0; + // Call `Sys.gpio_irq_control` to enable our interrupt, returning + // whether it has fired. + let fired = match sys + .gpio_irq_control(notifications::BUTTON_MASK, IrqControl::Enable) + { + Ok(fired) => fired, + // If the sys task panicked, okay, let's just try to enable the IRQ + // again. + Err(_) => continue, + }; ringbuf_entry!(Trace::GpioIrqControl { - enable_mask: notifications::BUTTON_MASK, - disable_mask, + mask: notifications::BUTTON_MASK, + op: IrqControl::Enable, + fired }); - sys.gpio_irq_control(disable_mask, notifications::BUTTON_MASK); - - // Wait for the user button to be pressed. - // - // We only care about notifications, so we can pass a zero-sized recv - // buffer, and the kernel's task ID. - let recvmsg = sys_recv_closed( - &mut [], - notifications::BUTTON_MASK, - TaskId::KERNEL, - ) - // Recv from the kernel never returns an error. - .unwrap_lite(); - let notif = recvmsg.operation; - ringbuf_entry!(Trace::Notification(notif)); - // If the notification is for the button, toggle the LED. - if notif == notifications::BUTTON_MASK { + // If the button has changed state, toggle the LED. + if fired { ringbuf_entry!(Trace::LedToggle { led }); user_leds.led_toggle(led).unwrap_lite(); } + + // Wait for the user button to be pressed. + let notif = sys_recv_notification(notifications::BUTTON_MASK); + ringbuf_entry!(Trace::Notification(notif)); } } diff --git a/task/power/src/bsp/gimlet_bcdef.rs b/task/power/src/bsp/gimlet_bcdef.rs index b78772002..c737eb0d9 100644 --- a/task/power/src/bsp/gimlet_bcdef.rs +++ b/task/power/src/bsp/gimlet_bcdef.rs @@ -3,8 +3,8 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::{ - i2c_config, i2c_config::sensors, Device, DeviceType, Ohms, - PowerControllerConfig, PowerState, SensorId, + i2c_config, i2c_config::sensors, Device, DeviceType, PowerControllerConfig, + PowerState, SensorId, }; use drv_i2c_devices::max5970::*; @@ -77,9 +77,45 @@ pub(crate) fn get_state() -> PowerState { } } +/// Number of seconds (really, timer firings) between writes to the trace +/// buffer. +const TRACE_SECONDS: u32 = 10; + +/// Number of trace records to store. +/// +/// TODO: explain rationale for this value. +const TRACE_DEPTH: usize = 52; + +/// This enum and its corresponding ringbuf are being used to attempt to isolate +/// cases of this bug: +/// +/// https://github.com/oxidecomputer/mfg-quality/issues/140 +/// +/// Unless that bug report is closed or says otherwise, be careful modifying +/// this type, as you may break data collection. +/// +/// The basic theory here is: +/// +/// - Every `TRACE_SECONDS` seconds, the task wakes up and writes one `Now` +/// entry. +/// +/// - We then record one `Max5970` trace entry per MAX5970 being monitored. +/// +/// Tooling can then collect this ringbuf periodically and get recent events. #[derive(Copy, Clone, PartialEq)] enum Trace { + /// Written before trace records; the `u32` is the number of times the task + /// has woken up to process its timer. This is not exactly equivalent to + /// seconds because of the way the timer is maintained, but is approximately + /// seconds. Now(u32), + + /// Trace record written for each MAX5970. + /// + /// The `last_bounce_detected` field and those starting with `crossbounce_` + /// are copied from running state and may not be updated on every trace + /// event. The other fields are read while emitting the trace record and + /// should be current. Max5970 { sensor: SensorId, last_bounce_detected: Option, @@ -101,8 +137,12 @@ enum Trace { None, } -ringbuf!(Trace, 52, Trace::None); +ringbuf!(Trace, TRACE_DEPTH, Trace::None); +/// Records fields from `dev` and merges them with previous state from `peaks`, +/// updating `peaks` in the process. +/// +/// If any I2C operation fails, this will abort its work and return. fn trace_max5970( dev: &Max5970, sensor: SensorId, @@ -129,6 +169,7 @@ fn trace_max5970( _ => return, }; + // TODO: this update should probably happen after all I/O is done. if peaks.iout.bounced(min_iout, max_iout) || peaks.vout.bounced(min_vout, max_vout) { @@ -251,7 +292,7 @@ impl State { // // Trace the detailed state every ten seconds, provided that we are in A0. // - if state == PowerState::A0 && self.fired % 10 == 0 { + if state == PowerState::A0 && self.fired % TRACE_SECONDS == 0 { ringbuf_entry!(Trace::Now(self.fired)); for ((dev, sensor), peak) in CONTROLLER_CONFIG diff --git a/task/sensor-api/src/lib.rs b/task/sensor-api/src/lib.rs index 837441f00..9f9917adc 100644 --- a/task/sensor-api/src/lib.rs +++ b/task/sensor-api/src/lib.rs @@ -8,13 +8,10 @@ use userlib::*; -use core::convert::TryFrom; - use derive_idol_err::IdolError; use drv_i2c_api::ResponseCode; use hubpack::SerializedSize; use num_derive::FromPrimitive; -use num_traits::FromPrimitive; use serde::{Deserialize, Serialize}; /// A validated sensor ID. diff --git a/task/sensor/build.rs b/task/sensor/build.rs index c58bbe9b6..da98c3962 100644 --- a/task/sensor/build.rs +++ b/task/sensor/build.rs @@ -4,7 +4,6 @@ fn main() -> Result<(), Box> { build_util::expose_target_board(); - build_util::build_notifications()?; idol::Generator::new() .with_counters( idol::CounterSettings::default().with_server_counters(false), diff --git a/task/sensor/src/main.rs b/task/sensor/src/main.rs index a2b8afaee..f426ca1ce 100644 --- a/task/sensor/src/main.rs +++ b/task/sensor/src/main.rs @@ -71,11 +71,8 @@ struct ServerImpl { err_time: SensorArray, nerrors: SensorArray, - deadline: u64, } -const TIMER_INTERVAL: u64 = 1000; - impl idl::InOrderSensorImpl for ServerImpl { fn get( &mut self, @@ -243,24 +240,20 @@ impl ServerImpl { impl NotificationHandler for ServerImpl { fn current_notification_mask(&self) -> u32 { - notifications::TIMER_MASK + // We don't use notifications, don't listen for any. + 0 } fn handle_notification(&mut self, _bits: u32) { - self.deadline = self.deadline.wrapping_add(TIMER_INTERVAL); - sys_set_timer(Some(self.deadline), notifications::TIMER_MASK); + unreachable!() } } #[export_name = "main"] fn main() -> ! { - let deadline = sys_get_timer().now; - - // - // This will put our timer in the past, and should immediately kick us. - // - sys_set_timer(Some(deadline), notifications::TIMER_MASK); - + // N.B. if you are staring at this macro thinking that it looks like it + // doesn't do anything and might be obsolescent, the key is the :upper. This + // macro exists exclusively to uppercase the field names below. macro_rules! declare_server { ($($name:ident: $t:ty = $n:expr;)*) => {{ paste::paste! { @@ -271,7 +264,6 @@ fn main() -> ! { }; let ($($name),*) = ($(SensorArray($name)),*); ServerImpl { - deadline, $($name),* } }} @@ -308,5 +300,3 @@ mod idl { include!(concat!(env!("OUT_DIR"), "/server_stub.rs")); } - -include!(concat!(env!("OUT_DIR"), "/notifications.rs")); diff --git a/task/sp_measure/src/main.rs b/task/sp_measure/src/main.rs index d2bfdba9e..d64d69f49 100644 --- a/task/sp_measure/src/main.rs +++ b/task/sp_measure/src/main.rs @@ -70,9 +70,7 @@ fn main() -> ! { // Wait for a notification that will never come, politer than // busy looping forever - if sys_recv_closed(&mut [], 1, TaskId::KERNEL).is_err() { - panic!(); - } + sys_recv_notification(1); } } diff --git a/task/spd/src/main.rs b/task/spd/src/main.rs index 64ad7a2fe..bbefb6689 100644 --- a/task/spd/src/main.rs +++ b/task/spd/src/main.rs @@ -31,7 +31,7 @@ use ringbuf::{ringbuf, ringbuf_entry}; use task_jefe_api::Jefe; use task_packrat_api::Packrat; use userlib::{ - sys_irq_control, sys_recv_closed, task_slot, FromPrimitive, TaskId, + sys_irq_control, sys_recv_notification, task_slot, FromPrimitive, }; task_slot!(SYS, sys); @@ -104,13 +104,8 @@ fn main() -> ! { None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], - notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, - ); + // Only listen to our Jefe notification. + sys_recv_notification(notifications::JEFE_STATE_CHANGE_MASK); } } } @@ -254,7 +249,7 @@ fn main() -> ! { sys_irq_control(notification, true); }, wfi: |notification| { - let _ = sys_recv_closed(&mut [], notification, TaskId::KERNEL); + sys_recv_notification(notification); }, }; diff --git a/task/thermal/src/bsp/gimlet_bcdef.rs b/task/thermal/src/bsp/gimlet_bcdef.rs index 94afb990c..3f3d11ca1 100644 --- a/task/thermal/src/bsp/gimlet_bcdef.rs +++ b/task/thermal/src/bsp/gimlet_bcdef.rs @@ -11,7 +11,6 @@ use crate::{ }, i2c_config::{devices, sensors}, }; -use core::convert::TryInto; pub use drv_gimlet_seq_api::SeqError; use drv_gimlet_seq_api::{PowerState, Sequencer}; use drv_i2c_devices::max31790::Max31790; diff --git a/task/thermal/src/bsp/sidecar_bcd.rs b/task/thermal/src/bsp/sidecar_bcd.rs index 4b51d4ea0..38809c0d7 100644 --- a/task/thermal/src/bsp/sidecar_bcd.rs +++ b/task/thermal/src/bsp/sidecar_bcd.rs @@ -8,7 +8,6 @@ use crate::control::{ ChannelType, Device, FanControl, Fans, InputChannel, PidConfig, TemperatureSensor, }; -use core::convert::TryInto; use drv_i2c_devices::max31790::Max31790; use drv_i2c_devices::tmp451::*; pub use drv_sidecar_seq_api::SeqError; diff --git a/task/thermal/src/control.rs b/task/thermal/src/control.rs index 414de8829..9afa4a2ea 100644 --- a/task/thermal/src/control.rs +++ b/task/thermal/src/control.rs @@ -242,6 +242,7 @@ pub(crate) struct DynamicInputChannel { //////////////////////////////////////////////////////////////////////////////// #[derive(Copy, Clone)] +#[allow(dead_code)] // used only by the debugger pub struct TimestampedSensorError { pub timestamp: u64, pub id: SensorId, @@ -418,11 +419,14 @@ impl OneSidedPidState { } else { (-out_pd, output_limit - out_pd) }; - self.integral = self.integral.clamp(integral_min, integral_max); + // f32::clamp is not inlining well as of 2024-04 so we do it by hand + // here and below. + self.integral = self.integral.max(integral_min).min(integral_max); // Clamp output values to valid range. let out = out_pd + self.integral; - out.clamp(0.0, output_limit) + // same issue with f32::clamp (above) + out.max(0.0).min(output_limit) } } diff --git a/task/thermal/src/main.rs b/task/thermal/src/main.rs index e112da87d..ed6fe2e10 100644 --- a/task/thermal/src/main.rs +++ b/task/thermal/src/main.rs @@ -37,7 +37,6 @@ use crate::{ bsp::{Bsp, PowerBitmask, SeqError}, control::ThermalControl, }; -use core::convert::TryFrom; use drv_i2c_api::ResponseCode; use drv_i2c_devices::max31790::I2cWatchdog; use idol_runtime::{NotificationHandler, RequestError}; diff --git a/task/uartecho/src/main.rs b/task/uartecho/src/main.rs index 801128b4f..3a66a175b 100644 --- a/task/uartecho/src/main.rs +++ b/task/uartecho/src/main.rs @@ -47,11 +47,7 @@ fn main() -> ! { loop { // Wait for uart interrupt; if we haven't enabled tx interrupts, this // blocks until there's data to receive. - let _ = sys_recv_closed( - &mut [], - notifications::USART_IRQ_MASK, - TaskId::KERNEL, - ); + sys_recv_notification(notifications::USART_IRQ_MASK); // Walk through our tx state machine to handle echoing lines back; note // that many of these cases intentionally break after refilling diff --git a/task/udpecho/src/main.rs b/task/udpecho/src/main.rs index 587b97504..69cf04706 100644 --- a/task/udpecho/src/main.rs +++ b/task/udpecho/src/main.rs @@ -38,12 +38,7 @@ fn main() -> ! { Ok(()) => break, Err(SendError::QueueFull) => { // Our outgoing queue is full; wait for space. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(SendError::ServerRestarted) => { // Welp, lost an echo, we'll just soldier on. @@ -53,12 +48,7 @@ fn main() -> ! { } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted, the poor thing; just retry. diff --git a/task/udprpc/src/main.rs b/task/udprpc/src/main.rs index 1edc4c887..9556470a9 100644 --- a/task/udprpc/src/main.rs +++ b/task/udprpc/src/main.rs @@ -159,24 +159,14 @@ fn main() -> ! { // packets off our recv queue at the top of our main // loop. Err(SendError::QueueFull) => { - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } } } } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted (probably due to the watchdog); just retry. diff --git a/test/test-suite/src/main.rs b/test/test-suite/src/main.rs index ab826a939..bd840a8a5 100644 --- a/test/test-suite/src/main.rs +++ b/test/test-suite/src/main.rs @@ -1171,6 +1171,8 @@ fn test_timer_notify() { let deadline = start_time + 2; sys_set_timer(Some(deadline), ARBITRARY_NOTIFICATION); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], ARBITRARY_NOTIFICATION, TaskId::KERNEL) .unwrap(); @@ -1193,6 +1195,8 @@ fn test_timer_notify_past() { let deadline = start_time; sys_set_timer(Some(deadline), ARBITRARY_NOTIFICATION); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], ARBITRARY_NOTIFICATION, TaskId::KERNEL) .unwrap(); @@ -1417,6 +1421,8 @@ fn test_irq_notif() { trigger_test_irq(); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], notifications::TEST_IRQ_MASK, TaskId::KERNEL) .unwrap(); @@ -1465,6 +1471,9 @@ fn test_irq_status() { // do a `RECV` call to consume the posted notification. Now, we have the // second IRQ pending, and no notification posted. + // + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. sys_recv_closed(&mut [], notifications::TEST_IRQ_MASK, TaskId::KERNEL) .unwrap(); diff --git a/test/tests-stm32g0/app-g070.toml b/test/tests-stm32g0/app-g070.toml index 64f587204..0976d155b 100644 --- a/test/tests-stm32g0/app-g070.toml +++ b/test/tests-stm32g0/app-g070.toml @@ -6,7 +6,7 @@ memory = "memory-g070.toml" [kernel] name = "demo-stm32g0-nucleo" -requires = {flash = 19040, ram = 2828} +requires = {flash = 19112, ram = 2828} features = ["g070"] stacksize = 2048