feat: sync system clock via SNTP on boot#178
Conversation
The ESP32 has no battery-backed RTC, so on every cold boot the clock
sits at the Unix epoch until something sets it. Today the only path
that updates the clock is the get_current_time tool, which only runs
when the LLM decides to call it. That leaves every early-boot
code path (session log timestamps, cron, anything that reads time())
with bogus values until the user happens to message the bot.
This patch adds a small time_sync module that starts SNTP against
pool.ntp.org as soon as WiFi has an IP, sets MIMI_TIMEZONE, and
exposes time_sync_wait() / time_sync_is_set() for callers that need
to gate on a valid clock.
main/time/time_sync.{h,c} new module (~80 LoC)
main/CMakeLists.txt register the new source
main/mimi.c kick time_sync_start() right after
wifi_manager_wait_connected() succeeds
esp_netif (already in REQUIRES) provides esp_netif_sntp.h, so no
new component dependency is needed. SNTP runs in the background:
time_sync_start() returns immediately and the sync_cb logs when
the first sample arrives.
📝 WalkthroughWalkthroughThis PR adds SNTP-based system time synchronization for an ESP device. A new ChangesSNTP Time Synchronization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
main/mimi.c (1)
154-188: ⚡ Quick winConsider briefly waiting for first sync before starting time-sensitive services.
cron_service_start()(Line 186) andheartbeat_start()(Line 187) are launched immediately aftertime_sync_start()returns, but SNTP typically needs 1–3 s to deliver the first sample. Any cron evaluation or heartbeat tick that fires in that window still sees epoch time — the exact failure mode this PR sets out to fix. The newtime_sync_wait()API exists for this purpose; a short bounded wait (e.g. 3–5 s) here would close the gap without meaningfully delaying boot.⏱️ Sketch
/* Start network-dependent services */ ESP_ERROR_CHECK(agent_loop_start()); ESP_ERROR_CHECK(telegram_bot_start()); ESP_ERROR_CHECK(feishu_bot_start()); + /* Best-effort: let SNTP land before time-sensitive services tick. */ + if (!time_sync_wait(5000)) { + ESP_LOGW(TAG, "Proceeding without confirmed time sync"); + } cron_service_start(); heartbeat_start();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main/mimi.c` around lines 154 - 188, The code starts cron_service_start() and heartbeat_start() immediately after time_sync_start(), allowing services to run before SNTP has provided a valid time; call time_sync_wait() with a short bounded timeout (e.g. 3000–5000 ms) after time_sync_start() and before cron_service_start() and heartbeat_start() to block until first sync or timeout, and handle the return (log on timeout/failure but proceed) so time-sensitive services see a sane RTC before they run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/mimi.c`:
- Line 156: time_sync_start() return value is discarded; capture its esp_err_t
result in main (where time_sync_start() is called) and handle failures by
logging the error (e.g., using ESP_LOGE or processLogger equivalent) or
propagating the error upward; specifically, assign the result of
time_sync_start() to a variable, check for != ESP_OK, and emit a clear error
message that includes the error code and context (e.g., "time_sync_start
failed") so failures like ESP_ERR_NO_MEM or SNTP init errors are visible at
boot.
---
Nitpick comments:
In `@main/mimi.c`:
- Around line 154-188: The code starts cron_service_start() and
heartbeat_start() immediately after time_sync_start(), allowing services to run
before SNTP has provided a valid time; call time_sync_wait() with a short
bounded timeout (e.g. 3000–5000 ms) after time_sync_start() and before
cron_service_start() and heartbeat_start() to block until first sync or timeout,
and handle the return (log on timeout/failure but proceed) so time-sensitive
services see a sane RTC before they run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a8a755e-d6e4-4d04-be2c-069d59b29282
📒 Files selected for processing (4)
main/CMakeLists.txtmain/mimi.cmain/time/time_sync.cmain/time/time_sync.h
| ESP_LOGI(TAG, "WiFi connected: %s", wifi_manager_get_ip()); | ||
| /* Kick SNTP as soon as we have an IP so the RTC is set | ||
| * before time-sensitive code (session logs, cron, etc.). */ | ||
| time_sync_start(); |
There was a problem hiding this comment.
Don't silently discard time_sync_start() failure.
time_sync_start() returns esp_err_t and can fail with ESP_ERR_NO_MEM or a propagated SNTP init error. Discarding it makes a broken time sync indistinguishable from a working one at runtime — the only signal will be the absence of the "System time synchronized" log a few seconds later. Log the failure (or surface it) so it's diagnosable from boot output.
🪵 Proposed fix
/* Kick SNTP as soon as we have an IP so the RTC is set
* before time-sensitive code (session logs, cron, etc.). */
- time_sync_start();
+ esp_err_t sntp_err = time_sync_start();
+ if (sntp_err != ESP_OK) {
+ ESP_LOGW(TAG, "Time sync start failed: %s", esp_err_to_name(sntp_err));
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main/mimi.c` at line 156, time_sync_start() return value is discarded;
capture its esp_err_t result in main (where time_sync_start() is called) and
handle failures by logging the error (e.g., using ESP_LOGE or processLogger
equivalent) or propagating the error upward; specifically, assign the result of
time_sync_start() to a variable, check for != ESP_OK, and emit a clear error
message that includes the error code and context (e.g., "time_sync_start
failed") so failures like ESP_ERR_NO_MEM or SNTP init errors are visible at
boot.
1 participant
Summary
The ESP32-S3 has no battery-backed RTC, so on every cold boot
time(NULL)returns 0 (Unix epoch) until something explicitly sets the clock. Today the only code path that sets the clock is theget_current_timetool (main/tools/tool_get_time.c:54), which only runs when the LLM decides to call it.That leaves every early-boot consumer of
time()with bogus values until the user happens to message the bot:time(NULL)may fire immediately or never.This PR adds a tiny
time_syncmodule that starts SNTP againstpool.ntp.orgas soon as WiFi has an IP, appliesMIMI_TIMEZONE, and exposes two helpers for callers that need to gate on a valid clock:What's in the diff
main/time/time_sync.{h,c}— new ~80-LoC module wrappingesp_netif_sntp_init.main/CMakeLists.txt— register the new source.esp_netifis already inREQUIRES, so no new component dependency.main/mimi.c— calltime_sync_start()right afterwifi_manager_wait_connected()succeeds (and only on that path, so onboarding-mode boots are unaffected).SNTP runs in the background and logs
System time synchronized: ...from the sync callback when the first sample arrives (typically 1–3 seconds afterWIFI_CONNECTED). Callingtime_sync_start()more than once is a no-op.Test plan
idf.py fullclean && idf.py buildidf.py -p <PORT> flash monitortime_sync: System time synchronized: YYYY-MM-DD HH:MM:SSlog appears within ~5 s ofwifi: Connected!1970-01-01Summary by CodeRabbit