From 6c330dd92d08a1d7f398b087785bbc153d7923f0 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:12:09 -0700 Subject: [PATCH 1/8] feat(lpc-hardware): a registry that can say when ownership changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Endpoint availability changes on exactly two events — a claim succeeds, or a lease is released — but the registry had no way to say so. Consumers that wanted to know were left to ask again and again, which is how the output flush seam ended up re-enumerating every endpoint on the board every frame for a sink that could never open. The counter starts nonzero so a consumer defaulting its "last seen" to zero sees a change instead of mistaking a fresh registry for a familiar one, and only successful mutations bump it: a failed claim changes nothing anyone can observe, and signalling it would let one hopeless endpoint wake every parked consumer on every attempt. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- .../lpc-hardware/src/registry/hw_registry.rs | 130 +++++++++++++++++- 1 file changed, 129 insertions(+), 1 deletion(-) diff --git a/lp-core/lpc-hardware/src/registry/hw_registry.rs b/lp-core/lpc-hardware/src/registry/hw_registry.rs index 3da5544a9..32a786908 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,98 @@ 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(); From 7a3da96483454e9d6a7f63204daabd33e9be4df3 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:22:13 -0700 Subject: [PATCH 2/8] perf(lpc-engine): let a failed output sink wait instead of asking every frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sink that cannot open — a project authored for four strips loaded onto a one-strip board, a pin another node holds — re-attempted the open on every flush, and every attempt enumerates every endpoint every driver offers, each one built with a formatted spec, parsed, validated, and given a freshly computed status. On quad-strips under the emulator that was 45.8% of all cycles spent relearning the same "no" sixty times a second, with a warning logged each time. Sinks now park on the registry's generation and are skipped until hardware ownership actually moves. Recovery is no slower for it: releasing a pin bumps the generation and the very next flush picks the waiting sink up. The generation is sampled before any open in the flush, so a claim this frame's own opens made cannot park a sink against a number that already counts it. Nothing caches endpoint status. The tempting version of this change — memoize what the registry answered — optimizes a query the success path never makes and would hand two owners the same pin the first time a cached "available" went stale. ADR: docs/adr/2026-07-31-output-sink-retry-policy.md Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- .../2026-07-31-output-sink-retry-policy.md | 105 +++++++ lp-app/lpa-server/src/project.rs | 4 + .../lpc-engine/src/engine/engine_services.rs | 263 +++++++++++++++++- .../src/engine/output_flush_tests.rs | 4 + .../lpc-hardware/src/registry/hw_registry.rs | 6 +- lp-core/lpc-shared/src/output/memory.rs | 36 +++ lp-core/lpc-shared/src/output/provider.rs | 21 ++ lp-fw/fw-emu/src/output.rs | 4 + lp-fw/fw-esp32-common/src/output/provider.rs | 4 + 9 files changed, 442 insertions(+), 5 deletions(-) create mode 100644 docs/adr/2026-07-31-output-sink-retry-policy.md 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/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_services.rs b/lp-core/lpc-engine/src/engine/engine_services.rs index 36abde50c..560c582c2 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,6 +203,7 @@ impl EngineServices { display_options, channel_handle: None, last_byte_count: None, + parked_at_generation: None, }, ); } @@ -212,6 +229,9 @@ impl EngineServices { existing.endpoint = endpoint; 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 +341,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 +363,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 +393,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 +406,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 +440,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(()) @@ -646,6 +698,150 @@ 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 +853,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 +928,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/registry/hw_registry.rs b/lp-core/lpc-hardware/src/registry/hw_registry.rs index 32a786908..8c94fef79 100644 --- a/lp-core/lpc-hardware/src/registry/hw_registry.rs +++ b/lp-core/lpc-hardware/src/registry/hw_registry.rs @@ -348,7 +348,11 @@ mod tests { .unwrap(); let before = registry.generation(); - assert!(registry.claim_bundle(HwClaim::new("empty", vec![])).is_err()); + assert!( + registry + .claim_bundle(HwClaim::new("empty", vec![])) + .is_err() + ); assert!( registry .claim_bundle(HwClaim::new("reserved", vec![HwAddress::gpio(12)])) 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/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 0529f1e87..c24034735 100644 --- a/lp-fw/fw-esp32-common/src/output/provider.rs +++ b/lp-fw/fw-esp32-common/src/output/provider.rs @@ -150,6 +150,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 { From fd1572e1d80740d9965a5caf8bb01c632b3da4d6 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:24:48 -0700 Subject: [PATCH 3/8] perf(lpc-hardware): one survey of the board per open, not three MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening an endpoint by spec enumerated the drivers three times over: once to choose the endpoint, once to work out which driver owned the id it returned, and once more inside the driver to recover the GPIO the id already named. Each pass builds every endpoint the board declares, and each endpoint costs a formatted spec, a parse, and a live status made of several registry lookups — on a board declaring every GPIO, hundreds of them per pass. The by-spec and by-address opens now walk the drivers once, stopping at the first available match, and open on the driver they found it in. The virtual driver answers "which GPIO is this?" from the manifest instead of from a list of statuses it does not use. Preference and error selection are unchanged: an available endpoint still wins over a claimed one, a claimed one still reaches its driver so the failure comes with that driver's account of why, and a missing one is still UnknownEndpoint. A new test holds the survey to a single pass. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- .../drivers/ws281x/virtual_ws281x_driver.rs | 19 +- lp-core/lpc-hardware/src/hw_system.rs | 208 ++++++++++++------ 2 files changed, 160 insertions(+), 67 deletions(-) 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", From b31f20a1daba14027de1176be8fdf0ac6caac289 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:28:18 -0700 Subject: [PATCH 4/8] perf(lpc-engine): stop cloning every output definition every frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-reading the authored output configs each tick cloned every output's definition — endpoint spec and all — purely to escape a borrow, then almost always discovered nothing had changed and threw the copies away. On silicon that is a heap allocation per output per frame for no result. The tree and the services are now borrowed as the separate fields they are, so the defs can be read in place while the sinks are updated, and the comparison borrows the authored endpoint rather than copying it first. Only a genuine change pays for a copy. The dead `loaded_node_def_for_entry` goes with it: the loop calls the same lookup it wrapped, and leaving an unused method behind to be greeted by a warning helps nobody. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- lp-core/lpc-engine/src/engine/engine.rs | 38 +++++++------- .../lpc-engine/src/engine/engine_services.rs | 52 +++++++++++++++++-- 2 files changed, 69 insertions(+), 21 deletions(-) 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 560c582c2..cc32db8d5 100644 --- a/lp-core/lpc-engine/src/engine/engine_services.rs +++ b/lp-core/lpc-engine/src/engine/engine_services.rs @@ -208,14 +208,19 @@ impl EngineServices { ); } + /// 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; @@ -226,7 +231,7 @@ 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 @@ -567,6 +572,47 @@ 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()); From c308b6e04a4d2bbed86128862c9297b60c5fd1fa Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:34:08 -0700 Subject: [PATCH 5/8] test(fw-emu): give the emulator the four WS281x channels the S3 has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The emulator ran on a board declaring a single timing resource, so a four-strip project lit one strip there and left three endpoints that could never open — while the same project on the desk S3 lights all four. Every emulator profile of a multi-output project was therefore measuring a failure path that silicon does not take. The new board is the old one plus exactly what it lacked: three more timing resources and the D9/D8/D7 labels four-strip projects address, using the S3's own GPIO numbers for them. D10 keeps GPIO 18, where the virtual board has always had it. The single-RMT board stays as it is — several tests want a board where outputs contend, and that is a fair thing to test. Deliberately sequenced after the measurement: making the endpoints exist first would have made the retry cost vanish without anything being fixed. Profiled after the swap, the steady frame is 1.554M cycles against 1.534M before it — the difference is three more strips actually being written. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- .../lpc-hardware/src/manifest/hw_manifest.rs | 52 +++++++++++++++++++ lp-fw/fw-emu/src/main.rs | 5 +- 2 files changed, 56 insertions(+), 1 deletion(-) 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-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 From 7170d56ae8ba0a79652aa2796e6f3df9c491c75c Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:35:55 -0700 Subject: [PATCH 6/8] docs: record the endpoint-status half of the S3 frame-cost debt as closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The debt entry predicted the profile would end up dominated by the resolver once endpoint status stopped being recomputed per frame, and it is: memcpy, the allocator, QueryKey and SlotPath::parse are what remains. Says so, names the sibling plan that owns them, and points at the profile that is now their baseline. Also records what the fix actually was, since the entry's framing — "make endpoint status persist across frames" — turned out to describe the wrong mechanism. Nothing persists; the sinks simply stopped asking. The desk-S3 re-measurement the exit criteria call for is recorded as not done: the board was not attached. Left explicit rather than quietly dropped. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- docs/debt/s3-frame-cost-scales-per-fixture.md | 39 +++++++++++++++++++ lp-core/lpc-hardware/src/lib.rs | 6 +++ 2 files changed, 45 insertions(+) diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index b6c64395c..b114a468c 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -68,6 +68,45 @@ 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 fps not re-measured**: the S3 (d8:3b:da:47:29:70) was not attached + (only an esp32c6 and one unresponsive port; identified via + `espflash board-info`, not auto-picked). Expected ~flat regardless — the S3 + opens all four channels on frame one and so never paid this cost in steady + state. What silicon gains is that a *misconfigured* output no longer costs a + board enumeration and a serial log line every frame it stays wrong. + + **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-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`. From d5679f93d148004e3703fef1106b8ac27b856d71 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:47:13 -0700 Subject: [PATCH 7/8] style: rustfmt the new flush-seam tests Co-Authored-By: Claude Opus 5 --- lp-core/lpc-engine/src/engine/engine_services.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lp-core/lpc-engine/src/engine/engine_services.rs b/lp-core/lpc-engine/src/engine/engine_services.rs index cc32db8d5..d7702b76c 100644 --- a/lp-core/lpc-engine/src/engine/engine_services.rs +++ b/lp-core/lpc-engine/src/engine/engine_services.rs @@ -599,7 +599,11 @@ mod tests { services.update_output_sink_config(buffer_id, &config); } - assert_eq!(provider.open_calls(), 1, "no reopen for an unchanged config"); + assert_eq!( + provider.open_calls(), + 1, + "no reopen for an unchanged config" + ); assert_eq!( provider .inner() @@ -803,7 +807,8 @@ mod tests { // 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) { + let (open_sink, parked_sink, parked_endpoint) = if provider.inner().is_endpoint_open(&held) + { (holder, waiter, waiting.clone()) } else { (waiter, holder, held.clone()) From fbb13c6c60e8435f592666318a8e7f9cd5c969b6 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Sat, 1 Aug 2026 07:29:51 -0700 Subject: [PATCH 8/8] docs(debt): close the S3 half of the frame-cost measurement on silicon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The desk S3 was attached the day after the fix landed, so the measurement the exit criteria asked for is no longer an IOU. quad-strips holds 20 fps at a 48 ms tick — flat, as predicted: the S3 opens all four channels on frame one and so never paid the retry cost that dominated the emulator. A flat number cannot prove which image produced it, so the parked-sink path was exercised on the board instead: one output aimed at a pin that does not exist gives two warnings and then silence across ~1,250 frames, where the old code would have logged one per frame. That identifies the build and demonstrates the fix on silicon at the same time. Plan: 2026-07-31-2224-hw-endpoint-status-cache Co-Authored-By: Claude Opus 5 --- docs/debt/s3-frame-cost-scales-per-fixture.md | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index b114a468c..a1338948a 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -95,12 +95,27 @@ nothing else helps, by measurement. S3's four channels (steady frame 1.554M — the +1.3% is three more strips actually being written). - **Desk-S3 fps not re-measured**: the S3 (d8:3b:da:47:29:70) was not attached - (only an esp32c6 and one unresponsive port; identified via - `espflash board-info`, not auto-picked). Expected ~flat regardless — the S3 - opens all four channels on frame one and so never paid this cost in steady - state. What silicon gains is that a *misconfigured* output no longer costs a - board enumeration and a serial log line every frame it stays wrong. + **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%,