Skip to content

Add sync_time_accurately to set the RS panel clock on the minute - #559

Open
lnxsrt wants to merge 2 commits into
aqualinkd:masterfrom
lnxsrt:fix/accurate-panel-time-set
Open

lnxsrt wants to merge 2 commits into
aqualinkd:masterfrom
lnxsrt:fix/accurate-panel-time-set

Conversation

@lnxsrt

@lnxsrt lnxsrt commented Sep 22, 2026

Copy link
Copy Markdown

Smaller replacement for the time-programming part of #555, as suggested there.

Problem

The SET TIME menu has no seconds field, and the panel starts its clock at HH:MM:00 on the final ENTER. set_allbutton_time() adds 30s and commits whenever the keypresses finish, so the panel lands up to a minute out.

Change

New option sync_time_accurately, default no. When it's on, set_allbutton_time():

  1. aims at a minute boundary,
  2. stages YEAR, MONTH, DAY and HOUR as it does today, then sets MINUTE without the final ENTER,
  3. holds that ENTER until the boundary arrives.

Staging time depends on the RS485 bus, so it isn't predicted. If staging runs past the boundary, MINUTE is stepped on one and the next boundary is used instead. While it's holding, MINUTE is nudged one step and back every 20s so the panel doesn't drop out of SET TIME.

With the option off, the behaviour is unchanged.

Two supporting changes to the numeric setter:

  • setAqualinkNumericField_noenter() sets a field without accepting it.
  • The iteration cap now returns false rather than break → return true, so a field that was never set isn't reported as set. Existing callers ignore the result, so nothing else changes.

Use

Set sync_time_accurately = yes. The existing set_date_time action, reachable from REST (/api/set_date_time), MQTT (<topic>/set_date_time/set) and WebSocket, can then be called from cron or Home Assistant at a quiet time. The hourly automatic check picks up the option too.

Testing

On an RS8 running the equivalent logic on #555's branch: commits landed 11 ms and 87 ms after the boundary, including a walk from 01/31/98. The panel held SET TIME for 39s across a nudge without timing out.

On this exact branch: it builds clean and the option parses both ways. It still needs a hardware run, which I'll post here.

Follow-ups, as separate PRs

  • The hour field can be set an hour fast when select_sub_menu_item() acts on a stale DAY message. It's independent of this change, so it's going in its own small PR.
  • Drift detection: minute-rollover timing and a tolerance tighter than 2 minutes.

🤖 Generated with Claude Code

The SET TIME menu has no seconds field and the panel starts its clock at HH:MM:00
on the final ENTER. set_allbutton_time() adds 30s and commits whenever the
keypresses finish, so the panel lands up to a minute out.

With sync_time_accurately = yes (default no), it aims at a minute boundary,
stages every field, and holds the final ENTER until that minute arrives. Staging
time depends on the RS485 bus so it is not predicted: if staging ran past the
minute, MINUTE is stepped on one and the next boundary used. While holding, MINUTE
is nudged one step and back every 20s so the panel does not time out of SET TIME.

The default path is unchanged. Supporting changes:
- setAqualinkNumericField_noenter() sets a field without accepting it.
- The iteration cap in the numeric setter returns false instead of breaking to
  'return true', so a field that was never set is no longer reported as set.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ballle98 added a commit to ballle98/AqualinkD that referenced this pull request Sep 23, 2026
