From 2a49d1d1a454e657b7468304c311dba30dd784c6 Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sun, 11 Oct 2026 13:05:46 +0200 Subject: [PATCH] docs: thin the commentary back to what the code does not say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments had drifted from explaining the code to arguing with the alternatives: a quarter of every file, most of it a paragraph on why each private constant is not some other number. That is a tax on reading, and the second paragraph of such a block is almost never the one somebody needed. 448 comment lines go. The rule was: a private item keeps its summary line and loses the essay; a test keeps its name, which is a sentence already, and loses the doc that restated it. Nothing public lost a line — `deny(missing_docs)` is the floor — and nothing citing a Blue Book section, a field convention or a recorded trap was touched. Three blocks were put back condensed after the sweep, because they carried facts and not opinions: the accepted source URL forms, which io::ErrorKinds are transient on which source, and what 1115 and 1279 actually measure. 536 tests, fmt and clippy clean. --- crates/xtce-gs-cli/src/main.rs | 6 -- crates/xtce-gs-cli/src/run.rs | 65 +------------ crates/xtce-gs-engine/src/decode.rs | 23 ----- crates/xtce-gs-engine/src/error.rs | 7 -- crates/xtce-gs-engine/src/lib.rs | 34 ++----- crates/xtce-gs-engine/src/sctime.rs | 1 - crates/xtce-gs-engine/src/session.rs | 93 +------------------ crates/xtce-gs-gui/src/lib.rs | 12 --- crates/xtce-gs-gui/src/panels/plots.rs | 26 ------ crates/xtce-gs-gui/src/panels/status.rs | 10 -- crates/xtce-gs-gui/src/panels/table.rs | 10 -- crates/xtce-gs-gui/src/panels/tree.rs | 6 -- crates/xtce-gs-gui/tests/against_a_session.rs | 6 -- crates/xtce-gs-link/src/derand.rs | 15 --- crates/xtce-gs-link/src/frame.rs | 17 ---- crates/xtce-gs-link/src/pipeline.rs | 19 ---- crates/xtce-gs-link/src/rs.rs | 69 -------------- crates/xtce-gs-link/src/source.rs | 36 +------ crates/xtce-gs-link/src/sync.rs | 43 --------- 19 files changed, 17 insertions(+), 481 deletions(-) diff --git a/crates/xtce-gs-cli/src/main.rs b/crates/xtce-gs-cli/src/main.rs index 275d6f8..a0165c7 100644 --- a/crates/xtce-gs-cli/src/main.rs +++ b/crates/xtce-gs-cli/src/main.rs @@ -65,12 +65,6 @@ fn main() -> ExitCode { } /// Prints a failure and everything under it, once each. -/// -/// Every variant of every error enum in this workspace is `#[error("{0}")]` over a `#[from]` -/// field, so the outermost message and its source's message are usually the same string. A -/// chain printed without this prints it three times — `CliError`, `EngineError`, then -/// `LinkError` — and an operator reading three identical lines learns to read none of them. -/// Only a cause that *says something new* is printed. fn report(error: &dyn std::error::Error) { let mut previous = error.to_string(); eprintln!("error: {previous}"); diff --git a/crates/xtce-gs-cli/src/run.rs b/crates/xtce-gs-cli/src/run.rs index 993d430..92150aa 100644 --- a/crates/xtce-gs-cli/src/run.rs +++ b/crates/xtce-gs-cli/src/run.rs @@ -55,16 +55,8 @@ pub const PROGRESS_PACKETS: u64 = 1000; /// Lengths worth trying on a stream nobody has documented. /// -/// 1115 is RS(255, 223) interleaved 5 deep, which is most of the missions in reach; 1279 is -/// *that same link* measured marker to marker with the parity and the ASM counted, which is -/// what an operator is usually told when they are told a number at all; 2048 is a common -/// uncoded frame; 892 is RS(255, 223) interleaved 4 deep. -/// -/// They are not all the same quantity, and that is the point of printing two columns per -/// row: 1279 matching as a *stride* and 1115 matching as a *frame* are the same link, and an -/// operator handed 1279 as a `--frame-length` gets a station that locks once and then finds -/// nothing. [`advise`] is what turns a measured stride back into flags, and it is the line to -/// read rather than the table. +/// 1115 is RS(255,223) interleaved 5 deep; 1279 is that same link measured marker to marker, +/// with the parity and the ASM counted, which is the number an operator is usually quoted. const FRAME_LENGTH_CANDIDATES: [u64; 4] = [1115, 1279, 2048, 892]; /// Symbols in a Reed-Solomon codeword: CCSDS 131.0-B-5 section 4. @@ -197,12 +189,6 @@ fn status_line(line: &mut String, snapshot: &StatsSnapshot, framed: bool) { } /// Prints the log lines `cursor` has not seen to stderr and moves it past them. -/// -/// A cursor and never [`EventLog::clear`]: the log belongs to the session and the interface -/// may be reading it too. [`EventLog::push`] bumps a collapsed line's count on every repeat, -/// and the cursor is counted in pushes, so a line that keeps happening reprints with a rising -/// count — which is what an operator watching a failing link wants, and what a printer that -/// remembered the message instead of the position would suppress. fn print_events(log: &Mutex, cursor: &mut EventCursor, verbose: bool) { for line in unprinted_events(log, cursor, verbose) { eprintln!("{line}"); @@ -210,30 +196,9 @@ fn print_events(log: &Mutex, cursor: &mut EventCursor, verbose: bool) } /// How many pushes into an [`EventLog`] a printer has already considered. -/// -/// A position and deliberately **not** a timestamp. [`Utc::now`] is `SystemTime::now`, whose -/// granularity here is about a microsecond, so two events raised back to back usually carry -/// the *same* stamp: a loop building 100 000 pairs of `Event::warning` and comparing their -/// `time` found 93 258 of them tied. A cursor that skipped `time <= last` therefore dropped -/// the second of such a pair, permanently, because the cursor only moves forward — and the -/// line that costs an operator most is the last one, since [`headless`] prints once more -/// after [`Session::shutdown`] and that is the line saying why the session stopped. type EventCursor = u64; /// The lines `cursor` has not seen yet, oldest first, moving it past them. -/// -/// # Why a push count is an exact cursor -/// -/// [`EventLog::total`] counts every push, repeats included, and [`EventLog::iter`] yields the -/// distinct entries still held with how many pushes each stands for. So the entries account -/// for the last `sum(count)` pushes, and the entry ending at push `n` can be named by `n` -/// alone. That name survives eviction — dropping the oldest entry lowers `sum(count)` by -/// exactly what it stood for, leaving every surviving `n` where it was — and only the *last* -/// entry's count can grow, which is what makes a collapsed line reprint when, and only when, -/// its count has risen. -/// -/// The cursor moves past every entry it considers, printed or not, so an [`Severity::Info`] -/// line held back without `--verbose` cannot make the warning behind it reprint. fn unprinted_events(log: &Mutex, cursor: &mut EventCursor, verbose: bool) -> Vec { let mut lines: Vec = Vec::new(); let guard = log.lock().unwrap_or_else(PoisonError::into_inner); @@ -273,14 +238,6 @@ fn unprinted_events(log: &Mutex, cursor: &mut EventCursor, verbose: bo /// default disposition kills the process outright, and a process killed by a signal runs no /// destructor: up to [`xtce_gs_engine::record::RECORD_BUFFER_BYTES`] of recording never /// reaches the file and [`xtce_gs_engine::record::Recorder`]'s `Drop` safety net never runs. -/// -/// Measured with `xtce-gs run … --headless --record`, killed two seconds in: with ctrl-c -/// alone handled, `kill -TERM` ended the process with status 143, the log stopped at -/// `reading file://…` with no final counter line, and the recording ended at the last full -/// buffer; with this, `kill -TERM` and `kill -INT` both exit 0, print the final line, and -/// leave a recording exactly as long as the `bytes=` the station reports. -/// -/// Windows has no SIGTERM and `ctrl_c` there already covers what the console sends. async fn stop_requested() -> std::io::Result<()> { #[cfg(unix)] { @@ -536,13 +493,6 @@ async fn export_loop( } /// Whether a failure is the reader at the other end of the output going away. -/// -/// `xtce-gs export … | head -1` is an ordinary way to look at an export — this module's -/// header names redirection as what the subcommand is for — and Rust masks SIGPIPE, so a -/// write to a pipe nobody is reading comes back as `EPIPE` instead of ending the process. -/// Treating that as a failure gives exit 1 and `error: Broken pipe (os error 32)`, which -/// aborts any `set -e` script that samples the header of a CSV. Nothing is lost that the -/// operator asked for: the reader stopped listening first. fn is_broken_pipe(error: &EngineError) -> bool { matches!(error, EngineError::Io(io) if io.kind() == std::io::ErrorKind::BrokenPipe) } @@ -907,11 +857,6 @@ fn report_frame_trial( /// whole number of 255-symbol codewords when the link is coded. So the block is the stride /// less the four marker octets, and a block that divides by 255 is Reed-Solomon with the /// quotient as the interleaving depth. -/// -/// The parity cannot be recovered from the stride — E=8 and E=16 produce the same block -/// length and differ only in how much of it is the frame — so both are offered. Guessing one -/// would be guessing the frame length, and a frame length that is wrong by 80 octets per -/// codeword is a synchroniser that never finds a second marker. fn advise(stride: u64) -> Vec { let Some(block) = stride .checked_sub(CCSDS_ASM.len() as u64) @@ -956,12 +901,6 @@ mod tests { use super::*; /// A definition with one container and one 32-bit parameter. - /// - /// Written from the test rather than read out of the sibling `xtce-rs` checkout the way - /// the engine's and the link crate's fixtures are: a test that skips depending on what - /// else the developer happens to have cloned is a test that stops being run, and nothing - /// below needs a mission — what is under test is where the export loop stops and what it - /// reports, not what the values mean. const MINIMAL_XTCE: &str = r#" diff --git a/crates/xtce-gs-engine/src/decode.rs b/crates/xtce-gs-engine/src/decode.rs index 5d54a48..fd64238 100644 --- a/crates/xtce-gs-engine/src/decode.rs +++ b/crates/xtce-gs-engine/src/decode.rs @@ -154,11 +154,6 @@ pub struct SequenceTracker { last: Vec>, /// Per APID: how many gaps have been reported quietly since the last full line, and how /// many packets have arrived contiguously since the last gap. - /// - /// A dropout is one line and that is right. A link losing one packet in three is a - /// thousand lines a second, each naming a different sequence count — so `EventLog`'s - /// collapsing cannot fold them, and a bounded log fills with them while the one line - /// that says something else is evicted. See [`SequenceTracker::observe`]. burst: Vec, gaps: u64, missing: u64, @@ -374,12 +369,6 @@ pub struct PacketDecoder { stats: Arc, events: Vec, /// Containers already warned about undescribed trailing bits, indexed by container. - /// - /// A definition that does not describe a whole packet does not describe *any* packet of - /// that container, so the warning is one fact about the definition and not one fact per - /// packet. Without this, a 7 200-packet replay puts 7 200 identical lines on a log that - /// holds 2 048 — the event log collapses repeats, but only while nothing else is - /// interleaved, and on a live station something always is. warned: Vec, } @@ -726,24 +715,12 @@ mod tests { /// Gaps and missing packets, derived from *which packets were removed*. /// - /// The point is that this shares no arithmetic with [`SequenceTracker::observe`]: no - /// modulus, no subtraction of one count from another, no reading of a sequence count at - /// all. It counts holes in a list. An earlier version of this helper retyped `observe`'s - /// own `(current + 16_384 - previous - 1) % 16_384`, which could only ever prove that the - /// same expression gives the same answer twice. - /// /// `apids[i]` is the APID of position `i` in the *complete* stream and `removed` lists /// the positions taken out of it. Each APID is counted on its own, because CCSDS /// 133.0-B-2 §4.1.3.4 gives each APID its own count. Within one APID's own sub-stream a /// maximal run of consecutive removed positions is one gap that says the run's length is /// missing — except a run touching the start or the end of that sub-stream, which has no /// surviving packet on one side and so nothing that could notice it. - /// - /// Exact only while a per-APID run is shorter than a whole counter's worth: at 16 383 the - /// tracker reports 16 382 and at 16 384 the repeated count is reported as nothing at all, - /// where this would still report the run. Both call sites remove one to three in a row, - /// which is what a link loses; a fixture that removed thousands would need the wrap rules - /// here, and would be back to retyping `observe`. fn expected_from_removals(apids: &[u16], removed: &[usize]) -> (u64, u64) { let mut kept_by_apid: BTreeMap> = BTreeMap::new(); for (index, apid) in apids.iter().enumerate() { diff --git a/crates/xtce-gs-engine/src/error.rs b/crates/xtce-gs-engine/src/error.rs index 5b2aff9..01b1fa1 100644 --- a/crates/xtce-gs-engine/src/error.rs +++ b/crates/xtce-gs-engine/src/error.rs @@ -31,13 +31,6 @@ pub enum EngineError { Io(#[from] std::io::Error), /// The configuration cannot be carried out. - /// - /// A history depth of zero, a limits file whose JSON does not parse, a spacecraft time - /// parameter no definition declares. `serde_json::Error` is deliberately *not* a variant - /// of its own: the contract fixes this enum at five, and a parse failure is something the - /// operator wrote, which is what this variant means. The message carries the file, the - /// line and the column, because "invalid configuration" sends someone reading JSON by - /// eye. #[error("{0}")] Config(String), } diff --git a/crates/xtce-gs-engine/src/lib.rs b/crates/xtce-gs-engine/src/lib.rs index 2731416..d3be93a 100644 --- a/crates/xtce-gs-engine/src/lib.rs +++ b/crates/xtce-gs-engine/src/lib.rs @@ -25,36 +25,21 @@ //! # } //! ``` //! -//! # The decision the rest follows from -//! -//! `Decoder::decode` returns values that borrow the packet buffer *and* the definition. -//! Nothing with that type can sit in a history or cross a channel, so [`decode::project`] is -//! where the borrow ends and [`xtce_gs_core::Value`] is what it ends into. After that point +//! `Decoder::decode` returns values that borrow the packet buffer *and* the definition, and +//! nothing with that type can sit in a history or cross a channel. [`decode::project`] is +//! where the borrow ends and [`xtce_gs_core::Value`] is what it ends into; past that point //! nothing in this workspace names a lifetime. //! -//! # What this crate refuses -//! -//! * **It does not stop on a bad packet.** A packet the definition does not describe is a -//! counter and a line in the log. A station that returned an error for one would stop on -//! the first, and every real downlink has them. -//! * **It does not guess a time.** A configured spacecraft clock that cannot be resolved -//! fails [`Session::start`]; a packet that does not carry it falls back to ground receipt -//! and says which it used through [`xtce_gs_core::Batch::spacecraft`]. -//! * **It does not archive.** [`record::Recorder`] appends bytes and nothing else — no index, -//! no per-packet wrapper — so that a recording replays through the same source that made -//! it. A parameter archive is a different program. -//! * **It does not draw.** Nothing here depends on egui, and the store is left in a state the -//! interface can read at any instant rather than one it has to be told about. +//! A packet the definition does not describe is a counter and a log line, not an error — +//! every real downlink has them. A recording is bytes and nothing else, so that it replays +//! through the same source that wrote it. #![deny(missing_docs)] #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic)] #![warn(clippy::pedantic)] #![allow(clippy::module_name_repetitions)] -// Spacecraft time is fixed-point arithmetic over fields of known width — a two-octet day -// count, a three-octet binary fraction — and a plot axis is `f64` where a timestamp is `i64`. -// Every `as` in this crate narrows or widens one of those with the width check that makes it -// exact alongside; flagging them one at a time would mean an `#[allow]` on most of -// `sctime.rs`. +// Spacecraft time is fixed-point arithmetic over fields of known width, and every `as` here +// carries the width check that makes it exact. #![allow( clippy::cast_possible_truncation, clippy::cast_possible_wrap, @@ -62,8 +47,7 @@ clippy::cast_sign_loss, clippy::cast_lossless )] -// Tests are allowed to assert loudly; the no-panic rule is about library code reached by a -// live downlink, not about test setup. +// The no-panic rule is about code a live downlink reaches, not about test setup. #![cfg_attr( test, allow( diff --git a/crates/xtce-gs-engine/src/sctime.rs b/crates/xtce-gs-engine/src/sctime.rs index 498f5cb..834ea10 100644 --- a/crates/xtce-gs-engine/src/sctime.rs +++ b/crates/xtce-gs-engine/src/sctime.rs @@ -338,7 +338,6 @@ mod tests { use super::*; - /// A CUC that is all zeroes is the epoch itself, whatever the widths. #[test] fn a_cuc_of_zero_is_the_ccsds_epoch_and_that_epoch_is_1958() { let time = cuc_from_bytes(&[0, 0, 0, 0, 0, 0], 4, 2, Utc::CCSDS_EPOCH).unwrap(); diff --git a/crates/xtce-gs-engine/src/session.rs b/crates/xtce-gs-engine/src/session.rs index 19c9be1..7b39b78 100644 --- a/crates/xtce-gs-engine/src/session.rs +++ b/crates/xtce-gs-engine/src/session.rs @@ -82,29 +82,10 @@ pub const PACKET_QUEUE_DEPTH: usize = 1024; /// Consecutive failed reads tolerated before the source is called gone. /// -/// A read failure is not always the end, and the failures this budget is for are the ones the -/// sources this station actually opens can produce: -/// -/// * `Interrupted` on a file replay. `std::io::Read::read` is documented to surface `EINTR` -/// rather than retry it, and [`xtce_gs_link::source::FileReplay`] reads a `tokio::fs::File` -/// through exactly that — so a signal landing in the blocking read is a failure whose only -/// correct answer is another read, and a process taking signals can take several. -/// * `ConnectionReset` on `tcp://`. A peer that vanishes resets rather than closing, and the -/// reset is reported on the next read; a station that ended the pass on the first one would -/// end it over a feeder that was restarted. -/// * `TimedOut` on `tcp-listen://`, which `source::per_connection` deliberately excludes from -/// the disconnects it swallows, so it arrives here instead. -/// -/// **Not UDP.** [`xtce_gs_link::source::UdpSource::bind`] binds and never calls `connect` and -/// never sends, so there is no last send for an ICMP port-unreachable to be reported against -/// and the `ConnectionReset`-on-a-connected-socket case does not arise on this path. -/// -/// `WouldBlock` is in [`is_transient`]'s set too, as belt-and-braces: tokio's readiness loop -/// consumes it before a read ever returns, so it is not what this budget is sized for. -/// -/// A reset socket answers every read the same way and answers it immediately, so the retry is -/// a spin unless it is bounded: a failure that repeats this many times with no successful read -/// in between is a source that is not coming back, and the session stops. +/// The transient ones are `Interrupted` (a signal landing in a file replay's blocking read), +/// `ConnectionReset` (a `tcp://` peer that vanished rather than closed) and `TimedOut` on +/// `tcp-listen://`. A reset socket answers every read the same way and answers it at once, so +/// without a bound the retry is a spin. const READ_ERROR_LIMIT: u32 = 16; /// What the engine calls when a batch has been ingested. @@ -537,17 +518,6 @@ impl Dispatch { } /// Appends to the ring, dropping its oldest *packet* when it is full. The live policy. - /// - /// A [`Acquired::Restart`] in the ring is never what gets dropped. It is one byte of - /// bookkeeping and it is the whole reason the restart travels in the queue rather than - /// beside it: losing it leaves `PacketDecoder` and `SequenceTracker` holding state from - /// before a stream boundary, and the sequence counts across that boundary then read as - /// thousands of packets missing that were never sent. Dropping the oldest packet costs - /// one packet; dropping the marker costs the rest of the pass. - /// - /// The only source that is both lossy and able to restart is `tcp-listen://`, so this - /// runs when a feeder reconnects while the decoder is behind — which is exactly when the - /// ring is full. fn push(&mut self, item: Acquired) { if self.backlog.len() >= self.capacity { let oldest_packet = self @@ -569,17 +539,6 @@ impl Dispatch { } /// Empties the ring into the channel, waiting for room, at the end of a stream. - /// - /// Called once, when the source has ended: the last few hundred packets of a pass are in - /// the ring precisely when the pass was busy at the end, and they are exactly the ones an - /// operator is about to look at. - /// - /// Two of the four sources reach it, and only one of those has anything in the ring. - /// `Ok(0)` comes from `tcp://` when the peer closes and from `file://` at the end of the - /// replay; `udp://` documents that it never returns `Ok(0)` — a bound socket has no end — - /// and `tcp-listen://` re-accepts instead of ending, so neither arrives here at all. Of - /// the two that do, `file://` is the source that is not lossy, so its ring is empty by - /// construction and this is a no-op. `tcp://` is the one case with packets to save. async fn drain(&mut self, stop: &mut watch::Receiver) -> Result<(), Gone> { while let Some(item) = self.backlog.pop_front() { self.wait_and_send(item, stop).await?; @@ -608,17 +567,6 @@ struct Acquisition { } /// Whether a read failure is worth another read. -/// -/// Everything not listed is fatal, including [`LinkError::Config`] — a source that refuses its -/// own configuration will refuse it again — and [`LinkError::Ended`], which is not a failure at -/// all but a read issued after the stream finished. -/// -/// The `Ended` arm cannot fire from the only caller, and it is the caller that makes it so: -/// `Ended` is produced only by a read *after* one that returned `Ok(0)`, and [`acquire`] -/// breaks out of its loop on the `Ok(0)` and never issues that second read. `LinkError`'s own -/// doc says the same from the other side — "a caller that stops on `Ok(0)` never sees -/// `Ended`". The arm stays because an exhaustive match over three variants costs nothing and -/// makes the next variant somebody adds a compile error here rather than a silent `false`. fn is_transient(error: &LinkError) -> bool { use std::io::ErrorKind::{ConnectionReset, Interrupted, TimedOut, WouldBlock}; match error { @@ -705,17 +653,6 @@ async fn open(task: &mut Acquisition) -> Option<(Source, Option)> { } /// Says why acquisition stopped, and tells the two reasons apart. -/// -/// The channel to the decode thread closes for two reasons that look identical from here: the -/// session was shut down and the receiver went with it, or the decode thread ended on its own -/// — which, the sender aside, means it panicked. An operator who closed a window should not -/// read that a thread died, and one whose thread died must not read nothing at all. -/// -/// The stop flag is read here rather than carried in `Gone`, which would be exact and would -/// make four call sites match on a two-variant enum. What that exactness would buy is the -/// right sentence in one interleaving: a decode thread that panics in the same instant the -/// operator closes the window is reported as a close. That is a race between two things that -/// both end the session, and the counters say the same either way. fn report_decoder_gone(task: &Acquisition) { if *task.stop.borrow() { log( @@ -1222,22 +1159,6 @@ mod tests { } /// A session shut down before the acquisition task is ever polled opens nothing. - /// - /// The ordering is fixed by the runtime and not by a sleep: this is a current-thread - /// test, so the task `Session::start` spawned cannot be polled until this future - /// awaits, and `Session::shutdown` is synchronous. The stop is therefore in the watch - /// before the task's first poll, every run. - /// - /// Two things are asserted, and they are the two that leave a trace: no "reading" line - /// on the event log and no recording file. An operator who closed the window before it - /// opened would otherwise be left with a zero-byte recording and a log claiming a station - /// is reading a source nobody is reading. The third guard on this path — the `select!` - /// over `Source::connect` — is not separately asserted here, because what it buys is time - /// on a `tcp://` connect to an unrouted address and a file open leaves nothing behind. - /// - /// `stop.is_closed()` is how this waits for the task to have *finished*: the acquisition - /// task holds the only receiver, and `is_running` cannot answer it because `shutdown` - /// flips that flag itself. #[tokio::test] async fn a_session_stopped_before_the_first_poll_opens_neither_source_nor_recording() { let base = std::env::temp_dir().join(format!( @@ -1411,12 +1332,6 @@ mod tests { } /// A recording cut off mid-packet must say so. - /// - /// A space packet declares its own length, so a stream that ends inside one leaves the - /// assembler holding a partial that nothing can complete. Before the flush at the - /// `Ok(0)` arm it went with the pipeline: no counter, no event, a clean exit and a packet - /// count one short of what the operator counted in the file. The same hole was found in - /// `xtce-gs export` first, which is why this one is here. #[tokio::test] async fn a_stream_that_ends_inside_a_packet_reports_the_partial() { let (definition, stream) = jpss(); diff --git a/crates/xtce-gs-gui/src/lib.rs b/crates/xtce-gs-gui/src/lib.rs index a0eca8d..343f5ad 100644 --- a/crates/xtce-gs-gui/src/lib.rs +++ b/crates/xtce-gs-gui/src/lib.rs @@ -191,12 +191,6 @@ struct Failure { impl eframe::App for Failure { /// One panel, one message, and no session behind it. - /// - /// No [`eframe::App::logic`] and no repaint on a timer: there is nothing here that moves - /// on its own. Input still draws — `egui_winit` answers a pointer event with - /// `EventResponse { repaint: true, .. }`, which `eframe` turns into the next pass — and - /// the window's own close button does not go through egui at all, so a station cannot - /// get stuck on its own error dialog. fn ui(&mut self, ui: &mut egui::Ui, _frame: &mut eframe::Frame) { egui::CentralPanel::default().show(ui, |ui| { ui.heading("The session did not start"); @@ -332,12 +326,6 @@ mod tests { use super::*; /// A context that has drawn, and so is not asking for anything. - /// - /// A fresh [`egui::Context`] starts with an outstanding repaint of its own — egui runs a - /// couple of passes at the start on purpose — so asking a brand new one whether a - /// repaint has been requested answers yes before anybody has requested one. Running - /// passes until it settles is what makes `has_requested_repaint` mean what these tests - /// read it as. fn settled_context() -> egui::Context { let context = egui::Context::default(); for _ in 0..8 { diff --git a/crates/xtce-gs-gui/src/panels/plots.rs b/crates/xtce-gs-gui/src/panels/plots.rs index 0fb993c..d86a4bd 100644 --- a/crates/xtce-gs-gui/src/panels/plots.rs +++ b/crates/xtce-gs-gui/src/panels/plots.rs @@ -124,16 +124,6 @@ pub const EMPTY_WINDOW_SECONDS: f64 = 60.0; const HUE_STEPS: u64 = 1024; /// Seconds either side of the Unix epoch that a tick label is clamped to. -/// -/// [`xtce_gs_core::Utc`] holds nanoseconds in an `i64`, which reaches 2262. The panel clamps -/// its tick formatting to well inside that: an axis past 2262-04-10 is not a view of any -/// telemetry, and a tick label is produced from whatever the operator drags the axis to. -/// -/// This was originally a guard against a real arithmetic panic — `Utc::civil` multiplied its -/// own day count back out by 86 400 000 000 000, which overflows an `i64` within a day of -/// either end, on the drawing thread, from a drag. Core now floor-divides instead and cannot -/// overflow for any input, so this is no longer load-bearing; it stays because a tick label -/// reading "2262-04-11" is noise either way. const CIVIL_LIMIT_SECONDS: f64 = 9_223_200_000.0; /// FNV-1a's 64-bit offset basis (Fowler–Noll–Vo, 1991). @@ -877,22 +867,6 @@ fn plot_menu(response: &egui::Response, grid: &Grid<'_>, index: usize, group: &[ const BAND_OPACITY: f32 = 1.0 / 12.0; /// Draws a parameter's warning and alarm bounds across the plot. -/// -/// Dashed and thin so they read as annotation rather than as telemetry, and unnamed so that -/// `egui_plot` keeps them out of the legend — `LegendWidget::try_new` skips an item whose name -/// is empty, which is the only way to add four lines per series without burying the names. -/// -/// The lines are inside the y auto-bounds, so a plot of a parameter with limits shows the -/// headroom rather than the noise. When that is the wrong trade — an alarm bound decades away -/// from the value — the escape is the plot's own `autoscale: false` and `y_range`, which are -/// saved in the layout. -/// -/// The two ranges are also shaded, between the warning bound and the alarm bound on each side -/// — that is the band an operator reads as "getting close" without having to compare a value -/// against two numbers. Both decisions the shading needs are made from the theme rather than -/// from a constant: the fill is the same colour as the line at a twelfth of its opacity, so -/// it is as dark or as light as the rest of the window, and it is drawn *before* the series, -/// so telemetry is never behind it. fn limit_lines( plot_ui: &mut egui_plot::PlotUi<'_>, limit: &Limit, diff --git a/crates/xtce-gs-gui/src/panels/status.rs b/crates/xtce-gs-gui/src/panels/status.rs index eb790d2..418f1b5 100644 --- a/crates/xtce-gs-gui/src/panels/status.rs +++ b/crates/xtce-gs-gui/src/panels/status.rs @@ -692,16 +692,6 @@ fn settings_menu(ui: &mut egui::Ui, layout: &Layout) -> Action { /// Appends the Reed-Solomon cell: frames repaired, frames lost, and how deep the repairs went. /// -/// The depth is `rs_symbols / rs_corrected` — symbols repaired per frame that needed -/// repairing — and it is the number `xtce_gs_link::rs` argues an operator has to be able to -/// watch: a decoder beyond its capacity does not fail, it lands on a different valid codeword -/// and hands back a frame that passes every check. The frame counts beside it say a repair -/// happened; only the depth says how much margin was left. -/// -/// It is appended to the same cell rather than given one of its own, and only while something -/// has actually been repaired, so a clean pass leaves the row exactly the width it has today — -/// which is what `gs-gui-status-row2` was blocked on. -/// /// Not coloured against a threshold. The capacity is `E` symbols per codeword times the /// interleaving depth (CCSDS 131.0-B-5 §4.4.1), and neither number reaches this panel: /// [`xtce_gs_core::LinkStats`] carries counters and not the coding configuration, so a diff --git a/crates/xtce-gs-gui/src/panels/table.rs b/crates/xtce-gs-gui/src/panels/table.rs index 7ec2c8a..3ffdd6f 100644 --- a/crates/xtce-gs-gui/src/panels/table.rs +++ b/crates/xtce-gs-gui/src/panels/table.rs @@ -219,16 +219,6 @@ impl Table { } /// Orders `rows` by the column in use. - /// - /// The *first* click on a column is the order that column is worth looking at, which for - /// three of the four is the largest first: the newest value, the worst limit, the - /// chattiest parameter. Only the name sorts ascending. [`Table::set_sort`] then reverses - /// on a second click, so `descending` here means "reversed from that", which is what - /// [`Table::is_descending`] documents and not "reversed from `Ord`". - /// - /// The name is the tie-break in every order, and it is looked up twice per comparison - /// rather than cached on the [`Row`]: a row carries the values an operator reads, and a - /// few hundred of them make a handful of array indexings per comparison, not a hash. fn sort_rows(&mut self, db: &XtceDb) { let reversed = self.descending; let direction = |ordering: Ordering| { diff --git a/crates/xtce-gs-gui/src/panels/tree.rs b/crates/xtce-gs-gui/src/panels/tree.rs index 99e11ac..79f5bb5 100644 --- a/crates/xtce-gs-gui/src/panels/tree.rs +++ b/crates/xtce-gs-gui/src/panels/tree.rs @@ -189,12 +189,6 @@ impl Tree { } /// Whether a parameter's declared type has a place on a numeric axis. -/// -/// Integer, float and boolean do — a boolean plots as 0 and 1, which is how an operator -/// watches a relay. A string, a binary blob, an enumeration label and the composite kinds do -/// not: an enumeration's engineering value is a label, and a label has no order. The two time -/// kinds are left out as well; an `AbsoluteTimeParameterType` is an instant, and a plot of an -/// instant against itself is a straight line the operator did not ask for. fn is_plottable(db: &XtceDb, parameter: ParamId) -> bool { matches!( db.type_of(parameter).map(|kind| &kind.kind), diff --git a/crates/xtce-gs-gui/tests/against_a_session.rs b/crates/xtce-gs-gui/tests/against_a_session.rs index 47eaf00..3eece87 100644 --- a/crates/xtce-gs-gui/tests/against_a_session.rs +++ b/crates/xtce-gs-gui/tests/against_a_session.rs @@ -204,12 +204,6 @@ fn every_panel_fills_from_a_session_that_really_decoded_a_downlink() { } /// The layout that ships beside the JPSS definition opens on three full plots. -/// -/// The README and the v0.1.0 notes both say so, which is reason enough to assert it: a layout -/// is resolved by *qualified name* against the definition, and a name that no longer matches -/// is dropped with a note rather than an error — the station starts either way. So a typo in -/// the shipped file would leave a visitor looking at the empty window this layout exists to -/// prevent, and nothing would say why. #[test] fn the_shipped_layout_opens_on_three_full_plots() { let definition = testdata("jpss1_geolocation_xtce_v1.xml"); diff --git a/crates/xtce-gs-link/src/derand.rs b/crates/xtce-gs-link/src/derand.rs index dbe65ce..cfc8c9d 100644 --- a/crates/xtce-gs-link/src/derand.rs +++ b/crates/xtce-gs-link/src/derand.rs @@ -67,11 +67,6 @@ pub const SEQUENCE_LENGTH: usize = 255; /// The 255-byte sequence of CCSDS 131.0-B-5 §10.4.2, derived from the shift register at /// compile time. -/// -/// A constant rather than a lazily built table: it is 255 bytes, it never changes, and a -/// `static` behind a lock would cost an atomic per frame to produce a number that was decided -/// in 1984. Private, because [`sequence`] is the accessor the contract names; see the module -/// documentation for the bit order it is derived under. const SEQUENCE: [u8; SEQUENCE_LENGTH] = build_sequence(); /// Runs the degree-8 shift register of CCSDS 131.0-B-5 §10.4.2 for one full period. @@ -150,11 +145,6 @@ mod tests { assert_eq!(&sequence()[..PUBLISHED_START.len()], &PUBLISHED_START); } - /// The head alone does not pin the taps: a wrong tap set can reproduce the first octets - /// and diverge later in the period. This is the byte the published tables end on. - /// - /// Checked against the independent table in `opensatelliteproject/libsathelper` - /// (`src/derandomizer.cpp`), whose whole 255 bytes agree with this derivation. #[test] fn the_sequence_ends_on_the_byte_the_published_tables_end_on() { assert_eq!(sequence()[SEQUENCE_LENGTH - 1], 0x58); @@ -164,9 +154,6 @@ mod tests { ); } - /// `h(x)` is primitive, so the run of 2040 bits is eight whole periods of a maximal-length - /// sequence and every byte but the all-zero one turns up exactly once. A non-primitive tap - /// set fails this even when it happens to match the head. #[test] fn every_byte_value_but_zero_appears_exactly_once() { let mut seen = [0u32; 256]; @@ -208,7 +195,6 @@ mod tests { assert_eq!(data, original); } - /// The claim the module makes in prose, asserted: there is one operation, not two. #[test] fn randomize_and_derandomize_are_the_same_operation() { let original: Vec = (0..600u32).map(|i| (i % 256) as u8).collect(); @@ -226,7 +212,6 @@ mod tests { assert_eq!(data, [] as [u8; 0]); } - /// A codeword longer than one period repeats the sequence rather than running off it. #[test] fn the_sequence_repeats_after_a_whole_period() { let mut data = vec![0u8; SEQUENCE_LENGTH * 2]; diff --git a/crates/xtce-gs-link/src/frame.rs b/crates/xtce-gs-link/src/frame.rs index 437e687..8dbe957 100644 --- a/crates/xtce-gs-link/src/frame.rs +++ b/crates/xtce-gs-link/src/frame.rs @@ -577,11 +577,6 @@ mod tests { /// A 32-byte frame with every primary header field that *can* differ from its /// neighbours set to a value nothing else has. /// - /// Version 0, spacecraft 0x2AB, virtual channel 5, OCF flagged; master count 0x11, - /// virtual channel count 0x22; secondary header flagged, sync and order clear, segment - /// length id 0b11, first header pointer 5. Insert zone 4, secondary header 3, data field - /// 13, OCF 4, FECF 2. - /// /// Three fields are *not* distinguishable here, for two different reasons. Two of them /// are fixed by this frame's clear synchronisation flag: CCSDS 132.0-B-3 §4.1.2.7.4 /// reserves the packet order flag and sets it to `0`, and §4.1.2.7.5 sets the segment @@ -790,12 +785,6 @@ mod tests { assert!(!frame.packet_order_flag()); } - /// The segment length identifier's two bits sit directly below the packet order flag, and - /// `distinguishable_frame` has the identifier at `0b11` with the flag clear — so a mask - /// widened into the flag answers correctly there. Here the flag is set and the identifier - /// is not, which CCSDS 132.0-B-3 §4.1.2.7.4 and §4.1.2.7.5 NOTE 2 both allow while the - /// synchronisation flag is set, because both fields are undefined on a VCA frame — the - /// frame kind TODO(gs-link-frame-vca) is still open about. Only the header word is read. #[test] fn the_segment_length_identifier_does_not_reach_into_the_packet_order_flag() { // Sync set, packet order set, segment length id 0b01, pointer 0. @@ -807,12 +796,6 @@ mod tests { assert_eq!(frame.segment_length_id(), 0b01); } - /// The secondary header length is the low six bits of the identification octet and the - /// two above it are a version number, which CCSDS 132.0-B-3 §4.1.3.2.2.2 requires to be - /// `00` — so on every other fixture in this file a mask widened into them reads the same - /// length. This octet is `0x42`: version `01`, length field 2, three octets in all. - /// Accepting it is TODO(gs-link-frame-sh-zero)'s open decision not to check those bits; - /// what this pins is that the length is read from its own six and no more. #[test] fn the_secondary_header_length_does_not_reach_into_the_version_bits() { let mut bytes = vec![0x00, 0x00, 0x00, 0x00, 0x98, 0x00]; diff --git a/crates/xtce-gs-link/src/pipeline.rs b/crates/xtce-gs-link/src/pipeline.rs index 532e9d3..450374e 100644 --- a/crates/xtce-gs-link/src/pipeline.rs +++ b/crates/xtce-gs-link/src/pipeline.rs @@ -368,12 +368,6 @@ enum Reason { } /// A bounded, repeat-collapsing queue of lines for the operator. -/// -/// The event log downstream collapses repeats too, but it cannot help here: the cost being -/// avoided is the `format!` that builds the message, which happens on this side of the queue. -/// A run of identical lines is therefore reported as one line with the count appended, which -/// means the log's own repeat count undercounts — the trade is deliberate, and the number the -/// operator reads is the one in the message. #[derive(Debug)] struct Events { queued: Vec, @@ -1081,13 +1075,6 @@ mod tests { } /// The first `count` packets of the JPSS recording. - /// - /// Panics if the recording is not there, and that is the point. This used to fall back to - /// packets made up on the spot, which kept every test below green while it quietly stopped - /// testing the thing it is named after — a real pass, with the field widths and the - /// sequence counts a real spacecraft produced. The recording is vendored in this - /// repository now, so its absence means somebody deleted it, and a deleted fixture should - /// cost a red test rather than a silent change of subject. fn packets(count: usize) -> Vec> { let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(JPSS); let bytes = std::fs::read(&path).unwrap_or_else(|error| { @@ -1156,12 +1143,6 @@ mod tests { } /// Wraps packets in transfer frames the way CCSDS 132.0-B-3 section 4.1 lays them out. - /// - /// The coding order here is the inverse of [`Pipeline::push`]'s and has to be flipped with - /// it: randomise the transfer frame, then Reed-Solomon encode the randomised frame, then - /// put the marker in front of the block. A fixture that mirrors the pipeline cannot decide - /// which order is right — it passes either way round — which is exactly why the argument - /// is written out in `Pipeline::push` rather than left to this test. fn fixture(count: usize, rs: Option, scramble: bool, asm: bool) -> Fixture { let expected = packets(count); let frame_length = rs.map_or(PLAIN_FRAME_LENGTH, RsConfig::frame_bytes); diff --git a/crates/xtce-gs-link/src/rs.rs b/crates/xtce-gs-link/src/rs.rs index 68d3b69..e7112b1 100644 --- a/crates/xtce-gs-link/src/rs.rs +++ b/crates/xtce-gs-link/src/rs.rs @@ -706,11 +706,6 @@ impl ReedSolomon { /// Forney's formula: the magnitude of the error at each located position. /// - /// Applies each correction to `codeword`, records the positions that actually changed in - /// `repaired`, and returns how many there were. A located position whose magnitude comes - /// out zero was not in error, so it is not counted — the caller's number is symbols - /// repaired, not roots found. - /// /// It cannot fail. Karn's `decode_rs.h` refuses here when the formal derivative of /// `lambda` vanishes at a root — a repeated root, describing no real error pattern — but /// this decoder has already excluded that: it runs only when the Chien search returned @@ -962,16 +957,6 @@ mod tests { } /// Asserts that a block `decode` returned `Ok` for really is a codeword. - /// - /// This is what a success from a Reed-Solomon decoder claims and the only part of the - /// claim a test can check: re-encode the data half of what came back and the parity must - /// come back identical. Asserting instead that the result differs from the transmitted - /// block is a tautology — `Ok` bounds the changes by `E`, the received word was `E + 1` - /// away, so the triangle inequality already settles it — and a decoder that dropped its - /// final syndrome re-check would pass that assertion while handing back rubbish. - /// - /// It cannot see a received word that landed inside *another* codeword's decoding sphere; - /// nothing can. That is the case the caller's `E + 1` doc describes. fn assert_is_a_codeword(code: &ReedSolomon, block: &[u8]) { let mut reencoded = Vec::new(); code.encode(&block[..code.data_length()], &mut reencoded) @@ -1013,8 +998,6 @@ mod tests { } } - /// Both directions, because a table built backwards passes a one-directional test: it is - /// still an involution pair, just the wrong one. #[test] fn the_dual_basis_transforms_undo_each_other_in_both_directions() { for symbol in 0..=u8::MAX { @@ -1023,8 +1006,6 @@ mod tests { } } - /// The transform is linear and invertible over GF(2)^8, so it permutes the 256 octets. A - /// table with a repeated entry would still round-trip for the values it happened to cover. #[test] fn the_dual_basis_transform_is_a_permutation_of_every_byte() { let mut seen = [false; 256]; @@ -1057,9 +1038,6 @@ mod tests { } } - /// The encoder reads the generator in index form and adds, which silently multiplies by - /// one where a coefficient is zero. No generator with consecutive roots has a zero - /// coefficient, and this is where that assumption is written down. #[test] fn no_generator_coefficient_is_zero() { for (parity_symbols, generator) in [(16usize, &GENERATOR_E8), (32, &GENERATOR_E16)] { @@ -1076,8 +1054,6 @@ mod tests { assert_eq!(ROOT_SPACING * INVERSE_ROOT_SPACING % CODEWORD_SYMBOLS, 1); } - /// `F(x) = 0x187` has to be primitive or the shift register visits fewer than 255 values - /// and the log table has holes in it. #[test] fn the_field_tables_invert_each_other_over_the_whole_field() { assert_eq!(ALPHA_TO[0], 1); @@ -1094,9 +1070,6 @@ mod tests { assert!(!seen[0], "zero is not a power of alpha"); } - /// The whole of the encoder in one assertion: field, generator, root range, the LFSR, the - /// basis transform on the parity and the absence of one on the data. A table comparison - /// alone would not catch a wrong first root. #[test] fn a_codeword_matches_phil_karns_encoder_byte_for_byte() { let code = codec(32, 1); @@ -1154,9 +1127,6 @@ mod tests { } } - /// Every position, because the Chien search maps a root to a position through the inverse - /// of the root spacing and a decoder that uses the wrong step corrects position `11 * i` - /// instead of `i`. On a single error at position 0 the two agree. #[test] fn every_single_symbol_position_in_a_block_is_correctable() { let code = codec(32, 1); @@ -1201,16 +1171,6 @@ mod tests { } /// One symbol past the capacity, over many messages. - /// - /// `Uncorrectable` is asserted as *usually*, not always, and that is not a hedge: a - /// received word that is `E + 1` symbols from the transmitted codeword can be `E` or fewer - /// from a different one, and then the decoder lands on that codeword, the syndromes of the - /// result really are zero, and no check inside a Reed-Solomon decoder can tell. What must - /// hold every time is the weaker statement below — the decoder either refuses, or does not - /// hand back something it claims is the transmitted frame when it is not. For RS(255, 223) - /// the fraction of received words that fall inside another sphere is about `1 / 16!`, so - /// in practice every trial here refuses; the loop asserts three quarters so that the day - /// it does happen the suite reports it rather than flapping. #[test] fn one_error_past_the_capacity_is_usually_refused_and_is_never_silently_wrong() { let code = codec(32, 1); @@ -1248,13 +1208,6 @@ mod tests { ); } - /// The entire reason interleaving exists, in one test. - /// - /// Symbol `i` of the block belongs to codeword `i mod I`, so `E * I` consecutive symbols - /// hit every residue class exactly `E` times: every codeword gets exactly its capacity and - /// the burst comes out. `(E + 1) * I` consecutive symbols hit every class `E + 1` times - /// and every codeword is over. Both hold wherever the burst starts, which is why the step - /// past the capacity is `+ I` and not `+ 1`. #[test] fn a_burst_of_the_capacity_times_the_depth_is_corrected_and_one_depth_more_is_not() { let code = codec(32, 5); @@ -1292,16 +1245,6 @@ mod tests { /// The mis-correction the two loops above only assert is *not silently wrong*, built on /// purpose so that it happens every run. - /// - /// Two codewords whose messages differ in one symbol differ in exactly `2E + 1` places: - /// at least that, because `2E + 1` is the minimum distance of an MDS code, and at most - /// that, because only the one message symbol and the `2E` check symbols can move. Move a - /// received word `E + 1` of those steps and it sits `E` from the *other* codeword, so a - /// decoder that is working correctly repairs it into that one, reports `Ok(E)`, and hands - /// on a frame that was never transmitted. Nothing inside a Reed-Solomon decoder can see - /// this — the result really is a codeword, which is what [`assert_is_a_codeword`] checks - /// and all a success can ever claim. It is the module documentation's reason for counting - /// corrections instead of trusting them. #[test] fn a_constructed_near_miss_is_corrected_into_the_wrong_codeword_and_called_success() { let code = codec(32, 1); @@ -1369,8 +1312,6 @@ mod tests { } } - /// A short frame silently padded to a codeword decodes cleanly to bytes nobody sent, so - /// the encoder refuses rather than pads. #[test] fn a_message_of_the_wrong_length_is_refused() { let code = codec(16, 2); @@ -1404,8 +1345,6 @@ mod tests { assert_eq!(deep.interleave(), 5); } - /// One error in the last codeword must not leave the first `I - 1` repaired: the caller - /// takes the first `data_length` bytes as a frame and would parse a half-corrected one. #[test] fn an_uncorrectable_codeword_leaves_the_whole_block_as_it_was_found() { let code = codec(16, 3); @@ -1452,20 +1391,12 @@ mod tests { } /// Reed-Solomon cannot tell you that you forgot to derandomise. /// - /// The pipeline is `synchronise → derandomise → Reed-Solomon`, and the temptation is to - /// put the error-correcting code first, on the theory that a check this strong would - /// notice a block that had not been derandomised yet. It does not, and the reason is - /// worth stating once where somebody reordering the pipeline will read it. - /// /// The randomiser sequence of CCSDS 131.0-B-5 §10.4 is 255 octets. An RS(255,223) /// codeword is 255 symbols over GF(2^8), and this code is linear: the sum of two /// codewords is a codeword, and over GF(2) that sum is XOR. The sequence happens to be a /// codeword itself — so a randomised frame, which is `codeword XOR sequence`, is also a /// codeword. Exactly. Zero syndromes, zero corrections, and a block that disagrees with /// the frame it came from in every single symbol. - /// - /// So the only thing standing between a misordered pipeline and 255 bytes of confident - /// nonsense is the order itself. #[test] fn reed_solomon_cannot_see_that_the_derandomiser_was_skipped() { // Both codes CCSDS 131.0-B-5 defines, because the first question anybody asks is diff --git a/crates/xtce-gs-link/src/source.rs b/crates/xtce-gs-link/src/source.rs index 097e93f..f7867c7 100644 --- a/crates/xtce-gs-link/src/source.rs +++ b/crates/xtce-gs-link/src/source.rs @@ -38,15 +38,6 @@ const ACCEPTED: &str = "udp://host:port, tcp://host:port, tcp-listen://host:port file:///path?rate=&chunk=&repeat, or a bare path"; /// A read buffer that does not print itself. -/// -/// The sources read into their own scratch and copy out, rather than growing the caller's -/// buffer and truncating it back: `read_chunk` is awaited inside a `tokio::select!` against -/// the session's shutdown, and a future cancelled between the grow and the truncate would -/// leave the caller holding a chunk of zeroes it never received. Copying the bytes that did -/// arrive costs a `memcpy` of what was read; the alternative costs correctness. -/// -/// `Debug` is hand-written because the UDP scratch is 64 KiB and a derived one would render -/// every byte of it into whatever log line asked for the source. struct Scratch(Vec); impl Scratch { @@ -174,7 +165,7 @@ fn parse_file(rest: &str) -> Result { impl FromStr for SourceSpec { type Err = LinkError; - /// Parses one of the four URL forms. + /// Parses one of the four URL forms: /// /// ```text /// udp://0.0.0.0:10015 @@ -184,22 +175,7 @@ impl FromStr for SourceSpec { /// /path/to/stream.dat /// ``` /// - /// A string with no scheme is a file path, because that is what an operator types and - /// because no other variant can be named by a bare word. `repeat` is a bare flag rather - /// than `repeat=true`: it is either there or it is not. - /// - /// The query is parsed only after `file://`. A bare path is taken whole, `?` and all, - /// because `?` is a legal character in a file name on every platform this runs on and - /// splitting one silently would open a different file than the operator typed. The cost - /// is that a `file://` URL cannot name a path containing `?`; such a path has to be given - /// bare, and the replay options then come from the command line instead. - /// - /// # Errors - /// - /// [`LinkError::Config`] naming what could not be parsed — an unknown scheme, an address - /// without a port, a `rate` or `chunk` that is not a number. A file path is not checked - /// for existence here; that is [`Source::connect`]'s failure to report, and reporting it - /// twice means reporting it differently twice. + /// A string with no scheme is a file path. fn from_str(s: &str) -> Result { let s = s.trim(); if s.is_empty() { @@ -617,14 +593,6 @@ impl TcpListenSource { } /// Whether an `io::Error` describes one connection rather than the socket underneath it. -/// -/// `accept` reports a connection the client abandoned during the handshake as an error of the -/// listener, and a peer that vanishes reports as a reset rather than a clean close. Neither -/// says anything about this station's ability to keep listening, so neither ends the session. -/// -/// `TimedOut` is deliberately *not* in the set. On a socket with no receive timeout it means -/// the keepalives failed, which is a link this station should report rather than a peer it -/// should quietly re-accept in a loop nobody sees. fn per_connection(error: &std::io::Error) -> bool { use std::io::ErrorKind::{ BrokenPipe, ConnectionAborted, ConnectionReset, Interrupted, NotConnected, diff --git a/crates/xtce-gs-link/src/sync.rs b/crates/xtce-gs-link/src/sync.rs index 3141cd8..5413fcf 100644 --- a/crates/xtce-gs-link/src/sync.rs +++ b/crates/xtce-gs-link/src/sync.rs @@ -325,17 +325,6 @@ impl Synchronizer { } /// Scans forward for the marker. `true` when lock was taken. - /// - /// On failure the buffer is trimmed to the tail that could still *begin* a marker, which - /// is what bounds the buffer on a stream that never syncs and what keeps the rescan from - /// being quadratic: the next scan restarts near the end of the buffer, not at its head. - /// - /// `self.marker` is never empty here, and the guarantee is not in this function: the - /// three places that set [`SyncState::Searching`] — `new`, `reset` and `locate_frame`'s - /// lock-loss path — each do so only when the marker is non-empty, and `locate_frame` - /// calls this only while searching. A fourth such site has to keep that, because an - /// empty marker matches at the cursor immediately and would count a lock the empty-marker - /// mode promises never to count. fn acquire(&mut self) -> bool { let marker_len = self.marker.len(); let offsets: u8 = if self.bit_slip { 8 } else { 1 }; @@ -496,8 +485,6 @@ mod tests { assert_eq!(sync.take_counters().bytes_discarded, 37); } - /// The last frame of a stream that ends cleanly has no marker after it. A synchroniser - /// that waits for one loses it. #[test] fn the_last_frame_is_emitted_although_no_marker_follows_it() { let mut sync = Synchronizer::new(16, CCSDS_ASM.to_vec(), false); @@ -516,8 +503,6 @@ mod tests { assert_eq!(drain(&mut sync), vec![frame(0x02, 16)]); } - /// The whole point of the flywheel: a bit error inside the marker must not cost the frame - /// behind it, which Reed-Solomon can still correct. #[test] fn a_single_corrupted_marker_costs_neither_the_lock_nor_the_frame() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false); @@ -533,8 +518,6 @@ mod tests { assert_eq!(counters.losses, 0); } - /// With a flywheel of four, four misses in a row are tolerated and the fifth loses lock. - /// The skeleton's rule is `misses > flywheel`, and this test is what pins it. #[test] fn loses_lock_on_the_fifth_consecutive_missing_marker_with_a_flywheel_of_four() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false); @@ -559,8 +542,6 @@ mod tests { assert_eq!(counters.markers_found, 1); } - /// Every one of the seven offsets, not one of them: an off-by-one in the shift helper - /// passes at four bits and fails at one. #[test] fn a_stream_slipped_by_each_of_the_seven_bit_offsets_still_yields_the_frames() { for bits in 1..8u8 { @@ -589,13 +570,6 @@ mod tests { } /// A stream that never syncs must not be a memory leak with a counter on it. - /// - /// Two things this has to get right to see anything. The fixture must contain the bytes - /// the scan is looking for — a stream with no `0x1A` in it fails on the first comparison - /// at every index and every bit offset, so it never reaches the trim, a partial match or - /// a near-lock. And the assertion must be on the buffer itself: `buffered()` is - /// `len - cursor`, which is small the moment the cursor moves whether or not `push` ever - /// released the prefix behind it, so it cannot observe the leak it is meant to catch. #[test] fn the_buffer_does_not_grow_on_a_stream_that_never_syncs() { // `1A CF FC` is the first three octets of the marker and `1A` alone is its first, so @@ -637,11 +611,6 @@ mod tests { ); } - /// The flywheel tolerates *consecutive* misses — `DEFAULT_FLYWHEEL`'s doc says so — and - /// `self.misses = 0` on a matched marker is the whole of what makes that true. Damage - /// every other marker: no two in a row, so a counter that resets never passes one, and a - /// counter that only ever climbs passes four and drops lock partway through the stream. - /// Every existing flywheel test damages an unbroken run, where the two behave alike. #[test] fn marker_damage_that_is_never_consecutive_costs_no_lock_however_much_of_it_arrives() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false); @@ -672,10 +641,6 @@ mod tests { assert_eq!(counters.frames, 12); } - /// A marker at a non-zero bit offset spans one byte more than its length, so the last - /// position the scan reaches — `buffer.len() - marker_len` — is one it cannot decide: - /// `marker_at` refuses it for want of the straddle byte. The extra byte in the trim's - /// `keep` is what holds that position until the next push brings the rest of it. #[test] fn a_slipped_marker_at_the_trim_boundary_survives_the_trim() { for bits in 1..8u8 { @@ -723,9 +688,6 @@ mod tests { assert_eq!(bare.take_counters().frames, 0); } - /// A synchroniser that can never cut a frame must not hold the stream it will never cut. - /// `locate_frame` returns before the tail trim in this mode, so the trim is not what - /// bounds it; `push` is. #[test] fn a_frame_length_of_zero_discards_what_it_can_never_cut() { let mut sync = Synchronizer::new(0, CCSDS_ASM.to_vec(), true); @@ -772,7 +734,6 @@ mod tests { assert_eq!(counters.losses, 0, "a boundary is not a lock loss"); } - /// A marker split across two reads is the common case on TCP, not an edge case. #[test] fn a_marker_that_straddles_two_pushes_is_still_found() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false); @@ -801,8 +762,6 @@ mod tests { assert_eq!(from_one_chunk.len(), 3); } - /// Once lock is lost the search resumes ahead of the cursor and finds the next real - /// marker, rather than sitting in `Searching` on a stream that is perfectly fine again. #[test] fn lock_is_regained_at_the_next_good_marker_after_a_loss() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false); @@ -819,8 +778,6 @@ mod tests { assert_eq!(counters.locks, 2); } - /// The frame is handed out as a borrow of the buffer, and correcting it in place is the - /// reason it is: what the caller writes is what the synchroniser holds. #[test] fn the_frame_is_a_mutable_borrow_of_the_synchronisers_own_buffer() { let mut sync = Synchronizer::new(8, CCSDS_ASM.to_vec(), false);