diff --git a/docs/adr/2026-07-31-output-sink-retry-policy.md b/docs/adr/2026-07-31-output-sink-retry-policy.md new file mode 100644 index 000000000..aa3c7fb2c --- /dev/null +++ b/docs/adr/2026-07-31-output-sink-retry-policy.md @@ -0,0 +1,105 @@ +# ADR: Output sinks wait for a hardware change instead of asking every frame + +- **Status:** Accepted +- **Date:** 2026-07-31 +- **Deciders:** Photomancer +- **Supersedes:** None +- **Superseded by:** None +- **Relates:** `docs/debt/s3-frame-cost-scales-per-fixture.md` (the measurement + that prompted this; its other half — per-frame dataflow re-resolution — is a + separate change) + +## Context + +An output sink whose channel is not open re-attempted `OutputProvider::open` +on every flush, which is every frame. For a sink that *can* open, that is one +attempt and then nothing. For a sink that cannot — a project authored for +`ws281x:rmt:D9` loaded onto a board that has no `D9`, a pin another node holds, +a fifth strip on a four-channel board — it is an attempt per frame, forever. + +Each attempt is not cheap. Opening by spec enumerates every endpoint every +registered driver offers; each endpoint is constructed with a formatted spec +string, parsed and validated, and carries a freshly computed status, which is +itself several registry lookups. On `projects/test/quad-strips` under the +emulator (esp32-c6 cycle model), where three of four authored endpoints do not +exist on the virtual board, this was **45.8% of all self cycles** in +`HwRegistry::endpoint_status_for` and **90.2% inclusive** under +`OutputProvider::open` — the frame spent mostly on relearning the same "no", +sixty times a second, plus a `log::warn!` per failed sink per frame. + +The obvious framing — "cache endpoint status in the registry" — is the wrong +one. A sink that opened successfully never asks again, so there is no repeated +query to memoize on the success path; the repetition is entirely the retry +loop. And caching status is actively dangerous: a cached `Available` on a pin +that has since been claimed would hand two owners the same hardware. The +question was never "what did status say last time" but "is it worth asking +again at all". + +## Decision + +**Sinks park on failure, and a hardware generation counter tells them when to +wake.** + +Three pieces, one per layer: + +1. `HwRegistry::generation() -> u64` changes whenever a claim succeeds or a + lease is released — the only two events that can turn a refusal into an + acceptance. Reserved status and capability support come from the manifest, + which is immutable after construction, so they need no signal. Failed + claims deliberately do **not** bump: signalling them would let one hopeless + endpoint wake every parked sink on every attempt, which is the storm this + ends. + +2. `OutputProvider::hardware_generation() -> u64` forwards it. The default is + `0` — "this hardware never changes" — which is the honest answer for a + provider that owns no registry: its failures are about the caller's own + configuration, not about ownership. Registry-backed providers forward; + wrappers delegate. + +3. `EngineServices` records `parked_at_generation` on a sink whose open + failed, and skips that sink while the generation still matches. The value + is sampled **before** any open in the flush, so a claim made by this frame's + own opens cannot park a sink against a number that already includes it. + Parking is cleared on re-authored config, on provider swap, and on success. + +Consequences worth stating: + +- A parked sink does not fail its frame. The first failure is logged and + returned exactly as before; subsequent frames are silent and free. Recovery + logs one line. This turns per-frame log spam — which on silicon is per-frame + serial traffic — into two lines per episode. +- Recovery latency is unchanged: a released pin bumps the generation, and the + next flush retries. Nothing polls, nothing waits out a timer. +- Nothing caches endpoint status. Reserved pins and claim conflicts are still + computed from live registry state at every open attempt, so the semantics + that would be dangerous to stale cannot go stale. + +## Alternatives considered + +**Cache `HwEndpointStatus` in the registry.** Rejected: it optimizes a query +the success path does not make, and a stale `Available` on a claimed pin is a +correctness bug in exchange for no measured win. + +**Time-based backoff** (retry a failed sink about once a second). Rejected: it +delays legitimate recovery by an arbitrary constant, keeps re-enumerating +forever for endpoints that will never exist, and puts a timing policy in a +layer that has no clock of its own. It would also still need the generation to +recover promptly, so it is strictly additional machinery. + +**Give up on failed sinks permanently.** Rejected: contention is normal and +transient. A sink that lost a race for the RMT channel must light when the +winner releases it, and today it does. + +If a transient failure ever appears that no claim or release follows — an RMT +peripheral that fails to initialize and later would not — a slow fallback +retry can be added on top of this without changing the contract. + +## Notes + +The measured win is emulator-side, because that board really does lack the +authored pins. On the desk ESP32-S3 all four channels open on the first frame, +so steady-state silicon paid little for this and gains little back; its own +flat per-fixture cost is the dataflow resolver, tracked separately in the debt +entry. What silicon does gain is that a *misconfigured* output — the common +authoring mistake — no longer costs a frame's worth of enumeration and a line +of serial output every frame it stays wrong. diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index b6c64395c..a1338948a 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -68,6 +68,60 @@ nothing else helps, by measurement. resolution machinery (dataflow resolver + endpoint status) instead. The suspicious shapes: `clear_frame_cache` discarding all resolution work every tick, and endpoint status recomputed per frame per channel. +- **2026-07-31 (later)** — **Endpoint-status half closed** (PR #244, + plan `2026-07-31-2224-hw-endpoint-status-cache`, ADR + `2026-07-31-output-sink-retry-policy.md`). The 45.8% was not status lookup + but a **failed-open retry storm**: `ensure_channel_open` re-attempted any + handle-less sink every frame, and the emulator board declared one WS281x + channel and no `D9`/`D8`/`D7`, so three of quad-strips' four sinks could + never open and re-enumerated the whole board — 256 endpoints, each with a + formatted spec and a live status — sixty times a second, forever. + + Fixed by *not asking*, not by caching: sinks park on a new + `HwRegistry::generation()` (bumped only on successful claim/release) and + wake when hardware ownership actually moves. No endpoint status is stored + anywhere, so reserved-pin and claim-conflict semantics cannot go stale. Also + collapsed the 3 enumerations per open attempt to 1, and stopped + `refresh_output_sink_configs` cloning every output def per tick. + + Measured, frame-for-frame (8 frames both runs, `events.jsonl` B→E): + **steady frame 16.42M → 1.53M cycles, 10.7×**; total attributed 65.8M → + 6.2M. `endpoint_status_for`, `VirtualWs281xDriver::endpoints`, + `endpoint_for_spec`, `validate_spec` and the `core::fmt` machinery are all + **absent from the top-20 self cycles**. Per-frame warn spam → 7 lines for + the whole run. Profiles: + `2026-07-31T22-42-28--…quad-strips--steady-render` (before) and + `…23-29-27` (after); `…23-33-40` is after the emulator board was given the + S3's four channels (steady frame 1.554M — the +1.3% is three more strips + actually being written). + + **Desk-S3 re-measured 2026-08-01** (d8:3b:da:47:29:70, identified by MAC via + `espflash board-info`; branch firmware flashed, quad-strips pushed): + **20 fps, tick 48 ms — flat**, stable over 13 consecutive `[perf]` readings. + That is exactly the prediction: the S3 opens all four channels on frame one, + so it never paid this cost in steady state, and its flat ~8.4 ms/fixture is + the resolver. + + Since an unchanged fps cannot itself prove the new image was running, the + parked-sink path was exercised on silicon instead: quad-strips with one + output re-pointed at `ws281x:rmt:NOT-A-PIN` produced **2 warnings and then + silence across ~1,250 frames** (the two being the designed settle — first + attempt, then one retry after the other three opens bumped the generation). + The old code logs one per frame, so this both proves the image and confirms + the fix on hardware. fps held at 20 with the dead output; tick 47 ms, the + 1 ms being one fewer strip to write. + + Not measured: the same misconfigured-output case on *pre-fix* firmware, which + would quantify what silicon saves there. The saving is a board enumeration + plus a serial line per frame; on the S3's ~40-resource manifest that is far + smaller than the emulator's 256, and it was not worth a second flash cycle to + put a number on. + + **Still open: the resolver half** — the profile is now dominated by exactly + what this entry predicted would remain: memcpy 18.7%, allocator 12.8%+8.0%, + `QueryKey::eq` 5.7%, `EngineSession::resolve` 2.7%, `SlotPath::parse`. That + is the dataflow resolver re-resolving from cold each tick, owned by plan + `2026-07-31-2225-persist-dataflow-resolution`; `…23-33-40` is its baseline. **Exit criteria** — A profiled optimization pass that makes resolved bindings and endpoint status persist across frames (invalidate on tree/binding/ diff --git a/lp-app/lpa-server/src/project.rs b/lp-app/lpa-server/src/project.rs index 03f33cecf..fc7c1161f 100644 --- a/lp-app/lpa-server/src/project.rs +++ b/lp-app/lpa-server/src/project.rs @@ -491,6 +491,10 @@ impl OutputProvider for SharedOutputProvider { fn close(&self, handle: OutputChannelHandle) -> Result<(), lpc_hardware::OutputError> { self.0.borrow().close(handle) } + + fn hardware_generation(&self) -> u64 { + self.0.borrow().hardware_generation() + } } fn project_root_path(name: &str) -> Result { diff --git a/lp-core/lpc-engine/src/engine/engine.rs b/lp-core/lpc-engine/src/engine/engine.rs index 859ba3145..809d30286 100644 --- a/lp-core/lpc-engine/src/engine/engine.rs +++ b/lp-core/lpc-engine/src/engine/engine.rs @@ -314,15 +314,6 @@ impl Engine { } } - pub(crate) fn loaded_node_def_for_entry<'a, N>( - &self, - registry: &'a ProjectRegistry, - entry: &RuntimeNodeEntry, - ) -> Option<&'a NodeDef> { - let location = entry.def_location.as_ref()?; - loaded_registry_def(registry, location).ok() - } - // Consumed by texture and shader node test modules (and the // texture-def-root test in `project_read_stream`). #[cfg(all(test, any(feature = "node-texture", feature = "node-shader")))] @@ -397,20 +388,31 @@ impl Engine { result } + /// Re-read every output's authored configuration for this tick. + /// + /// The tree and the services are borrowed as separate fields so the defs + /// can be read while the sinks are updated. Collecting the work first + /// instead cost a full clone of every output's definition — endpoint spec + /// and all — on every frame, to almost always discover that nothing had + /// changed. fn refresh_output_sink_configs(&mut self, registry: &ProjectRegistry) { - let mut updates = Vec::new(); - for entry in self.tree.entries() { - let Some(buffer_id) = self.runtime_output_sink_buffer_id(entry.id) else { + let tree = &self.tree; + let services = &mut self.services; + for entry in tree.entries() { + let buffer_id = match entry.state.value() { + NodeEntryState::Alive(node) => node.runtime_output_sink_buffer_id(), + _ => None, + }; + let Some(buffer_id) = buffer_id else { continue; }; - let Some(NodeDef::Output(def)) = self.loaded_node_def_for_entry(registry, entry) else { + let Some(location) = entry.def_location.as_ref() else { continue; }; - updates.push((buffer_id, def.clone())); - } - - for (buffer_id, def) in updates { - self.services.update_output_sink_config(buffer_id, &def); + let Ok(NodeDef::Output(def)) = loaded_registry_def(registry, location) else { + continue; + }; + services.update_output_sink_config(buffer_id, def); } } diff --git a/lp-core/lpc-engine/src/engine/engine_services.rs b/lp-core/lpc-engine/src/engine/engine_services.rs index 36abde50c..d7702b76c 100644 --- a/lp-core/lpc-engine/src/engine/engine_services.rs +++ b/lp-core/lpc-engine/src/engine/engine_services.rs @@ -28,6 +28,16 @@ struct OutputSinkBinding { display_options: Option, channel_handle: Option, last_byte_count: Option, + /// Hardware generation observed when this sink's last open attempt failed, + /// or `None` while the sink has an open channel or a retry is due. + /// + /// A sink whose endpoint does not exist on the board — a project authored + /// for four strips loaded onto a one-strip board, say — can never open, and + /// re-attempting costs a full enumeration of every endpoint the drivers + /// offer. Parking on the generation means such a sink is asked once per + /// *hardware change* rather than once per frame, while a pin freed by + /// another node still lights it on the very next flush. + parked_at_generation: Option, } /// Failure while flushing one registered output sink. @@ -141,6 +151,12 @@ impl EngineServices { /// Replace the optional [`OutputProvider`] used when flushing sinks after each tick. pub fn set_output_provider(&mut self, provider: Option>) { self.close_output_sinks(); + // A different provider is a different world: what the old one refused + // says nothing about what this one will do, so no sink stays parked + // across the swap. + for sink in self.output_sinks.values_mut() { + sink.parked_at_generation = None; + } self.output_provider = provider; } @@ -187,18 +203,24 @@ impl EngineServices { display_options, channel_handle: None, last_byte_count: None, + parked_at_generation: None, }, ); } + /// Re-read an output's authored configuration, reopening the channel only + /// if it actually changed. + /// + /// Called for every output on every tick, so the unchanged path — which is + /// nearly every call — must not allocate. The comparison borrows the + /// authored endpoint; only a genuine change pays for a copy of it. pub fn update_output_sink_config(&mut self, buffer_id: RuntimeBufferId, config: &OutputDef) { - let endpoint = endpoint_from_output_config(config); let display_options = display_options_from_output_config(config); let Some(existing) = self.output_sinks.get(&buffer_id) else { self.register_output_sink(buffer_id, config); return; }; - if existing.endpoint == endpoint + if existing.endpoint == *config.endpoint() && output_options_eq(&existing.display_options, &display_options) { return; @@ -209,9 +231,12 @@ impl EngineServices { .remove(&buffer_id) .expect("output sink existed above"); self.close_output_sink(&mut existing); - existing.endpoint = endpoint; + existing.endpoint = endpoint_from_output_config(config); existing.display_options = display_options; existing.last_byte_count = None; + // A re-authored endpoint is a fresh question for the hardware, so the + // sink stops waiting on a generation change it no longer cares about. + existing.parked_at_generation = None; self.output_sinks.insert(buffer_id, existing); } @@ -321,6 +346,15 @@ fn flush_registered_sinks( let mut first_error: Option = None; let mut failed = 0usize; + // Read once per flush, not once per sink: the answer is the same for every + // sink in the frame, and asking is a virtual call through the provider. + // + // Read it *before* any open in this flush, too. A successful open bumps the + // generation, so a value sampled afterwards would park a sink against a + // number that already reflects this frame's own claims — and a release that + // raced the open would be missed rather than retried. + let generation = provider.hardware_generation(); + for (buffer_id, sink) in sinks.iter_mut() { let Some(versioned) = buffers.get(*buffer_id) else { continue; @@ -334,7 +368,11 @@ fn flush_registered_sinks( continue; } - if let Err(error) = flush_one_sink(provider, sink, *buffer_id, bytes) { + if sink.parked_at_generation == Some(generation) { + continue; + } + + if let Err(error) = flush_one_sink(provider, sink, *buffer_id, bytes, generation) { failed += 1; log::warn!("EngineServices: {error}"); first_error.get_or_insert(error); @@ -360,6 +398,7 @@ fn flush_one_sink( sink: &mut OutputSinkBinding, buffer_id: RuntimeBufferId, bytes: &[u8], + generation: u64, ) -> Result<(), OutputFlushError> { if bytes.len() % 6 != 0 { return Err(OutputFlushError::MisalignedPayload { @@ -372,7 +411,7 @@ fn flush_one_sink( let led_triplets = u16_payload.len() / 3; let byte_count = (led_triplets as u32).saturating_mul(3).max(3); - ensure_channel_open(provider, sink, byte_count).map_err(|error| { + ensure_channel_open(provider, sink, byte_count, generation).map_err(|error| { OutputFlushError::Provider { endpoint: sink.endpoint.clone(), error, @@ -406,22 +445,40 @@ fn decode_bytes_as_u16_le(bytes: &[u8]) -> Vec { out } +/// Open the sink's channel if it has none, parking it on failure. +/// +/// `generation` is the provider's hardware generation sampled before any open +/// this flush; a sink that fails records it and is skipped until it changes. fn ensure_channel_open( provider: &dyn OutputProvider, sink: &mut OutputSinkBinding, byte_count: u32, + generation: u64, ) -> Result<(), OutputError> { if sink.channel_handle.is_some() { return Ok(()); } let bc = sink.last_byte_count.unwrap_or(3).max(byte_count).max(3); - let handle = provider.open( + let handle = match provider.open( &sink.endpoint, bc, OutputFormat::Ws2811, sink.display_options.clone(), - )?; + ) { + Ok(handle) => handle, + Err(error) => { + sink.parked_at_generation = Some(generation); + return Err(error); + } + }; + + if sink.parked_at_generation.take().is_some() { + log::info!( + "EngineServices: output {} recovered and is writing again", + sink.endpoint + ); + } sink.channel_handle = Some(handle); sink.last_byte_count = Some(bc); Ok(()) @@ -515,6 +572,51 @@ mod tests { assert_eq!(provider.open_channel_count(), 1); } + /// Every tick re-reads every output's authored config, so the unchanged + /// case — which is nearly every one of them — must leave the channel + /// exactly as it found it. Reopening here would drop a live channel and + /// re-claim the pin sixty times a second. + #[test] + fn re_applying_an_unchanged_config_leaves_the_channel_alone() { + let provider = Rc::new(CountingOutputProvider::new(MemoryOutputProvider::new())); + let mut services = EngineServices::new(TreePath::parse("/p.show").expect("tree path")); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone(&provider))))); + + let mut buffers = RuntimeBufferStore::new(); + let buffer_id = output_buffer(&mut buffers, Revision::new(1)); + let config = OutputDef::new(endpoint("ws281x:rmt:D10")); + services.register_output_sink(buffer_id, &config); + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect("initial flush opens the channel"); + let handle = provider + .inner() + .get_handle_for_endpoint(&endpoint("ws281x:rmt:D10")) + .expect("channel handle"); + let generation = provider.hardware_generation(); + + for _ in 0..32 { + services.update_output_sink_config(buffer_id, &config); + } + + assert_eq!( + provider.open_calls(), + 1, + "no reopen for an unchanged config" + ); + assert_eq!( + provider + .inner() + .get_handle_for_endpoint(&endpoint("ws281x:rmt:D10")), + Some(handle) + ); + assert_eq!( + provider.hardware_generation(), + generation, + "an unchanged config must not churn the hardware claim" + ); + } + #[test] fn unregister_output_sink_closes_open_channel() { let provider = Rc::new(MemoryOutputProvider::new()); @@ -646,6 +748,151 @@ mod tests { assert_eq!(provider.open_channel_count(), good.len()); } + /// An endpoint the board does not have can never open, so the flush seam + /// must ask about it once and then wait — not once per frame. + /// + /// Every attempt costs a full enumeration of every endpoint the drivers + /// offer, with a status lookup and a formatted spec per endpoint. On a + /// four-strip project loaded onto a one-strip board that was 45.8% of all + /// cycles, spent entirely on learning the same "no" 60 times a second. + #[test] + fn a_sink_that_cannot_open_is_asked_once_not_once_per_frame() { + let provider = Rc::new(CountingOutputProvider::new( + MemoryOutputProvider::with_hardware_manifest( + lpc_hardware::default_esp32s3_hardware_manifest(), + ), + )); + let mut services = EngineServices::new(TreePath::parse("/p.show").expect("tree path")); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone(&provider))))); + + let mut buffers = RuntimeBufferStore::new(); + let buffer_id = output_buffer(&mut buffers, Revision::new(1)); + services.register_output_sink(buffer_id, &OutputDef::new(endpoint("ws281x:rmt:NOT-A-PIN"))); + + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect_err("the first frame reports the failure"); + assert_eq!(provider.open_calls(), 1); + + for _ in 0..16 { + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect("a parked sink does not keep failing the frame"); + } + + assert_eq!( + provider.open_calls(), + 1, + "a parked sink must not re-attempt while the hardware is unchanged" + ); + } + + /// Parking must not cost recovery: when the pin a sink was waiting for is + /// freed, the very next flush picks it up, with no config change and + /// nothing to poke. + #[test] + fn freeing_the_contended_hardware_lights_the_waiting_sink() { + let provider = Rc::new(CountingOutputProvider::new(MemoryOutputProvider::new())); + let mut services = EngineServices::new(TreePath::parse("/p.show").expect("tree path")); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone(&provider))))); + + let mut buffers = RuntimeBufferStore::new(); + let holder = output_buffer(&mut buffers, Revision::new(1)); + let waiter = output_buffer(&mut buffers, Revision::new(1)); + let held = endpoint("ws281x:rmt:D10"); + let waiting = endpoint("ws281x:rmt:GPIO19"); + services.register_output_sink(holder, &OutputDef::new(held.clone())); + services.register_output_sink(waiter, &OutputDef::new(waiting.clone())); + + // The board has one RMT resource, so exactly one of these opens; drive + // the flush until both have had their attempt. + let _ = services.flush_dirty_output_sinks(Revision::new(1), &buffers); + let (open_sink, parked_sink, parked_endpoint) = if provider.inner().is_endpoint_open(&held) + { + (holder, waiter, waiting.clone()) + } else { + (waiter, holder, held.clone()) + }; + let _ = open_sink; + assert!(!provider.inner().is_endpoint_open(&parked_endpoint)); + + // Releasing the winner's claim is a hardware change, so the parked sink + // gets its retry. + services.unregister_output_sink(open_sink); + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect("the freed resource lets the waiting sink open"); + + assert!( + provider.inner().is_endpoint_open(&parked_endpoint), + "a sink parked on contention must open once the contention clears" + ); + assert!(services.output_sinks.contains_key(&parked_sink)); + } + + /// Re-authoring the endpoint is a new question, so it must be asked at once + /// rather than waiting on a hardware change that may never come. + #[test] + fn re_authoring_a_parked_sink_retries_without_a_hardware_change() { + let provider = Rc::new(CountingOutputProvider::new( + MemoryOutputProvider::with_hardware_manifest( + lpc_hardware::default_esp32s3_hardware_manifest(), + ), + )); + let mut services = EngineServices::new(TreePath::parse("/p.show").expect("tree path")); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone(&provider))))); + + let mut buffers = RuntimeBufferStore::new(); + let buffer_id = output_buffer(&mut buffers, Revision::new(1)); + services.register_output_sink(buffer_id, &OutputDef::new(endpoint("ws281x:rmt:NOT-A-PIN"))); + let _ = services.flush_dirty_output_sinks(Revision::new(1), &buffers); + let generation_before = provider.hardware_generation(); + + let good = endpoint("ws281x:rmt:D10"); + services.update_output_sink_config(buffer_id, &OutputDef::new(good.clone())); + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect("the re-authored endpoint opens"); + + assert_eq!( + provider.hardware_generation(), + generation_before + 1, + "only the successful claim should have moved the generation" + ); + assert!(provider.inner().is_endpoint_open(&good)); + } + + /// A new provider is a new world; nothing the old one refused should keep + /// a sink parked against it. + #[test] + fn swapping_the_provider_unparks_sinks() { + let strict = Rc::new(CountingOutputProvider::new(MemoryOutputProvider::new())); + let mut services = EngineServices::new(TreePath::parse("/p.show").expect("tree path")); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone(&strict))))); + + let mut buffers = RuntimeBufferStore::new(); + let buffer_id = output_buffer(&mut buffers, Revision::new(1)); + // The strict single-RMT board has no such pin; the permissive provider + // that replaces it accepts any endpoint. + let demo = endpoint("ws281x:rmt:D4"); + services.register_output_sink(buffer_id, &OutputDef::new(demo.clone())); + let _ = services.flush_dirty_output_sinks(Revision::new(1), &buffers); + assert_eq!(strict.open_calls(), 1); + + let permissive = Rc::new(CountingOutputProvider::new( + MemoryOutputProvider::new_permissive(), + )); + services.set_output_provider(Some(Box::new(SharedCountingProvider(Rc::clone( + &permissive, + ))))); + services + .flush_dirty_output_sinks(Revision::new(1), &buffers) + .expect("the replacement provider accepts the endpoint"); + + assert_eq!(permissive.open_calls(), 1, "the swap must unpark the sink"); + assert!(permissive.inner().is_endpoint_open(&demo)); + } + fn endpoint(spec: &'static str) -> HwEndpointSpec { HwEndpointSpec::from_static(spec) } @@ -657,6 +904,61 @@ mod tests { )) } + /// Counts `open` calls, which is how the parking tests tell "asked once" + /// from "asked every frame". + struct CountingOutputProvider { + inner: MemoryOutputProvider, + open_calls: core::cell::Cell, + } + + impl CountingOutputProvider { + fn new(inner: MemoryOutputProvider) -> Self { + Self { + inner, + open_calls: core::cell::Cell::new(0), + } + } + + fn inner(&self) -> &MemoryOutputProvider { + &self.inner + } + + fn open_calls(&self) -> usize { + self.open_calls.get() + } + + fn hardware_generation(&self) -> u64 { + self.inner.hardware_generation() + } + } + + struct SharedCountingProvider(Rc); + + impl OutputProvider for SharedCountingProvider { + fn open( + &self, + endpoint: &HwEndpointSpec, + byte_count: u32, + format: OutputFormat, + options: Option, + ) -> Result { + self.0.open_calls.set(self.0.open_calls.get() + 1); + self.0.inner.open(endpoint, byte_count, format, options) + } + + fn write(&self, handle: OutputChannelHandle, data: &[u16]) -> Result<(), OutputError> { + self.0.inner.write(handle, data) + } + + fn close(&self, handle: OutputChannelHandle) -> Result<(), OutputError> { + self.0.inner.close(handle) + } + + fn hardware_generation(&self) -> u64 { + self.0.inner.hardware_generation() + } + } + struct SharedMemoryOutputProvider(Rc); impl OutputProvider for SharedMemoryOutputProvider { @@ -677,5 +979,9 @@ mod tests { fn close(&self, handle: OutputChannelHandle) -> Result<(), OutputError> { self.0.close(handle) } + + fn hardware_generation(&self) -> u64 { + self.0.hardware_generation() + } } } diff --git a/lp-core/lpc-engine/src/engine/output_flush_tests.rs b/lp-core/lpc-engine/src/engine/output_flush_tests.rs index 6cf349fc3..59bda894f 100644 --- a/lp-core/lpc-engine/src/engine/output_flush_tests.rs +++ b/lp-core/lpc-engine/src/engine/output_flush_tests.rs @@ -62,6 +62,10 @@ impl OutputProvider for RcMemoryOutput { fn close(&self, handle: OutputChannelHandle) -> Result<(), lpc_hardware::OutputError> { self.0.close(handle) } + + fn hardware_generation(&self) -> u64 { + self.0.hardware_generation() + } } fn endpoint(spec: &'static str) -> HwEndpointSpec { diff --git a/lp-core/lpc-hardware/src/drivers/ws281x/virtual_ws281x_driver.rs b/lp-core/lpc-hardware/src/drivers/ws281x/virtual_ws281x_driver.rs index 2f0bf0dd0..20f6052e9 100644 --- a/lp-core/lpc-hardware/src/drivers/ws281x/virtual_ws281x_driver.rs +++ b/lp-core/lpc-hardware/src/drivers/ws281x/virtual_ws281x_driver.rs @@ -100,13 +100,26 @@ impl VirtualWs281xDriver { } } + /// The GPIO an endpoint id names, without building the endpoint list. + /// + /// Every entry in that list carries a freshly computed status — several + /// registry lookups each — and none of it is wanted here: the id already + /// determines the address. Walking the manifest directly answers the same + /// question for the cost of a spec string per candidate. fn gpio_for_endpoint( &self, endpoint_id: &HwEndpointId, ) -> Result { - for endpoint in self.endpoints() { - if endpoint.id() == endpoint_id { - return Ok(endpoint.address().clone()); + // A board with no timing resource offers no endpoints at all, so no id + // can belong to this driver. + if !self.timing_addresses.is_empty() { + for resource in self.registry.manifest().resources() { + if !resource.supports(HwCapability::GpioOutput) { + continue; + } + if self.endpoint_id(&ws281x_rmt_spec(resource.display_label())) == *endpoint_id { + return Ok(resource.address().clone()); + } } } diff --git a/lp-core/lpc-hardware/src/hw_system.rs b/lp-core/lpc-hardware/src/hw_system.rs index d58daaf73..2532d06cb 100644 --- a/lp-core/lpc-hardware/src/hw_system.rs +++ b/lp-core/lpc-hardware/src/hw_system.rs @@ -97,10 +97,11 @@ impl HardwareSystem { address: &HwAddress, config: Ws281xConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_address(self.ws281x_endpoints(), address) { - EndpointAddressMatch::Available(endpoint) => self.open_ws281x(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_ws281x(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.ws281x_drivers, |endpoint| { + endpoint.address() == address + }) { + Some((driver, endpoint_id)) => self.ws281x_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Ws281x, endpoint_id: HwEndpointId::new(address.as_str()), }), @@ -112,10 +113,9 @@ impl HardwareSystem { spec: &HwEndpointSpec, config: Ws281xConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_spec(self.ws281x_endpoints(), spec) { - EndpointAddressMatch::Available(endpoint) => self.open_ws281x(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_ws281x(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.ws281x_drivers, |endpoint| endpoint.spec() == spec) { + Some((driver, endpoint_id)) => self.ws281x_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Ws281x, endpoint_id: HwEndpointId::new(spec.as_str()), }), @@ -147,10 +147,11 @@ impl HardwareSystem { address: &HwAddress, config: ButtonConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_address(self.button_endpoints(), address) { - EndpointAddressMatch::Available(endpoint) => self.open_button(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_button(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.button_drivers, |endpoint| { + endpoint.address() == address + }) { + Some((driver, endpoint_id)) => self.button_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Button, endpoint_id: HwEndpointId::new(address.as_str()), }), @@ -162,10 +163,9 @@ impl HardwareSystem { spec: &HwEndpointSpec, config: ButtonConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_spec(self.button_endpoints(), spec) { - EndpointAddressMatch::Available(endpoint) => self.open_button(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_button(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.button_drivers, |endpoint| endpoint.spec() == spec) { + Some((driver, endpoint_id)) => self.button_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Button, endpoint_id: HwEndpointId::new(spec.as_str()), }), @@ -197,10 +197,11 @@ impl HardwareSystem { address: &HwAddress, config: RadioConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_address(self.radio_endpoints(), address) { - EndpointAddressMatch::Available(endpoint) => self.open_radio(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_radio(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.radio_drivers, |endpoint| { + endpoint.address() == address + }) { + Some((driver, endpoint_id)) => self.radio_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Radio, endpoint_id: HwEndpointId::new(address.as_str()), }), @@ -212,10 +213,9 @@ impl HardwareSystem { spec: &HwEndpointSpec, config: RadioConfig, ) -> Result, HardwareEndpointError> { - match endpoint_for_spec(self.radio_endpoints(), spec) { - EndpointAddressMatch::Available(endpoint) => self.open_radio(endpoint.id(), config), - EndpointAddressMatch::Unavailable(endpoint) => self.open_radio(endpoint.id(), config), - EndpointAddressMatch::Missing => Err(HardwareEndpointError::UnknownEndpoint { + match find_endpoint(&self.radio_drivers, |endpoint| endpoint.spec() == spec) { + Some((driver, endpoint_id)) => self.radio_drivers[driver].open(&endpoint_id, config), + None => Err(HardwareEndpointError::UnknownEndpoint { kind: HwEndpointKind::Radio, endpoint_id: HwEndpointId::new(spec.as_str()), }), @@ -256,48 +256,40 @@ where endpoints } -enum EndpointAddressMatch { - Available(HwEndpoint), - Unavailable(HwEndpoint), - Missing, -} - -fn endpoint_for_address(endpoints: Vec, address: &HwAddress) -> EndpointAddressMatch { - let mut first_match = None; - for endpoint in endpoints { - if endpoint.address() != address { - continue; - } - if endpoint.is_available() { - return EndpointAddressMatch::Available(endpoint); - } - if first_match.is_none() { - first_match = Some(endpoint); - } - } - match first_match { - Some(endpoint) => EndpointAddressMatch::Unavailable(endpoint), - None => EndpointAddressMatch::Missing, - } -} - -fn endpoint_for_spec(endpoints: Vec, spec: &HwEndpointSpec) -> EndpointAddressMatch { - let mut first_match = None; - for endpoint in endpoints { - if endpoint.spec() != spec { - continue; - } - if endpoint.is_available() { - return EndpointAddressMatch::Available(endpoint); - } - if first_match.is_none() { - first_match = Some(endpoint); +/// The driver offering the wanted endpoint, as an index into `drivers`, with +/// that endpoint's id. +/// +/// Prefers an available endpoint and otherwise reports the first match, so an +/// endpoint that exists but is claimed still reaches its driver and fails with +/// that driver's own account of why. +/// +/// Enumerating a driver costs a formatted spec and a live status lookup *per +/// endpoint it offers* — on a board declaring every GPIO, hundreds of them. So +/// the walk stops at the first available match and the caller opens on the +/// driver found here, rather than enumerating once to pick an endpoint and +/// again to discover which driver owns it. +fn find_endpoint( + drivers: &[D], + matches: impl Fn(&HwEndpoint) -> bool, +) -> Option<(usize, HwEndpointId)> +where + D: EndpointDriver, +{ + let mut first_match: Option<(usize, HwEndpointId)> = None; + for (index, driver) in drivers.iter().enumerate() { + for endpoint in driver.endpoints() { + if !matches(&endpoint) { + continue; + } + if endpoint.is_available() { + return Some((index, endpoint.id().clone())); + } + if first_match.is_none() { + first_match = Some((index, endpoint.id().clone())); + } } } - match first_match { - Some(endpoint) => EndpointAddressMatch::Unavailable(endpoint), - None => EndpointAddressMatch::Missing, - } + first_match } #[cfg(test)] @@ -440,6 +432,94 @@ mod tests { } } + /// Opening by spec must enumerate a driver once. + /// + /// It used to cost three passes — one to choose the endpoint, one to work + /// out which driver owned the id, and one inside the driver to recover the + /// GPIO — and each pass computes a live status for every endpoint the board + /// declares. That is the difference between a lookup and a survey. + #[test] + fn opening_by_spec_enumerates_the_driver_once() { + struct CountingWs281xDriver { + inner: VirtualWs281xDriver, + enumerations: core::cell::Cell, + } + + impl crate::HwDriver for CountingWs281xDriver { + fn driver_id(&self) -> &str { + self.inner.driver_id() + } + + fn display_label(&self) -> &str { + self.inner.display_label() + } + } + + impl Ws281xDriver for CountingWs281xDriver { + fn endpoints(&self) -> Vec { + self.enumerations.set(self.enumerations.get() + 1); + self.inner.endpoints() + } + + fn open( + &self, + endpoint_id: &HwEndpointId, + config: Ws281xConfig, + ) -> Result, HardwareEndpointError> { + self.inner.open(endpoint_id, config) + } + } + + let registry = Rc::new(HwRegistry::new(HwManifest::virtual_single_rmt_gpio_board())); + let driver = Rc::new(CountingWs281xDriver { + inner: VirtualWs281xDriver::new(Rc::clone(®istry)), + enumerations: core::cell::Cell::new(0), + }); + + struct SharedDriver(Rc); + + impl crate::HwDriver for SharedDriver { + fn driver_id(&self) -> &str { + self.0.driver_id() + } + + fn display_label(&self) -> &str { + self.0.display_label() + } + } + + impl Ws281xDriver for SharedDriver { + fn endpoints(&self) -> Vec { + self.0.endpoints() + } + + fn open( + &self, + endpoint_id: &HwEndpointId, + config: Ws281xConfig, + ) -> Result, HardwareEndpointError> { + self.0.open(endpoint_id, config) + } + } + + let mut system = HardwareSystem::new(Rc::clone(®istry)); + system.add_ws281x_driver(Box::new(SharedDriver(Rc::clone(&driver)))); + + let output = system + .open_ws281x_by_spec( + &HwEndpointSpec::from_static("ws281x:rmt:D10"), + Ws281xConfig::new(3), + ) + .expect("D10 opens"); + + assert_eq!( + driver.enumerations.get(), + 1, + "one open should survey the board once" + ); + drop(output); + } + fn test_manifest() -> HwManifest { HwManifest::new( "test", diff --git a/lp-core/lpc-hardware/src/lib.rs b/lp-core/lpc-hardware/src/lib.rs index e7d4a5c17..00d74f1bf 100644 --- a/lp-core/lpc-hardware/src/lib.rs +++ b/lp-core/lpc-hardware/src/lib.rs @@ -11,6 +11,12 @@ //! lists endpoints, and opens an endpoint by authored [`HwEndpointSpec`], //! internal [`HwEndpointId`], or physical [`HwAddress`]. //! +//! Listing endpoints is not cheap — each one carries a freshly computed status +//! — so a caller that has been refused an endpoint should wait rather than ask +//! again. [`HwRegistry::generation`] is the signal to wait on: it changes when +//! ownership moves, which is the only thing that can turn a refusal into an +//! acceptance. See `docs/adr/2026-07-31-output-sink-retry-policy.md`. +//! //! Rendering and protocol-adjacent color processing live above this crate. For //! example, [`Ws281xOutput`] accepts already-rendered RGB bytes; display //! pipeline options remain in `lpc-shared`. diff --git a/lp-core/lpc-hardware/src/manifest/hw_manifest.rs b/lp-core/lpc-hardware/src/manifest/hw_manifest.rs index 468845da9..ed7c636c5 100644 --- a/lp-core/lpc-hardware/src/manifest/hw_manifest.rs +++ b/lp-core/lpc-hardware/src/manifest/hw_manifest.rs @@ -67,6 +67,58 @@ impl HwManifest { .with_description("Virtual board profile for tests and emulation with GPIO resources, one shared WS281x/RMT resource, and one radio endpoint.") } + /// Virtual board with four WS281x channels, as the XIAO ESP32-S3 Plus has. + /// + /// [`Self::virtual_single_rmt_gpio_board`] declares one timing resource, so + /// a four-strip project running on it can only ever light one strip: the + /// other three outputs fail to open and stay failed. That is a fine board + /// for testing contention, and several tests rely on it for exactly that, + /// but it makes the emulator disagree with the hardware it stands in for — + /// on the desk S3 all four strips light. + /// + /// This board is the single-RMT one plus what the S3 has that it lacked: + /// three more timing resources, and the `D9`/`D8`/`D7` labels the four-strip + /// projects address. The GPIO numbers behind those three labels are the + /// S3's own. `D10` keeps GPIO 18 rather than the S3's GPIO 9, because the + /// virtual board has always put it there and nothing is gained by moving + /// it; this is a stand-in, not a model of the real pinout. + pub fn virtual_quad_rmt_gpio_board() -> Self { + let mut resources = Vec::new(); + for pin in 0..=255 { + let display_label = match pin { + 18 => alloc::format!("D10"), + 8 => alloc::format!("D9"), + 7 => alloc::format!("D8"), + 44 => alloc::format!("D7"), + _ => alloc::format!("GPIO{pin}"), + }; + resources.push(HwResource::new( + HwAddress::gpio(pin), + [HwCapability::GpioOutput, HwCapability::GpioInput], + display_label, + )); + } + for channel in 0..4 { + resources.push(HwResource::new( + HwAddress::rmt_ws281x(channel), + [HwCapability::Rmt, HwCapability::Ws281xOutput], + alloc::format!("RMT WS281x {channel}"), + )); + } + resources.push(HwResource::new( + HwAddress::radio(0), + [HwCapability::Radio], + "Virtual Radio 0", + )); + Self::new("virtual-quad-rmt", "Virtual Quad-RMT Board", resources) + .with_target(HardwareTarget::Rv32imacEmu) + .with_description( + "Virtual board profile for tests and emulation with GPIO resources, four \ + WS281x/RMT timing resources matching the XIAO ESP32-S3 Plus, and one radio \ + endpoint.", + ) + } + pub fn board_id(&self) -> &str { &self.board_id } diff --git a/lp-core/lpc-hardware/src/registry/hw_registry.rs b/lp-core/lpc-hardware/src/registry/hw_registry.rs index 3da5544a9..8c94fef79 100644 --- a/lp-core/lpc-hardware/src/registry/hw_registry.rs +++ b/lp-core/lpc-hardware/src/registry/hw_registry.rs @@ -1,5 +1,5 @@ use alloc::string::{String, ToString}; -use core::cell::RefCell; +use core::cell::{Cell, RefCell}; use lp_collection::{VecMap, VecSet}; use crate::{ @@ -12,10 +12,23 @@ use crate::{ /// The registry validates capabilities against the [`HwManifest`], tracks active /// [`HardwareLease`]s, and reports endpoint status for drivers. It uses interior /// mutability so shared driver handles can coordinate claims in `no_std` code. +/// +/// # Generation +/// +/// [`HwRegistry::generation`] is the registry's change signal: it changes +/// whenever a claim succeeds or a lease is released, which is exactly when an +/// endpoint that could not be opened might now open (or the reverse). Consumers +/// compare it for *inequality* only — that it currently counts upward is an +/// implementation detail, not a promise. +/// +/// Only ownership needs signalling. Reserved status comes from the manifest, +/// which is immutable after construction, and capability support with it, so +/// neither can change under a holder of this registry. #[derive(Debug)] pub struct HwRegistry { manifest: HwManifest, state: RefCell, + generation: Cell, } #[derive(Debug, Clone)] @@ -39,6 +52,10 @@ impl HwRegistry { active_by_address: VecMap::new(), addresses_by_lease: VecMap::new(), }), + // Starts nonzero so a consumer whose "last seen" value defaults to + // zero sees an initial change rather than mistaking a fresh + // registry for one it has already observed. + generation: Cell::new(1), } } @@ -46,6 +63,21 @@ impl HwRegistry { &self.manifest } + /// Change signal for hardware ownership; see the type-level docs. + pub fn generation(&self) -> u64 { + self.generation.get() + } + + /// Bump after a claim or release actually changed ownership. + /// + /// Only successful mutations bump. A *failed* claim changes nothing + /// observable, and signalling it would let one permanently-failing open + /// retrigger every parked consumer on every attempt — the retry storm this + /// signal exists to end. + fn bump_generation(&self) { + self.generation.set(self.generation.get().wrapping_add(1)); + } + pub fn claim_bundle(&self, claim: HwClaim) -> Result { self.validate_claim(&claim)?; @@ -64,6 +96,8 @@ impl HwRegistry { addresses.insert(address.clone()); } state.addresses_by_lease.insert(lease_id, addresses); + drop(state); + self.bump_generation(); Ok(HardwareLease::new( lease_id, @@ -85,6 +119,8 @@ impl HwRegistry { for address in addresses { state.active_by_address.remove(&address); } + drop(state); + self.bump_generation(); Ok(()) } @@ -255,6 +291,102 @@ mod tests { assert!(matches!(result, Err(HwError::ReservedResource { .. }))); } + /// Reads must not look like changes: the generation is what parks a + /// consumer that could not open an endpoint, so a registry nobody has + /// mutated must keep answering with the same value no matter how often it + /// is interrogated. + #[test] + fn reads_do_not_change_the_generation() { + let registry = registry(); + let before = registry.generation(); + + registry.endpoint_status_for(&HwAddress::gpio(18)); + registry.endpoint_status_for(&HwAddress::gpio(999)); + registry.is_claimed(&HwAddress::gpio(18)); + registry.claimant_for(&HwAddress::gpio(18)); + registry + .ensure_capability(&HwAddress::gpio(18), HwCapability::GpioOutput) + .unwrap(); + + assert_eq!(registry.generation(), before); + } + + #[test] + fn successful_claim_and_release_each_change_the_generation() { + let registry = registry(); + let fresh = registry.generation(); + + let lease = registry + .claim_bundle(HwClaim::new("output", vec![HwAddress::gpio(18)])) + .unwrap(); + let claimed = registry.generation(); + assert_ne!(claimed, fresh, "claim should signal a change"); + + registry.release(&lease).unwrap(); + let released = registry.generation(); + assert_ne!(released, claimed, "release should signal a change"); + assert_ne!(released, fresh); + } + + /// A failed claim changes nothing, so it must signal nothing. If it did, + /// a permanently unopenable endpoint would unpark every other waiting + /// consumer on each attempt and the per-frame retry storm would return. + #[test] + fn failed_claims_and_releases_leave_the_generation_alone() { + let manifest = HwManifest::new( + "board", + "Board", + [ + HwResource::new(HwAddress::gpio(18), [HwCapability::GpioOutput], "D6"), + HwResource::new(HwAddress::gpio(12), [HwCapability::GpioOutput], "GPIO12") + .reserved("crashes during GPIO scan"), + ], + ); + let registry = HwRegistry::new(manifest); + let held = registry + .claim_bundle(HwClaim::new("holder", vec![HwAddress::gpio(18)])) + .unwrap(); + let before = registry.generation(); + + assert!( + registry + .claim_bundle(HwClaim::new("empty", vec![])) + .is_err() + ); + assert!( + registry + .claim_bundle(HwClaim::new("reserved", vec![HwAddress::gpio(12)])) + .is_err() + ); + assert!( + registry + .claim_bundle(HwClaim::new("taken", vec![HwAddress::gpio(18)])) + .is_err() + ); + assert!( + registry + .claim_bundle(HwClaim::new( + "dupe", + vec![HwAddress::gpio(12), HwAddress::gpio(12)], + )) + .is_err() + ); + assert!( + registry + .claim_bundle(HwClaim::new("unknown", vec![HwAddress::gpio(200)])) + .is_err() + ); + + assert_eq!(registry.generation(), before); + + // A lease released twice: the second call finds no lease and must not + // signal either. + registry.release(&held).unwrap(); + let after_release = registry.generation(); + assert!(registry.release(&held).is_err()); + assert_eq!(registry.generation(), after_release); + } + #[test] fn unsupported_capability_fails() { let registry = registry(); diff --git a/lp-core/lpc-shared/src/output/memory.rs b/lp-core/lpc-shared/src/output/memory.rs index 483f12ec3..0bbe9be59 100644 --- a/lp-core/lpc-shared/src/output/memory.rs +++ b/lp-core/lpc-shared/src/output/memory.rs @@ -246,6 +246,13 @@ impl OutputProvider for MemoryOutputProvider { Ok(()) } + + fn hardware_generation(&self) -> u64 { + // Permissive mode opens nothing against the registry, so its answer is + // simply constant — which is the right answer there: a permissive + // failure is about the caller's own config, not about ownership. + self.hardware_system.registry().generation() + } } impl MemoryOutputProvider { @@ -380,6 +387,35 @@ mod tests { assert_eq!(provider.get_data(handle), Some(vec![1, 2, 3])); } + /// The generation is what tells a parked caller to try again, so it must + /// move when a channel claims or frees hardware — and must not move when + /// the channel is merely being written to, which happens every frame. + #[test] + fn hardware_generation_tracks_claims_not_writes() { + let provider = MemoryOutputProvider::new(); + let before_open = provider.hardware_generation(); + + let handle = provider + .open(&endpoint("ws281x:rmt:D10"), 3, OutputFormat::Ws2811, None) + .expect("output opens"); + let after_open = provider.hardware_generation(); + assert_ne!(after_open, before_open, "claiming hardware is a change"); + + provider.write(handle, &[1, 2, 3]).expect("write succeeds"); + assert_eq!( + provider.hardware_generation(), + after_open, + "writing frames must not look like a hardware change" + ); + + provider.close(handle).expect("output closes"); + assert_ne!( + provider.hardware_generation(), + after_open, + "releasing hardware is a change" + ); + } + #[test] fn opening_two_outputs_on_different_pins_contends_for_rmt() { let provider = MemoryOutputProvider::new(); diff --git a/lp-core/lpc-shared/src/output/provider.rs b/lp-core/lpc-shared/src/output/provider.rs index 786d811fc..7cd72ea92 100644 --- a/lp-core/lpc-shared/src/output/provider.rs +++ b/lp-core/lpc-shared/src/output/provider.rs @@ -76,4 +76,25 @@ pub trait OutputProvider { /// # Returns /// Returns `Ok(())` on success, or `OutputError` if handle is invalid fn close(&self, handle: OutputChannelHandle) -> Result<(), OutputError>; + + /// Change signal for endpoint availability. + /// + /// The value changes whenever an endpoint that refused to [`open`] might + /// now accept — that is, whenever hardware ownership moved. Callers + /// compare it for inequality only; nothing about ordering or magnitude is + /// promised. + /// + /// This exists so a caller holding a failed endpoint can wait instead of + /// asking: re-attempting an open costs a full enumeration of the board, + /// and a sink whose pin is simply not there would pay that on every frame + /// forever. + /// + /// The default answers "this hardware never changes", which is correct for + /// providers that own no registry: their failures are permanent until the + /// caller's own configuration changes. + /// + /// [`open`]: OutputProvider::open + fn hardware_generation(&self) -> u64 { + 0 + } } diff --git a/lp-fw/fw-emu/src/main.rs b/lp-fw/fw-emu/src/main.rs index d22c15295..de2adbcd2 100644 --- a/lp-fw/fw-emu/src/main.rs +++ b/lp-fw/fw-emu/src/main.rs @@ -120,7 +120,10 @@ pub extern "C" fn _lp_main() -> ! { // Create filesystem (in-memory) let base_fs = alloc::boxed::Box::new(LpFsMemory::new()); - let hardware_registry = Rc::new(HwRegistry::new(HwManifest::virtual_single_rmt_gpio_board())); + // Four timing channels, as the desk S3 has: a four-strip project should + // light four strips here too, rather than one plus three endpoints that + // never open. + let hardware_registry = Rc::new(HwRegistry::new(HwManifest::virtual_quad_rmt_gpio_board())); let hardware_system = Rc::new(HardwareSystem::with_virtual_drivers(hardware_registry)); // Create output provider diff --git a/lp-fw/fw-emu/src/output.rs b/lp-fw/fw-emu/src/output.rs index bb760a974..772115a66 100644 --- a/lp-fw/fw-emu/src/output.rs +++ b/lp-fw/fw-emu/src/output.rs @@ -107,6 +107,10 @@ impl OutputProvider for SyscallOutputProvider { println!("[output] close: handle={:?}", handle); Ok(()) } + + fn hardware_generation(&self) -> u64 { + self.hardware_system.registry().generation() + } } impl SyscallOutputProvider { diff --git a/lp-fw/fw-esp32-common/src/output/provider.rs b/lp-fw/fw-esp32-common/src/output/provider.rs index ab3f7f18a..81e5ae590 100644 --- a/lp-fw/fw-esp32-common/src/output/provider.rs +++ b/lp-fw/fw-esp32-common/src/output/provider.rs @@ -166,6 +166,10 @@ impl OutputProvider for Esp32OutputProvider { .ok_or_else(|| OutputError::InvalidHandle { handle: handle_id })?; Ok(()) } + + fn hardware_generation(&self) -> u64 { + self.hardware_system.registry().generation() + } } fn capped_byte_count_for_len(data_len: usize) -> (u32, bool) {