Assisted-by: Codex:gpt-5
Signed-off-by: Lee Ballard <ballle98@gmail.com>
Comment thread source/allbutton_aq_programmer.c Outdated
Comment on lines +1108 to +1127
// Wait for 'target', nudging MINUTE one step and back so the menu stays open. The numeric
// fields do not wrap, so from 0 step up rather than down.
static bool settime_hold_until(struct aqualinkdata *aqdata, time_t target, int min)
{
time_t next_nudge = time(0) + AQ_SETTIME_KEEPALIVE;
int other = (min > 0) ? min - 1 : min + 1;

while (time(0) < target) {
if (isAqualinkDStopping())
return false;
if (time(0) >= next_nudge && target - time(0) > AQ_SETTIME_NUDGE_GUARD) {
if (! setAqualinkNumericField_noenter(aqdata, "MINUTE", other) ||
! setAqualinkNumericField_noenter(aqdata, "MINUTE", min))
return false;
next_nudge = time(0) + AQ_SETTIME_KEEPALIVE;
}
delay(100);
}
return true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current delay(100) polls ten times per second and limits commit precision to that interval. This uses one interruptible nanosleep() per real event. The target remains wall-clock time, while the keepalive deadline uses CLOCK_MONOTONIC, so a system-time correction cannot suppress menu keepalives. Please also add <errno.h>, <time.h>, and "timespec_subtract.h" to the includes.

Suggested change
// Wait for 'target', nudging MINUTE one step and back so the menu stays open. The numeric
// fields do not wrap, so from 0 step up rather than down.
static bool settime_hold_until(struct aqualinkdata *aqdata, time_t target, int min)
{
time_t next_nudge = time(0) + AQ_SETTIME_KEEPALIVE;
int other = (min > 0) ? min - 1 : min + 1;
while (time(0) < target) {
if (isAqualinkDStopping())
return false;
if (time(0) >= next_nudge && target - time(0) > AQ_SETTIME_NUDGE_GUARD) {
if (! setAqualinkNumericField_noenter(aqdata, "MINUTE", other) ||
! setAqualinkNumericField_noenter(aqdata, "MINUTE", min))
return false;
next_nudge = time(0) + AQ_SETTIME_KEEPALIVE;
}
delay(100);
}
return true;
}
static bool settime_sleep_for(struct timespec remaining)
{
while (nanosleep(&remaining, &remaining) != 0) {
if (errno != EINTR || isAqualinkDStopping())
return false;
}
return !isAqualinkDStopping();
}
// Wait for target, nudging MINUTE one step and back so the menu stays open.
static bool settime_hold_until(struct aqualinkdata *aqdata, time_t target, int min)
{
const struct timespec target_time = { .tv_sec = target, .tv_nsec = 0 };
struct timespec next_nudge;
if (clock_gettime(CLOCK_MONOTONIC, &next_nudge) != 0)
return false;
next_nudge.tv_sec += AQ_SETTIME_KEEPALIVE;
for (;;) {
struct timespec realtime;
struct timespec monotonic;
struct timespec until_target;
struct timespec until_nudge;
struct timespec sleep_for;
bool nudge_due;
bool can_nudge;
if (isAqualinkDStopping())
return false;
if (clock_gettime(CLOCK_REALTIME, &realtime) != 0 ||
clock_gettime(CLOCK_MONOTONIC, &monotonic) != 0)
return false;
if (timespec_subtract(&until_target, &target_time, &realtime) ||
(until_target.tv_sec == 0 && until_target.tv_nsec == 0))
return true;
can_nudge = until_target.tv_sec > AQ_SETTIME_NUDGE_GUARD ||
(until_target.tv_sec == AQ_SETTIME_NUDGE_GUARD &&
until_target.tv_nsec > 0);
nudge_due =
timespec_subtract(&until_nudge, &next_nudge, &monotonic);
if (can_nudge && nudge_due) {
int other = min > 0 ? min - 1 : min + 1;
if (!setAqualinkNumericField_noenter(aqdata, "MINUTE", other) ||
!setAqualinkNumericField_noenter(aqdata, "MINUTE", min))
return false;
if (clock_gettime(CLOCK_MONOTONIC, &next_nudge) != 0)
return false;
next_nudge.tv_sec += AQ_SETTIME_KEEPALIVE;
continue;
}
sleep_for = until_target;
if (can_nudge && !nudge_due &&
(until_nudge.tv_sec < sleep_for.tv_sec ||
(until_nudge.tv_sec == sleep_for.tv_sec &&
until_nudge.tv_nsec < sleep_for.tv_nsec)))
sleep_for = until_nudge;
if (!settime_sleep_for(sleep_for))
return false;
}
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted as suggested in 4461656, with the includes you listed. I also added one guard for the opposite case: if the host clock steps backwards, the wall-clock target moves further away and the keepalive would keep SET TIME open indefinitely. It now gives up if the target is ever more than AQ_SETTIME_MAX_HOLD (AQ_SETTIME_LEAD + 60 = 75 s) away. A real target can't be: it's chosen at most LEAD + 59 s out, and each step adds at most 60 s after the previous boundary has passed.

On the RS8: ENTER queued 0.61, 0.62 and 0.67 ms after the boundary, where the 100 ms polling gave 11–87 ms. Nudges came exactly 20.000 s apart on the monotonic timer. The longest hold was 71.7 s with two nudges, and the panel didn't time out.

Comment thread source/allbutton_aq_programmer.c Outdated
@@ -1094,12 +1170,21 @@ void *set_allbutton_time( void *ptr )
time_t now = time(0); // get time now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acquiring the active programming task before choosing the target is correct. I would move this clock capture a little later, immediately after select_menu_item(aqdata, "SET TIME") succeeds and before YEAR is staged. Menu navigation can take an installation-dependent amount of time, so including it in the target wait makes the chosen boundary unnecessarily stale.

The complete target date and time still needs to be chosen before any fields are written, not immediately before the final ENTER. Computing localtime_r(target) after entering SET TIME handles hour/day rollover, including a next-day target, before YEAR/MONTH/DAY/HOUR/MINUTE are staged. If field staging then crosses an hour boundary, cancel/retry is safer than changing only MINUTE because the accepted date/hour would no longer match.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 4461656. Both paths now read the clock once select_menu_item("SET TIME") succeeds. They also both use localtime_r(), because moving the read later widened the window in which another thread's localtime() call (logging, the scheduler, mongoose) could overwrite the shared buffer. The default path is otherwise unchanged. It just samples a few seconds later, so its +30 s estimate is slightly less stale.

The full target date and time is still chosen before any field is written. Crossing an hour was already handled: settime_commit_on_boundary() only ever steps MINUTE within the hour already accepted, and it cancels at min == 59 rather than changing MINUTE across an hour.


send_cmd(KEY_ENTER);

if (! _aqconfig_.sync_time_accurately) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking/pre-existing: YEAR, MONTH, DAY, and HOUR immediately above still ignore their setters' return values. This PR did not introduce that behavior, and the accurate path does check the new MINUTE staging/commit result. However, now that the numeric setter correctly reports its iteration-limit failure as false, these older calls still discard that useful result and can continue sending programming commands after a helper has cancelled the menu. This would be worth addressing in a separate follow-up rather than expanding this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll do this as a follow-up once #559 and #560 are in. #560 already checks HOUR's result, so the follow-up covers YEAR, MONTH, DAY and the default path's MINUTE. It touches the same lines as both PRs, so it's easier after they've merged.

…T TIME

- Hold with nanosleep() until the target instead of polling every 100ms, so
  the ENTER goes out within about a millisecond of the boundary rather than
  anywhere up to 100ms after it. The keepalive timer uses CLOCK_MONOTONIC so a
  system clock correction cannot stop the nudges. (Suggested in review.)
- Give up if the target is ever more than AQ_SETTIME_MAX_HOLD away. A backwards
  clock step moves the wall-clock target further off, and the keepalive would
  otherwise hold SET TIME open indefinitely. A legitimate target is never more
  than AQ_SETTIME_LEAD + 59s out.
- Read the clock after SET TIME is confirmed, for both paths, so however long
  the menu walk takes is not part of the wait. Both paths now use localtime_r(),
  as localtime()'s buffer is shared with other threads. The default path's
  behaviour is otherwise the same; it now samples a few seconds later, which
  makes its +30s estimate slightly less stale.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lnxsrt

lnxsrt commented Sep 24, 2026

Copy link
Copy Markdown
Author

Review changes are pushed (4461656). Here are the hardware results from the RS8, triggered with /api/set_date_time.

This PR on its own:

ENTER after the boundary Panel shows
run 1 0.61 ms 6:35 PM at 5:35 ❌
run 2 0.62 ms 5:36 PM ✅ (nudge at 20.001 s, 44.6 s hold)

Run 1's wrong hour is not from this PR. It's the race #560 fixes, and the log shows it exactly:

41.958  'DAY 24'      -> ENTER queued
42.180  RIGHT queued  <- pressed before any HOUR line had arrived
42.225  'HOUR 5 PM'   <- the reply to the DAY ENTER, from before the RIGHT
42.443  ENTER queued  <- select_sub_menu_item matched it and accepted...
42.490  'HOUR 6 PM'   <- ...the hour the RIGHT had already moved to

It happens whenever the panel's hour already matches the target, which is the normal case for a routine resync. Run 2 only came out right because the panel was on 6 PM by then, so the walk went the long way round and landed on 5 PM. The default path makes the same select_sub_menu_item() call, so it's exposed the same way.

This PR merged with #560:

Hour step ENTER after the boundary Panel shows
accurate no RIGHT before HOUR 5 PM, accepted directly 0.67 ms 5:57 PM ✅ (two nudges, 71.7 s hold)
default no RIGHT before HOUR 5 PM, accepted directly n/a (+30 s estimate) 5:58 PM ✅ (17.6 s fast, as the estimate allows)

I switched the option off through the web UI between those two runs. It took effect without a restart, because set_allbutton_time() reads it each time.

So I'd suggest merging #560 before this one. Without it, turning sync_time_accurately on commits within a millisecond of the boundary but can still land on the wrong hour. The two conflict only trivially, and I'll rebase whichever goes second.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants