feat: make timezone configurable via NVS and CLI#111
Open
jjacke13 wants to merge 2 commits intomemovai:mainfrom
Open
feat: make timezone configurable via NVS and CLI#111jjacke13 wants to merge 2 commits intomemovai:mainfrom
jjacke13 wants to merge 2 commits intomemovai:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds persistent POSIX timezone support: new timezone APIs (init/set/get) with NVS-backed storage and env application, a serial CLI Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as Serial CLI
participant TZ as tool_get_time
participant NVS as NVS Storage
participant Env as Environment
User->>CLI: set_tz POSIX_TZ
activate CLI
CLI->>CLI: parse & validate args
CLI->>TZ: timezone_set(POSIX_TZ)
activate TZ
TZ->>NVS: nvs_set_str("tz_config","timezone", POSIX_TZ)
activate NVS
NVS-->>TZ: persist ACK
deactivate NVS
TZ->>Env: setenv("TZ", POSIX_TZ, 1) / tzset()
activate Env
Env-->>TZ: env applied
deactivate Env
TZ->>TZ: update s_timezone
TZ-->>CLI: return status
deactivate TZ
CLI->>User: print confirmation
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![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/cli/serial_cli.c (1)
507-511: Prefersizeofover hardcoded loop bound.The loop bound is hardcoded to
6, which matches the current array size but is fragile if namespaces are added/removed in the future.♻️ Proposed fix to use sizeof
const char *namespaces[] = { MIMI_NVS_WIFI, MIMI_NVS_TG, MIMI_NVS_LLM, MIMI_NVS_PROXY, MIMI_NVS_SEARCH, MIMI_NVS_TZ }; - for (int i = 0; i < 6; i++) { + for (size_t i = 0; i < sizeof(namespaces) / sizeof(namespaces[0]); i++) { nvs_handle_t nvs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 507 - 511, The for-loop over the namespaces array currently uses a hardcoded bound of 6; replace that fragile literal by computing the array length with sizeof so the loop uses the real element count (e.g., compute a count from sizeof(namespaces)/sizeof(namespaces[0]) and use it in the for condition). Update the loop that iterates over the const char *namespaces[] (and any related declarations) to use this computed count instead of the literal 6 to ensure correctness if the array is changed.
🤖 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 50-65: timezone_set currently updates s_timezone and calls
setenv/tzset even when NVS operations fail, but it doesn't inform the user;
update timezone_set to check and handle NVS return values: if nvs_open fails,
call ESP_LOGW (or ESP_LOGE) to warn that persistence failed and the change is
temporary; also check the return of nvs_set_str and nvs_commit and log a
warning/error if either fails before closing the handle; keep the in-memory
update and setenv/tzset behavior but ensure any NVS failure emits a clear log
referencing s_timezone and the failing NVS function (nvs_open, nvs_set_str,
nvs_commit) so users know the setting may not persist.
---
Nitpick comments:
In `@main/cli/serial_cli.c`:
- Around line 507-511: The for-loop over the namespaces array currently uses a
hardcoded bound of 6; replace that fragile literal by computing the array length
with sizeof so the loop uses the real element count (e.g., compute a count from
sizeof(namespaces)/sizeof(namespaces[0]) and use it in the for condition).
Update the loop that iterates over the const char *namespaces[] (and any related
declarations) to use this computed count instead of the literal 6 to ensure
correctness if the array is changed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
main/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.examplemain/tools/tool_get_time.cmain/tools/tool_get_time.hmain/tools/tool_registry.c
main/tools/tool_get_time.c
Outdated
Comment on lines
+50
to
+65
| void timezone_set(const char *tz) | ||
| { | ||
| strncpy(s_timezone, tz, sizeof(s_timezone) - 1); | ||
| s_timezone[sizeof(s_timezone) - 1] = '\0'; | ||
|
|
||
| nvs_handle_t nvs; | ||
| if (nvs_open(MIMI_NVS_TZ, NVS_READWRITE, &nvs) == ESP_OK) { | ||
| nvs_set_str(nvs, MIMI_NVS_KEY_TZ, s_timezone); | ||
| nvs_commit(nvs); | ||
| nvs_close(nvs); | ||
| } | ||
|
|
||
| setenv("TZ", s_timezone, 1); | ||
| tzset(); | ||
| ESP_LOGI(TAG, "Timezone updated to: %s", s_timezone); | ||
| } |
There was a problem hiding this comment.
NVS write failure is silently ignored.
If nvs_open fails, the timezone is still updated in memory and applied via setenv/tzset, but the user receives a success message ("Timezone updated to..."). This could lead to confusion when the setting doesn't persist across reboots.
Consider logging a warning when NVS persistence fails so users know the setting is temporary.
📝 Proposed fix to log NVS failure
void timezone_set(const char *tz)
{
strncpy(s_timezone, tz, sizeof(s_timezone) - 1);
s_timezone[sizeof(s_timezone) - 1] = '\0';
nvs_handle_t nvs;
if (nvs_open(MIMI_NVS_TZ, NVS_READWRITE, &nvs) == ESP_OK) {
nvs_set_str(nvs, MIMI_NVS_KEY_TZ, s_timezone);
nvs_commit(nvs);
nvs_close(nvs);
+ } else {
+ ESP_LOGW(TAG, "Failed to persist timezone to NVS");
}
setenv("TZ", s_timezone, 1);
tzset();
ESP_LOGI(TAG, "Timezone updated to: %s", s_timezone);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_get_time.c` around lines 50 - 65, timezone_set currently
updates s_timezone and calls setenv/tzset even when NVS operations fail, but it
doesn't inform the user; update timezone_set to check and handle NVS return
values: if nvs_open fails, call ESP_LOGW (or ESP_LOGE) to warn that persistence
failed and the change is temporary; also check the return of nvs_set_str and
nvs_commit and log a warning/error if either fails before closing the handle;
keep the in-memory update and setenv/tzset behavior but ensure any NVS failure
emits a clear log referencing s_timezone and the failing NVS function (nvs_open,
nvs_set_str, nvs_commit) so users know the setting may not persist.
Contributor
|
My idea here is that u can just tell Mimi that your time zone in memory system now. |
Author
this was the default format that the code was accepting. I think it is POSIX format |
Author
You mean write an instruction in MEMORY.md ? I tried that but almost all reminders that the agent scheduled via cron jobs, either never hit, or came at completely wrong times. I also made changes to the code to accept custom providers and i tried with various models (minimax, glm, gemini). |
jjacke13
force-pushed
the
feat/configurable-timezone
branch
from
799f4e2 to
d02e1e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
main/tools/tool_get_time.c (1)
55-64:⚠️ Potential issue | 🟡 MinorPropagate persistence failures back to the CLI.
timezone_set()always applies the in-memory change and logs success, even if the NVS open/write/commit path fails.cmd_set_tzthen prints a success message for a setting that may disappear on reboot.Verification: inspect the setter and the CLI caller. Expected result: the NVS return values are not checked and the CLI success path is unconditional.
#!/bin/bash rg -n -C2 'timezone_set|nvs_open|nvs_set_str|nvs_commit|Timezone set\.' main/tools/tool_get_time.c main/cli/serial_cli.c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_get_time.c` around lines 55 - 64, timezone_set currently applies setenv/tzset and logs success regardless of NVS failures; change timezone_set to check and propagate NVS return values from nvs_open/nvs_set_str/nvs_commit (return an esp_err_t or bool), only call setenv/tzset and ESP_LOGI("Timezone updated...") if the NVS operations succeed, and update cmd_set_tz to inspect the returned status and report success or failure to the CLI accordingly; reference the timezone_set function and its use in cmd_set_tz and the NVS calls nvs_open, nvs_set_str, nvs_commit so you update both the setter and the caller to handle and surface errors.
🧹 Nitpick comments (1)
main/cli/serial_cli.c (1)
571-575: Avoid hardcoding the namespace count.The new
MIMI_NVS_TZentry is paired with a literal6. Deriving the loop bound fromnamespaceskeepsconfig_resetfrom drifting the next time a namespace is added or removed.♻️ Proposed fix
- for (int i = 0; i < 6; i++) { + for (size_t i = 0; i < sizeof(namespaces) / sizeof(namespaces[0]); ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 571 - 575, The loop over the namespaces array is using a hardcoded count (for (int i = 0; i < 6; i++)) which will break when entries like MIMI_NVS_TZ are added/removed; change the loop to derive its bound from the namespaces array length (e.g., compute element count with sizeof(namespaces)/sizeof(namespaces[0]) or an existing ARRAY_SIZE macro) and iterate using that value in the function that calls config_reset so the loop over namespaces remains correct when the array changes.
🤖 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/cli/serial_cli.c`:
- Line 563: The timezone row currently shows “(empty) [not set]” even when
timezone_init() will actually use the compile-time fallback MIMI_TIMEZONE;
update the config display logic so the "Timezone" entry reports the effective
runtime value: in config_show (or in print_config when key ==
MIMI_NVS_TZ/MIMI_NVS_KEY_TZ) detect the case where NVS and MIMI_SECRET_TIMEZONE
are empty and substitute the fallback MIMI_TIMEZONE (or call the runtime
accessor that returns the resolved timezone after timezone_init()) so the CLI
matches the runtime timezone.
In `@main/tools/tool_get_time.c`:
- Around line 17-18: s_timezone is vulnerable to silent truncation and accepts
empty strings; update the setter (set_tz) to validate the input length before
copying: reject zero-length strings and any string whose byte length is >=
sizeof(s_timezone) (or otherwise would overflow), return an error instead of
truncating, and only copy when the size is valid (use a bounded copy that
ensures NUL termination). Also ensure callers of set_tz and timezone_init treat
a rejected/empty set_tz as “not configured” (so device state remains consistent
across reboots) and add clear error reporting when input is invalid.
---
Duplicate comments:
In `@main/tools/tool_get_time.c`:
- Around line 55-64: timezone_set currently applies setenv/tzset and logs
success regardless of NVS failures; change timezone_set to check and propagate
NVS return values from nvs_open/nvs_set_str/nvs_commit (return an esp_err_t or
bool), only call setenv/tzset and ESP_LOGI("Timezone updated...") if the NVS
operations succeed, and update cmd_set_tz to inspect the returned status and
report success or failure to the CLI accordingly; reference the timezone_set
function and its use in cmd_set_tz and the NVS calls nvs_open, nvs_set_str,
nvs_commit so you update both the setter and the caller to handle and surface
errors.
---
Nitpick comments:
In `@main/cli/serial_cli.c`:
- Around line 571-575: The loop over the namespaces array is using a hardcoded
count (for (int i = 0; i < 6; i++)) which will break when entries like
MIMI_NVS_TZ are added/removed; change the loop to derive its bound from the
namespaces array length (e.g., compute element count with
sizeof(namespaces)/sizeof(namespaces[0]) or an existing ARRAY_SIZE macro) and
iterate using that value in the function that calls config_reset so the loop
over namespaces remains correct when the array changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac7a0841-672a-40a3-851d-5e58b50b99e2
📒 Files selected for processing (6)
main/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.examplemain/tools/tool_get_time.cmain/tools/tool_get_time.hmain/tools/tool_registry.c
🚧 Files skipped from review as they are similar to previous changes (3)
- main/tools/tool_get_time.h
- main/mimi_secrets.h.example
- main/tools/tool_registry.c
The timezone was hardcoded as MIMI_TIMEZONE. This adds runtime configuration following the same pattern as WiFi, API keys, and model: build-time default in mimi_secrets.h, overridable at runtime via NVS with the new `set_tz` CLI command. Timezone is also applied at startup via timezone_init() so the system no longer runs in UTC until the first get_current_time call.
- Reject empty and oversize strings in timezone_set() with proper error codes - config_show now displays the effective runtime timezone via timezone_get() instead of showing "(empty)" when the hardcoded fallback is active - CLI handler checks timezone_set() return and reports errors
jjacke13
force-pushed
the
feat/configurable-timezone
branch
from
4f22ec9 to
eac4c55
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main/tools/tool_get_time.c (1)
64-69:⚠️ Potential issue | 🟠 MajorHandle and surface NVS persistence failures in
timezone_set.At Line 65–68,
nvs_set_str/nvs_commitreturn values are ignored. A failed write can leave runtime TZ updated but not persisted, with no user-visible signal.🛠️ Suggested fix
nvs_handle_t nvs; - if (nvs_open(MIMI_NVS_TZ, NVS_READWRITE, &nvs) == ESP_OK) { - nvs_set_str(nvs, MIMI_NVS_KEY_TZ, s_timezone); - nvs_commit(nvs); - nvs_close(nvs); - } + esp_err_t nvs_err = nvs_open(MIMI_NVS_TZ, NVS_READWRITE, &nvs); + if (nvs_err == ESP_OK) { + nvs_err = nvs_set_str(nvs, MIMI_NVS_KEY_TZ, s_timezone); + if (nvs_err == ESP_OK) { + nvs_err = nvs_commit(nvs); + } + nvs_close(nvs); + } + if (nvs_err != ESP_OK) { + ESP_LOGW(TAG, "Timezone applied but not persisted (%s)", esp_err_to_name(nvs_err)); + }#!/bin/bash # Read-only verification: inspect timezone_set persistence path and return-code handling. rg -n -C3 'esp_err_t timezone_set|nvs_open\(MIMI_NVS_TZ, NVS_READWRITE, &nvs\)|nvs_set_str\(nvs, MIMI_NVS_KEY_TZ, s_timezone\)|nvs_commit\(nvs\)' main/tools/tool_get_time.c🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_get_time.c` around lines 64 - 69, timezone_set currently ignores NVS return codes for nvs_open, nvs_set_str and nvs_commit which can leave the runtime TZ changed but not persisted; update timezone_set to check and handle each esp_err_t from nvs_open, nvs_set_str and nvs_commit (use the returned error to log via your logger and return/propagate a failure code from timezone_set), ensure nvs_close is still called on opened handles (use a local nvs handle variable as now), and include the failing function name and esp_err_t value in the error message so callers can detect/present persistence failures for MIMI_NVS_TZ and MIMI_NVS_KEY_TZ when writing s_timezone.
🧹 Nitpick comments (1)
main/cli/serial_cli.c (1)
601-605: Avoid hardcoded namespace count in reset loop.Line 605 hardcodes
6, which can drift from thenamespaces[]array length and silently skip/reset wrong entries after future edits.♻️ Suggested refactor
- for (int i = 0; i < 6; i++) { + for (size_t i = 0; i < (sizeof(namespaces) / sizeof(namespaces[0])); i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 601 - 605, The reset loop hardcodes the count (for (int i = 0; i < 6; i++)) which can drift from the actual number of entries in the namespaces[] array; change the loop to compute the number of elements dynamically (e.g., using sizeof(namespaces)/sizeof(namespaces[0]) or a helper like ARRAY_SIZE) and iterate up to that computed length so all namespace entries (MIMI_NVS_WIFI, MIMI_NVS_TG, MIMI_NVS_LLM, MIMI_NVS_PROXY, MIMI_NVS_SEARCH, MIMI_NVS_TZ) are always handled correctly by the reset logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@main/tools/tool_get_time.c`:
- Around line 64-69: timezone_set currently ignores NVS return codes for
nvs_open, nvs_set_str and nvs_commit which can leave the runtime TZ changed but
not persisted; update timezone_set to check and handle each esp_err_t from
nvs_open, nvs_set_str and nvs_commit (use the returned error to log via your
logger and return/propagate a failure code from timezone_set), ensure nvs_close
is still called on opened handles (use a local nvs handle variable as now), and
include the failing function name and esp_err_t value in the error message so
callers can detect/present persistence failures for MIMI_NVS_TZ and
MIMI_NVS_KEY_TZ when writing s_timezone.
---
Nitpick comments:
In `@main/cli/serial_cli.c`:
- Around line 601-605: The reset loop hardcodes the count (for (int i = 0; i <
6; i++)) which can drift from the actual number of entries in the namespaces[]
array; change the loop to compute the number of elements dynamically (e.g.,
using sizeof(namespaces)/sizeof(namespaces[0]) or a helper like ARRAY_SIZE) and
iterate up to that computed length so all namespace entries (MIMI_NVS_WIFI,
MIMI_NVS_TG, MIMI_NVS_LLM, MIMI_NVS_PROXY, MIMI_NVS_SEARCH, MIMI_NVS_TZ) are
always handled correctly by the reset logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d37ea8b-3662-4a29-9cb3-7f13f170e5e9
📒 Files selected for processing (6)
main/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.examplemain/tools/tool_get_time.cmain/tools/tool_get_time.hmain/tools/tool_registry.c
🚧 Files skipped from review as they are similar to previous changes (2)
- main/mimi_config.h
- main/mimi_secrets.h.example
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MIMI_TIMEZONE "PST8PDT,M3.2.0,M11.1.0"— now configurable at runtimemimi_secrets.h, overridable via NVS + CLItimezone_init(), so the system no longer runs in UTC until the firstget_current_timecallChanges
mimi_config.hMIMI_SECRET_TIMEZONE,MIMI_NVS_TZ,MIMI_NVS_KEY_TZmimi_secrets.h.exampleMIMI_SECRET_TIMEZONEentrytool_get_time.htimezone_init(),timezone_set(),timezone_get()tool_get_time.cMIMI_TIMEZONEtool_registry.ctimezone_init()at startupserial_cli.cset_tzcommand, show timezone inconfig_show, include inconfig_resetUsage
Persists across reboots via NVS. Priority: NVS →
MIMI_SECRET_TIMEZONE→MIMI_TIMEZONE(hardcoded fallback).Summary by CodeRabbit