Conversation
…tive drift - Refactored EffectsManager to use an absolute nanosecond timeline for rendering. - Updated interfaces.h and LEDEffectBase to use microsecond precision for delta time. - Updated all LED effects to utilize the high-precision delta time. - Standardized timing across SpeedTracker, monitor UI, and connection delays to microseconds. - Improved global millis() accuracy and added delayMicros utility.
- Fixed Schedule logic to correctly handle time ranges crossing midnight. - Updated Canvas and Controller deserialization to handle missing optional fields. - Switched Canvas feature mutex to recursive_mutex to prevent deadlocks.
- Refactored HSV2RGB to use hsv2rgb_rainbow for better speed and color accuracy. - Optimized blendWith using lerp8. - Added memcpy fast-path to PixelsToByteArray to reduce CPU overhead. - Defaulted logging level to debug.
| : c; | ||
| } | ||
|
|
||
| static constexpr array<uint8_t, 7> GlyphRows(char c) |
There was a problem hiding this comment.
Took a while to get this constexpr - but it was worth it :-)
There was a problem hiding this comment.
Pull request overview
This PR improves frame timing accuracy by switching feature frame generation to use an explicit system_clock::time_point, expands socket diagnostics, and adds a new scrolling Stock Banner effect that consumes the V2 stock API.
Changes:
- Update LED feature frame headers to use an explicit display timestamp and improve frametime scheduling in
EffectsManager. - Add new Stock Banner scrolling ticker effect and expose it via effect serialization/catalog.
- Extend socket channel diagnostics (failed connect count + last socket error) and improve server shutdown handling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| webserver.h | Makes server wait/shutdown more robust by guarding invalid futures and logging exceptions. |
| main.cpp | Clarifies startup logging by separating dashboard vs API endpoints. |
| interfaces.h | Updates feature frame API to accept a display timestamp; adds new socket diagnostic getters. |
| ledfeature.h | Implements timestamped frame generation and fixes reversed multi-row pixel mapping. |
| effectsmanager.h | Improves frame pacing precision and passes computed display timestamps into features. |
| socketchannel.h | Adds connection failure diagnostics and adjusts frame send batching logic and response aging check. |
| apihelpers.h | Ensures sockets are restarted/stopped alongside canvas effect start/stop flows. |
| apihelpers.cpp | Registers StockBanner in the effect catalog for the web UI. |
| effects/stockbannereffect.h | Introduces the StockBanner effect with HTTP quote fetching and rendering. |
| secrets.h.example | Adds configuration placeholder for the realtime quote key used by StockBanner. |
| tests/tests.cpp | Adds a unit test validating reversed multi-row feature pixel mirroring; provides test logger/static defs. |
| tests/Makefile | Adds link libs needed by new/updated dependencies used in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t tempCount = 0; | ||
| size_t tempBytes = 0; | ||
| auto queueCopy = _frameQueue; | ||
| while (!queueCopy.empty() && tempCount < kMaxBatchSize) | ||
| { | ||
| tempBytes += queueCopy.front().size(); | ||
| tempCount++; | ||
| queueCopy.pop(); | ||
| } | ||
|
|
||
| combinedBuffer.reserve(tempBytes); |
| addrinfo hints{}; | ||
| hints.ai_family = AF_UNSPEC; | ||
| hints.ai_socktype = SOCK_STREAM; | ||
|
|
|
|
||
| try | ||
| { | ||
| const string path = "/?ticker=" + symbol + "&v=2&profile=hub75&points=1"; |
| const string symbol = _quotes[*index].symbol; | ||
| _lastRequestStart = now; | ||
| _pendingFetch = async(launch::async, [this, symbol]() | ||
| { | ||
| return FetchQuote(symbol); | ||
| }); |
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| class StockBanner : public LEDEffectBase |
Resolved conflicts keeping robertlipe's sub-millisecond timing changes (GetDataFrame with targetTime, microseconds deltaTime for effects) while taking upstream bug fixes (overflow-safe buffer size cast, resilient invalid-byte handling in socketchannel, SetId preservation in from_json, clientBufferCount default 500). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
timing precision & adds aurora borealis effect
performance and color
rbergen
left a comment
There was a problem hiding this comment.
Code review findings from reading the full diff.
| uint32_t _stripHeight = 0; | ||
| bool _stripDirty = true; | ||
|
|
||
| future<FetchResult> _pendingFetch; |
There was a problem hiding this comment.
Blocking destructor on std::async future. When StockBanner is destroyed while a fetch is in progress (e.g. because the effect is stopped), the destructor of a future obtained from std::async(launch::async, ...) blocks until the async task completes — up to _requestTimeout (1500 ms). Since Stop() can be called from the effects manager thread, this stalls the whole render loop. Consider using a detached thread with a shared cancellation flag, or storing the future in an optional and resetting it under a condition that allows early exit.
|
|
||
| try | ||
| { | ||
| const string path = "/?ticker=" + symbol + "&v=2&profile=hub75&points=1"; |
There was a problem hiding this comment.
No URL-encoding on user-supplied symbols. Ticker symbols from the JSON config are concatenated directly into the query string. A value like FOO&bar=1 would silently corrupt the request. Stock tickers are normally alphanumeric, but since these come from user config it's worth at least validating they match [A-Z0-9.\-]+ before building the URL.
| const int bottomScale = LargestTextScale(priceText + (changeText.empty() ? "" : " " + changeText), | ||
| static_cast<int>(_compactQuoteWidth) - 8, | ||
| bottomHeight - 2); | ||
| const int width = static_cast<int>(max<uint32_t>({ |
There was a problem hiding this comment.
Tile width calculated twice. RebuildStrip computes each tile's width here to sum up the total strip width, and then RenderQuoteTile (line 576) recomputes it independently. If those two calculations ever diverge — e.g. due to a future edit — tiles will misalign. Consider extracting a ComputeTileWidth(const Quote&) helper that both callers use.
| { | ||
| (void) deltaTime; | ||
|
|
||
| EnsureQuoteList(); |
There was a problem hiding this comment.
EnsureQuoteList() runs O(n²) work every frame. At the end of EnsureQuoteList, _symbols is always repopulated from _quotes, so it is never empty after the first call. That means the reconciliation loop (with its inner find_if over _quotes) runs on every Update() call. A _symbolsDirty flag that from_json sets and EnsureQuoteList clears would make this a no-op on the hot path.
| auto nextFrameTime = steady_clock::now(); | ||
| const auto effectiveFps = max<uint16_t>(1, _fps); | ||
| const auto frameDuration = duration_cast<steady_clock::duration>(duration<double>(1.0 / effectiveFps)); | ||
| const auto effectDelta = duration_cast<milliseconds>(frameDuration + 500us); |
There was a problem hiding this comment.
Magic 500 µs offset is undocumented. effectDelta is passed as deltaTime to every effect's Update(). It is now a fixed value rather than the actual elapsed time, and the 500 µs padding has no explanation. Effects that use deltaTime for physics or animation will receive a slightly inflated constant every frame. Please document why 500 µs was chosen (scheduler wake-up margin? rounding buffer?).
| if (tempBytes > 0) | ||
| size_t tempCount = 0; | ||
| size_t tempBytes = 0; | ||
| auto queueCopy = _frameQueue; |
There was a problem hiding this comment.
queueCopy allocation is vestigial with kMaxBatchSize = 1. The copy of the whole queue exists only to compute tempBytes for combinedBuffer.reserve(). With batch size 1, this copies at most one item to determine one frame's size — a combinedBuffer.reserve(_frameQueue.front().size()) would do the same without the copy.
Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…tions/actions/checkout-7 Bump actions/checkout from 6 to 7
rbergen
left a comment
There was a problem hiding this comment.
This PR suffers from a more-involved-than-usual merge conflict, almost certainly caused by a number of PRs from Robert's hand that I recently merged.
I can take a shot at resolving them if you'd like me to - I went through the merged PRs fairly recently so I still have some memories about which changes were made why.
|
If you could, I’d appreciate it! I’m working on the 899 PR in the NDS folder…
- Dave
… On Jun 28, 2026, at 9:14 AM, Rutger van Bergen ***@***.***> wrote:
@rbergen commented on this pull request.
This PR suffers from a more-involved-than-usual merge conflict, almost certainly caused by a number of PRs from Robert's hand that I recently merged.
I can take a shot at resolving them if you'd like me to - I went through the merged PRs fairly recently so I still have some memories about which changes were made why.
—
Reply to this email directly, view it on GitHub <#126?email_source=notifications&email_token=AA4HCFZQUXS2T5FUBT4HGPT5CE76BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYG43TQNJYGYZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4587785863>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFZMDZIKQ643PPEBW3T5CE76BAVCNFSNUABFKJSXA33TNF2G64TZHM4DQOJRGQYDAMRYHNEXG43VMU5TINRYHAZDINZZHEZ2C5QC>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AA4HCF33SPQKRPHVPNBXYUD5CE76BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYG43TQNJYGYZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/AA4HCF6P5GWU5N2FSCFI7FL5CE76BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYG43TQNJYGYZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because you authored the thread.
|
|
@davepl I've been looking into this merge conflict, and I think I have a plan to bring In the Was that change to a shared/max offset intentional (e.g. to keep all features on a canvas frame-synchronized), or an incidental side effect of restructuring the timing loop for accuracy? If it's intentional, is it meant to replace per-feature offsetting everywhere, or only in specific cases? As you will understand, your answer to this question determines whether I will go one way with the conflict resolution, or the other. |
Documents the file-by-file conflict analysis between v2 and main, and tracks progress resolving them incrementally on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the 4 files that conflict between main and v2 by keeping main's side as-is for now: effectsmanager.h, interfaces.h, ledfeature.h, socketchannel.h. This establishes a compilable baseline that includes v2's cleanly-auto-merged additions (StockBanner effect, secrets key, tests) but NOT yet v2's intended changes to the 4 placeholder files. Each placeholder will be replaced with its real, hand-analyzed resolution in a follow-up commit — see pr126-merge-plan.md for the analysis and progress tracking. effectsmanager.h in particular is deferred to a separate pass since it requires blending both branches' logic rather than picking a side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three issues, none part of the actual conflict set, that only showed up once v2's cleanly-auto-merged additions were compiled against main's (placeholder-resolved) interfaces: - effects/stockbannereffect.h: StockBanner::Update() declared its unused deltaTime parameter as milliseconds, but ILEDEffect::Update() (from main) takes microseconds - hid the virtual instead of overriding it. Parameter value is unused, so retyping it is behavior-neutral. - tests/tests.cpp: DummyEffectsManager::UpdateCurrentEffect() had the same milliseconds/microseconds mismatch against IEffectsManager, matching v2's own (now-superseded) interface signature instead of main's. - socketchannel.h: pre-existing -Wreorder issue on main itself (ctor initializer list order didn't match member declaration order for _totalQueuedBytes/_lastInvalidByteWarning) - invisible on the main Makefile (no -Werror) but breaks the -Werror tests build. Unrelated to the v2 merge; fixed here only because it blocked verifying every step per the plan in pr126-merge-plan.md. With these, `make` and `make -C tests` both succeed and 13/14 tests pass. The one expected failure, APITest.ReversedMatrixFeatureMirrorsRowsForHardware, exercises the multi-row-reversed pixel bug in ledfeature.h that this branch's ledfeature.h placeholder (main's original, buggy-for-this-case version) hasn't been replaced with its real resolution yet - tracked as Step B in pr126-merge-plan.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These two files turned out to be compilation-coupled and are resolved together: interfaces.h's ISocketChannel gains v2's GetFailedConnectCount()/ GetLastSocketError() pure virtuals, which socketchannel.h must implement or the class becomes abstract. interfaces.h (Step A): the one real conflict (GetDataFrame's parameter name, targetTime vs displayTime) resolves to main's targetTime, since main's per-feature TimeOffset() semantics are being kept (see ledfeature.h, Step B, and pr126-merge-plan.md). The non-conflicting GetFailedConnectCount/ GetLastSocketError declarations from v2 are carried over as-is. socketchannel.h (Step C): WorkerLoop() keeps main's structure wholesale (persistent combinedBuffer/reserve, throttled response polling via ReadSocketResponse()) rather than v2's per-iteration buffer allocation and inline per-send response reads. v2's RecordConnectFailure()/ RecordConnectSuccess() diagnostics helpers and their call sites are kept, as is v2's _lastSocketError/_failedConnectCount tracking. The one real conflict this creates - the ConnectSocket() success-logging tail - is resolved by calling RecordConnectSuccess() (mutex-guarded bookkeeping) while keeping main's tiered log level (info once, debug on reconnects) rather than v2's uniform info-level log on every reconnect. Note: the initial placeholder-merge commit (5de9659) resolved conflicts via `git checkout --ours`, which discards a file's entire merge result, not just the conflicting hunks - so it silently dropped v2's non-conflicting RecordConnectFailure/Success feature and the matching interfaces.h declarations entirely. This commit restores them by reconstructing the true 3-way merge (git merge-file against the branches' actual tips) rather than hand-editing from the placeholder state. `make` and `make -C tests` pass; 13/14 tests pass (the remaining failure, ReversedMatrixFeatureMirrorsRowsForHardware, is expected until ledfeature.h is resolved in the next commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real conflicts, both resolved by combining rather than picking a side: - GetPixelData()'s fast-path condition keeps main's bounds check but adds v2's extra `&& (!_reversed || _height == 1)` guard. Without it, a full-canvas multi-row reversed feature would hit Utilities::ConvertPixelsToByteArray(), which reverses the whole flattened pixel buffer - correct for a single row, but wrong across rows (it reverses row order too, not just each row's pixel order). v2 added this guard specifically to route multi-row reversed features to the slow path, which handles per-row reversal correctly. - GetDataFrame() keeps main's targetTime parameter and per-feature TimeOffset() application, rejecting v2's change to a shared canvas-wide max offset (see the "Question to send to the PR author" in pr126-merge-plan.md - this preserves the existing per-feature contract pending the author's confirmation). As with the previous commit, this reconstructs the true 3-way merge rather than hand-editing from the placeholder state, which also restores v2's non-conflicting per-row reversal fix in the slow path and the failedConnectCount/lastSocketError fields in to_json (both silently dropped by the initial `git checkout --ours` placeholder). `make` and `make -C tests` pass; all 14 tests pass, including ReversedMatrixFeatureMirrorsRowsForHardware which exercises the fast-path guard fix above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Records what actually happened while executing Steps 1-3, A, B, C, notably the git checkout --ours pitfall (it discards a file's whole merge result, not just conflicting hunks) and the A/C compilation coupling, so a future session picking up Step D (effectsmanager.h) doesn't repeat either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author confirmed (2026-07-05) the shared/max time-offset change in v2 was not intentional - likely an incidental side effect of vibe-coded visual work on the StockBanner effect. Confirms the Step B resolution already applied in ledfeature.h (keep main's per-feature TimeOffset()) was correct. Author's guidance: take whichever path resolves the merge fastest. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The last of the 4 conflicting files, and the only one needing genuine hand-blending rather than picking a side: main's Start() (schedule-based effect switching, heartbeat/blackout frames, drift detection, >1s sane reset) and v2's Start() (this PR's actual purpose - frametime accuracy) independently rewrote the same worker-thread loop since v2 branched off before the scheduling work landed on main. Kept main's outer control flow as the skeleton (schedule check -> active/ inactive branch -> heartbeat/blackout -> drift detection -> sane reset), and spliced in v2's timing-precision improvements: - frameDuration now computed via a double-precision intermediate (duration_cast<steady_clock::duration>(duration<double>(1.0/fps))) instead of truncating nanoseconds(1'000'000'000LL / fps) up front - the actual fix this PR is titled for. - FPS clamped to a minimum of 1 (max<uint16_t>(1, _fps)). - Catch-up avoidance: if the loop falls more than one frame behind, jump the frame counter forward to match real elapsed time instead of sending a rapid-fire burst of queued frames. This sits alongside (not instead of) main's existing >1s sane-reset, which remains as a backstop for more severe stalls (e.g. system suspend) since it also re-centers the epoch and avoids unbounded frame-counter growth. - Frames are now compressed via Socket()->CompressFrame() before being enqueued, in both the active-effect and heartbeat send paths - a v2 addition that doesn't conflict with anything on main. - to_from_json_map gets v2's StockBanner registration alongside main's AuroraEffect one (both additive, both kept). Explicitly NOT carried over: v2's change to compute a single canvas-wide maximum TimeOffset() and bake it into one shared displayTime for every feature. main's (and this branch's, per ledfeature.h/Step B) per-feature TimeOffset() application inside GetDataFrame() is kept instead. The PR author confirmed this wasn't an intentional design change (see pr126-merge-plan.md). As with the earlier files, this reconstructs the true 3-way merge (git merge-file against the branches' actual tips) rather than hand-editing from the placeholder state, which also restores v2's non-conflicting #include "effects/stockbannereffect.h" and #include <algorithm>. This is the last of the 4 conflicting files. `make` and `make -C tests` pass; all 14 tests pass. The merge is otherwise complete pending a final review pass and manual smoke test per pr126-merge-plan.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All 4 conflicting files (interfaces.h, ledfeature.h, socketchannel.h, effectsmanager.h) are resolved and committed on this branch. Full build and test suite pass. Remaining work before landing: manual smoke test and a final diff read-through, per the execution plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every effect subclassing LEDEffectBase now passes its own class name as
the type string to the base constructor (static TypeName member), instead
of only AuroraEffect doing so while everything else fell back to the
default "LEDEffectBase" string. jsonPair<T>() keys to_from_json_map by
T::TypeName instead of typeid(T).name(), so the AuroraEffect-only special
case (make_pair("AuroraEffect", jsonPair<AuroraEffect>().second)) is no
longer needed - every entry is now a plain jsonPair<T>().
This makes to_json's mangled-typeid fallback dead code (effect.Type() now
always matches on the first lookup), so it's removed. from_json instead
gains a small fallback: if the JSON "type" string isn't found directly,
strip any leading digits (the Itanium ABI name-length prefix baked into
old typeid-mangled saves, e.g. "15StarfieldEffect") and retry. This keeps
config files saved before this change - and the previously-shipped
config.led.example, which had the same old mangled names - deserializing
correctly instead of silently replacing those effects with magenta fill.
config.led.example itself is also updated to the new clean type strings,
so a fresh checkout no longer needs the fallback at all.
`make` and `make -C tests` pass; all 14 tests pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Working doc used to track the merge conflict resolution as it progressed; no longer needed now that all conflicts are resolved and verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Was an integer number of millis before, now a proper timespan.
Adds the StockBanner scrolling ticker effect. Consumes the V2 stock API.