From d42ca4f0303426820e1b94233db95a3c75ed4e25 Mon Sep 17 00:00:00 2001 From: Stan Grams Date: Thu, 6 Aug 2026 20:57:59 +0200 Subject: [PATCH] [fix](trx-config): validate every rig, not just the legacy flat one ServerConfig::validate() checked the flat [rig]/[audio]/[behavior] fields and gave [[rigs]] entries only an id/audio-port uniqueness pass, and validate_sdr() returned early unless the *flat* access type was "sdr". A multi-rig SDR station therefore got no Nyquist, stream_opus, duplicate-decoder or tx_enabled checking at all, and a rig entry with frame_duration_ms = 7 or a missing baud rate started and failed at runtime. Move the per-rig rules into validate_rig_instance() and validate_sdr_instance() and run them over resolved_rigs(), which already synthesises the flat layout as a single entry. Both layouts now go through the same code, and multi-rig messages name the rig they came from. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SyX26FCpMQxiBoC7r5K1A7 Signed-off-by: Stan Grams --- src/trx-config/src/server.rs | 615 ++++++++++++++++++++++------------- 1 file changed, 388 insertions(+), 227 deletions(-) diff --git a/src/trx-config/src/server.rs b/src/trx-config/src/server.rs index 3139874e..b5654570 100644 --- a/src/trx-config/src/server.rs +++ b/src/trx-config/src/server.rs @@ -471,238 +471,71 @@ impl Default for SdrChannelConfig { } impl ServerConfig { + /// Validate the whole configuration. + /// + /// Everything that belongs to a rig is checked through `resolved_rigs()`, + /// so the legacy flat `[rig]` / `[audio]` / … layout and `[[rigs]]` entries + /// are held to exactly the same rules. pub fn validate(&self) -> Result<(), String> { validate_log_level(self.general.log_level.as_deref())?; validate_coordinates(self.general.latitude, self.general.longitude)?; - if self.rig.initial_freq_hz == 0 { - return Err("[rig].initial_freq_hz must be > 0".to_string()); - } - - validate_access(&self.rig.access)?; - - if self.behavior.poll_interval_ms == 0 { - return Err("[behavior].poll_interval_ms must be > 0".to_string()); - } - if self.behavior.poll_interval_tx_ms == 0 { - return Err("[behavior].poll_interval_tx_ms must be > 0".to_string()); - } - if self.behavior.max_retries == 0 { - return Err("[behavior].max_retries must be > 0".to_string()); - } - if self.behavior.retry_base_delay_ms == 0 { - return Err("[behavior].retry_base_delay_ms must be > 0".to_string()); - } - validate_tokens("[listen.auth].tokens", &self.listen.auth.tokens)?; if self.listen.enabled && self.listen.port == 0 { return Err("[listen].port must be > 0 when listener is enabled".to_string()); } - if self.audio.enabled { - if self.audio.port == 0 { - return Err("[audio].port must be > 0 when audio is enabled".to_string()); - } - if !self.audio.rx_enabled && !self.audio.tx_enabled { - return Err( - "[audio] enabled but both rx_enabled and tx_enabled are false".to_string(), - ); - } - if self.audio.sample_rate < 8_000 || self.audio.sample_rate > 192_000 { - return Err("[audio].sample_rate must be in range 8000..=192000".to_string()); - } - if !(1..=2).contains(&self.audio.channels) { - return Err("[audio].channels must be 1 or 2".to_string()); - } - match self.audio.frame_duration_ms { - 3 | 5 | 10 | 20 | 40 | 60 => {} - _ => { - return Err( - "[audio].frame_duration_ms must be one of: 3, 5, 10, 20, 40, 60" - .to_string(), - ) - } - } - if self.audio.bitrate_bps == 0 { - return Err("[audio].bitrate_bps must be > 0".to_string()); - } - } + self.validate_rig_uniqueness()?; - if self.pskreporter.enabled { - if self.pskreporter.host.trim().is_empty() { - return Err("[pskreporter].host must not be empty".to_string()); - } - if self.pskreporter.port == 0 { - return Err("[pskreporter].port must be > 0".to_string()); - } - if self.pskreporter.receiver_locator.is_none() - && (self.general.latitude.is_none() || self.general.longitude.is_none()) - { - return Err( - "[pskreporter] enabled requires either [pskreporter].receiver_locator \ - or [general].latitude and [general].longitude" - .to_string(), - ); - } - } - - if self.aprsfi.enabled { - if self.aprsfi.host.trim().is_empty() { - return Err("[aprsfi].host must not be empty".to_string()); - } - if self.aprsfi.port == 0 { - return Err("[aprsfi].port must be > 0".to_string()); - } - } - - if let Some(max_gain) = self.sdr.gain.max_value { - if !max_gain.is_finite() { - return Err("[sdr.gain].max_value must be finite".to_string()); - } - if max_gain < 0.0 { - return Err("[sdr.gain].max_value must be >= 0".to_string()); - } - } - validate_sdr_squelch_config("[sdr.squelch]", &self.sdr.squelch)?; - validate_sdr_nb_config("[sdr.noise_blanker]", &self.sdr.noise_blanker)?; - - // Multi-rig uniqueness checks. - if !self.rigs.is_empty() { - let mut seen_ids: std::collections::HashSet = std::collections::HashSet::new(); - let mut seen_ports: std::collections::HashSet = std::collections::HashSet::new(); - let mut enabled_count = 0usize; - for rig in &self.rigs { - if !rig.enable { - continue; - } - enabled_count += 1; - // Check for explicit duplicate IDs (empty IDs are auto-generated later). - if !rig.id.trim().is_empty() && !seen_ids.insert(rig.id.clone()) { - return Err(format!("[[rigs]] duplicate rig id: \"{}\"", rig.id)); - } - if rig.audio.enabled && !seen_ports.insert(rig.audio.port) { - return Err(format!( - "[[rigs]] duplicate audio port {} (rig id: \"{}\")", - rig.audio.port, rig.id - )); - } - if let Some(max_gain) = rig.sdr.gain.max_value { - if !max_gain.is_finite() { - return Err(format!( - "[[rigs]] [sdr.gain].max_value must be finite (rig id: \"{}\")", - rig.id - )); - } - if max_gain < 0.0 { - return Err(format!( - "[[rigs]] [sdr.gain].max_value must be >= 0 (rig id: \"{}\")", - rig.id - )); - } - } - validate_sdr_squelch_config( - &format!("[[rigs]] [sdr.squelch] (rig id: \"{}\")", rig.id), - &rig.sdr.squelch, - )?; - validate_sdr_nb_config( - &format!("[[rigs]] [sdr.noise_blanker] (rig id: \"{}\")", rig.id), - &rig.sdr.noise_blanker, - )?; - } - if enabled_count == 0 { - return Err( - "[[rigs]] has no enabled entries; set at least one [[rigs]].enable = true" - .to_string(), - ); - } - } - - if self.decode_logs.enabled { - if self.decode_logs.dir.trim().is_empty() { - return Err("[decode_logs].dir must not be empty when enabled".to_string()); - } - if self.decode_logs.aprs_file.trim().is_empty() - || self.decode_logs.cw_file.trim().is_empty() - || self.decode_logs.ft8_file.trim().is_empty() - || self.decode_logs.wspr_file.trim().is_empty() - { - return Err("[decode_logs] file names must not be empty when enabled".to_string()); - } + let multi = !self.rigs.is_empty(); + for rig in &self.resolved_rigs() { + validate_rig_instance(&rig_prefix(multi, rig), rig, &self.general)?; } Ok(()) } - /// Validate SDR-specific config rules (see SDR.md §11). - /// Returns a Vec of error strings; empty means valid. - pub fn validate_sdr(&self) -> Vec { - let mut errors = Vec::new(); - - // Only validate if access type is "sdr" - let is_sdr = self.rig.access.access_type.as_deref() == Some("sdr"); - if !is_sdr { - return errors; + /// Check that enabled `[[rigs]]` entries do not collide with one another. + fn validate_rig_uniqueness(&self) -> Result<(), String> { + if self.rigs.is_empty() { + return Ok(()); } - // args must be non-empty - if self - .rig - .access - .args - .as_deref() - .map(str::is_empty) - .unwrap_or(true) - { - errors.push("[rig.access] args must be non-empty for type = \"sdr\"".into()); - } + let mut seen_ids: std::collections::HashSet<&str> = std::collections::HashSet::new(); + let mut seen_ports: std::collections::HashSet = std::collections::HashSet::new(); + let mut enabled_count = 0usize; - // sample_rate must be non-zero - if self.sdr.sample_rate == 0 { - errors.push("[sdr] sample_rate must be > 0".into()); - } - - // Every channel's IF must fit within the captured bandwidth - let half_rate = self.sdr.sample_rate as i64 / 2; - for ch in &self.sdr.channels { - let channel_if = self.sdr.center_offset_hz + ch.offset_hz; - if channel_if.abs() >= half_rate { - errors.push(format!( - "[sdr.channels] id=\"{}\" IF frequency {} Hz exceeds Nyquist limit ±{} Hz", - ch.id, channel_if, half_rate + for rig in self.rigs.iter().filter(|r| r.enable) { + enabled_count += 1; + // Empty ids are auto-generated later, so only explicit ones collide. + if !rig.id.trim().is_empty() && !seen_ids.insert(rig.id.as_str()) { + return Err(format!("[[rigs]] duplicate rig id: \"{}\"", rig.id)); + } + if rig.audio.enabled && !seen_ports.insert(rig.audio.port) { + return Err(format!( + "[[rigs]] duplicate audio port {} (rig id: \"{}\")", + rig.audio.port, rig.id )); } } - // At most one channel may have stream_opus = true - let opus_count = self.sdr.channels.iter().filter(|c| c.stream_opus).count(); - if opus_count > 1 { - errors.push(format!( - "[sdr.channels] at most one channel may have stream_opus = true (found {})", - opus_count - )); + if enabled_count == 0 { + return Err( + "[[rigs]] has no enabled entries; set at least one [[rigs]].enable = true" + .to_string(), + ); } + Ok(()) + } - // tx_enabled must be false with SDR backend - if self.audio.tx_enabled { - errors.push("[audio] tx_enabled must be false when using the soapysdr backend".into()); - } - - // Decoder names must not appear in more than one channel - let mut seen: std::collections::HashMap = std::collections::HashMap::new(); - for ch in &self.sdr.channels { - for dec in &ch.decoders { - if let Some(prev_id) = seen.get(dec) { - errors.push(format!( - "[sdr.channels] decoder \"{}\" appears in both \"{}\" and \"{}\"", - dec, prev_id, ch.id - )); - } else { - seen.insert(dec.clone(), ch.id.clone()); - } - } - } - - errors + /// Validate SDR-specific config rules for every rig (see SDR.md §11). + /// Returns a Vec of error strings; empty means valid. + pub fn validate_sdr(&self) -> Vec { + let multi = !self.rigs.is_empty(); + self.resolved_rigs() + .iter() + .flat_map(|rig| validate_sdr_instance(&rig_prefix(multi, rig), rig)) + .collect() } /// Load configuration from a specific file path. @@ -817,7 +650,205 @@ fn validate_coordinates(latitude: Option, longitude: Option) -> Result } } -fn validate_access(access: &AccessConfig) -> Result<(), String> { +/// Message prefix naming the rig a problem belongs to. +/// +/// Empty for the legacy flat layout, so its messages read exactly as before. +fn rig_prefix(multi: bool, rig: &RigInstanceConfig) -> String { + if multi { + format!("[[rigs]] \"{}\": ", rig.id) + } else { + String::new() + } +} + +/// Validate everything that belongs to a single rig. +/// +/// Called once per entry of `resolved_rigs()`, which is what makes the flat +/// layout and `[[rigs]]` entries obey the same rules. +fn validate_rig_instance( + prefix: &str, + rig: &RigInstanceConfig, + general: &GeneralConfig, +) -> Result<(), String> { + if rig.rig.initial_freq_hz == 0 { + return Err(format!("{prefix}[rig].initial_freq_hz must be > 0")); + } + + validate_access(prefix, &rig.rig.access)?; + + if rig.behavior.poll_interval_ms == 0 { + return Err(format!("{prefix}[behavior].poll_interval_ms must be > 0")); + } + if rig.behavior.poll_interval_tx_ms == 0 { + return Err(format!("{prefix}[behavior].poll_interval_tx_ms must be > 0")); + } + if rig.behavior.max_retries == 0 { + return Err(format!("{prefix}[behavior].max_retries must be > 0")); + } + if rig.behavior.retry_base_delay_ms == 0 { + return Err(format!("{prefix}[behavior].retry_base_delay_ms must be > 0")); + } + + if rig.audio.enabled { + if rig.audio.port == 0 { + return Err(format!("{prefix}[audio].port must be > 0 when audio is enabled")); + } + if !rig.audio.rx_enabled && !rig.audio.tx_enabled { + return Err(format!( + "{prefix}[audio] enabled but both rx_enabled and tx_enabled are false" + )); + } + if rig.audio.sample_rate < 8_000 || rig.audio.sample_rate > 192_000 { + return Err(format!( + "{prefix}[audio].sample_rate must be in range 8000..=192000" + )); + } + if !(1..=2).contains(&rig.audio.channels) { + return Err(format!("{prefix}[audio].channels must be 1 or 2")); + } + match rig.audio.frame_duration_ms { + 3 | 5 | 10 | 20 | 40 | 60 => {} + _ => { + return Err(format!( + "{prefix}[audio].frame_duration_ms must be one of: 3, 5, 10, 20, 40, 60" + )) + } + } + if rig.audio.bitrate_bps == 0 { + return Err(format!("{prefix}[audio].bitrate_bps must be > 0")); + } + } + + if rig.pskreporter.enabled { + if rig.pskreporter.host.trim().is_empty() { + return Err(format!("{prefix}[pskreporter].host must not be empty")); + } + if rig.pskreporter.port == 0 { + return Err(format!("{prefix}[pskreporter].port must be > 0")); + } + if rig.pskreporter.receiver_locator.is_none() + && (general.latitude.is_none() || general.longitude.is_none()) + { + return Err(format!( + "{prefix}[pskreporter] enabled requires either [pskreporter].receiver_locator \ + or [general].latitude and [general].longitude" + )); + } + } + + if rig.aprsfi.enabled { + if rig.aprsfi.host.trim().is_empty() { + return Err(format!("{prefix}[aprsfi].host must not be empty")); + } + if rig.aprsfi.port == 0 { + return Err(format!("{prefix}[aprsfi].port must be > 0")); + } + } + + if let Some(max_gain) = rig.sdr.gain.max_value { + if !max_gain.is_finite() { + return Err(format!("{prefix}[sdr.gain].max_value must be finite")); + } + if max_gain < 0.0 { + return Err(format!("{prefix}[sdr.gain].max_value must be >= 0")); + } + } + validate_sdr_squelch_config(&format!("{prefix}[sdr.squelch]"), &rig.sdr.squelch)?; + validate_sdr_nb_config(&format!("{prefix}[sdr.noise_blanker]"), &rig.sdr.noise_blanker)?; + + if rig.decode_logs.enabled { + if rig.decode_logs.dir.trim().is_empty() { + return Err(format!( + "{prefix}[decode_logs].dir must not be empty when enabled" + )); + } + if rig.decode_logs.aprs_file.trim().is_empty() + || rig.decode_logs.cw_file.trim().is_empty() + || rig.decode_logs.ft8_file.trim().is_empty() + || rig.decode_logs.wspr_file.trim().is_empty() + { + return Err(format!( + "{prefix}[decode_logs] file names must not be empty when enabled" + )); + } + } + + Ok(()) +} + +/// Validate the SDR pipeline of a single rig (see SDR.md §11). +/// +/// Returns every problem found rather than stopping at the first, so a +/// misconfigured channel list can be fixed in one pass. +fn validate_sdr_instance(prefix: &str, rig: &RigInstanceConfig) -> Vec { + let mut errors = Vec::new(); + + if rig.rig.access.access_type.as_deref() != Some("sdr") { + return errors; + } + + if rig + .rig + .access + .args + .as_deref() + .map(str::is_empty) + .unwrap_or(true) + { + errors.push(format!( + "{prefix}[rig.access] args must be non-empty for type = \"sdr\"" + )); + } + + if rig.sdr.sample_rate == 0 { + errors.push(format!("{prefix}[sdr] sample_rate must be > 0")); + } + + // Every channel's IF must fit within the captured bandwidth. + let half_rate = rig.sdr.sample_rate as i64 / 2; + for ch in &rig.sdr.channels { + let channel_if = rig.sdr.center_offset_hz + ch.offset_hz; + if channel_if.abs() >= half_rate { + errors.push(format!( + "{prefix}[sdr.channels] id=\"{}\" IF frequency {} Hz exceeds Nyquist limit ±{} Hz", + ch.id, channel_if, half_rate + )); + } + } + + let opus_count = rig.sdr.channels.iter().filter(|c| c.stream_opus).count(); + if opus_count > 1 { + errors.push(format!( + "{prefix}[sdr.channels] at most one channel may have stream_opus = true (found {})", + opus_count + )); + } + + if rig.audio.tx_enabled { + errors.push(format!( + "{prefix}[audio] tx_enabled must be false when using the soapysdr backend" + )); + } + + // A decoder may only be fed by one channel. + let mut seen: std::collections::HashMap<&str, &str> = std::collections::HashMap::new(); + for ch in &rig.sdr.channels { + for dec in &ch.decoders { + if let Some(prev_id) = seen.get(dec.as_str()) { + errors.push(format!( + "{prefix}[sdr.channels] decoder \"{}\" appears in both \"{}\" and \"{}\"", + dec, prev_id, ch.id + )); + } else { + seen.insert(dec.as_str(), ch.id.as_str()); + } + } + } + + errors +} + +fn validate_access(prefix: &str, access: &AccessConfig) -> Result<(), String> { let serial_fields_set = access.port.is_some() || access.baud.is_some(); let tcp_fields_set = access.host.is_some() || access.tcp_port.is_some(); @@ -828,38 +859,34 @@ fn validate_access(access: &AccessConfig) -> Result<(), String> { match access.access_type.as_deref().unwrap_or("serial") { "serial" => { if access.port.as_deref().unwrap_or("").trim().is_empty() { - return Err( - "[rig.access].port must be set for serial access ([rig.access].type='serial')" - .to_string(), - ); + return Err(format!( + "{prefix}[rig.access].port must be set for serial access ([rig.access].type='serial')" + )); } if access.baud.unwrap_or(0) == 0 { - return Err( - "[rig.access].baud must be > 0 for serial access ([rig.access].type='serial')" - .to_string(), - ); + return Err(format!( + "{prefix}[rig.access].baud must be > 0 for serial access ([rig.access].type='serial')" + )); } } "tcp" => { if access.host.as_deref().unwrap_or("").trim().is_empty() { - return Err( - "[rig.access].host must be set for tcp access ([rig.access].type='tcp')" - .to_string(), - ); + return Err(format!( + "{prefix}[rig.access].host must be set for tcp access ([rig.access].type='tcp')" + )); } if access.tcp_port.unwrap_or(0) == 0 { - return Err( - "[rig.access].tcp_port must be > 0 for tcp access ([rig.access].type='tcp')" - .to_string(), - ); + return Err(format!( + "{prefix}[rig.access].tcp_port must be > 0 for tcp access ([rig.access].type='tcp')" + )); } } "sdr" => { - // SDR-specific validation is handled by validate_sdr() + // SDR-specific validation is handled by validate_sdr_instance(). } other => { return Err(format!( - "[rig.access].type '{}' is invalid (expected 'serial', 'tcp', or 'sdr')", + "{prefix}[rig.access].type '{}' is invalid (expected 'serial', 'tcp', or 'sdr')", other )) } @@ -1499,6 +1526,140 @@ port = 4531 ); } + // --- Per-rig validation: [[rigs]] entries obey the same rules as the + // legacy flat layout, which they previously escaped entirely. --- + + #[test] + fn test_validate_rejects_bad_audio_in_rig_entry() { + let toml_str = r#" +[[rigs]] +id = "hf" +[rigs.rig] +model = "ft817" +[rigs.rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 +[rigs.audio] +port = 4531 +frame_duration_ms = 7 +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + let err = cfg + .validate() + .expect_err("expected the rig entry's audio config to be validated"); + assert!( + err.contains("frame_duration_ms") && err.contains("hf"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_validate_rejects_incomplete_access_in_rig_entry() { + let toml_str = r#" +[[rigs]] +id = "hf" +[rigs.rig] +model = "ft817" +[rigs.rig.access] +type = "serial" +port = "/dev/ttyUSB0" +[rigs.audio] +port = 4531 +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + let err = cfg + .validate() + .expect_err("expected the rig entry's access config to be validated"); + assert!(err.contains("baud"), "unexpected error: {err}"); + } + + #[test] + fn test_validate_sdr_checks_rig_entries() { + // sample_rate 1_000_000 => Nyquist 500_000; the channel sits beyond it, + // two channels claim the Opus stream, and TX is on with an SDR backend. + let toml_str = r#" +[[rigs]] +id = "sdr" +[rigs.rig] +model = "soapysdr" +[rigs.rig.access] +type = "sdr" +args = "driver=rtlsdr" +[rigs.audio] +port = 4532 +tx_enabled = true +[rigs.sdr] +sample_rate = 1000000 +center_offset_hz = 0 +[[rigs.sdr.channels]] +id = "ch_high" +offset_hz = 600000 +stream_opus = true +[[rigs.sdr.channels]] +id = "ch_two" +offset_hz = 10000 +stream_opus = true +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + let errors = cfg.validate_sdr(); + assert!( + errors.iter().any(|e| e.contains("Nyquist")), + "expected a Nyquist error, got: {errors:?}" + ); + assert!( + errors.iter().any(|e| e.contains("stream_opus")), + "expected a stream_opus error, got: {errors:?}" + ); + assert!( + errors.iter().any(|e| e.contains("tx_enabled")), + "expected a tx_enabled error, got: {errors:?}" + ); + assert!( + errors.iter().all(|e| e.contains("sdr")), + "every error should name the rig it belongs to: {errors:?}" + ); + } + + #[test] + fn test_validate_sdr_ignores_non_sdr_rig_entries() { + let toml_str = r#" +[[rigs]] +id = "hf" +[rigs.rig] +model = "ft817" +[rigs.rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 +[rigs.audio] +port = 4531 +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + assert!(cfg.validate_sdr().is_empty()); + } + + #[test] + fn test_validate_rejects_bad_squelch_in_rig_entry() { + let toml_str = r#" +[[rigs]] +id = "sdr" +[rigs.rig] +model = "soapysdr" +[rigs.rig.access] +type = "sdr" +args = "driver=rtlsdr" +[rigs.audio] +port = 4532 +tx_enabled = false +[rigs.sdr.squelch] +threshold_db = 10.0 +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + let err = cfg.validate().expect_err("expected a squelch error"); + assert!(err.contains("threshold_db"), "unexpected error: {err}"); + } + #[test] fn test_validate_accepts_multi_rig_unique_ids_and_ports() { let toml_str = r#"