timing precision & adds aurora borealis effect#101
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.
|
I'm going to leave this comment on this PR only, but it applies to #102, #103 and #104 as well. Before writing this comment I synced all 4 PRs with the current state of main, on which CI does pass, so I feel comfortable concluding the problem I'm describing in this comment is caused by the changes the PRs are proposing. On all PRs, CI fails because the API test suite does, comprehensively (9 out of 9 tests), because of an unexpected response code. What I think is particularly relevant is the response code reported to be returned, which is 0 - that's clearly not a valid HTTP response code in any scenario. The tests can be executed by running tests/tests after a full build. I noticed that the 4 PRs have one commit in common (1bb2390), which makes me think the root cause is likely to be embedded in that one. |
|
All four have been updated and pushed. (I outsourced this.) PTAL Fixes Pushed to PRs 101-104: Recapping the pushed fixes for PRs 101-104. ✦ I have ensured that all needed fixes are pushed to PRs 101 through 104 on the Summary of Actions:
All branches are now up to date with origin and include the latest cumulative |
robertlipe
changed the title
|
@robertlipe The test suite still fails across all four PRs. Have you established they do pass successfully? |
|
Multi-week turnaround (I know, I know...) to get build results is worse than my days of turning in assignments on punched cards and then waiting for a printout with a syntax error. ndscpp git checkout timing-precision ndscpp make && tests/tests Running main() from /private/tmp/googletest-20250808-4780-aa0gxy/googletest-1.17.0/googletest/src/gtest_main.cc [----------] 2 tests from SolarTest [----------] Global test environment tear-down I'll dig into fresh checkouts or something, but if it's not something pretty obvious, I'll probably just withdraw the changes. My tree works fine for me. |
|
You can actually hold for now. I believe the problem may be at the github actions side. I'll investigate and comment when I gave a conclusion. |
|
Reading that more carefully, the top-level Makefile never called the child Makefile. (It caught my eye that the output tested solar when that code isn't even in this branch.) Perhaps intentional, but doesn't exactly encourage devs to track tests. But even with that... ndscpp make -C tests && tests/tests [----------] Global test environment tear-down The reason these are failing in CI is because the tests are buggy. They assume ndscpp is running and apparently nobody is starting one. Look carefully. [ RUN ] APITest.GetController It's failing (but not displaying any errors, alas) because it can't open a socket on 7777 This isn't a problem in the PR. It's a problem that (only) external contributions are expected to pass a test suite that has clearly never been run before. I just went to a completely different machine. ➜ blah cd NDSCPP ➜ NDSCPP git:(timing-precision) make -C tests && tests/tests ➜ NDSCPP git:(timing-precision) make -C tests && tests/tests [ FAILED ] APITest.GetController (6 ms) [ RUN ] APITest.GetSockets [ FAILED ] APITest.GetSockets (0 ms) [ FAILED ] APITest.GetSpecificSocket (0 ms) [ FAILED ] APITest.CanvasCRUD (0 ms) [ FAILED ] APITest.CanvasFeatureOperations (0 ms) [ FAILED ] APITest.MultipleCanvasOperations (62 ms) [ FAILED ] APITest.MultipleFeatureOperations (0 ms) [ FAILED ] APITest.RapidCreationDeletion (6 ms) [ FAILED ] APITest.CanvasFeatureEffectWithSchedule (0 ms) [----------] Global test environment tear-down 9 FAILED TESTS So that carnage exactly matches what's in reported by CI. Let's start ndscpp. ➜ NDSCPP git:(timing-precision) cp config.led.example config.led ➜ NDSCPP git:(timing-precision) ./ndscpp Running main() from /private/tmp/googletest-20250430-4705-6kzjyz/googletest-1.17.0/googletest/src/gtest_main.cc [----------] Global test environment tear-down All tests pass. In short, I posit that everything passes. No, I didn't go through all three; I've spent quite enough time debugging buggy tests already. If there's an actual problem/regression exposed in the PRs, I'll look into it, but since my workhorse branch on my workhorse machine (that's been dutifully delivering blinkies now for months) has evolved way beyond the April submits and is passing all the tests, I really think this is broken CI. Consider it a feature request that a 'make check' or 'make tests' or whatever target should be present that does whatever the project considers a requirement for submissions and CI should use that. No developer will know, for example, that tests/tests doesn't require a setup with twenty canvases to not incur |
|
I found it. Your split of build-time and runtime config breaks CI, because config.led is no longer generated as part of make all. That makes ndscpp fail on startup when the test suite is executed. I'll push an update to robertlipe:timing-precision, which you can then propagate to the other PR head branches. |
|
I made and pushed the change, and CI now passes. |
|
I now see some of our messages crossed while we were both analyzing. For purposes of clarity:
|
|
Ah, yes, the build used to semiirandomly CLOBBER the USER'S config.led
instead of treating it like a precious artifact. I now remember fixing that
months ago. (You're free to push back on me for "introducing" this problem,
but we also both know how that conversation will go.)
I've clicked 'sync fork'. Maybe that's enough.
…On Thu, Jun 11, 2026 at 12:43 AM Rutger van Bergen ***@***.***> wrote:
*rbergen* left a comment (PlummersSoftwareLLC/NDSCPP#101)
<#101 (comment)>
I now see some of our messages crossed while we were both analyzing. For
purposes of clarity:
- Before this PR, it was enough to build ndcspp and the test suite
using make all && make -C tests, start ndscpp using ./ndscpp and then
run the tests with tests/tests once ndscpp had had a chance to settle
down. One of the reasons this worked was that in the Makefile, the build of
ndcspp was marked as dependent on both secrets.h and config.led, which made
make generate those files from the examples if they didn't exist.
- Due to a change introduced in this PR, config.led is no longer a
build-time dependency, and hence won't be generated with make all if
it doesn't already exist. That makes ndscpp fail at startup, and hence the
tests too.
- The fact that the embedded configuration points to systems that will
only exist at those addresses in one place - and I actually doubt they
still do, considering how long ago the example config was put together - is
irrelevant for success or failure of the tests; dealing with that is part
of what should work.
- I've now updated the CI script, which is part of the repo, to
explicitly execute a make config.led before executing the test suite.
I pushed this update to robertlipe:timing-precision which makes CI pass.
- You can pick up this change and apply it to the head branches for
#102 <#102>, #103
<#103> and #104
<#104> as well; I
expect that will fix the CI failures in those.
—
Reply to this email directly, view it on GitHub
<#101?email_source=notifications&email_token=ACCSD36MUXUM2TCSWCPWIED47JBIJA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRXG42TKMRQG442M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4677552079>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD3766CVXUFSOFIUSIHL47JBIJAVCNFSNUABFKJSXA33TNF2G64TZHM4DQOJRGQYDAMRYHNEXG43VMU5TIMZQGEYTGMJUHE22C5QC>
.
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/ACCSD36JFXTRA4EZO5OGF4347JBIJA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRXG42TKMRQG442M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ACCSD33ER4YYO76ASEOFX5D47JBIJA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRXG42TKMRQG442M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Yeah, I saw you do that. :) That won't be enough, but I cherry-picked the commit and applied it to the other PR branches too. That should work. It just means you'll have to pull those branches to get the fix locally as well - which is functionally irrelevant unless you're GitHub. |
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>
2 participants
Improve timing precision to sub-millisecond accuracy and fix accumulative drift
The current implementation suffers from cumulative truncation errors because it calculates frame durations in microseconds using integer division (e.g., 1,000,000 / 30 = 33,333µs instead of 33,333.333µs). This leads to a persistent drift of approximately 36ms per hour at 30 FPS, and potentially more at other frame rates.