Repository navigation
Conversation
The RV3028 time write set year 00h, burst-wrote 00h-05h, then wrote the real year, so a write cut short left year 2000 and the next boot took no time from it. That split the write into three accesses against the manual's one-access rule (Application Manual Rev 1.4, 4.5), relying on the Seconds write restarting the prescaler, and used a real date as the "not set" mark. The write now sets 06h to A0h, which is not BCD, then writes all seven registers 00h-06h in one burst, year last. The chip keeps each byte as it is acknowledged, so a write cut short still leaves the mark. On read at an rv3028-eeprom-config descriptor, a year of A0h-FEh reads as "time not yet set": a New Year counts a left mark on (A0h to A1h, A9h to B0h) and never into BCD. FFh stays a failed read, since a read cut short returns 1s and the year is the last byte. The BSF bracket and the 5 s retry around the write are unchanged, and so is the 2025 floor, so a power-on-reset chip reading 2000-01-01 still gives no time. The same write order and year handling are used by Zephyr and MeshCore for the RV3028; the three are kept consistent in behaviour. Link: zephyrproject-rtos/zephyr#121389 Link: meshcore-dev/MeshCore#3421 Link: liquidraver#98 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The binding still described the old write order (year 00h first, the real year last). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: a write cut on the A0h byte itself leaves the old year and the old, intact time, so the docs now say "cut after the mark". The read comment no longer says a counted-on mark never reaches BCD, since only A0h, A9h and FFh were rolled over. The header lines the first edit left as fragments are rewrapped, and so is the binding text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: a counted-on mark skips xAh-xFh, and another tool can write any of them, so the header no longer calls the whole range the mark. The behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The year register holds two BCD digits and no century, and the save encoded y % 100. So a time before 2000 was stored as a year near the end of the century: a CLI `time 900000000` (1998), accepted by a node whose clock had no time yet, was written as year 98 and restored at the next boot as 2098, past the 2025 floor. A year from 2100 was stored as 2100-2106 minus a century, which the floor did reject. On the RV3028, BCD-encoding 2100 as a three-digit year would also equal the A0h mark, which is why Zephyr and MeshCore bound the write to 2000-2099 too. rtc_time_block() now fails outside 2000-2099, and the save writes nothing then, for every chip. At an rv3028-eeprom-config descriptor it writes the A0h mark instead and drops any pending retry, so the next boot takes no time rather than the older time the chip still holds. A retry whose run-on time crosses 2099 does the same. Other chips keep their older time. Local review: the docs now say a write cut between the mark and the year byte gives no time, rather than any cut after the mark, and that a mark gives no time rather than always logging "time not yet set", since a set power-loss flag or a switchover logs first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review of the range check: the refusal ignored whether its A0h write landed, logged "year marked unset" either way and had already dropped any pending retry, so a failed mark left the chip's older time for the next boot to restore. The mark is now checked and, if it fails, repeated every 5 s up to the same 13 attempts, sharing the time write's repeat state; a newer save replaces it either way. The header says only a time within 2000-2099 is saved, and the 2000-2099 reason is ZephCore's two-digit year, since the DS3231 and PCF8563 have a century bit that ZephCore does not use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: the mark was confirmed by its acknowledgement alone, without the BSF clear-before / check-after a time write gets, so a switchover during it (5.10) could leave the byte unstored while the log said "year marked unset". It now takes the same bracket, and a failure is repeated. A repeat that switches from writing the time to marking restarts the try count, so the final warning counts mark attempts only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review of fdf2332: restarting the try count when a repeat switched from writing the time to marking let one save repeat up to 23 times, against the documented 13 attempts, and its final log overstated the mark attempts by one. The count now runs across both, and the final log says "save not confirmed" rather than naming one kind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: the DS3231 and PCF8563 have a century bit, which ZephCore writes as 0 and masks off on read, so the limit is the encoding rather than every chip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRTC saves now enforce the 2000–2099 range. RV3028 saves mark out-of-range years as unset, retry unconfirmed writes, and skip unset-year values during restore. ChangesRTC year handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant zephcore_rtc_save
participant rtc_time_block
participant RV3028
zephcore_rtc_save->>rtc_time_block: Encode epoch
rtc_time_block-->>zephcore_rtc_save: Return range result
alt Epoch is in range
zephcore_rtc_save->>RV3028: Write 0xA0 year marker
zephcore_rtc_save->>RV3028: Write seven time bytes in one burst
else Epoch is out of range
zephcore_rtc_save->>RV3028: Clear BSF and write unset-year marker
zephcore_rtc_save->>RV3028: Read status and check BSF
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No identified issue blocks merging. The reported hardware checks did not include a real power-loss cut or an FFh read, so those cases remain unverified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
PORF handling and A0h marker diagnostics remain unresolved.
Review effort: Lite
Findings: None
What changed in this PR
Updates RV3028 persistence to use an A0h unset marker and reject years outside 2000–2099.
Changes:
- Writes the marker before a single seven-byte time burst.
- Detects
A0h–FEhas unset. - Documents marker behavior and year-range validation.
| File | Summary |
|---|---|
zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml |
Documents marker-based writes and supported year range. |
zephcore/adapters/clock/ZephyrRTCDiscover.h |
Updates the RTC behavior contract. |
zephcore/adapters/clock/ZephyrRTCDiscover.c |
Implements validation, marker writes, retries, and detection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Answering the overview headline of Copilot's round on
No change needed. Both are deliberate, and the code is consistent with them.
|
|
Proposed upstream PR, for review before anything is opened on liquidraver/ZephCore. Clean branch: Title: rtc: write the RV3028 time with an A0h year mark, and only 2000-2099 BodyA follow-up to liquidraver#98. The Zephyr and MeshCore counterparts are zephyrproject-rtos/zephyr#121389 and meshcore-dev/MeshCore#3421. Code links are permalinks: Cross-project consistency. This change is part of a coordinated update to RV3028 time handling across three codebases, kept consistent in behaviour: Zephyr (zephyrproject-rtos/zephyr#121389), MeshCore (meshcore-dev/MeshCore#3421) and ZephCore (follow-up to liquidraver#98). It replaces the rule all three adopted on 2026-10-05: zero the year, write 00h-05h, then write the year. That rule split the time write into separate accesses against the manual's one-access rule (RV-3028-C7 Application Manual Rev 1.4, §4.5), with the argument that the Seconds write restarts the prescaler, so no tick lands before the year write. It also used year 00 as the "not set" marker, which made 2000-01-01 unusable as a date. Hardware tests on an RV-3028-C7 then showed three things: a write cut short by an interface reset keeps the bytes already acknowledged and leaves the rest unchanged (tested with the 950 ms bus timeout, §4.5.1); the year register keeps a non-BCD value while the clock counts; and a New Year rollover counts a left mark up like a BCD value (A0h to A1h, A9h to B0h), so it stays a non-BCD year. The shared rule is now: write 06h := A0h, then one 7-byte burst 00h-06h, and read a year of A0h-FEh as not set and FFh as a failed read. Retry policy and never-set detection (PORF) are left to each project. All three projects also take only years 2000-2099 on a write, refusing any other year before touching the chip, since a BCD-encoded year outside that range either aliases into it or, for 2100, equals the A0h mark. The changeAt a descriptor with
For every chip:
Unchanged: the 2025 floor, so a power-on-reset chip (2000-01-01) still gives no time, and PORF: no time is taken while it is set, and only a confirmed time write clears it. The header contract and the TestingRAK4631 + RAK12002, repeater debug builds on
Iteration history and review: #50. 🤖 Generated with Claude Code |
Upstream's "comment diet vol.2" (9d6b21a) cut the RTC header contracts to a few sentences each, which conflicted with this branch's paragraphs. The header keeps upstream's text, plus one sentence each for the A0h mark on read and the 2000-2099 write range with its A0h mark. This branch's own new comments in ZephyrRTCDiscover.c are trimmed to the same density. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: the trimmed rv3028_write_time() comment said a write cut before the year byte leaves the mark, which a cut on the A0h byte itself does not (ed8802f had this right), and the trimmed header called the whole A0h-FEh range the mark a cut write leaves (b9fb124 had narrowed it). The time block comment again names the two-digit year as ZephCore's encoding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… range Local review: "the A0h mark, or a New Year's count on from it" is false for the xAh-xFh values in the range, which a count from A0h never reaches; the code and header now state the rule alone. The Kconfig help and ARCHITECTURE.md said every sync writes the RTC, which a time outside 2000-2099 no longer does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: ARCHITECTURE.md said an out-of-range time is not written, but at an RV3028 with rv3028-eeprom-config its year is marked instead. A year of FFh reaching the A0h-FEh check has passed the BSF-checked read, so it is a stored value, not a cut read; the comment no longer gives a reason. A Kconfig help line the last edit left long is wrapped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
liquidraver#103 closed in favour of liquidraver#105 (year range only), per the maintainer's review. Iteration continues on #54, which starts from this branch's head. Branch kept as the record of the coordinated A0h version. |
Fork iteration PR for the RV3028 time-write change, a follow-up to liquidraver#98. It stays open as the iteration record. Upstream sees only a squashed clean branch, and only once Pieter says so.
Cross-project consistency. This change is part of a coordinated update to RV3028 time handling across three codebases, kept consistent in behaviour: Zephyr (zephyrproject-rtos/zephyr#121389), MeshCore (meshcore-dev/MeshCore#3421) and ZephCore (liquidraver#103, a follow-up to liquidraver#98). It replaces the rule all three adopted on 2026-10-05: zero the year, write 00h-05h, then write the year. That rule split the time write into separate accesses against the manual's one-access rule (RV-3028-C7 Application Manual Rev 1.4, §4.5), with the argument that the Seconds write restarts the prescaler, so no tick lands before the year write. It also used year 00 as the "not set" marker, which made 2000-01-01 unusable as a date. Hardware tests on an RV-3028-C7 then showed three things: a write cut short by an interface reset keeps the bytes already acknowledged and leaves the rest unchanged (tested with the 950 ms bus timeout, §4.5.1); the year register keeps a non-BCD value while the clock counts; and a New Year rollover counts a left mark up like a BCD value (A0h to A1h, A9h to B0h), so it stays a non-BCD year. The shared rule is now: write 06h := A0h, then one 7-byte burst 00h-06h, and read a year of A0h-FEh as not set and FFh as a failed read. Retry policy and never-set detection (PORF) are left to each project.
The change, at a descriptor with
rv3028-eeprom-configonly (the RAK4631's RAK12002):rv3028_write_time()): 06h :=A0hon its own, then one 7-byte burst to 00h-06h, year last. It replaces year00h, a 6-byte burst, then the year. Both the first save and the 5 s retry go through it. The BSF bracket and the retry policy (every 5 s, 13 attempts) are unchanged.rtc_probe()): a year byte ofA0h-FEhlogstime not yet set (year A0h)and takes no time, and the chip stays the write-back target.FFhstill falls to the existing "time unreadable" path. Before this change, any non-BCD year logged "time unreadable", so the behaviour is the same and only the log line differs.time 900000000(1998) on a node whose clock had no time yet was written as year 98 and restored at the next boot as 2098, past the 2025 floor. At anrv3028-eeprom-configdescriptor the refusal writes the A0h mark instead, so the next boot takes no time rather than the chip's older one. The mark is confirmed like a time write (BSF cleared before and checked after) and repeated like one (every 5 s, 13 attempts, replaced by a newer save), and a repeat whose run-on time leaves 2000-2099 marks instead. Other chips keep their older time. The range is part of the shared rule; marking on a refusal is ZephCore's own policy (Zephyr returns-EINVALwith no side effects, and MeshCore keeps the old time).01fd56a: no time is taken while it is set, and only a confirmed write clears it.zephcore,rtc-i2cbinding description describe both. They say a write cut between the mark and the year byte gives no time: a cut on the A0h byte itself leaves the old, intact time, and a cut after the year byte leaves the whole new time.Testing (RAK4631 + RAK12002, repeater debug builds on
dev9bb7ccf, Zephyr74b7173. A-D ran ona1cd253, and the commits up tob9fb124change only comments and the binding text. E-F ran one3f48f8, then again onb0abaffandfdf2332;b4aebcachanges only the retry count's reset and one log line, and the board runs it;e53d3a0is comment-only):RTC rtc-rv3028@52: restored 2026-10-06 15:53:27 UTC.time <now>, thenreboot: restored the set time.time <now>gavetime write not confirmed, repeating every 5 s, thennot confirmed after 13 tries. Afterreboot:rtc-rv3028@52 present, time not yet set (year A0h).time not yet set (year A0h)at boot. Thentime <now>andreboot:RTC rtc-rv3028@52: restored 2026-10-06 15:59:40 UTC, so the one-burst write clears the mark.time outside 2000-2099 not written, year marked unset.e3f48f8over the chip E left:time not yet set (year A0h)at boot. Thentime <now>andreboot:RTC rtc-rv3028@52: restored 2026-10-06 16:23:36 UTC(onb0abaff:16:30:32; onfdf2332:16:43:03).Local review. Each pushed head carries a recorded local pass. The passes found no correctness defect in the A0h write or read. On the range work, they found that the refusal's A0h mark was neither checked nor BSF-bracketed (fixed in
b0abaff,fdf2332), and that restarting the try count on a switch to marking broke the 13-attempt cap (fixed inb4aebca). The rest were comment claims wider than the code or the measurements: "a write cut short" became "cut between the mark and the year byte", "never into BCD" was dropped, the A0h-FEh range is no longer called the mark itself, and the 2000-2099 limit is attributed to ZephCore's year encoding, since the DS3231 and PCF8563 have a century bit.🤖 Generated with Claude Code
Summary by CodeRabbit