From f0abe00c2fbad6771f0277bc804ea2557b042e69 Mon Sep 17 00:00:00 2001 From: DRSDavidSoft <4673812+DRSDavidSoft@users.noreply.github.com> Date: Mon, 19 Oct 2026 21:53:34 +0330 Subject: [PATCH 1/2] fix: prevent AVR strip sender register aliasing --- .github/workflows/firmware.yml | 5 ++ Project/AddressableLeds.cpp | 9 ++- Tools/Firmware/verify-strip-waveform.mjs | 91 ++++++++++++++++++++++++ docs/STRIP-STREAMING.md | 22 ++++++ 4 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 Tools/Firmware/verify-strip-waveform.mjs diff --git a/.github/workflows/firmware.yml b/.github/workflows/firmware.yml index 11b6405e..d3615966 100644 --- a/.github/workflows/firmware.yml +++ b/.github/workflows/firmware.yml @@ -101,6 +101,11 @@ jobs: --clean --version "$PCCONTROLLER_VERSION" + - name: "Verify linked strip output waveform" + run: >- + node Tools/Firmware/verify-strip-waveform.mjs + .build/firmware/PCController.ino_atmega328p_16000000L.lst + - name: "💾 Assert the safe 1 KiB default EEPROM image" run: >- node .github/scripts/assert-firmware-defaults.mjs diff --git a/Project/AddressableLeds.cpp b/Project/AddressableLeds.cpp index 153baf23..86c08ee6 100644 --- a/Project/AddressableLeds.cpp +++ b/Project/AddressableLeds.cpp @@ -92,9 +92,12 @@ void show() { "nop" "\n\t" "sbiw %[count], 1" "\n\t" "brne head20%=" "\n" - : [port] "+e"(port), [byte] "+r"(byte), [bit] "+r"(bit), - [next] "+r"(next), [count] "+w"(count) - : [ptr] "e"(ptr), [hi] "r"(hi), [lo] "r"(lo)); + // The loop overwrites these operands before its final use of hi/lo/port. + // Early-clobber prevents next (initially lo) from sharing lo's register. + : [byte] "+&r"(byte), [bit] "+&d"(bit), [next] "+&r"(next), + [count] "+&w"(count), [ptr] "+&e"(ptr) + : [port] "e"(port), [hi] "r"(hi), [lo] "r"(lo) + : "memory"); SREG = savedSreg; _delay_us(80); #elif defined(PCCONTROLLER_NATIVE) diff --git a/Tools/Firmware/verify-strip-waveform.mjs b/Tools/Firmware/verify-strip-waveform.mjs new file mode 100644 index 00000000..9a1504f6 --- /dev/null +++ b/Tools/Firmware/verify-strip-waveform.mjs @@ -0,0 +1,91 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +// Exercise the actual linked AVR bit loop, including compiler register allocation. +import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' + +const listingPath = process.argv[2] +if (!listingPath) throw new Error('Usage: node Tools/Firmware/verify-strip-waveform.mjs AVR-LISTING.lst') +const listing = readFileSync(listingPath, 'utf8') +const start = listing.match(/^([0-9a-f]+) :/m) +assert.ok(start, 'linked strip bit-loop label is missing') +const instructions = [] +for (const line of listing.slice(start.index).split('\n')) { + const match = line.trimEnd().match(/^\s*([0-9a-f]+):\s+(?:(?:[0-9a-f]{2})\s+)+\s*([a-z]+)\s*([^;]*)(?:;\s*(.*))?$/) + if (!match) continue + const [, address, op, operandText, comment = ''] = match + const target = comment.match(/^0x([0-9a-f]+)/) + instructions.push({ address: parseInt(address, 16), op, args: operandText.trim().split(/\s*,\s*/), target: target && parseInt(target[1], 16) }) + if (op === 'brne') break +} +assert.equal(instructions.at(-1)?.op, 'brne', 'complete strip byte loop is required') +const byAddress = new Map(instructions.map((instruction, index) => [instruction.address, index])) +const hi = instructions[0].args[1] +const byte = instructions.find(i => i.op === 'sbrc').args[0] +const bit = instructions.find(i => i.op === 'dec').args[0] +const moves = instructions.filter(i => i.op === 'mov') +const next = moves[0].args[0] +const lo = moves[1].args[1] +const count = instructions.find(i => i.op === 'sbiw').args[0] +assert.notEqual(next, lo, `compiler aliased changing bit operand and constant low operand (${lo})`) +assert.equal(new Set([hi, lo, byte, bit, next, count]).size, 6, 'loop operands must have independent registers') + +function verify(bytes) { + const registers = { [hi]: 64, [lo]: 0, [byte]: bytes[0], [bit]: 8, [next]: 0, [count]: bytes.length } + let pointer = 1, pc = 0, cycles = 0, zero = false, carry = 0, level = 0 + const rises = [], falls = [] + for (let steps = 0; pc < instructions.length; steps++) { + assert.ok(steps < bytes.length * 8 * 30, 'strip loop did not terminate') + const instruction = instructions[pc] + const [a, b] = instruction.args + let advance = 1, elapsed = 1 + switch (instruction.op) { + case 'st': { + elapsed = 2 + const newLevel = registers[b] & 64 + if (newLevel !== level) (newLevel ? rises : falls).push(cycles + elapsed) + level = newLevel + break + } + case 'sbrc': if (!(registers[a] & (1 << Number(b)))) { advance = 2; elapsed = 2 }; break + case 'mov': registers[a] = registers[b]; break + case 'dec': registers[a] = (registers[a] - 1) & 255; zero = registers[a] === 0; break + case 'breq': + case 'brne': + if (instruction.op === 'breq' ? zero : !zero) { + advance = byAddress.get(instruction.target) - pc + elapsed = 2 + } + break + case 'adc': { + const sum = registers[a] + registers[b] + carry + registers[a] = sum & 255 + carry = sum > 255 ? 1 : 0 + break + } + case 'rjmp': advance = byAddress.get(instruction.target) - pc; elapsed = 2; break + case 'nop': break + case 'ldi': registers[a] = Number(b); break + case 'ld': registers[a] = bytes[pointer++] ?? 0; elapsed = 2; break + case 'sbiw': registers[a]--; zero = registers[a] === 0; elapsed = 2; break + default: throw new Error(`Unmodeled AVR instruction ${instruction.op}`) + } + assert.ok(Number.isFinite(advance), `invalid branch target at ${instruction.address.toString(16)}`) + cycles += elapsed + pc += advance + } + assert.equal(level, 0, 'data pin must finish low') + assert.equal(rises.length, bytes.length * 8, 'one rising edge per data bit') + assert.equal(falls.length, rises.length, 'one falling edge per data bit') + for (let i = 0; i < rises.length; i++) { + const value = (bytes[i >> 3] >> (7 - (i & 7))) & 1 + assert.equal(rises[i] - rises[0], i * 20, `bit ${i} period`) + assert.equal(falls[i] - rises[i], value ? 13 : 5, `bit ${i} high width`) + } + assert.equal(pointer, bytes.length + 1, 'byte pointer must advance across the full frame') +} + +// Every byte value, transitions both ways, and full 100-pixel frame length. +for (let value = 0; value < 256; value++) verify([value, value ^ 255, value]) +verify(Array.from({ length: 300 }, (_, i) => i & 255)) +console.log('PASS: linked AVR strip loop: 256 byte patterns, byte boundaries, 300-byte frame; 20-cycle cells and 5/13-cycle high pulses.') diff --git a/docs/STRIP-STREAMING.md b/docs/STRIP-STREAMING.md index 6ca573f9..43b09eae 100644 --- a/docs/STRIP-STREAMING.md +++ b/docs/STRIP-STREAMING.md @@ -68,3 +68,25 @@ events remain concurrent because they do not write a host request. The 100-pixel limit is intentional on the AVR SRAM budget. Higher counts require a firmware target/profile with enough memory and a matching advertised limit; the UI and native encoder reject counts above this build's maximum. + +## Checking the compiled sender + +Acknowledged frames do not prove electrical output. The AVR assembly loop must +declare operands overwritten before the last input use as early-clobber outputs. +In particular, `next` starts equal to `lo`; without early-clobber, GCC can assign +both to the same register. The first one bit then replaces the constant low +value with high, and later writes cannot bring the data pin low. A memory clobber +alone does not prevent this register alias. The advancing frame pointer must +also be a read/write operand, and the buffer reads require a memory clobber. + +After compiling, exercise the actual linked instructions with: + +```console +node Tools/Firmware/verify-strip-waveform.mjs .build/firmware/PCController.ino_atmega328p_16000000L.lst +``` + +The firmware CI job runs this gate. It checks register independence, every byte +value, byte boundaries, and a 300-byte frame, including 20-cycle cells and the +existing 5/13-cycle high pulses. The test rejects unrecognized instruction +sequences instead of assuming their timing. Physical LED acceptance remains a +separate check using the host's `strip rainbow 100 20` command. From 7374de431d9b1bd9f5850852a15cf9cb2edd7f77 Mon Sep 17 00:00:00 2001 From: DRSDavidSoft <4673812+DRSDavidSoft@users.noreply.github.com> Date: Mon, 19 Oct 2026 21:59:35 +0330 Subject: [PATCH 2/2] fix: avoid extra idle time after late strip frames --- Tools/Controller/internal/control/strip.go | 4 +++- Tools/Controller/internal/control/strip_test.go | 13 ++++++++++--- Tools/Firmware/verify-strip-waveform.mjs | 3 +++ docs/STRIP-STREAMING.md | 8 ++++++++ 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/Tools/Controller/internal/control/strip.go b/Tools/Controller/internal/control/strip.go index 3ff5d5c6..e2fd00f6 100644 --- a/Tools/Controller/internal/control/strip.go +++ b/Tools/Controller/internal/control/strip.go @@ -298,7 +298,9 @@ func (outputs *OutputScheduler) streamStripRainbow(ctx context.Context, count, f func nextStripFrameTime(previous, now time.Time, period time.Duration) time.Time { next := previous.Add(period) if !next.After(now) { - next = next.Add((now.Sub(next)/period + 1) * period) + // A late transfer already consumed the frame budget. Render current + // animation time immediately and rebase, without a catch-up burst. + next = now } return next } diff --git a/Tools/Controller/internal/control/strip_test.go b/Tools/Controller/internal/control/strip_test.go index d6efdcab..5e6fb902 100644 --- a/Tools/Controller/internal/control/strip_test.go +++ b/Tools/Controller/internal/control/strip_test.go @@ -120,14 +120,21 @@ func TestStripStreamRecoversAfterTransientFrameTimeout(t *testing.T) { } } -func TestNextStripFrameTimeSkipsBufferedIntervals(t *testing.T) { +func TestNextStripFrameTimeRebasesLateTransfers(t *testing.T) { period := 50 * time.Millisecond previous := time.Unix(100, 0) if got, want := nextStripFrameTime(previous, previous.Add(10*time.Millisecond), period), previous.Add(period); !got.Equal(want) { t.Fatalf("ordinary next frame = %s, want %s", got, want) } - if got, want := nextStripFrameTime(previous, previous.Add(121*time.Millisecond), period), previous.Add(150*time.Millisecond); !got.Equal(want) { - t.Fatalf("delayed next frame = %s, want %s", got, want) + for _, delay := range []time.Duration{50, 70, 121} { + now := previous.Add(delay * time.Millisecond) + got := nextStripFrameTime(previous, now, period) + if !got.Equal(now) { + t.Fatalf("late transfer %s: next frame = %s, want %s", delay*time.Millisecond, got, now) + } + if following := nextStripFrameTime(got, now.Add(10*time.Millisecond), period); !following.Equal(now.Add(period)) { + t.Fatalf("late transfer must rebase subsequent pacing: got %s", following) + } } } diff --git a/Tools/Firmware/verify-strip-waveform.mjs b/Tools/Firmware/verify-strip-waveform.mjs index 9a1504f6..e67b4c32 100644 --- a/Tools/Firmware/verify-strip-waveform.mjs +++ b/Tools/Firmware/verify-strip-waveform.mjs @@ -3,6 +3,9 @@ // Exercise the actual linked AVR bit loop, including compiler register allocation. import assert from 'node:assert/strict' import { readFileSync } from 'node:fs' +import { loadProjectEnv } from '../Build/env.mjs' + +loadProjectEnv() const listingPath = process.argv[2] if (!listingPath) throw new Error('Usage: node Tools/Firmware/verify-strip-waveform.mjs AVR-LISTING.lst') diff --git a/docs/STRIP-STREAMING.md b/docs/STRIP-STREAMING.md index 43b09eae..bdcefe52 100644 --- a/docs/STRIP-STREAMING.md +++ b/docs/STRIP-STREAMING.md @@ -90,3 +90,11 @@ value, byte boundaries, and a 300-byte frame, including 20-cycle cells and the existing 5/13-cycle high pulses. The test rejects unrecognized instruction sequences instead of assuming their timing. Physical LED acceptance remains a separate check using the host's `strip rainbow 100 20` command. + +The requested frame rate is a ceiling, not a delivery measurement. A 100-pixel +frame requires seven acknowledged data chunks plus a show request. On the tested +serial connection, complete frame commands took 66–76 ms, already exceeding a +20 FPS frame's 50 ms budget. When a transfer finishes late, the scheduler renders +the current animation time immediately and rebases its deadline; waiting for the +next original grid boundary would unnecessarily reduce this case to about 10 FPS. +Short transfers still wait for their deadline, and no stale frames are queued.