Conversation
…artup sync_panel_time left the panel roughly a minute behind, and never corrected it. Four separate causes: 1. The panel has no seconds field, so it starts its clock at HH:MM:00 when the entry is committed. set_allbutton_time() read the clock up front and added a fixed 30s, so the panel ended up behind by however long the menu walk took (5s when every field already matches, up to ~26s otherwise). Now it picks a minute boundary, waits for it OUTSIDE the menu, then walks the fields so the commit lands on the boundary. Measured on an RS8: 35ms. 2. checkAqualinkTime() parsed the panel's 'HH:MM' as HH:MM:00 when it really means "somewhere in that minute". Every comparison read 0-59s slow (+30 average) even for a perfect panel. Uses the minute's midpoint now. 3. ACCEPTABLE_TIME_DIFF was 120s, so a panel a minute out could never be detected - which is why one stayed a minute slow indefinitely. 4. The hour was silently set an hour fast whenever select_sub_menu_item() raced the display: coming out of the DAY field the last message received is still the DAY line, so it pressed RIGHT before it had seen the hour, then matched the pre-keypress "HOUR n xM" the panel had already queued. The result was also ignored. Replaced with setAqualinkHourField(), which waits for the value each keypress should produce, the same way setAqualinkNumericField() already does. Timing the panel's minute ROLLOVER rather than reading HH:MM pins its clock to within half a message gap (~4s on an RS panel, vs +/-30s), so the tolerance can drop to 15s. Falls back to the coarse estimate when no usable rollover is available, and scales the tolerance by the measured window. Also adds force_panel_time_sync_at_startup (default off) to sync once per start even when the panel is inside tolerance. Deferred 60s so it does not stall startup, and skips the hourly rate limit so arming it actually takes effect. Not fixed: the iaqtouch, onetouch and PDA set-time paths have the same missing-elapsed-time bug as (1) and no +30 compensation at all, so they remain ~30s plus programming time slow. I have no hardware to test them on. select_sub_menu_item()'s race is also left alone - its other callers are menu navigation rather than value setting, where a stray keypress is far less damaging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| else // Must be 13 or more | ||
| sprintf(hour, "HOUR %d PM", result->tm_hour - 12); | ||
|
|
||
| waitForSingleThreadOrTerminate(threadCtrl, AQ_SET_TIME); |
There was a problem hiding this comment.
The target can be stale by the time this returns. Because the thread deliberately does not reserve the programming slot during the pre-wait, another programmer can start before this call and make this thread wait up to 120 seconds. It will then continue using the already-computed target and can program a past minute. Please reserve the slot before final scheduling, or re-check and recompute the target immediately after acquiring it when the original boundary is no longer reachable. AI-assisted review: GPT-5.6.
| } | ||
| // MINUTE is the last field, so its ENTER is the one that commits. Park the field on | ||
| // the target minute but keep hold of that keypress. | ||
| setAqualinkNumericField_noenter(aqdata, "MINUTE", tm_target.tm_min); |
There was a problem hiding this comment.
Please check this return value and abort or cancel on failure. If the MINUTE field is not found, this currently waits for the target boundary and sends two ENTERs from an unknown menu state. The YEAR, MONTH, and DAY helper results above should be checked as well. When this is rebased over the newer command-queue work, the final send_cmd results should also be checked so a failed commit is not reported as successful. AI-assisted review: GPT-5.6.
| hourly checks is negligible; what this needs to catch is a panel that lost power. | ||
| NOTE the old value of 120 could never detect a panel a minute out, which is why one | ||
| stayed a minute slow indefinitely. */ | ||
| #define ACCEPTABLE_TIME_DIFF 40 |
There was a problem hiding this comment.
This tolerance is shared by the PDA path even though the PR says PDA set-time is not covered. PDA skips the precise rollover estimator, but its setter samples the current minute before walking all five fields and commits later. If that leaves it roughly 30–50 seconds behind, the midpoint estimate varies by ±30 seconds, so a 40-second threshold can randomly trigger another inaccurate sync on later hourly checks. Please scope the tighter fallback tolerance, and preferably startup forcing, to boundary-aware setters, or retain the old PDA tolerance until its setter is upgraded. AI-assisted review: GPT-5.6.
| # Keep the panel time synced with systemtime. Make sure to set systemtime / NTP correctly. | ||
| sync_panel_time = yes | ||
|
|
||
| # Sync the panel time once at every AqualinkD startup, even when the panel clock is |
There was a problem hiding this comment.
This description no longer matches the implementation: the PR changes the normal fallback tolerance from two minutes to 40 seconds, and to 15 seconds with a precise rollover, while the new setter intentionally waits outside SET TIME before opening the menu rather than holding that menu for a minute or two. Please update this text so users understand the actual interruption and thresholds. AI-assisted review: GPT-5.6.
|
Thanks for digging into the actual commit timing. I think the core direction is right: the final commit instant determines the clock phase, observing a displayed-minute rollover is much more informative than treating For terminology, the tested panel is an RS8 using the classic RS/AllButton programming path ( This is how I understand the current PR design: sequenceDiagram
autonumber
participant Panel as RS8 / AllButton panel
participant Rx as Time-message handler
participant Check as checkAqualinkTime()
participant Clock as System clock
participant Worker as set_allbutton_time()
participant Slot as Active programming slot
Panel->>Rx: Display HH:MM (trigger: each panel time message)
Rx->>Check: Check panel clock
Check->>Clock: Read current time
Check->>Check: Observe HH:MM rollover on every message
alt Forced startup sync is due or hourly check is due
Check->>Check: Estimate offset from rollover, else HH:MM midpoint
alt Offset is outside tolerance or sync is forced
Check-->>Worker: Start time-programming task
Worker->>Clock: Read now and choose future minute boundary
Worker->>Worker: Wait outside programming until target - estimated walk
Worker->>Slot: Wait until this task becomes active
Note over Worker,Slot: The precomputed target can become stale if another programming task owns the slot here
Slot-->>Worker: Task is active
Worker->>Panel: Enter SET TIME and program date/hour
Worker->>Panel: Stage minute without final ENTER
Worker->>Clock: Wait until target boundary
Worker->>Panel: Queue final ENTER to start HH:MM:00
Panel-->>Worker: Next panel status/poll transports command
Worker->>Panel: ENTER again to leave SET TIME
Worker->>Slot: Release programming slot
else Within tolerance
Check-->>Rx: No update
end
else Check is rate-limited
Check-->>Rx: No update
end
My suggested shared RS/AllButton and PDA design is below. The important change is to wait until this task is the active programmer before taking the authoritative wall-clock sample. I would also use a fixed sequenceDiagram
autonumber
participant Panel as RS/AllButton or PDA panel
participant Rx as Interface time-message handler
participant Check as Shared clock checker
participant Worker as Time-sync task
participant Slot as Active programming slot
participant Adapter as RS or PDA time adapter
participant Clock as System clock
Panel->>Rx: Report panel time (trigger: normal home/status time message)
Rx->>Check: Evaluate periodic or forced synchronization
Check->>Check: Estimate offset using best measurement available
alt Offset exceeds interface tolerance or forced sync is due
Check-->>Worker: Start time-sync task
Worker->>Slot: Wait to become active programming task
Slot-->>Worker: Task is active
Worker->>Adapter: Navigate to and confirm SET TIME programming
Adapter-->>Worker: SET TIME is ready
Worker->>Clock: Capture program_start (monotonic) and now (wall clock)
Worker->>Worker: next_boundary = top of next minute
alt now.second < 60 - MAX_TIME_PROGRAM
Worker->>Worker: target = next_boundary
Worker->>Adapter: Stage date, hour, and minute without final commit
loop After each field/response
Worker->>Clock: Check monotonic elapsed time
alt Elapsed > MAX_TIME_PROGRAM or a field fails
Worker->>Adapter: Cancel SET TIME
Worker->>Slot: Release programming slot
end
end
Adapter-->>Worker: All fields staged, final commit withheld
Worker->>Clock: remaining = target - current wall time
alt remaining > 0
Worker->>Clock: Wait until target
Worker->>Adapter: Queue final commit immediately
Adapter->>Panel: Send commit on next permitted status/poll response
Note over Adapter,Panel: Expected phase error is bounded mainly by the panel poll/response cadence and should be measured, likely less than one second
Panel-->>Rx: Later home/status message reports new time
Rx->>Check: Measure resulting offset/rollover phase
Worker->>Slot: Release programming slot
else Target was missed
Worker->>Adapter: Cancel rather than commit stale time
Worker->>Slot: Release programming slot
end
else Less than MAX_TIME_PROGRAM remains before next minute
Worker->>Adapter: Cancel and retry at the next safe opportunity
Worker->>Slot: Release programming slot
end
else Within tolerance
Check-->>Rx: No programming task
end
For PDA, this would require a counterpart to the new AllButton “no enter” numeric helper, because the current PDA numeric-field helper selects each field automatically. Both adapters would expose the same conceptual operations: enter/confirm SET TIME, stage fields without committing the last field, commit, and cancel. I would keep the rollover-based measurement where the interface supplies enough information, but make synchronization tolerance interface-aware until both setters implement boundary-aligned commit. The physical commit timestamp should also be recorded so the validator can quantify transport jitter rather than assuming it. Why I prefer this ordering
This does not eliminate the physical delay before the panel permits the final command to be sent. It makes that delay the primary remaining uncertainty, keeps it out of the scheduling calculation, and lets the validator quantify whether it is consistently below one second. Synchronization frequency and maintenance windowOne distinction is that the current code checks once per hour but does not necessarily program the clock once per hour. The inexpensive observation/check can remain frequent so rollover estimates stay current and a lost clock is detected promptly. Taking over the panel menu for a corrective write should be much less frequent. For ordinary crystal drift, I suggest:
A separate policy should be explicit for a gross discontinuity such as a wrong weekday/date, a multi-minute offset after panel power loss, a manual request, or AI-assisted review/comment: GPT-5.6. |
Review: the new 40s fallback (and 15s precise) tolerance was applied to every panel, but only set_allbutton_time() commits on a minute boundary. The iAQ Touch and PDA setters read the clock before walking their fields and commit seconds later, so they cannot land closer than a few tens of seconds - holding them to the tighter figures just re-programs the panel every hour to the same wrong time. iAQ Touch was the worse case and was not raised in review: observe_panel_ rollover() only skips PDA, so an RS panel with extended iAQ Touch programming got the rollover estimator AND the 15s tolerance while AQ_SET_TIME routed to set_aqualink_iaqtouch_time(). That is a guaranteed re-sync every hour, forever. isPanelTimeSetterBoundaryAware() mirrors the AQ_SET_TIME routing in aq_programmer() and lives beside it so the two stay together. Non-boundary-aware panels keep the original 120s tolerance until their setters are fixed. The rollover measurement is still used for the figure we report on those panels, since the measurement is sound regardless of which setter we use - only the tolerance depends on what the setter can achieve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: the pre-wait ran before waitForSingleThreadOrTerminate(), which sleeps up to 120s waiting for another programmer to finish. A partial block fell through and reused the already-computed target, so the panel was programmed for a minute in the past, the boundary wait returned immediately and it committed - leaving the clock behind by however long we waited. (A full 120s timeout pthread_exit()s, so only 1-119s of blocking hit this.) Claim the slot first, then read the clock and pick the boundary, so the target is always fresh. The wait still happens before the SET TIME menu is opened, so the panel is not held in a programming menu any longer than the walk needs - that was the point of moving it out, and it is preserved. The slot is now held for the wait too, but that is bounded at roughly 70s, well inside the 120s other programmers will wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: the YEAR, MONTH, DAY and MINUTE helper results were all ignored. The MINUTE one mattered most and was made worse by this PR: on failure the helper has already called cancel_menu(), so the old code went on to wait for the boundary and fire two ENTERs into whatever the panel was showing by then. YEAR/MONTH/DAY being unchecked predates this PR. All five fields are now checked and jump to a single exit that cancels the menu, so the panel is left out of programming rather than parked mid-entry. checkAqualinkTime() sees the clock is still wrong and retries next cycle. Also fixes setAqualinkNumericField_ex()'s iteration cap, which did 'break' and fell through to 'return true' - reporting a field it never managed to set as successful, which would have made these new checks useless. No existing caller tests that return value, so nothing else changes. Not done: the review also asked for the final send_cmd() results to be checked. send_cmd() is void in both overloads on this base, so there is nothing to test. That one needs the queue rework it refers to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review: the text still described the old behaviour. It said the normal tolerance was two minutes (now 15s with a rollover, 40s without, 120s on iAQ Touch/PDA) and that setting the clock holds SET TIME for "a minute or two", when the wait now happens before the menu is opened and the menu is only held for the keypress walk. Also mentions the 60s startup deferral. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed target Adopts three of the refinements from the design discussion on aqualinkd#555. Derived staging cost instead of a learned one. Both field setters step one unit per keypress with no wraparound, so the press count is known from what the panel is displaying: |target - current| for the numeric fields, (t - c + 24) % 24 for the hour. That replaces _settime_walk_secs, which was wrong exactly when it mattered - a run where every field already matched measured ~1s, and the next run after a power loss needs 116 presses. This is also why the suggested fixed MAX_TIME_PROGRAM of 15s is not used. At the measured 264ms per keypress the routine case is ~1s but a power-loss correction needs ~44s of staging, so a 15s deadline would cancel every attempt at exactly the point the sync matters most. The derived figure covers both ends without a constant that is wrong at one of them. Re-check the boundary after SET TIME is confirmed. Navigation is the variable part (select_menu_item retries up to three times), so it is now excluded from the accuracy calculation: the target is verified once the panel is actually showing YEAR, and retargeted a minute if the walk ran long. Retarget rather than cancel because checkAqualinkTime() is rate limited to once an hour, so cancelling here means no retry until then - cancelling becomes correct once out of tolerance observations are coalesced into a pending request. Cancel instead of committing a missed target. Previously an overrun logged a warning and committed anyway, knowingly setting the panel slow. It now commits only when doing so still beats where the panel already is, and otherwise cancels the menu and leaves it for the next check. A stalled field is already an explicit error rather than a degraded commit: setAqualinkNumericField_ex()'s iteration cap returns false and every field result is checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — the direction is right and I've adopted most of it. Note four commits landed after you wrote this, so a couple of your points were already in flight. Already pushed before this comment
Just pushed (
|
| scenario | presses | staging |
|---|---|---|
| all fields already correct | 4 | 5s |
| panel 5 min slow | 9 | 7s |
| panel an hour out | 27 | 13s |
| minute 30 away | 34 | 15s |
after power loss (01/01/00 12:00 AM) |
116 | 44s |
At 15s a power-loss correction cancels on every retry and the clock never gets set. Rather than raise the constant, the press count is computable in advance — |target − current| for numeric fields, (t − c + 24) % 24 for the hour, read from the panel's own display. That gives your bounded, non-learned property without a constant that's wrong at one end of the range.
Your per-field deadline is partly redundant now: a stalled field is already an explicit error via the iteration cap plus the checked results, and an overrun is caught by the pre-commit check.
One coupling worth flagging: cancel-and-retry needs a retry path. checkAqualinkTime() is rate limited to once an hour, so cancelling means no retry until then. That's why a missed boundary currently retargets one minute instead of cancelling — cancelling becomes the better answer once out-of-tolerance observations are coalesced into a pending request, as you describe. Those two pieces need to land together.
What I'd like to keep out of this PR
The shared coordinator and RS/PDA adapters. I think this is the right long-term structure and your split is sound — adapter owns menu semantics and key handling, coordinator owns activation, wall-clock sampling, deadline, boundary wait, commit and cancel. But it rewrites PDA and iAQ Touch paths I have no hardware to test, and turns a bug fix into an architecture change. I'd rather land this, which fixes a live bug on a panel I can actually measure, and do the refactor as a follow-up with a validated reference implementation to port from. PDA also needs its own "stage without committing" helper first, as you note.
The daily maintenance window. Agreed in principle, and the monotonic-clock point is correct — last_checked uses time(0), which is mildly self-defeating in a clock-correcting feature. Two caveats for when it's designed: it interacts with tolerance, since programming once a day means the panel sits up to 15s plus a day's drift however tight the threshold; and it needs the gross-error bypass you describe, or a clock lost at 04:00 stays wrong for 23 hours. On this panel I measured no detectable drift over 13 minutes after a sync, so this is polish rather than a fix — happy to open it as a separate issue.
Measurements, for the record
Three consecutive syncs committed 35 ms, 61 ms and 87 ms after the target boundary. Checked independently against the panel's own rollover: it first reported the new minute 0.3s after the system's :00.
You asked for the commit phase error to be measured rather than assumed — that's it, and it's well under your one-second expectation. The remaining uncertainty is the rollover estimator itself, which carries a ±4s sawtooth because the ~8s display cadence beats against the 60s minute. Verified against a panel provably within 35 ms, so the readings measure the estimator's own error: spread −5..+3s over nine minutes, every value inside the ±5s it reports. Averaging several rollovers would remove that and allow a tighter tolerance; I left it, since 15s already catches anything real.
🤖 Generated with Claude Code
ballle98
left a comment
There was a problem hiding this comment.
Thanks for the thorough update. This revision resolves the important points from my first pass: the programming slot is acquired before sampling the clock, tolerance now follows the actual setter capability, every field result is checked, the target is rechecked after menu navigation, and a missed target is no longer committed when doing so would make the clock no better.
The measured/derived staging budget is a good improvement over learning the previous run and, given the 44-second power-loss case, is more suitable than an unconditional 15-second limit. I found one remaining edge case in the retarget path, noted inline. I would still consider enforcing an elapsed deadline derived from the calculated staging time plus bounded slack, so unexpectedly slow-but-progressing responses cannot keep the transaction open without limit.
The shared PDA coordinator and once-daily quiet-window policy can reasonably remain follow-up work, as discussed. The exact PR head b7d0548 builds successfully with make; no automated checks are currently attached to the PR.
AI-assisted review: GPT-5.6.
| (int)(target - now), buf, stage_secs); | ||
| goto settime_failed; | ||
| } | ||
| target += 60; |
There was a problem hiding this comment.
When this adds 60 seconds, tm_target changes but presses and stage_secs still describe the old target. That is not always a one-press difference: crossing from an xx:59 target to (xx+1):00 can turn a nearby minute into 58 or 59 KEY_LEFT presses because setAqualinkNumericField_ex() does not wrap numeric fields. The cached estimate can therefore be short by roughly 20 seconds, and this reachability check may accept a new boundary that staging cannot reach. Please recompute the press count and staging duration after every retarget, then confirm the new target is still reachable before programming the fields. A focused test around a panel showing 12:58 or 12:59 while retargeting from 12:59 to 1:00 would cover the discontinuity.
Review: the retarget path updated tm_target but left presses and stage_secs describing the old target. Confirmed, and it is not a one-press difference - the numeric fields do not wrap, so from a panel showing 12:58 a 12:59 target costs 5s of staging and 13:00 costs 26s: panel 12:58 PM target 12:59 -> 5 presses, 5s panel 12:58 PM target 13:00 -> 63 presses, 26s panel 12:30 PM target 12:31 -> 5 presses, 5s (mid-hour, for contrast) panel 12:30 PM target 12:32 -> 6 presses, 6s The retarget now steps out a minute at a time recomputing the cost for each candidate, and abandons if none is reachable. The same staleness existed in the initial target selection, which was not raised: presses/stage_secs described the second pass's target rather than the one finally chosen. It happened to err generous, but only for a subtle reason. Both now go through settime_plan(), which iterates to a fixed point and guarantees on return that presses/stage/lead describe the target it hands back and that the target leaves room for them. Swept every 7th second of an hour-crossing window against five panel positions to check both invariants. Also adds the elapsed deadline asked for: stage_secs + 15s slack, enforced per iteration in the field setters. The iteration cap bounds keypresses, not time, so a panel answering slowly but steadily could hold the menu for minutes. Note this deliberately cannot interrupt a wait already in progress - waitForMessage() uses pthread_cond_wait() with no timeout, so a panel that goes completely silent still parks the thread. Bounding that needs pthread_cond_timedwait() across every menu-navigation caller, which is out of scope here. Fixing the staleness was a prerequisite for the deadline: a stale stage_secs would have handed the setters a deadline far too tight and aborted good syncs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Confirmed and fixed in
The retarget now steps out a minute at a time recomputing the cost for each candidate, and abandons if none is reachable. The same staleness was in the initial target selection, which I don't think you spotted: On severity, for the record: the stale figure could accept an unreachable boundary, but it needed navigation to overrun far enough that even the Elapsed deadlineAdded: One limit worth being explicit about: this cannot interrupt a wait already in progress. Fixing the staleness turned out to be a prerequisite for the deadline, incidentally: a stale TestingSix local harnesses now, all passing against the real compiled functions: forced-sync scenarios, rollover estimator, hour string/parse round-trip, setter routing across seven panel configurations, derived staging cost, and this retarget/deadline set. You mentioned no automated checks are attached to the PR. These are throwaway harnesses that 🤖 Generated with Claude Code |
|
I reviewed I built the exact PR head successfully. The acknowledged inability to interrupt a completely silent AI-assisted review/comment: GPT-5.6. |
|
One useful option for validating the follow-up PDA implementation: an RS panel supports all of the terminal protocols, including PDA emulation, so the RS8 used to test this PR should also be able to exercise For a temporary PDA-mode test, keep the physical panel size/type equivalent but configure the terminal as PDA, for example: panel_type = PD-8 Combo
device_id = 0x60Run That should let the same hardware measure PDA final-commit timing and home-screen rollover behavior instead of leaving the PDA portion entirely untested. AI-assisted review/comment: GPT-5.6. |
|
I may be missing something here, but this seems massively complicated implimentation and won't fix the underlying issue for a wide range of panels. Seems a far simpler solution would be to allow the user to configure windows when time can be automatically set (ie between 1am and 6am). Then simply holding up the last enter key press until the next minute. (obviously panel could time out, so may need to hit up / down to stop that). But basically implementing what the original PR was trying to avoid ("panel isn't held in a programming menu"), but allowing it to happen at pre-defined time when AqualinkD wouldn't be in use and there for not holding anything up. This would also work for ALL protocols very simply. The only protocol where this would work reliable across multiple panels would the the iaqtouch protocol, since that only takes a few key entries. Again, I may be missing something, please help me understand. |
|
That is probably a better way to do it. I just wanted to avoid long menu hold ups when it was adjusting the time. I've been running this for a few weeks and the drift on the RS-8 is pretty small. It hasn't been out the 15s tolerance yet. So I think waiting until a nightly maintenance period to correct small clock offsets should work. I'll work up that simplification this afternoon. |
|
I generally agree with simplifying this as sfeakes suggests. I think the scheduling policy can be separated from the mechanism that accurately programs the clock. AqualinkD appears to already have most of the external-trigger plumbing: That action is reachable through REST as Rather than adding maintenance-window scheduling and more timing policy inside AqualinkD, could the reduced scope:
A cron job, systemd timer, Home Assistant automation, or other external scheduler could invoke the action during a quiet period, such as nightly or weekly. That avoids adding another scheduler and maintenance-window configuration to AqualinkD while letting each installation choose its own policy. It would also give the validator a deterministic way to force a time update without changing the normal automatic-check interval. Gross errors after a panel reset could remain a separate automatic-remediation policy or follow-up; the accurate setter would be the same whether triggered automatically, through REST, or through MQTT. |
… takes Per review feedback: a keypress round trip is really the RS485 poll cycle, which depends on how many devices share the bus and what else is on it. It varies far too much between installations to derive from a measurement taken on one panel, so the 350ms/press figure this was using could be out by multiples elsewhere - and the deadline built on it would then abort syncs rather than mis-time them. So stop predicting. Stage every field, then simply hold the committing ENTER until the clock reaches the minute that was staged. If staging ran past that minute, step the MINUTE field on by one keypress and aim at the next one. That self-corrects at any bus speed, and it is the same answer for both ways of losing the boundary. Removed with it: settime_plan(), settime_expected_presses(), settime_stage_secs(), panel_display_to_tm(), settime_panel_offset() and the five press-rate constants. allbutton_aq_programmer.c is 162 lines shorter than before. The cost is that the panel now sits in SET TIME for up to a minute, which the earlier design was contorting itself to avoid. Two things make that acceptable: - Routine corrections are confined to a configurable quiet window, default 01:00-06:00, end hour exclusive, wrapping midnight if you want it to. Set both hours the same to allow corrections any time. - A panel more than AQ_TIME_DIFF_URGENT (5 min) out has lost its clock rather than drifted, so it is fixed straight away rather than waiting up to a day. force_panel_time_sync_at_startup bypasses the window too. The panel drops out of SET TIME on its own after a spell of inactivity, so the hold nudges the MINUTE field one step and straight back every 20s - two presses that leave the staged value where it was. It steps down then up (or up then down from minute 0) because the numeric fields do not wrap; a naive +1 from 59 would press LEFT 59 times. Verified on an RS8: the panel held SET TIME for 39 seconds across a nudge without dropping out. A nudge's duration is bus-dependent too, so it is not trusted either: after the hold, if the boundary has gone by more than AQ_SETTIME_COMMIT_SLOP the code steps on a minute rather than knowingly committing late. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found testing the above on an RS8 whose clock had been left at 01/31/98. The
sync corrected it, and the very next check reported the panel as 903,657,588
seconds out.
observe_panel_rollover() only compares the time string. After the clock is
rewritten the panel's reported time JUMPS, and the jump looked like a rollover:
22:19:35 '1:09 PM' last message before the sync
... clock rewritten to 5:20 PM ...
22:20:00 '5:20 PM' date message had NOT caught up, still '01/31/98'
so the minute start was computed from a 1998 date glued to a 2026 time. The
24.6s window was inside the 30s cap, so no existing guard rejected it, and the
scaled tolerance then accepted the result as a valid reading.
Three changes:
- panel_rollover_forget() is called whenever checkAqualinkTime() decides to
rewrite the clock. Clearing _ro_prev_sample also makes the first message after
the change establish a baseline rather than being paired with one from before.
- The rollover figure is sanity checked against the coarse HH:MM one. They
measure the same thing by different routes and cannot legitimately differ by
more than the sum of their errors (+/-30s coarse, +/-16s rollover), so a gap
over AQ_ROLLOVER_SANITY means the rollover is stale or the panel's date and
time messages are describing different moments - they arrive separately. This
catches the date/time skew, which forgetting alone does not.
- A sync about to run clears any pending forced startup sync. In the same log the
startup one-shot came due 17 seconds after the clock had already been set and
took the panel over a second time for nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@sfeakes you're right, and I've rewritten it your way in Conceding the specific pointThe derived timing multiplied a press count by 350ms, measured on exactly one RS8. A keypress round trip is really the RS485 poll cycle, so that constant is a property of one bus with one set of devices on it — on a busier bus it's out by multiples. Worse, I'd layered a deadline on top of it, so the failure mode wasn't a mis-timed clock, it was aborting syncs entirely on any panel slower than mine. That's the same argument I'd made against a fixed 15-second staging limit two reviews ago, and I should have applied it to my own constant. What it does nowStage every field, then hold the committing ENTER until the clock reaches the staged minute. If staging ran past that minute, step MINUTE on by one keypress and aim at the next. Self-correcting at any bus speed, no prediction anywhere. Gone: The quiet window is in as you described — On the menu timing out — measured, not assumedYou flagged it, so I tested it. The hold nudges MINUTE one step and straight back every 20s: two presses that leave the staged value exactly where it was. It steps down then up (or up then down from minute 0) because the numeric fields don't wrap — a naive From the RS8, panel clock deliberately left at 39 seconds in SET TIME across a nudge, no timeout. Commit landed 87 ms after the boundary. An earlier sync in the same run — the one that walked YEAR from 1998, 28 presses — committed 11 ms after its boundary. A nudge's duration is bus-dependent too, so it isn't trusted either: after the hold, if the boundary has gone by more than 5s it steps on a minute rather than knowingly committing late. Two bugs that testing turned up
Second: a pending Where I'd still want your viewThe other protocols aren't free. You said this would work for all of them very simply, and the approach does — but PDA's numeric helper advances fields automatically, so it needs a stage-without-committing variant like the Drift is small in practice. Running this for a few weeks, the RS8 hasn't left the 15s tolerance once — so in normal operation the nightly window will rarely have anything to do, and the interesting case is the power-loss one. 🤖 Generated with Claude Code |
|
ok can you get the changeset to 100-200 lines it's currently 776 |
|
I can remove all of the verbose comments, but getting to 100-200 lines isn't possible unless we want to remove functionality. I could submit a separate PR that fixes the clock setting. I think we all agree setting the clock accurately is the important thing here. I also think the tolerance should be tighter than 2 minutes. Some folks might retain some programming on their panel, and some with cron scheduling in aqualinkd. Having a 2 minute delta could be problematic. The only reliable way to tighten the tolerance is to measure the minute rollover and not just use the latest reported time. There's probably more to discuss on scheduling front. I like that aqualinkd can completely handle my pool automation standalone. I use it with Home Assistant, but I like to push as much of the automation as I can to aqualinkd as it lives in my RS-8 panel and can survive a botched Home Assistant update or misconfiguration. The current state of this PR will automatically correct a clock that is more than 5 minutes off on startup. So, a logical simplification would be to remove the "force_panel_time_sync_at_startup". Also, there is no pride of authorship here. I just wanted a tighter, more reliable time sync on the panel. |
|
@lnxsrt understand. I had the same issue in another project. I started with a PR that was close to 1000 lines and it just sat there no one was going to take the time to review all of it. for this PR I had AI generate mermaid sequence and state diagrams and it's still hard to grasp. Save this branch as a reference and push a MVP PR that just addresses the time accuracy when actively programming time. That small change could be used with an external cron job and existing API to regularly set the clock so it does not drift. Biggest bang for the buck. Then you can create seperate PRs for detecting small drift. Then a PR for scheduling inside of aqualinkd instead of outside using cron. Then work on the optimized scheduling to avoid long programing times. |
|
The important part of this PR would be in allbutton_aq_programmer.c. The function that programs time needs to be modified to read some form of variable and decide if to program “quickley”, or “slow”, the latter would dictate a pause on the last enter at the exact minute change. |
|
Done as suggested, split into small PRs:
Drift detection (minute-rollover timing and a tighter tolerance) can follow as its own PR once these land. Scheduling I'll leave to you, as you suggested. I'm leaving this PR open as the reference branch for now. Close it whenever it stops being useful. 🤖 Generated with Claude Code |
…ed target Adopts three of the refinements from the design discussion on aqualinkd#555. Derived staging cost instead of a learned one. Both field setters step one unit per keypress with no wraparound, so the press count is known from what the panel is displaying: |target - current| for the numeric fields, (t - c + 24) % 24 for the hour. That replaces _settime_walk_secs, which was wrong exactly when it mattered - a run where every field already matched measured ~1s, and the next run after a power loss needs 116 presses. This is also why the suggested fixed MAX_TIME_PROGRAM of 15s is not used. At the measured 264ms per keypress the routine case is ~1s but a power-loss correction needs ~44s of staging, so a 15s deadline would cancel every attempt at exactly the point the sync matters most. The derived figure covers both ends without a constant that is wrong at one of them. Re-check the boundary after SET TIME is confirmed. Navigation is the variable part (select_menu_item retries up to three times), so it is now excluded from the accuracy calculation: the target is verified once the panel is actually showing YEAR, and retargeted a minute if the walk ran long. Retarget rather than cancel because checkAqualinkTime() is rate limited to once an hour, so cancelling here means no retry until then - cancelling becomes correct once out of tolerance observations are coalesced into a pending request. Cancel instead of committing a missed target. Previously an overrun logged a warning and committed anyway, knowingly setting the panel slow. It now commits only when doing so still beats where the panel already is, and otherwise cancels the menu and leaves it for the next check. A stalled field is already an explicit error rather than a degraded commit: setAqualinkNumericField_ex()'s iteration cap returns false and every field result is checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Assisted-by: Codex:gpt-5 Signed-off-by: Lee Ballard <ballle98@gmail.com>
sync_panel_timeleaves the panel clock about a minute slow, and never corrects it. Tested on an RS8 (6520 REV HH).Why
The commit instant decides the accuracy. The panel has no seconds field — it starts its clock at
HH:MM:00the moment the entry is committed.set_allbutton_time()read the clock up front and added a fixed 30s, so the panel ended up behind by however long the menu walk took. Every digit is a keypress that round-trips through the panel display (~0.26s), and the MINUTE field can be up to 59 of them.The measurement was biased the same way.
checkAqualinkTime()parsed the panel'sHH:MMasHH:MM:00, when it actually means "somewhere in that minute". Every comparison read 0–59s slow (+30 average) even for a perfect panel.And it could never self-correct.
ACCEPTABLE_TIME_DIFFwas 120s, so a panel a minute out was always "close enough". That's why one stays a minute slow indefinitely.What changed
Commit on a minute boundary. Pick the boundary, wait for it outside the menu, then walk the fields so the commit lands on it. Waiting outside means the panel isn't held in a programming menu for a minute (where it may time out), and the programming thread isn't claimed either. The walk duration is measured and remembered to decide when to start the next one.
Measured on an RS8 — three consecutive syncs committed 35ms, 61ms and 87ms after the target.
Time the panel's minute rollover instead of reading
HH:MM. The panel repeats its display every ~8s, so the moment the shown minute changes pins its clock to within half a message gap — about ±4s, versus ±30s. That lets the tolerance drop to 15s, scaled up when the rollover is only loosely pinned, and falling back to the old estimate when none is usable.Verified against a panel provably within 35ms of correct, so the readings measure the estimator's own error: spread −5..+3s over nine minutes, every value inside the ±5s it reports.
Fix the hour being set an hour fast.
select_sub_menu_item()waits only for the final target string. Coming out of the DAY field the last message received is still the DAY line, so it pressed RIGHT before it had ever seen the hour, then matched the pre-keypressHOUR n xMthe panel had already queued — committing an hour too far. The result was also ignored, so it failed silently.Replaced with
setAqualinkHourField(), which waits for the value each keypress should produce — the same techniquesetAqualinkNumericField()already uses, and immune to the race. Tested across all 24×24 start/target hour combinations with a stale message present.New config option
force_panel_time_sync_at_startup(defaultno) syncs once per start even when the panel is inside tolerance — useful if your panel loses time when power cycled. Deferred 60s so it doesn't stall startup, and skips the hourly rate limit so arming it actually takes effect.Not covered
iaqtouch,onetouchand PDA set-time paths have the same missing-elapsed-time bug and no+30compensation at all, so they remain ~30s plus programming time slow. I have no hardware to test them on, so I left them alone rather than guess.select_sub_menu_item()'s race is untouched. Its other callers are menu navigation rather than value setting, where a stray keypress is far less damaging. Fixing it properly would mean re-verifying every caller.Rebased onto current
master; builds clean with no new warnings.