From 201ec9498dfd3bf79eef0b739cae3ec3292447db Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:17:24 +0100 Subject: [PATCH 01/14] fdl: telegram: Add custom error type for function code parsing Propagate upwards why parsing failed. --- src/fdl/telegram.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/fdl/telegram.rs b/src/fdl/telegram.rs index fd3d75f..104fc62 100644 --- a/src/fdl/telegram.rs +++ b/src/fdl/telegram.rs @@ -207,6 +207,15 @@ pub enum FunctionCode { }, } +#[expect(clippy::enum_variant_names)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[repr(u8)] +pub enum FcParseError { + InvalidRequestType, + InvalidResponseState, + InvalidResponseStatus, +} + impl FunctionCode { pub fn to_byte(self) -> u8 { match self { @@ -217,18 +226,20 @@ impl FunctionCode { } } - pub fn from_byte(b: u8) -> Result { + pub fn from_byte(b: u8) -> Result { if b & (1 << 6) != 0 { let fcv = b & (1 << 4) != 0; let fcb = b & (1 << 5) != 0; - let req = RequestType::from_u8(b & 0x8F).ok_or(())?; + let req = RequestType::from_u8(b & 0x8F).ok_or(FcParseError::InvalidRequestType)?; Ok(Self::Request { fcb: FrameCountBit::from_fcv_fcb(fcv, fcb), req, }) } else { - let state = ResponseState::from_u8((b & 0x30) >> 4).ok_or(())?; - let status = ResponseStatus::from_u8(b & 0x0F).ok_or(())?; + let state = ResponseState::from_u8((b & 0x30) >> 4) + .ok_or(FcParseError::InvalidResponseState)?; + let status = + ResponseStatus::from_u8(b & 0x0F).ok_or(FcParseError::InvalidResponseStatus)?; Ok(Self::Response { state, status }) } } From 832a693c440ffe99710ef35589c7044ce041c3a9 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:19:03 +0100 Subject: [PATCH 02/14] fdl: parameters: Use div_ceil() instead of manually implement the same --- src/fdl/parameters.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fdl/parameters.rs b/src/fdl/parameters.rs index 8c3323e..1ddb1f8 100644 --- a/src/fdl/parameters.rs +++ b/src/fdl/parameters.rs @@ -106,7 +106,7 @@ fn watchdog_factors(dur: crate::time::Duration) -> Option> let timeout_10ms: u32 = (dur.total_millis() / 10).try_into().or(Err(()))?; for f1 in 1..256 { - let f2 = (timeout_10ms + f1 - 1) / f1; + let f2 = timeout_10ms.div_ceil(f1); if f2 < 256 { return Ok((u8::try_from(f1).unwrap(), u8::try_from(f2).unwrap())); From 9f15366fad75c2b1fa10a3672e1f48b3a19d8092 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:24:02 +0100 Subject: [PATCH 03/14] dp: Fix relevant clippy lints --- src/dp/diagnostics.rs | 2 +- src/dp/peripheral.rs | 8 ++++---- src/dp/scan.rs | 7 +++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/dp/diagnostics.rs b/src/dp/diagnostics.rs index dcefd27..931d95b 100644 --- a/src/dp/diagnostics.rs +++ b/src/dp/diagnostics.rs @@ -152,7 +152,7 @@ impl ChannelError { 8 => ChannelError::LowerLimitUndershoot, 9 => ChannelError::Error, v @ 16..=31 => ChannelError::Vendor(v), - r @ _ => ChannelError::Reserved(r), + r => ChannelError::Reserved(r), } } diff --git a/src/dp/peripheral.rs b/src/dp/peripheral.rs index 20e76c6..35f2db2 100644 --- a/src/dp/peripheral.rs +++ b/src/dp/peripheral.rs @@ -314,7 +314,7 @@ impl<'a> Peripheral<'a> { /// Get the last diagnostics information received from this peripheral. #[inline] - pub fn last_diagnostics(&self) -> Option { + pub fn last_diagnostics(&self) -> Option> { self.diag.as_ref().map(|diag| PeripheralDiagnostics { flags: diag.flags, ident_number: diag.ident_number, @@ -399,7 +399,7 @@ impl<'a> Peripheral<'a> { // Groups buf[6] = self.options.groups; // User Prm Data - buf[7..].copy_from_slice(&user_parameters); + buf[7..].copy_from_slice(user_parameters); }, )) } else { @@ -420,7 +420,7 @@ impl<'a> Peripheral<'a> { }, config.len(), |buf| { - buf.copy_from_slice(&config); + buf.copy_from_slice(config); }, )) } else { @@ -598,7 +598,7 @@ impl<'a> Peripheral<'a> { if data_ok { if t.pdu.len() == self.pi_i.len() { - self.pi_i.copy_from_slice(&t.pdu); + self.pi_i.copy_from_slice(t.pdu); self.state = PeripheralState::DataExchange; Some(PeripheralEvent::DataExchanged) } else { diff --git a/src/dp/scan.rs b/src/dp/scan.rs index 8b7e1b8..635123e 100644 --- a/src/dp/scan.rs +++ b/src/dp/scan.rs @@ -12,6 +12,7 @@ pub enum DpScanEvent { PeripheralLost(crate::Address), } +#[derive(Debug)] pub struct DpScanner { stations: bitvec::BitArr!(for 128), cursor: crate::Address, @@ -19,6 +20,12 @@ pub struct DpScanner { current_address_done: bool, } +impl Default for DpScanner { + fn default() -> Self { + Self::new() + } +} + impl DpScanner { pub fn new() -> Self { Self { From 23b72cdb0e7beef92236291ab1d9eed7ee524015 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:25:32 +0100 Subject: [PATCH 04/14] fdl: Fix relevant clippy lints --- src/fdl/active.rs | 13 ++++++------- src/fdl/live_list.rs | 8 +++++++- src/fdl/mod.rs | 1 - src/fdl/telegram.rs | 1 + src/fdl/token_ring.rs | 6 ++---- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/fdl/active.rs b/src/fdl/active.rs index cf14e07..f53ec35 100644 --- a/src/fdl/active.rs +++ b/src/fdl/active.rs @@ -48,11 +48,11 @@ enum GapState { impl GapState { pub fn increment_wait(&mut self) { - match self { - GapState::Waiting { - ref mut rotation_count, - } => *rotation_count += 1, - _ => (), + if let GapState::Waiting { + ref mut rotation_count, + } = self + { + *rotation_count += 1; } } } @@ -938,8 +938,7 @@ impl FdlActiveStation { high_prio_only: bool, ) -> Option { if let Some(tx_res) = phy.transmit_telegram(now, |tx| { - let res = app.transmit_telegram(now, self, tx, high_prio_only); - res + app.transmit_telegram(now, self, tx, high_prio_only) }) { if let Some(addr) = tx_res.expects_reply() { let data = *self.state.get_use_token_data(); diff --git a/src/fdl/live_list.rs b/src/fdl/live_list.rs index ef15b20..008fc6d 100644 --- a/src/fdl/live_list.rs +++ b/src/fdl/live_list.rs @@ -18,6 +18,12 @@ pub struct LiveList { current_address_done: bool, } +impl Default for LiveList { + fn default() -> Self { + Self::new() + } +} + impl LiveList { pub fn new() -> Self { Self { @@ -28,7 +34,7 @@ impl LiveList { } } - pub fn iter_stations(&self) -> impl Iterator + DoubleEndedIterator + '_ { + pub fn iter_stations(&self) -> impl DoubleEndedIterator + '_ { self.stations.iter_ones().map(|a| u8::try_from(a).unwrap()) } diff --git a/src/fdl/mod.rs b/src/fdl/mod.rs index b291b66..c53f779 100644 --- a/src/fdl/mod.rs +++ b/src/fdl/mod.rs @@ -77,7 +77,6 @@ impl FdlApplication for () { addr: u8, telegram: Telegram, ) { - () } fn handle_timeout(&mut self, now: crate::time::Instant, fdl: &FdlActiveStation, addr: u8) { diff --git a/src/fdl/telegram.rs b/src/fdl/telegram.rs index 104fc62..99f3fef 100644 --- a/src/fdl/telegram.rs +++ b/src/fdl/telegram.rs @@ -1,5 +1,6 @@ #![cfg_attr(test, allow(non_local_definitions))] +#[expect(clippy::identity_op)] #[derive(Debug, PartialEq, Eq, Clone, Copy)] #[cfg_attr(test, derive(proptest_derive::Arbitrary))] #[repr(u8)] diff --git a/src/fdl/token_ring.rs b/src/fdl/token_ring.rs index b38774c..3d1488a 100644 --- a/src/fdl/token_ring.rs +++ b/src/fdl/token_ring.rs @@ -67,9 +67,7 @@ impl TokenRing { } } - pub fn iter_active_stations( - &self, - ) -> impl Iterator + DoubleEndedIterator + '_ { + pub fn iter_active_stations(&self) -> impl DoubleEndedIterator + '_ { self.active_stations .iter_ones() .map(|a| u8::try_from(a).unwrap()) @@ -153,7 +151,7 @@ impl TokenRing { .find(|a| *a < self.this_station) { previous - } else if let Some(last) = self.iter_active_stations().rev().next() { + } else if let Some(last) = self.iter_active_stations().next_back() { last } else { self.this_station From 0682d2b3296a198c6ae269eb4a5fd843443bc35e Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:26:26 +0100 Subject: [PATCH 05/14] phy: Fix relevant clippy lints --- src/phy/serial.rs | 10 ++-------- src/phy/simulator.rs | 5 ++--- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/phy/serial.rs b/src/phy/serial.rs index 7774f05..f020082 100644 --- a/src/phy/serial.rs +++ b/src/phy/serial.rs @@ -16,17 +16,11 @@ enum PhyData<'a> { impl PhyData<'_> { pub fn is_rx(&self) -> bool { - match self { - PhyData::Rx { .. } => true, - _ => false, - } + matches!(self, PhyData::Rx { .. }) } pub fn is_tx(&self) -> bool { - match self { - PhyData::Tx { .. } => true, - _ => false, - } + matches!(self, PhyData::Tx { .. }) } pub fn make_rx(&mut self) { diff --git a/src/phy/simulator.rs b/src/phy/simulator.rs index 37f1e90..69a7587 100644 --- a/src/phy/simulator.rs +++ b/src/phy/simulator.rs @@ -176,10 +176,9 @@ impl SimulatorBus { &self.stream[t.index..t.index + t.length] } - pub fn get_telegram(&self, t: &CapturedTelegram) -> Option { + pub fn get_telegram(&self, t: &CapturedTelegram) -> Option> { crate::fdl::Telegram::deserialize(self.get_telegram_data(t)) - .map(Result::ok) - .flatten() + .and_then(Result::ok) .map(|(t, _)| t) } From 01353b1da58fce11bcab17ca2d5d2753a4570049 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:27:12 +0100 Subject: [PATCH 06/14] Globally allow certain clippy lints These lints are too noisy or make suggestions that we do not prefer to use. --- src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index 46193de..def0005 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -77,6 +77,9 @@ // TODO: Remove this once the crate has matured. #![allow(dead_code)] #![allow(unused_variables)] +#![allow(clippy::len_zero)] +#![allow(clippy::collapsible_if)] +#![allow(clippy::manual_range_contains)] #![cfg_attr(not(any(feature = "std", test)), no_std)] mod consts; From 92cefaa7d9c6ce23de607df62c76abf1d092bbf0 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:28:02 +0100 Subject: [PATCH 07/14] fdl: active: Allow certain clippy lints Due to the design patterns used in the active station implementation, some clippy lints become invalid. Disable them for this module. --- src/fdl/active.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/fdl/active.rs b/src/fdl/active.rs index f53ec35..5dcdf7a 100644 --- a/src/fdl/active.rs +++ b/src/fdl/active.rs @@ -1,6 +1,12 @@ //! Implementation of an FDL active station. #![deny(unused_must_use)] +#![allow(clippy::unneeded_struct_pattern)] +#![allow(clippy::wildcard_in_or_patterns)] +#![allow(clippy::extra_unused_lifetimes)] +#![allow(clippy::unit_arg)] +#![allow(clippy::useless_conversion)] + use crate::fdl::FdlApplication; use crate::phy::ProfibusPhy; From 6a964f705af10f2d3ace6631fb23816e34ab5bf6 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:39:01 +0100 Subject: [PATCH 08/14] gsd-parser: Remove and deny all remaining as casts This also allows us to get rid of a number of assertions along the way as those are now directly covered by the try_from() errors. --- gsd-parser/src/lib.rs | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/gsd-parser/src/lib.rs b/gsd-parser/src/lib.rs index b1e4bad..9454b74 100644 --- a/gsd-parser/src/lib.rs +++ b/gsd-parser/src/lib.rs @@ -1,3 +1,5 @@ +#![deny(clippy::as_conversions)] + use std::collections::BTreeMap; use std::path::Path; use std::sync::Arc; @@ -156,37 +158,31 @@ impl UserPrmDataType { pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) { match self { UserPrmDataType::Unsigned8 => { - assert!(0 <= value && value <= 255); - s[..1].copy_from_slice(&(value as u8).to_be_bytes()); + s[..1].copy_from_slice(&u8::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Unsigned16 => { - assert!(0 <= value && value <= 65535); - s[..2].copy_from_slice(&(value as u16).to_be_bytes()); + s[..2].copy_from_slice(&u16::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Unsigned32 => { - assert!(0 <= value && value <= 4294967295); - s[..4].copy_from_slice(&(value as u32).to_be_bytes()); + s[..4].copy_from_slice(&u32::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Signed8 => { - assert!(-127 <= value && value <= 127); - s[..1].copy_from_slice(&(value as i8).to_be_bytes()); + s[..1].copy_from_slice(&i8::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Signed16 => { - assert!(-32767 <= value && value <= 32767); - s[..2].copy_from_slice(&(value as i16).to_be_bytes()); + s[..2].copy_from_slice(&u16::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Signed32 => { - assert!(2147483647 <= value && value <= 2147483647); - s[..4].copy_from_slice(&(value as i32).to_be_bytes()); + s[..4].copy_from_slice(&i32::try_from(value).unwrap().to_be_bytes()); } UserPrmDataType::Bit(b) => { assert!(value == 0 || value == 1); - s[0] |= (value as u8) << b; + s[0] |= u8::try_from(value).unwrap() << b; } UserPrmDataType::BitArea(first, last) => { let bit_size = last - first + 1; - assert!(value >= 0 && value < 2i64.pow(bit_size as u32)); - s[0] = (value as u8) << first; + assert!(value >= 0 && value < 2i64.pow(u32::from(bit_size))); + s[0] = u8::try_from(value).unwrap() << first; } } } @@ -387,7 +383,7 @@ impl<'a> PrmBuilder<'a> { self.update_prm_data_len(*offset, size); data_ref .data_type - .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset as usize)..]); + .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset)..]); } } @@ -401,7 +397,7 @@ impl<'a> PrmBuilder<'a> { data_ref.constraint.assert_valid(value); data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset as usize)..]); + .write_value_to_slice(value, &mut self.prm[(*offset)..]); self } @@ -417,7 +413,7 @@ impl<'a> PrmBuilder<'a> { data_ref.constraint.assert_valid(value); data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset as usize)..]); + .write_value_to_slice(value, &mut self.prm[(*offset)..]); self } From f4ce43107383ed34a763b293cf27bc63fe9b3789 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:40:19 +0100 Subject: [PATCH 09/14] gsd-parser: Fix relevant clippy lints --- gsd-parser/src/lib.rs | 1 + gsd-parser/src/parser.rs | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gsd-parser/src/lib.rs b/gsd-parser/src/lib.rs index 9454b74..b5e1718 100644 --- a/gsd-parser/src/lib.rs +++ b/gsd-parser/src/lib.rs @@ -1,3 +1,4 @@ +#![expect(clippy::result_large_err)] #![deny(clippy::as_conversions)] use std::collections::BTreeMap; diff --git a/gsd-parser/src/parser.rs b/gsd-parser/src/parser.rs index b45073a..adc7df8 100644 --- a/gsd-parser/src/parser.rs +++ b/gsd-parser/src/parser.rs @@ -57,7 +57,6 @@ where Ok(match pair.as_rule() { gsd_parser::Rule::number_list => pair .into_inner() - .into_iter() .map(|p| parse_number::(p)) .collect::>>()?, gsd_parser::Rule::dec_number | gsd_parser::Rule::hex_number => { @@ -96,7 +95,7 @@ pub fn parse( fn parse_inner(source: &str) -> ParseResult { use pest::Parser; - let gsd_pairs = gsd_parser::GsdParser::parse(gsd_parser::Rule::gsd, &source)? + let gsd_pairs = gsd_parser::GsdParser::parse(gsd_parser::Rule::gsd, source)? .next() .expect("pest grammar wrong?"); From a8a3b0f605e2cdb3fa9c78603592a4c17a6434d4 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:43:28 +0100 Subject: [PATCH 10/14] gsd-parser: Mark unclean unwrap() using expect("TODO") This helps showing where improved error handling still needs to be implemented. --- gsd-parser/src/lib.rs | 13 +++++++------ gsd-parser/src/parser.rs | 11 ++++++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/gsd-parser/src/lib.rs b/gsd-parser/src/lib.rs index b5e1718..dba024c 100644 --- a/gsd-parser/src/lib.rs +++ b/gsd-parser/src/lib.rs @@ -156,6 +156,7 @@ impl UserPrmDataType { } } + // TODO: Make this function fallible pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) { match self { UserPrmDataType::Unsigned8 => { @@ -394,7 +395,7 @@ impl<'a> PrmBuilder<'a> { .data_ref .iter() .find(|(_, r)| r.name == prm) - .unwrap(); + .expect("TODO"); data_ref.constraint.assert_valid(value); data_ref .data_type @@ -408,9 +409,9 @@ impl<'a> PrmBuilder<'a> { .data_ref .iter() .find(|(_, r)| r.name == prm) - .unwrap(); - let text_ref = data_ref.text_ref.as_ref().unwrap(); - let value = *text_ref.get(value).unwrap(); + .expect("TODO"); + let text_ref = data_ref.text_ref.as_ref().expect("TODO"); + let value = *text_ref.get(value).expect("TODO"); data_ref.constraint.assert_valid(value); data_ref .data_type @@ -430,9 +431,9 @@ impl<'a> PrmBuilder<'a> { pub fn parse_from_file>(file: P) -> GenericStationDescription { use std::io::Read; - let mut f = std::fs::File::open(file.as_ref()).unwrap(); + let mut f = std::fs::File::open(file.as_ref()).expect("TODO"); let mut source_bytes = Vec::new(); - f.read_to_end(&mut source_bytes).unwrap(); + f.read_to_end(&mut source_bytes).expect("TODO"); let source = String::from_utf8_lossy(&source_bytes); match parser::parse(file.as_ref(), &source) { diff --git a/gsd-parser/src/parser.rs b/gsd-parser/src/parser.rs index adc7df8..d1d8ca7 100644 --- a/gsd-parser/src/parser.rs +++ b/gsd-parser/src/parser.rs @@ -275,8 +275,10 @@ fn parse_inner(source: &str) -> ParseResult { "ext_user_prm_data_ref" => { let offset = parse_number(value_pair)?; let data_id = parse_number(pairs.next().unwrap())?; - let data_ref = - user_prm_data_definitions.get(&data_id).unwrap().clone(); + let data_ref = user_prm_data_definitions + .get(&data_id) + .expect("TODO") + .clone(); module_prm_data.data_ref.push((offset, data_ref)); } "ext_user_prm_data_const" => { @@ -482,7 +484,10 @@ fn parse_inner(source: &str) -> ParseResult { "ext_user_prm_data_ref" => { let offset = parse_number(value_pair)?; let data_id = parse_number(pairs.next().unwrap())?; - let data_ref = user_prm_data_definitions.get(&data_id).unwrap().clone(); + let data_ref = user_prm_data_definitions + .get(&data_id) + .expect("TODO") + .clone(); gsd.user_prm_data.data_ref.push((offset, data_ref)); // The presence of this keywords means `User_Prm_Data` and // `User_Prm_Data_Len` should be ignored. From 58a05e7ca025754a23a1f8e076534fbae3f08271 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 17:58:47 +0100 Subject: [PATCH 11/14] gsd-parser: Make write_value_to_slice() fallible Don't immediately panic when an out-of-range value is supplied to UserPrmDataType::write_value_to_slice() and functions using it. --- gsd-parser/src/lib.rs | 71 +++++++++++++++++++++++++------------ gsd-parser/tests/regress.rs | 6 ++-- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/gsd-parser/src/lib.rs b/gsd-parser/src/lib.rs index dba024c..a082f7b 100644 --- a/gsd-parser/src/lib.rs +++ b/gsd-parser/src/lib.rs @@ -142,6 +142,23 @@ pub enum UserPrmDataType { BitArea(u8, u8), } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PrmValueRangeError(pub(crate) ()); + +impl From for PrmValueRangeError { + fn from(_value: std::num::TryFromIntError) -> Self { + PrmValueRangeError(()) + } +} + +impl std::fmt::Display for PrmValueRangeError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + "value for user_prm is not in range for data type".fmt(f) + } +} + +impl std::error::Error for PrmValueRangeError {} + impl UserPrmDataType { pub fn size(self) -> usize { match self { @@ -156,37 +173,42 @@ impl UserPrmDataType { } } - // TODO: Make this function fallible - pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) { + pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) -> Result<(), PrmValueRangeError> { match self { UserPrmDataType::Unsigned8 => { - s[..1].copy_from_slice(&u8::try_from(value).unwrap().to_be_bytes()); + s[..1].copy_from_slice(&u8::try_from(value)?.to_be_bytes()); } UserPrmDataType::Unsigned16 => { - s[..2].copy_from_slice(&u16::try_from(value).unwrap().to_be_bytes()); + s[..2].copy_from_slice(&u16::try_from(value)?.to_be_bytes()); } UserPrmDataType::Unsigned32 => { - s[..4].copy_from_slice(&u32::try_from(value).unwrap().to_be_bytes()); + s[..4].copy_from_slice(&u32::try_from(value)?.to_be_bytes()); } UserPrmDataType::Signed8 => { - s[..1].copy_from_slice(&i8::try_from(value).unwrap().to_be_bytes()); + s[..1].copy_from_slice(&i8::try_from(value)?.to_be_bytes()); } UserPrmDataType::Signed16 => { - s[..2].copy_from_slice(&u16::try_from(value).unwrap().to_be_bytes()); + s[..2].copy_from_slice(&u16::try_from(value)?.to_be_bytes()); } UserPrmDataType::Signed32 => { - s[..4].copy_from_slice(&i32::try_from(value).unwrap().to_be_bytes()); + s[..4].copy_from_slice(&i32::try_from(value)?.to_be_bytes()); } UserPrmDataType::Bit(b) => { + if value != 0 && value != 1 { + return Err(PrmValueRangeError(())); + } assert!(value == 0 || value == 1); - s[0] |= u8::try_from(value).unwrap() << b; + s[0] |= u8::try_from(value)? << b; } UserPrmDataType::BitArea(first, last) => { let bit_size = last - first + 1; - assert!(value >= 0 && value < 2i64.pow(u32::from(bit_size))); - s[0] = u8::try_from(value).unwrap() << first; + if value < 0 || value >= 2i64.pow(u32::from(bit_size)) { + return Err(PrmValueRangeError(())); + } + s[0] = u8::try_from(value)? << first; } } + Ok(()) } } @@ -354,14 +376,14 @@ pub struct PrmBuilder<'a> { } impl<'a> PrmBuilder<'a> { - pub fn new(desc: &'a UserPrmData) -> Self { + pub fn new(desc: &'a UserPrmData) -> Result { let mut this = Self { desc, prm: Vec::new(), }; this.write_const_prm_data(); - this.write_default_prm_data(); - this + this.write_default_prm_data()?; + Ok(this) } fn update_prm_data_len(&mut self, offset: usize, size: usize) { @@ -379,17 +401,18 @@ impl<'a> PrmBuilder<'a> { } } - fn write_default_prm_data(&mut self) { + fn write_default_prm_data(&mut self) -> Result<(), PrmValueRangeError> { for (offset, data_ref) in self.desc.data_ref.iter() { let size = data_ref.data_type.size(); self.update_prm_data_len(*offset, size); data_ref .data_type - .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset)..]); + .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset)..])?; } + Ok(()) } - pub fn set_prm(&mut self, prm: &str, value: i64) -> &mut Self { + pub fn set_prm(&mut self, prm: &str, value: i64) -> Result<&mut Self, PrmValueRangeError> { let (offset, data_ref) = self .desc .data_ref @@ -399,11 +422,15 @@ impl<'a> PrmBuilder<'a> { data_ref.constraint.assert_valid(value); data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset)..]); - self + .write_value_to_slice(value, &mut self.prm[(*offset)..])?; + Ok(self) } - pub fn set_prm_from_text(&mut self, prm: &str, value: &str) -> &mut Self { + pub fn set_prm_from_text( + &mut self, + prm: &str, + value: &str, + ) -> Result<&mut Self, PrmValueRangeError> { let (offset, data_ref) = self .desc .data_ref @@ -415,8 +442,8 @@ impl<'a> PrmBuilder<'a> { data_ref.constraint.assert_valid(value); data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset)..]); - self + .write_value_to_slice(value, &mut self.prm[(*offset)..])?; + Ok(self) } pub fn as_bytes(&self) -> &[u8] { diff --git a/gsd-parser/tests/regress.rs b/gsd-parser/tests/regress.rs index 302ecb4..7fc886f 100644 --- a/gsd-parser/tests/regress.rs +++ b/gsd-parser/tests/regress.rs @@ -11,7 +11,7 @@ fn regress(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) { fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) { let name = gsd_file.file_stem().unwrap().to_string_lossy().to_string(); let gsd = gsd_parser::parse_from_file(gsd_file); - let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data); + let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data).unwrap(); // Try setting all the available parameters to some reasonable values. for (_, prm_ref) in gsd.user_prm_data.data_ref.iter() { @@ -22,14 +22,14 @@ fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) { // Fallback when the list only has one text... texts.keys().next().unwrap() }; - prm.set_prm_from_text(&prm_ref.name, text); + prm.set_prm_from_text(&prm_ref.name, text).unwrap(); } else { let v = match &prm_ref.constraint { gsd_parser::PrmValueConstraint::MinMax(_, max) => *max, gsd_parser::PrmValueConstraint::Enum(values) => *values.last().unwrap(), gsd_parser::PrmValueConstraint::Unconstrained => 1, }; - prm.set_prm(&prm_ref.name, v); + prm.set_prm(&prm_ref.name, v).unwrap(); } } From 35b3eca78baf9e5228996bae9ed1634be80c2e93 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 18:39:46 +0100 Subject: [PATCH 12/14] gsdtool: Catch new results from PrmBuilder For now just panic, this will need to be improved in the future. --- gsdtool/src/main.rs | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/gsdtool/src/main.rs b/gsdtool/src/main.rs index a006dad..1de5610 100644 --- a/gsdtool/src/main.rs +++ b/gsdtool/src/main.rs @@ -78,7 +78,7 @@ fn run_config_wizard(args: &ConfigWizardOptions) { println!(); println!("{}", style("Global parameters:").bold()); - let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data); + let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data).expect("TODO"); let mut global_parameters = vec![]; let mut had_parameters = false; for (_, prm_ref) in gsd.user_prm_data.data_ref.iter() { @@ -103,8 +103,9 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let sel_text = &texts_list[selection]; - prm.set_prm_from_text(&prm_ref.name, sel_text); + let sel_text = &texts_list.get(selection).expect("TODO"); + prm.set_prm_from_text(&prm_ref.name, sel_text) + .expect("TODO"); global_parameters.push((prm_ref.name.to_owned(), sel_text.to_string())); } else if let gsd_parser::PrmValueConstraint::MinMax(min, max) = prm_ref.constraint { @@ -121,8 +122,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = str::parse(&value).unwrap(); - prm.set_prm(&prm_ref.name, value); + let value: i64 = str::parse(&value).expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); global_parameters.push((prm_ref.name.to_owned(), value.to_string())); } else if let gsd_parser::PrmValueConstraint::Enum(values) = &prm_ref.constraint { @@ -141,8 +142,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = values[selection]; - prm.set_prm(&prm_ref.name, value); + let value: i64 = values.get(selection).copied().expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); global_parameters.push((prm_ref.name.to_owned(), value.to_string())); } else { @@ -159,8 +160,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = str::parse(&value_str).unwrap(); - prm.set_prm(&prm_ref.name, value); + let value: i64 = str::parse(&value_str).expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); global_parameters.push((prm_ref.name.to_owned(), value_str)); } @@ -254,7 +255,7 @@ fn run_config_wizard(args: &ConfigWizardOptions) { module_config.append(&mut module.config.to_vec()); - let mut prm = gsd_parser::PrmBuilder::new(&module.module_prm_data); + let mut prm = gsd_parser::PrmBuilder::new(&module.module_prm_data).expect("TODO"); let mut module_parameters = vec![]; for (_, prm_ref) in module.module_prm_data.data_ref.iter() { if !prm_ref.visible || !prm_ref.changeable { @@ -279,7 +280,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .unwrap(); let sel_text = &texts_list[selection]; - prm.set_prm_from_text(&prm_ref.name, sel_text); + prm.set_prm_from_text(&prm_ref.name, sel_text) + .expect("TODO"); module_parameters.push((prm_ref.name.to_owned(), sel_text.to_string())); } else if let gsd_parser::PrmValueConstraint::MinMax(min, max) = prm_ref.constraint @@ -297,8 +299,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = str::parse(&value).unwrap(); - prm.set_prm(&prm_ref.name, value); + let value: i64 = str::parse(&value).expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); module_parameters.push((prm_ref.name.to_owned(), value.to_string())); } else if let gsd_parser::PrmValueConstraint::Enum(values) = &prm_ref.constraint { @@ -317,8 +319,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = values[selection]; - prm.set_prm(&prm_ref.name, value); + let value: i64 = values.get(selection).copied().expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); module_parameters.push((prm_ref.name.to_owned(), value.to_string())); } else { @@ -335,8 +337,8 @@ fn run_config_wizard(args: &ConfigWizardOptions) { .interact() .unwrap(); - let value: i64 = str::parse(&value_str).unwrap(); - prm.set_prm(&prm_ref.name, value); + let value: i64 = str::parse(&value_str).expect("TODO"); + prm.set_prm(&prm_ref.name, value).expect("TODO"); module_parameters.push((prm_ref.name.to_owned(), value_str)); } From 7c04400c415c840a5f5c0be2df574f560376b2a2 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 19:18:24 +0100 Subject: [PATCH 13/14] gsd-parser: Add proper error types for all possible failures during user_prm setting Propagate all invalid situations during set_prm() or set_prm_from_text() to the user instead of panicking. --- gsd-parser/src/lib.rs | 130 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 106 insertions(+), 24 deletions(-) diff --git a/gsd-parser/src/lib.rs b/gsd-parser/src/lib.rs index a082f7b..7153b87 100644 --- a/gsd-parser/src/lib.rs +++ b/gsd-parser/src/lib.rs @@ -219,6 +219,28 @@ pub enum PrmValueConstraint { Unconstrained, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PrmValueConstraintError { + value: i64, + constraint: PrmValueConstraint, +} + +impl std::fmt::Display for PrmValueConstraintError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match &self.constraint { + PrmValueConstraint::MinMax(min, max) => { + write!(f, "value {} not in range {min}..={max}", self.value) + } + PrmValueConstraint::Enum(values) => { + write!(f, "value {} not in set {values:?}", self.value) + } + PrmValueConstraint::Unconstrained => unreachable!(), + } + } +} + +impl std::error::Error for PrmValueConstraintError {} + impl PrmValueConstraint { pub fn is_valid(&self, value: i64) -> bool { match self { @@ -228,21 +250,29 @@ impl PrmValueConstraint { } } - pub fn assert_valid(&self, value: i64) { + pub fn assert_valid(&self, value: i64) -> Result<(), PrmValueConstraintError> { match self { PrmValueConstraint::MinMax(min, max) => { - assert!( - *min <= value && value <= *max, - "value {value} not in range {min}..={max}", - ); + if *min > value || value > *max { + Err(PrmValueConstraintError { + value, + constraint: self.clone(), + }) + } else { + Ok(()) + } } PrmValueConstraint::Enum(values) => { - assert!( - values.contains(&value), - "value {value} not in set {values:?}", - ); + if !values.contains(&value) { + Err(PrmValueConstraintError { + value, + constraint: self.clone(), + }) + } else { + Ok(()) + } } - PrmValueConstraint::Unconstrained => (), + PrmValueConstraint::Unconstrained => Ok(()), } } } @@ -370,6 +400,46 @@ pub struct GenericStationDescription { pub unit_diag: UnitDiag, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SetPrmError { + PrmNotFound(String), + PrmWithoutTexts(String), + PrmTextNotFound { prm: String, text: String }, + ValueConstraint(PrmValueConstraintError), + ValueRange { value: i64, ty: UserPrmDataType }, +} + +impl From for SetPrmError { + fn from(value: PrmValueConstraintError) -> Self { + Self::ValueConstraint(value) + } +} + +impl std::fmt::Display for SetPrmError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SetPrmError::PrmNotFound(name) => { + write!(f, "prm {name} was not found") + } + SetPrmError::ValueConstraint(prm_value_constraint_error) => { + prm_value_constraint_error.fmt(f) + } + SetPrmError::ValueRange { value, ty } => { + write!(f, "value {value} is out of range for type {ty:?}") + } + SetPrmError::PrmWithoutTexts(name) => { + write!(f, "prm {name} does not have texts") + } + SetPrmError::PrmTextNotFound { prm, text } => { + write!(f, "prm {prm} does not have text {text:?}") + } + } + } +} + +impl std::error::Error for SetPrmError {} + +#[derive(Debug, PartialEq, Eq)] pub struct PrmBuilder<'a> { desc: &'a UserPrmData, prm: Vec, @@ -412,37 +482,49 @@ impl<'a> PrmBuilder<'a> { Ok(()) } - pub fn set_prm(&mut self, prm: &str, value: i64) -> Result<&mut Self, PrmValueRangeError> { + pub fn set_prm(&mut self, prm: &str, value: i64) -> Result<&mut Self, SetPrmError> { let (offset, data_ref) = self .desc .data_ref .iter() .find(|(_, r)| r.name == prm) - .expect("TODO"); - data_ref.constraint.assert_valid(value); + .ok_or_else(|| SetPrmError::PrmNotFound(prm.to_string()))?; + data_ref.constraint.assert_valid(value)?; data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset)..])?; + .write_value_to_slice(value, &mut self.prm[(*offset)..]) + .map_err(|_e| SetPrmError::ValueRange { + value, + ty: data_ref.data_type, + })?; Ok(self) } - pub fn set_prm_from_text( - &mut self, - prm: &str, - value: &str, - ) -> Result<&mut Self, PrmValueRangeError> { + pub fn set_prm_from_text(&mut self, prm: &str, value: &str) -> Result<&mut Self, SetPrmError> { let (offset, data_ref) = self .desc .data_ref .iter() .find(|(_, r)| r.name == prm) - .expect("TODO"); - let text_ref = data_ref.text_ref.as_ref().expect("TODO"); - let value = *text_ref.get(value).expect("TODO"); - data_ref.constraint.assert_valid(value); + .ok_or_else(|| SetPrmError::PrmNotFound(prm.to_string()))?; + let text_ref = data_ref + .text_ref + .as_ref() + .ok_or_else(|| SetPrmError::PrmWithoutTexts(prm.to_string()))?; + let value = *text_ref + .get(value) + .ok_or_else(|| SetPrmError::PrmTextNotFound { + prm: prm.to_string(), + text: value.to_string(), + })?; + data_ref.constraint.assert_valid(value)?; data_ref .data_type - .write_value_to_slice(value, &mut self.prm[(*offset)..])?; + .write_value_to_slice(value, &mut self.prm[(*offset)..]) + .map_err(|_e| SetPrmError::ValueRange { + value, + ty: data_ref.data_type, + })?; Ok(self) } From e3fdbf0107aa3baba3c9d1ec33830469906c3aeb Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 19:19:34 +0100 Subject: [PATCH 14/14] gsd-parser: tests: Check that invalid set_prm() does not panic Add more assertions to ensure invalid calls to set_prm() and set_prm_from_text() never panic. --- gsd-parser/tests/regress.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/gsd-parser/tests/regress.rs b/gsd-parser/tests/regress.rs index 7fc886f..57f69a2 100644 --- a/gsd-parser/tests/regress.rs +++ b/gsd-parser/tests/regress.rs @@ -23,6 +23,10 @@ fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) { texts.keys().next().unwrap() }; prm.set_prm_from_text(&prm_ref.name, text).unwrap(); + + // Test that trying a wrong text doens't panic + let res = prm.set_prm_from_text(&prm_ref.name, "InvalidTextAllTheWay"); + assert!(res.is_err()); } else { let v = match &prm_ref.constraint { gsd_parser::PrmValueConstraint::MinMax(_, max) => *max, @@ -30,8 +34,20 @@ fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) { gsd_parser::PrmValueConstraint::Unconstrained => 1, }; prm.set_prm(&prm_ref.name, v).unwrap(); + + // Test that an invalid value results in an error rather than a panic + let res = prm.set_prm(&prm_ref.name, i64::MIN); + assert!(res.is_err()); + + // Test that trying a text PRM doesn't panic + let res = prm.set_prm_from_text(&prm_ref.name, "InvalidTextAllTheWay"); + assert!(res.is_err()); } } + // Test that a non-existent PRM doesn't panic + let res = prm.set_prm("ThisPrmNeverEverExistsEver", 0); + assert!(res.is_err()); + insta::assert_debug_snapshot!(format!("{}-PRM", name).as_ref(), prm.as_bytes()); }