Conversation
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>
Assisted-by: Codex:gpt-5 Signed-off-by: Lee Ballard <ballle98@gmail.com>
| // 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; | ||
| } |
There was a problem hiding this comment.
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.
| // 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; | |
| } | |
| } |
There was a problem hiding this comment.
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.
| @@ -1094,12 +1170,21 @@ void *set_allbutton_time( void *ptr ) | |||
| time_t now = time(0); // get time now | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
…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>
|
Review changes are pushed ( This PR on its own:
Run 1's wrong hour is not from this PR. It's the race #560 fixes, and the log shows it exactly: 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 This PR merged with #560:
I switched the option off through the web UI between those two runs. It took effect without a restart, because So I'd suggest merging #560 before this one. Without it, turning 🤖 Generated with Claude Code |
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:00on 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, defaultno. When it's on,set_allbutton_time():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.falserather thanbreak→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 existingset_date_timeaction, 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
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.🤖 Generated with Claude Code