Fix: Add Time Initialization and Unix Timestamp#87
Fix: Add Time Initialization and Unix Timestamp#87wjc1207 wants to merge 0 commit intomemovai:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCron startup now synchronizes system time (calls tool_get_time up to 5 times with 1s backoff, aborting the cron task on persistent failure) and moves recomputation of Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Task
participant Tool as tool_get_time_execute
participant Scheduler as Job Scheduler
participant Logger as Logger
participant Kernel as OS
Cron->>Tool: tool_get_time_execute(buf[64])
alt success
Tool-->>Cron: time set
Cron->>Scheduler: recompute next_run for enabled EVERY/AT jobs
Scheduler-->>Cron: next_run updated
Cron->>Cron: continue init and enter main loop
else failure (retry up to 5x, 1s backoff)
loop up to 5 retries
Tool-->>Cron: failure
Cron->>Cron: wait 1s and retry
end
alt still failing
Cron->>Cron: clear cron task handle
Cron->>Logger: log "time sync failed"
Cron->>Kernel: delete self and return
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/cron/cron_service.c`:
- Around line 306-308: tool_get_time_execute's return value is ignored—capture
its return code and ensure successful time sync before entering the main cron
loop: call tool_get_time_execute(NULL, time_buf, sizeof(time_buf)) in a retry
loop (e.g., max retries and exponential/backoff sleep) and only proceed to start
the cron loop after it returns success; if retries exhaust, either abort the
cron task (return/error) or surface a fatal log via the existing logger. Use the
symbols tool_get_time_execute and time_buf to locate the call and gate the "main
cron loop" entry so the loop never starts on failed time sync.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/cron/cron_service.c`:
- Around line 314-317: The task self-deletes on time-sync failure but leaves the
global handle s_cron_task non-NULL; before calling vTaskDelete(NULL) in the
error branch (inside the time sync check where ESP_LOGE(TAG, "Time sync failed;
cron will not start") is logged), set s_cron_task = NULL so future
cron_service_start() can restart the task and cron_service_stop() won't try to
delete an invalid handle; update the code around the vTaskDelete(NULL) call to
clear s_cron_task first.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/cron/cron_service.c`:
- Around line 306-320: cron_service_start currently computes each job->next_run
using time(NULL) before the task performs time sync in cron_task_main, so
next_run can be stale; fix by ensuring next_run is recalculated after a
successful time sync (or perform time sync before computing next_run).
Specifically, either move the tool_get_time_execute/settimeofday logic into
cron_service_start before the loop that sets job->next_run, or add a recompute
step inside cron_task_main immediately after the ESP_OK return from
tool_get_time_execute that iterates enabled jobs and recalculates job->next_run
(the same logic used in cron_service_start) so cron_process_due_jobs won't
trigger everything at once. Use the functions/fields cron_service_start,
cron_task_main, tool_get_time_execute, settimeofday, cron_process_due_jobs,
job->next_run and time(NULL) to locate where to apply the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main/cron/cron_service.c (1)
309-313: Unnecessary 1-second delay after the final failed attempt.
vTaskDelayis called unconditionally inside the loop, so the 5th failed attempt still waits a full second before falling through to the error check. Move the delay to fire only when another attempt will follow.♻️ Proposed fix
for (int attempt = 0; attempt < 5; ++attempt) { err = tool_get_time_execute(NULL, time_buf, sizeof(time_buf)); if (err == ESP_OK) break; - vTaskDelay(pdMS_TO_TICKS(1000)); // backoff + if (attempt < 4) { + vTaskDelay(pdMS_TO_TICKS(1000)); // backoff + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cron/cron_service.c` around lines 309 - 313, The loop calling tool_get_time_execute repeatedly unconditionally calls vTaskDelay even after the final (5th) attempt; change the logic in the retry loop that uses variables/identifiers attempt, err, tool_get_time_execute, vTaskDelay and pdMS_TO_TICKS so the delay is executed only when another attempt will occur (e.g., wrap vTaskDelay in a conditional like if (attempt < 4) or if (attempt < max_attempts - 1)); keep the same retry count and backoff duration but avoid the 1-second sleep after the last failed attempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/cron/cron_service.c`:
- Around line 322-333: The recompute loop in cron_service.c leaves CRON_KIND_AT
jobs with at_epoch <= now enabled and with next_run <= 0, creating a
dead/disabled-once-past job; update the loop that iterates s_jobs so that for
job->kind == CRON_KIND_AT you set job->next_run = job->at_epoch when
job->at_epoch > now and otherwise mark the job disabled (job->enabled = false)
to mirror compute_initial_next_run behavior (referencing s_jobs, cron_job_t,
job->enabled, job->next_run, job->kind, CRON_KIND_AT, job->at_epoch, and
compute_initial_next_run).
---
Nitpick comments:
In `@main/cron/cron_service.c`:
- Around line 309-313: The loop calling tool_get_time_execute repeatedly
unconditionally calls vTaskDelay even after the final (5th) attempt; change the
logic in the retry loop that uses variables/identifiers attempt, err,
tool_get_time_execute, vTaskDelay and pdMS_TO_TICKS so the delay is executed
only when another attempt will occur (e.g., wrap vTaskDelay in a conditional
like if (attempt < 4) or if (attempt < max_attempts - 1)); keep the same retry
count and backoff duration but avoid the 1-second sleep after the last failed
attempt.
|
Thanks for your contribution! Could you squash your commit into 1 commit? It is easy to revert or cherry-pick. |
|
@coderabbitai review the code |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/tools/tool_get_time.c`:
- Around line 59-63: Store strftime's return value into a size_t (e.g., size_t r
= strftime(out, out_size, "%Y-%m-%d %H:%M:%S %Z (%A)", &local)); check r and
handle the zero case by ensuring the buffer is null-terminated (if out_size>0
set out[0] = '\0') and use r as the current length when computing the append
offset (size_t len = (r == 0 ? 0 : r)); then call snprintf(out + len, out_size >
len ? out_size - len : 0, " [epoch: %lld]", (long long)t) so you never call
strlen on an unterminated buffer and never write past out_size; apply this
change around the strftime/snprintf sequence (symbols: strftime, out, out_size,
local, t, snprintf).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
main/cron/cron_service.cmain/tools/tool_get_time.c
🚧 Files skipped from review as they are similar to previous changes (1)
- main/cron/cron_service.c
wjc1207
changed the title
wjc1207
changed the title
wjc1207
changed the title
wjc1207
force-pushed
the
main
branch
2 times, most recently
from
6e0b6eb to
2b77999
Compare
IRONICBo
left a comment
IRONICBo
left a comment
There was a problem hiding this comment.
The idea is right — cron needs valid time before computing next_run — but a few issues:
-
Conflicts with #85 which also touches
tool_get_time.c. The epoch append inparse_and_set_timechanges the tool output format fed to the LLM. This changes the contract for all callers, not just cron. If the LLM needs epoch, consider returning it in a separate field. -
The
CRON_KIND_ATwithat_epoch <= nowcase now setsjob->enabled = false— this silently disables jobs that were supposed to fire on startup. Needs an explicit design decision. -
Minor: the retry loop sleeps 1s unconditionally after the 5th failure.
if (attempt < 4) vTaskDelay(...)is cleaner.
Please resolve the conflict with #85 and clarify the epoch output format change.
wjc1207
force-pushed
the
main
branch
4 times, most recently
from
e213f18 to
1d9196d
Compare
|
@IRONICBo The |
wjc1207
force-pushed
the
main
branch
3 times, most recently
from
8f8d41e to
394c247
Compare
2 participants
🚀 Fix: Reliable Time Initialization & UNIX Timestamp Support
Summary
This PR guarantees that system time is fully initialized and synchronized before the cron service starts. It prevents incorrect scheduling after cold boots or unexpected resets and improves robustness of time-dependent features.
🧩 Problem
Previously, the cron service could start while the system clock was still at the default epoch (1970-01-01). This caused:
✅ Solution
This PR introduces a strict time initialization flow and adds native UNIX timestamp support.
Key improvements
🔧 Implementation Details
Startup flow
Behavior changes
next_runrecomputed on cron start📊 Before vs After
Before
cron_service_startcould run at1970-01-01After
cron_service_startalways runs with valid system time🤖 CodeRabbit Summary
Bug Fixes
New Features