V2 - Increase frametime accuracy by davepl · Pull Request #126 · PlummersSoftwareLLC/NDSCPP

davepl · 2026-06-18T01:02:07Z

Was an integer number of millis before, now a proper timespan.
Adds the StockBanner scrolling ticker effect. Consumes the V2 stock API.

davepl · 2026-06-18T01:03:22Z

+            : c;
+    }
+
+    static constexpr array<uint8_t, 7> GlyphRows(char c)


Took a while to get this constexpr - but it was worth it :-)

Copilot

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


rbergen

Code review findings from reading the full diff.

rbergen · 2026-06-21T17:39:34Z

+    uint32_t _stripHeight = 0;
+    bool _stripDirty = true;
+
+    future<FetchResult> _pendingFetch;


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.

rbergen · 2026-06-21T17:39:35Z

+
+        try
+        {
+            const string path = "/?ticker=" + symbol + "&v=2&profile=hub75&points=1";


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.

rbergen · 2026-06-21T17:39:35Z

+            const int bottomScale = LargestTextScale(priceText + (changeText.empty() ? "" : " " + changeText),
+                                                     static_cast<int>(_compactQuoteWidth) - 8,
+                                                     bottomHeight - 2);
+            const int width = static_cast<int>(max<uint32_t>({


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.

rbergen · 2026-06-21T17:39:35Z

+    {
+        (void) deltaTime;
+
+        EnsureQuoteList();


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.

rbergen · 2026-06-21T17:39:35Z

-            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);


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?).

rbergen · 2026-06-21T17:39:35Z

-                    if (tempBytes > 0) 
+                        size_t tempCount = 0;
+                        size_t tempBytes = 0;
+                        auto queueCopy = _frameQueue;


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.

rbergen · 2026-06-21T17:39:35Z

    {
+        for (auto &feature : canvas->Features())
+        {
+            feature->Socket()->Stop();


Socket restart may block while holding the canvas mutex. ApplyCanvasesRequest holds the controller mutex during this callback. Socket()->Stop() followed by Socket()->Start() can block for up to reconnectDelay (1 s) if the socket is mid-connect. That would stall any concurrent HTTP request handler waiting on the same mutex. Please verify Stop() returns promptly, or move the socket lifecycle calls outside the mutex.

rbergen

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.

davepl · 2026-06-28T16:15:36Z

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 added 2 commits June 17, 2026 10:11

Add StockBanner effect, use V2 quote interface

afb174d

Increase frametime accuracy

ba3f726

davepl commented Jun 18, 2026

View reviewed changes

davepl requested review from Copilot and rbergen June 18, 2026 01:06

Copilot started reviewing on behalf of davepl June 18, 2026 01:06 View session

Copilot AI reviewed Jun 18, 2026

View reviewed changes

rbergen reviewed Jun 21, 2026

View reviewed changes

rbergen reviewed Jun 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

V2 - Increase frametime accuracy#126

V2 - Increase frametime accuracy#126
davepl wants to merge 2 commits into
mainfrom
v2

davepl commented Jun 18, 2026 •

edited

Loading

Uh oh!

davepl Jun 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

rbergen left a comment

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen Jun 21, 2026

Uh oh!

rbergen left a comment

Uh oh!

davepl commented Jun 28, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

davepl commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

rbergen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbergen left a comment

Choose a reason for hiding this comment

Uh oh!

davepl commented Jun 28, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davepl commented Jun 18, 2026 •

edited

Loading