feat(Core): add automatic date and time sync via NTP#576
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
90c0957 to
2be4ee6
Compare
GitHub Actions
Inactive
2be4ee6 to
351ff6f
Compare
GitHub Actions
Inactive
351ff6f to
6912e59
Compare
c41f0f0 to
56568ac
Compare
GitHub Actions
Inactive
6912e59 to
049909a
Compare
1a2f418 to
bd6957c
Compare
049909a to
953ff8b
Compare
GitHub Actions
Inactive
bd6957c to
48ed868
Compare
953ff8b to
02c0478
Compare
GitHub Actions
Inactive
02c0478 to
71f34e6
Compare
48ed868 to
c976c0e
Compare
GitHub Actions
Inactive
c976c0e to
8053cce
Compare
71f34e6 to
85800d1
Compare
8053cce to
98a8377
Compare
GitHub Actions
Inactive
85800d1 to
9a44dd9
Compare
GitHub Actions
Inactive
Change-Id: ec43ed615ec2a61436a44abca62e59bf Change-Id-Short: lnvwlmtyulnx
9a44dd9 to
6ab3a5a
Compare
GitHub Actions
Inactive
dosubot
Bot
added
size:XL
|
Documentation Updates 1 document(s) were updated by changes in this PR: indexView Changes@@ -47,6 +47,20 @@
```toml
auto-share = false
+```
+
+### `auto-time`
+
+✏️
+
+Automatically synchronize the device time and timezone via NTP when WiFi connects.
+
+Uses `time.cloudflare.com` for NTP time sync and `ipapi.co` for timezone detection.
+
+A manual "Sync Time" option is also available in the clock menu (top menu) that allows you to manually trigger time synchronization even if automatic sync is disabled.
+
+```toml
+auto-time = false
```
### `auto-suspend`How did I do? Any feedback? |
WalkthroughThis PR implements automatic and manual device time synchronization: RTC is refactored for thread-safe sharing, Device exposes RTC and a TimeManager, TimeManager detects timezone (ipapi.co), queries NTP (sntpc/time.cloudflare.com), updates system clock and RTC, a TimeSync background task is added and scheduled from TaskManager on NetUp or manual menu select, UI/settings and i18n entries are added, and app/emulator wiring is updated to use Context. Sequence Diagram(s)sequenceDiagram
participant User
participant UI as EntryId::SyncTime
participant TaskMgr as TaskManager
participant TimeTask as TimeSyncTask
participant TimeMgr as TimeManager
participant IPAPI as ipapi.co
participant NTP as time.cloudflare.com
participant SysClock as SystemClock
participant Rtc
User->>UI: Select "Sync Time"
UI->>TaskMgr: dispatch Select(EntryId::SyncTime)
TaskMgr->>TimeTask: schedule/start TimeSyncTask(manual=true)
TimeTask->>TimeMgr: sync(ntp_host, manual, hub)
TimeMgr->>IPAPI: detect timezone
IPAPI-->>TimeMgr: timezone
TimeMgr->>NTP: query_ntp(host)
NTP-->>TimeMgr: offset
TimeMgr->>SysClock: settimeofday(adjusted time)
TimeMgr->>Rtc: set_time(synced time)
TimeMgr-->>TaskMgr: emit Event::ClockTick
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
🧹 Nitpick comments (1)
crates/core/src/task/mod.rs (1)
535-541: ⚡ Quick winConsider logging when time manager is unavailable.
If
CURRENT_DEVICE.time_manager()fails, the function silently returns. Adding a debug or warn log would help troubleshoot why time sync isn't working.💡 Optional improvement
if let Ok(time_manager) = crate::device::CURRENT_DEVICE.time_manager() { let task = Box::new(time_sync::TimeSyncTask::new(time_manager, manual)); if let Err(e) = self.start(task, hub.clone()) { tracing::warn!(error = %e, "failed to start time sync task"); } + } else { + tracing::debug!("time manager not available, skipping time sync"); }🤖 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 `@crates/core/src/task/mod.rs` around lines 535 - 541, The code silently skips starting the time sync when crate::device::CURRENT_DEVICE.time_manager() returns Err; update the branch to log that failure (e.g., using tracing::debug or tracing::warn) including the error so callers can see why TimeSyncTask wasn't created; locate the check around CURRENT_DEVICE.time_manager() and add a tracing::warn!(error = %e, "time manager unavailable, skipping time sync") (or similar) in the Err arm before returning, keeping the existing start(task, hub.clone()) logic intact.
🤖 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 `@contrib/Settings-sample.toml`:
- Line 13: Update the sample config comment that currently reads "# This will
also set the correct timezon, it uses" to fix the typo "timezon" -> "timezone"
so the user-facing comment is clear; locate the comment line beginning with that
text in contrib/Settings-sample.toml and replace "timezon" with "timezone".
In `@crates/core/src/context.rs`:
- Around line 71-74: CURRENT_DEVICE.rtc() is being silently swallowed by .ok(),
causing AlarmManager::new not to run and alarm-backed features to disappear;
instead, match on the Result from CURRENT_DEVICE.rtc() (do not call .ok()) and
handle errors explicitly: if the error indicates an unsupported platform keep
alarm_manager = None, otherwise log or propagate the real error (including the
error value) so failures to open /dev/rtc0 are visible; adjust the code around
alarm_manager and AlarmManager::new to use that match and ensure real RTC init
errors are not ignored.
In `@crates/core/src/device/time.rs`:
- Around line 33-52: The non-Kobo branch of the cfg_select in
set_system_timezone currently calls unimplemented!() which panics; change that
branch to perform a real no-op and return Ok(()) so Device::set_system_timezone
remains platform-agnostic. Locate the cfg_select block around
set_system_timezone (symbols: set_system_timezone, cfg_select!) and replace the
`_ => unimplemented!("set_system_timezone is only available on Kobo devices")`
arm with a simple `Ok(())` return, preserving the function's Result return type
and avoiding any panic on non-Kobo builds.
In `@crates/core/src/task/mod.rs`:
- Line 530: Fix the typo in the warning log message: locate the tracing::warn!
call that currently reads "Time sync task alredy running, not scheduling" and
change "alredy" to "already" so the message becomes "Time sync task already
running, not scheduling"; no other logic changes required.
In `@crates/core/src/time_manager.rs`:
- Around line 50-51: The manual sync currently uses `?` on `set_system_clock`
and `rtc.set_time`, which causes failures to propagate as generic errors and be
only logged by `TimeSyncTask::run()` instead of triggering a user notification;
change the `sync()` implementation so that instead of using `?` for these calls
you catch/map their errors into a distinct, descriptive error variant (e.g.,
`TimeSyncError::ManualSyncFailed` or similar) that includes the original
error/context (step: "set_system_clock" or "rtc.set_time"), and return that Err
from `sync()` so `TimeSyncTask::run()` can detect this variant and surface the
manual-sync notification; update any error construction in the `sync()` function
accordingly to preserve the underlying error details.
- Around line 26-29: The manual-path error notification is sending the raw
backend error to the UI; change the NotificationEvent::Show call inside
detect_and_set_timezone()'s Err branch (the block guarded by if manual {
hub.send(...) }) to use a localized user-facing string via the fl! macro (add
use crate::fl; if missing) and move the detailed error e into a log call (e.g.,
error! or tracing::error!) so the UI sees fl!("...") while logs retain
e.to_string() for debugging.
- Around line 98-109: query_ntp currently binds an IPv4 socket
(UdpSocket::bind("0.0.0.0:0")) then takes the first host.to_socket_addrs()
result which can be IPv6 and cause a mismatch; change the logic to iterate the
resolved SocketAddrs from host.to_socket_addrs(), for each addr choose or create
a UDP socket bound to the matching family (IPv4 -> 0.0.0.0:0, IPv6 -> [::]:0),
wrap it with UdpSocketWrapper and call sntpc::sync::get_time(addr, &socket,
context) until one succeeds (or return the last error); alternatively
prefer/filter IPv4 addresses first before trying IPv6 to avoid DNS-order
roulette, and keep using NTP_TIMEOUT and the existing
NtpContext/StdTimestampGen.
---
Nitpick comments:
In `@crates/core/src/task/mod.rs`:
- Around line 535-541: The code silently skips starting the time sync when
crate::device::CURRENT_DEVICE.time_manager() returns Err; update the branch to
log that failure (e.g., using tracing::debug or tracing::warn) including the
error so callers can see why TimeSyncTask wasn't created; locate the check
around CURRENT_DEVICE.time_manager() and add a tracing::warn!(error = %e, "time
manager unavailable, skipping time sync") (or similar) in the Err arm before
returning, keeping the existing start(task, hub.clone()) logic intact.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a820d4f-83ac-4633-9ad0-9bde57c360b0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
contrib/Settings-sample.tomlcrates/cadmus/src/app.rscrates/core/Cargo.tomlcrates/core/i18n/en-GB/cadmus_core.ftlcrates/core/src/context.rscrates/core/src/device/mod.rscrates/core/src/device/time.rscrates/core/src/lib.rscrates/core/src/rtc.rscrates/core/src/settings/mod.rscrates/core/src/task/mod.rscrates/core/src/task/time_sync.rscrates/core/src/time_manager.rscrates/core/src/view/common.rscrates/core/src/view/mod.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/view/settings_editor/kinds/mod.rscrates/emulator/src/main.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
crates/core/**/*.rs
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable >no_run>ignore. Useno_runwhen the example compiles but cannot execute (file I/O, network, database). Useignoreonly for private/pub(crate)items unreachable from the test harness with a comment explaining why. Use#to hide boilerplate setup lines. Verify withcargo test --doc.
Use typed SQL macros (sqlx::query!,sqlx::query_as!,sqlx::query_scalar!) in Rust code. Never use untypedsqlx::query()/sqlx::query_as()/sqlx::query_scalar(). Use.flatten()onquery_scalar!results for nullable columns (Option<Option<T>>→Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtimeORDER BYcolumn) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use thefl!macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include:label()implementations onSettingKindtraits, input fieldlabelstrings inEvent::OpenNamedInput, menu entry text (EntryKind::Command,EntryKind::RadioButton, etc.), and button labels, notification text.
Files:
crates/core/src/lib.rscrates/core/src/view/settings_editor/kinds/mod.rscrates/core/src/view/common.rscrates/core/src/task/time_sync.rscrates/core/src/device/mod.rscrates/core/src/view/mod.rscrates/core/src/settings/mod.rscrates/core/src/device/time.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/rtc.rscrates/core/src/time_manager.rs
crates/core/**/*.{sql,rs}
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
All SQL queries must list explicit column names. Do not use
SELECT *.
Files:
crates/core/src/lib.rscrates/core/src/view/settings_editor/kinds/mod.rscrates/core/src/view/common.rscrates/core/src/task/time_sync.rscrates/core/src/device/mod.rscrates/core/src/view/mod.rscrates/core/src/settings/mod.rscrates/core/src/device/time.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/rtc.rscrates/core/src/time_manager.rs
crates/core/**/*.{ftl,rs}
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
Every user-visible string needs a Fluent message ID in
crates/core/i18n/en-GB/cadmus_core.ftl. Usefl!("message-id")at the call site. For parameterised strings, use Fluent variables ({ $var }) in the.ftlfile and pass values viafl!("id", var = value).
Files:
crates/core/src/lib.rscrates/core/i18n/en-GB/cadmus_core.ftlcrates/core/src/view/settings_editor/kinds/mod.rscrates/core/src/view/common.rscrates/core/src/task/time_sync.rscrates/core/src/device/mod.rscrates/core/src/view/mod.rscrates/core/src/settings/mod.rscrates/core/src/device/time.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/rtc.rscrates/core/src/time_manager.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Prefer?operator overunwrap()/expect()in library and app code
Usethiserrorfor custom error types andanyhowfor ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use&stroverStringin function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoidunsafeunless required and documented in Rust
Avoid prematurecollect()— keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g.,Fingerprint(String)instead of bareString)
WrapString,i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
ImplementDisplay,FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO,FIXME,HACK,NOTE) are acceptable with context in Rust
Use structured fields with thetracingcrate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Usedebuglog level for development and troubleshooting detail
Useinfolog level for important runtime events
Usewarnlog level for recoverable issues (retries, fallbacks)
Useerrorlog level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...
Files:
crates/core/src/lib.rscrates/core/src/view/settings_editor/kinds/mod.rscrates/core/src/view/common.rscrates/core/src/task/time_sync.rscrates/core/src/device/mod.rscrates/core/src/view/mod.rscrates/core/src/settings/mod.rscrates/core/src/device/time.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/rtc.rscrates/emulator/src/main.rscrates/cadmus/src/app.rscrates/core/src/time_manager.rs
crates/core/**/*.ftl
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
crates/core/**/*.ftl: Keep Fluent message keys (.ftlfile keys) sorted alphabetically within each comment section.
Fluent message key naming convention: use kebab-case, prefixed by feature area. Usesettings-<category>-<description>for settings labels,settings-<category>-<description>-inputfor input fields, andnotification-<description>for notifications.
Files:
crates/core/i18n/en-GB/cadmus_core.ftl
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Use caret requirements for most dependencies in Cargo.toml
Alphabetize dependencies within logical groups in Cargo.toml
Usedefault-features = falsewhen fine-grained control is needed in Cargo.toml
Keep related crate families at compatible versions (e.g. allopentelemetry-*crates) in Cargo.toml
Files:
crates/core/Cargo.toml
🧠 Learnings (13)
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”
Applied to files:
crates/core/src/lib.rscrates/core/src/view/settings_editor/kinds/mod.rscrates/core/src/view/common.rscrates/core/src/task/time_sync.rscrates/core/src/device/mod.rscrates/core/src/view/mod.rscrates/core/src/settings/mod.rscrates/core/src/device/time.rscrates/core/src/view/settings_editor/category.rscrates/core/src/view/settings_editor/kinds/general.rscrates/core/src/view/settings_editor/kinds/identity.rscrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/rtc.rscrates/core/src/time_manager.rs
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.{ftl,rs} : Every user-visible string needs a Fluent message ID in `crates/core/i18n/en-GB/cadmus_core.ftl`. Use `fl!("message-id")` at the call site. For parameterised strings, use Fluent variables (`{ $var }`) in the `.ftl` file and pass values via `fl!("id", var = value)`.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftl
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.ftl : Keep Fluent message keys (`.ftl` file keys) sorted alphabetically within each comment section.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftl
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.ftl : Fluent message key naming convention: use kebab-case, prefixed by feature area. Use `settings-<category>-<description>` for settings labels, `settings-<category>-<description>-input` for input fields, and `notification-<description>` for notifications.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftl
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to Cargo.toml : Define dependency versions in root `Cargo.toml` under `[workspace.dependencies]`
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to **/Cargo.toml : Use caret requirements for most dependencies in Cargo.toml
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to **/Cargo.toml : Keep related crate families at compatible versions (e.g. all `opentelemetry-*` crates) in Cargo.toml
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to **/Cargo.toml : Alphabetize dependencies within logical groups in Cargo.toml
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: After modifying `Cargo.toml`, run `cargo xtask clippy` and `cargo xtask test --features default`
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to **/Cargo.toml : Use `default-features = false` when fine-grained control is needed in Cargo.toml
Applied to files:
crates/core/Cargo.toml
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*{test,tests}*.rs : Tests must use the shared `create_test_context` helper from `crate::context::test_helpers` instead of redefining it. Tests may wrap it for additional setup but must not reimplement base `Context` construction.
Applied to files:
crates/core/src/context.rscrates/core/src/task/mod.rscrates/emulator/src/main.rscrates/cadmus/src/app.rs
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*view*.rs : All `handle_event` methods in view components must have OpenTelemetry tracing attributes with the exact signature: `#[cfg_attr(feature = "tracing", tracing::instrument(skip(self, hub, bus, rq, context), fields(event = ?evt), ret(level=tracing::Level::TRACE)))]`, gated behind the `tracing` feature. The `skip()` parameter names must exactly match the actual parameter names, including underscore prefixes.
Applied to files:
crates/core/src/task/mod.rscrates/emulator/src/main.rscrates/cadmus/src/app.rs
📚 Learning: 2026-05-28T14:01:38.823Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-28T14:01:38.823Z
Learning: Applies to **/*.rs : Use `thiserror` for custom error types and `anyhow` for ad-hoc errors in Rust
Applied to files:
crates/core/src/rtc.rs
🪛 GitHub Actions: PR `#576` / 2_Analyze (rust).txt
crates/cadmus/src/app.rs
[warning] 124-124: CodeQL autobuild: macro expansion failed for 'format_err'.
[warning] 198-198: CodeQL autobuild: macro expansion failed for 'debug'.
[warning] 210-210: CodeQL autobuild: macro expansion failed for 'tracing::error'.
🔇 Additional comments (20)
crates/core/src/task/mod.rs (6)
47-47: LGTM!
98-100: LGTM!Also applies to: 122-123
59-64: LGTM!
399-428: LGTM!
666-666: LGTM!Also applies to: 845-867, 882-903
444-446: Fluent message IDnotification-not-onlineis present.
crates/core/i18n/en-GB/cadmus_core.ftldefinesnotification-not-online = WiFi must be connected for this action., matching thefl!("notification-not-online")notification.crates/core/src/task/time_sync.rs (1)
1-33: LGTM!crates/cadmus/src/app.rs (1)
25-25: LGTM!Also applies to: 788-788
crates/emulator/src/main.rs (1)
565-565: LGTM!Also applies to: 922-924
crates/core/src/settings/mod.rs (1)
259-259: LGTM!Also applies to: 999-999
crates/core/src/view/mod.rs (1)
865-865: LGTM!crates/core/src/view/common.rs (2)
343-347: LGTM!
346-346: Fluent message ID exists (top-menu-sync-time).top-menu-sync-time = Sync Timeis defined incrates/core/i18n/en-GB/cadmus_core.ftl.crates/core/src/view/settings_editor/category.rs (1)
4-5: LGTM!Also applies to: 67-67
crates/core/src/view/settings_editor/kinds/general.rs (2)
336-336: Fluent message ID check passes
fl!("settings-general-auto-time")maps tosettings-general-auto-time = Automatic Time Syncincrates/core/i18n/en-GB/cadmus_core.ftl(line 51), so the message ID exists.
327-363: AutoTime enum/Fluent references are present (no compile risk)
SettingIdentity::AutoTimeandToggleSettings::AutoTimeare defined incrates/core/src/view/settings_editor/kinds/mod.rs.settings-general-auto-time,settings-general-toggle-on, andsettings-general-toggle-offexist incrates/core/i18n/en-GB/cadmus_core.ftl.crates/core/i18n/en-GB/cadmus_core.ftl (1)
22-22: LGTM!Also applies to: 35-35, 51-51
crates/core/src/view/settings_editor/kinds/identity.rs (1)
23-23: LGTM!crates/core/src/view/settings_editor/kinds/mod.rs (1)
38-39: LGTM!crates/core/src/device/mod.rs (1)
170-177:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't panic on non-Kobo startup here.
Context::new()now callsCURRENT_DEVICE.rtc()unconditionally, so thisunimplemented!()turns emulator/non-Kobo startup into a hard panic before the caller can recover. Return anError a platform-specific no-op instead.Based on learnings, the PR objective says emulator builds should make automatic time handling a no-op, and
crates/core/src/context.rs:71-74pluscrates/emulator/src/main.rs:78-110route startup through this method.⛔ Skipped due to learnings
Learnt from: OGKevin Repo: OGKevin/cadmus PR: 540 File: crates/core/src/device/power/stub.rs:10-17 Timestamp: 2026-05-29T08:34:32.872Z Learning: In `crates/core/src/device/power/stub.rs`, the `StubPowerManager` intentionally uses `unimplemented!()` (panic) in both `suspend` and `resume` instead of returning `Err(...)`. This is by design: forgetting to implement a proper power manager for a target platform is treated as a developer/programming error that should crash hard ("fail-fast"), not a recoverable runtime error. Do not suggest replacing these panics with `Err` returns.Learnt from: CR Repo: OGKevin/cadmus PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-05-28T14:01:38.823Z Learning: Panic on errors in the emulator code path to catch issues early during developmentLearnt from: CR Repo: OGKevin/cadmus PR: 0 File: crates/core/AGENTS.md:0-0 Timestamp: 2026-05-23T17:00:50.322Z Learning: Applies to crates/core/**/*{test,tests}*.rs : Tests must use the shared `create_test_context` helper from `crate::context::test_helpers` instead of redefining it. Tests may wrap it for additional setup but must not reimplement base `Context` construction.Learnt from: OGKevin Repo: OGKevin/cadmus PR: 511 File: crates/core/build.rs:129-131 Timestamp: 2026-05-25T12:43:50.793Z Learning: In `crates/core/build.rs`, the `generate_bundled_assets()` function intentionally skips missing asset directories (`if !dir.exists() { return; }`) to support emulator/non-Kobo builds where OTA is not used and asset directories may not be present. Any fail-fast guard for missing required directories (e.g., `bin/`, `resources/`, `hyphenation-patterns/`) should be gated on the Kobo target (`TARGET == "arm-unknown-linux-gnueabihf"`) to avoid breaking emulator builds.
Change-Id: ec67e185bd8d5eed57ce57904f0563b1 Change-Id-Short: lntslyruomrm
1293e8d to
36e0a93
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/core/src/time_manager.rs (1)
130-132: 💤 Low valueSilent continuation on
set_read_timeoutfailure loses context.When
set_read_timeoutfails, the loop continues without updatinglast_err. If all addresses fail due to timeout configuration issues, the final error message won't reflect this root cause.🔧 Suggested improvement
- if socket.set_read_timeout(Some(NTP_TIMEOUT)).is_err() { - continue; + if let Err(e) = socket.set_read_timeout(Some(NTP_TIMEOUT)) { + last_err = Some(anyhow::anyhow!("set_read_timeout failed for {bind_addr}: {e}")); + continue; }🤖 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 `@crates/core/src/time_manager.rs` around lines 130 - 132, The loop silently continues when socket.set_read_timeout(Some(NTP_TIMEOUT)) fails, which omits updating last_err and hides timeout configuration failures; update the error handling in the loop that uses socket/set_read_timeout/NTP_TIMEOUT to set last_err with the returned error (or a contextual error including the set_read_timeout failure) before continuing so the final error reflects this root cause, ensuring any subsequent logging or the returned error uses that updated last_err.
🤖 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.
Nitpick comments:
In `@crates/core/src/time_manager.rs`:
- Around line 130-132: The loop silently continues when
socket.set_read_timeout(Some(NTP_TIMEOUT)) fails, which omits updating last_err
and hides timeout configuration failures; update the error handling in the loop
that uses socket/set_read_timeout/NTP_TIMEOUT to set last_err with the returned
error (or a contextual error including the set_read_timeout failure) before
continuing so the final error reflects this root cause, ensuring any subsequent
logging or the returned error uses that updated last_err.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d5e94765-68ab-4dc9-9f92-a6ed59f951ed
📒 Files selected for processing (5)
contrib/Settings-sample.tomlcrates/core/i18n/en-GB/cadmus_core.ftlcrates/core/src/context.rscrates/core/src/task/mod.rscrates/core/src/time_manager.rs
✅ Files skipped from review due to trivial changes (1)
- contrib/Settings-sample.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/core/src/context.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
crates/core/**/*.{ftl,rs}
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
Every user-visible string needs a Fluent message ID in
crates/core/i18n/en-GB/cadmus_core.ftl. Usefl!("message-id")at the call site. For parameterised strings, use Fluent variables ({ $var }) in the.ftlfile and pass values viafl!("id", var = value).
Files:
crates/core/i18n/en-GB/cadmus_core.ftlcrates/core/src/task/mod.rscrates/core/src/time_manager.rs
crates/core/**/*.ftl
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
crates/core/**/*.ftl: Keep Fluent message keys (.ftlfile keys) sorted alphabetically within each comment section.
Fluent message key naming convention: use kebab-case, prefixed by feature area. Usesettings-<category>-<description>for settings labels,settings-<category>-<description>-inputfor input fields, andnotification-<description>for notifications.
Files:
crates/core/i18n/en-GB/cadmus_core.ftl
crates/core/**/*.rs
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
crates/core/**/*.rs: Rustdoc examples should follow this preference order: fully compilable >no_run>ignore. Useno_runwhen the example compiles but cannot execute (file I/O, network, database). Useignoreonly for private/pub(crate)items unreachable from the test harness with a comment explaining why. Use#to hide boilerplate setup lines. Verify withcargo test --doc.
Use typed SQL macros (sqlx::query!,sqlx::query_as!,sqlx::query_scalar!) in Rust code. Never use untypedsqlx::query()/sqlx::query_as()/sqlx::query_scalar(). Use.flatten()onquery_scalar!results for nullable columns (Option<Option<T>>→Option<T>).
Untyped SQL queries are allowed only for dynamic SQL (e.g. runtimeORDER BYcolumn) if and only if the function has unit tests covering every dynamic path, with a comment explaining why the typed macro cannot be used.
All user-visible strings must use thefl!macro (use crate::fl;). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include:label()implementations onSettingKindtraits, input fieldlabelstrings inEvent::OpenNamedInput, menu entry text (EntryKind::Command,EntryKind::RadioButton, etc.), and button labels, notification text.
Files:
crates/core/src/task/mod.rscrates/core/src/time_manager.rs
crates/core/**/*.{sql,rs}
📄 CodeRabbit inference engine (crates/core/AGENTS.md)
All SQL queries must list explicit column names. Do not use
SELECT *.
Files:
crates/core/src/task/mod.rscrates/core/src/time_manager.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Prefer?operator overunwrap()/expect()in library and app code
Usethiserrorfor custom error types andanyhowfor ad-hoc errors in Rust
Use iterators over index-based loops in Rust
Use&stroverStringin function parameters when ownership is not needed
Prefer borrowing over cloning in Rust
Inline expressions directly into struct fields — avoid intermediate bindings solely to pass them into a struct literal
Avoidunsafeunless required and documented in Rust
Avoid prematurecollect()— keep iterators lazy in Rust
Ensure Rust code compiles without warnings
Prefer newtype wrappers over raw primitives for domain concepts (e.g.,Fingerprint(String)instead of bareString)
WrapString,i64, and similar primitives when the value represents a specific domain concept (fingerprints, file kinds, identifiers, etc.)
ImplementDisplay,FromStr, and other standard traits on newtype wrappers to keep them ergonomic
Match on enum variants instead of string comparisons — the compiler catches missing arms
Comment why, not what. Avoid inline comments — extract code into well-named, documented functions instead
Avoid dead-code comments (commented-out code) in Rust
Avoid changelog comments (Modified by X on date) in Rust
Avoid divider comments (//=====) in Rust
Annotations (TODO,FIXME,HACK,NOTE) are acceptable with context in Rust
Use structured fields with thetracingcrate — never use string formatting for log data
Use only structured fields for logging — data goes in fields, not format strings
Do not use module prefixes in logs — instrumentation scope provides context
Do not mix structured fields with format arguments in logging
Usedebuglog level for development and troubleshooting detail
Useinfolog level for important runtime events
Usewarnlog level for recoverable issues (retries, fallbacks)
Useerrorlog level for failures requiring attention
Return domain types directly from sqlx queries instead of parsin...
Files:
crates/core/src/task/mod.rscrates/core/src/time_manager.rs
🧠 Learnings (6)
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.{ftl,rs} : Every user-visible string needs a Fluent message ID in `crates/core/i18n/en-GB/cadmus_core.ftl`. Use `fl!("message-id")` at the call site. For parameterised strings, use Fluent variables (`{ $var }`) in the `.ftl` file and pass values via `fl!("id", var = value)`.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftlcrates/core/src/time_manager.rs
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.ftl : Keep Fluent message keys (`.ftl` file keys) sorted alphabetically within each comment section.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftl
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.ftl : Fluent message key naming convention: use kebab-case, prefixed by feature area. Use `settings-<category>-<description>` for settings labels, `settings-<category>-<description>-input` for input fields, and `notification-<description>` for notifications.
Applied to files:
crates/core/i18n/en-GB/cadmus_core.ftl
📚 Learning: 2026-05-23T14:57:43.820Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 478
File: crates/core/src/view/settings_editor/refresh_rate_by_kind_editor.rs:91-91
Timestamp: 2026-05-23T14:57:43.820Z
Learning: When reviewing Rust code that calls `ToggleableKeyboard::new(parent_rect: Rectangle, number_mode: bool)` (from `crates/core/src/view/toggleable_keyboard.rs`), treat the second argument as `number_mode` (numeric/number-layout mode), not as an initial visibility flag. In the constructor implementation, `visible` is initialized to `false` and the keyboard starts hidden; only the numeric layout changes based on `number_mode` (e.g., for numeric inputs like `refresh rate u8`). Therefore, do not flag `ToggleableKeyboard::new(rect, true)` as “making the keyboard start visible.”
Applied to files:
crates/core/src/task/mod.rscrates/core/src/time_manager.rs
📚 Learning: 2026-06-08T07:44:43.803Z
Learnt from: OGKevin
Repo: OGKevin/cadmus PR: 576
File: crates/core/src/device/time.rs:33-52
Timestamp: 2026-06-08T07:44:43.803Z
Learning: In `crates/core/src/device/time.rs`, the `set_system_timezone` function intentionally uses `unimplemented!()` (panic) in the non-Kobo/fallback branch (`_ => unimplemented!("set_system_timezone is only available on Kobo devices")`). This is by design: panicking on emulator and non-Kobo targets forces developers to notice they need to provide a proper implementation (or explicit no-op) for any new target platform, rather than silently doing nothing. Do not suggest replacing this panic with a silent `Ok(())` return.
Applied to files:
crates/core/src/time_manager.rs
📚 Learning: 2026-05-23T17:00:50.322Z
Learnt from: CR
Repo: OGKevin/cadmus PR: 0
File: crates/core/AGENTS.md:0-0
Timestamp: 2026-05-23T17:00:50.322Z
Learning: Applies to crates/core/**/*.rs : All user-visible strings must use the `fl!` macro (`use crate::fl;`). Never hardcode string literals for labels, buttons, placeholders, or notifications. User-visible strings include: `label()` implementations on `SettingKind` traits, input field `label` strings in `Event::OpenNamedInput`, menu entry text (`EntryKind::Command`, `EntryKind::RadioButton`, etc.), and button labels, notification text.
Applied to files:
crates/core/src/time_manager.rs
🔇 Additional comments (15)
crates/core/src/time_manager.rs (6)
16-23: LGTM!
25-72: LGTM!
74-90: LGTM!
92-110: LGTM!
112-150: LGTM!
152-167: LGTM!crates/core/src/task/mod.rs (6)
46-50: LGTM!
98-100: LGTM!
122-123: LGTM!
399-456: LGTM!
526-546: LGTM!
846-914: LGTM!crates/core/i18n/en-GB/cadmus_core.ftl (3)
22-23: LGTM!
36-36: LGTM!
52-52: LGTM!
1 participant
Automatic Time Synchronization via NTP
Adds automatic time and timezone synchronization for Kobo devices when WiFi connects, and a manual "Sync Time" option in the clock menu.
How it works
When WiFi comes up and
auto_timeis enabled, a backgroundTimeSyncTaskis spawned. It:ipapi.coto detect the current timezone, then updates/etc/localtime,/etc/timezone, and callstzset()to apply it immediately to the running process.time.cloudflare.comvia SNTP to get the current UTC time.settimeofday()and writes the time to the RTC.ClockTickevent so the UI reflects the updated time.A "Sync Time" entry is also added to the clock menu, allowing manual triggering. If the sync fails during a manual trigger, a notification is shown to the user.
Settings
A new
auto_timeboolean setting (defaultfalse) controls whether time sync runs automatically on WiFi connect. It is exposed as a toggle in the General settings category.RTC changes
Rtcis now wrapped inArc<Mutex<File>>and implementsClone, allowing it to be shared between the alarm manager and the new time manager. Read and write time ioctls (RTC_RD_TIME/RTC_SET_TIME) are added. TheRtcinstance is now owned byDevicevia aOnceCellrather than being constructed inapp.rsand threaded throughContext.New dependencies
sntpc+sntpc-net-std— SNTP clientchrono-tz— timezone name parsing and handlingChange-Id: ec43ed615ec2a61436a44abca62e59bf
Change-Id-Short: lnvwlmtyulnx
Closes: #342
Closes: CAD-34