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>
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 review (comment 6037017924): drop the A0h year mark, the one-burst write, rv3028_mark_year(), s_save_mark and the A0h read branch. The RV3028 write is dev's again (year 00h, 00h-05h, then the year): ZephCore's 2025 restore floor already rejects the year 2000 a cut write leaves. A time outside 2000-2099 is now not written for every chip, rv3028-eeprom-config included, and the chip keeps the time it has. The RV3028 retry stops if its run-on time leaves the range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local review: the rv3028-eeprom-config text ("written on each sync")
and the header's file summary still said every sync is written back.
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
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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved RV3028 retry, stale-time invalidation, and protocol implementation issues block approval.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Restricts RTC persistence to years 2000–2099 and updates related documentation.
Changes:
- Adds year-range validation for RTC writes.
- Updates configuration, API, binding, and architecture documentation.
- RV3028 retry cancellation and A0h invalidation handling remain unresolved.
| File | Summary |
|---|---|
zephcore/Kconfig |
Documents the supported RTC range. |
zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml |
Documents RTC persistence behavior; A0h protocol remains incomplete. |
zephcore/adapters/clock/ZephyrRTCDiscover.h |
Updates the RTC save contract. |
zephcore/adapters/clock/ZephyrRTCDiscover.c |
Enforces the year range; retry and invalidation handling need changes. |
docs/ARCHITECTURE.md |
Documents range-limited RTC persistence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+686
to
+688
| if (!rtc_time_block(d, epoch, blk)) { | ||
| LOG_WRN("RTC %s: time outside 2000-2099 not written", d->name); | ||
| return; |
Comment on lines
+686
to
+689
| if (!rtc_time_block(d, epoch, blk)) { | ||
| LOG_WRN("RTC %s: time outside 2000-2099 not written", d->name); | ||
| return; | ||
| } |
Comment on lines
+85
to
+88
| backup switch flag cleared before and checked after. A write sets | ||
| year 00h first and the real year last, so a write cut by a power | ||
| loss reads as "time not yet set", and a write not confirmed is | ||
| repeated every 5 s, up to 12 times. Set it only on a board whose |
Owner
Author
|
Upstream merged the squashed version as liquidraver#105. Closing this iteration PR unmerged, as the upstream workflow does. |
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
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.
Iteration branch for the cut-down replacement of liquidraver#103, per the maintainer's review (liquidraver#103 (comment)). It starts from #50's head (the A0h version coordinated with Zephyr and MeshCore) and cuts it to the 2000-2099 write range. Never merged; the clean branch
rtc-year-rangecarries the upstream commit.🤖 Generated with Claude Code