Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is well-scoped and includes a safe fallback path, with only a minor documentation inconsistency noted in review comments.
Pull request overview
This PR fixes a race condition in RV3028 RTC time reads by switching from multiple per-field I2C reads to a single burst read of the contiguous clock registers, improving timestamp coherence used by mesh packet stamping and neighbor aging.
Changes:
- Add an RV3028 burst read helper that validates BCD fields and DateTime validity before returning
unixtime(). - Fall back to the existing field-by-field RV3028 getters if the burst read or validation fails, logging the fallback only once.
- Add RTC detection and “selected driver” debug logging for easier diagnosis of RTC discovery and selection.
File summaries
| File | Description |
|---|---|
| src/helpers/AutoDiscoverRTCClock.cpp | Implements atomic/burst RV3028 time register reads with validation and adds clearer RTC detection/selection logging. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9bf12e1 to
ca63633
Compare
ca63633 to
c408f88
Compare
ptr727
changed the title
|
Replying to the Copilot overview on 12-hour mode. MeshCore never puts the chip in 12-hour mode. BSF handling. The overview doesn't say which part it means, so here is how BSF moves through the code, in case it helps. A read is used only if BSF reads 0 after it (L160). A set BSF is cleared, and the read is taken again once (L251). That retry also clears the BSF a power cut leaves set, which was confirmed on hardware (the ~60 s cut row in the description). A switchover garbled enough to fail validation is rejected before Status is read. The BSF it leaves set is then caught on the next call, and the time runs on in between. A write clears BSF first and requires it still 0 afterwards (L187-L191). Year-2000 writes. Zeroing the year first and writing it last is deliberate, and the description covers it under "Writes" and its limitations. A time inside the year 2000 is written as year 00, and it then reads as not set. The paths that set the time from a user don't reach that case: the CLI |
On a switchover from VDD to VBACKUP the RV-3028-C7 disables and resets its I2C interface, so an access in progress is cut short (Application Manual Rev. 1.4, 4.2 and 5.10). The rest of a read then returns 1s, or a byte that changed meanwhile, and rv3028_get_time() returned whatever it decoded. A write cut short leaves a mix of the old and new time, which the next read returned as valid. This can happen whenever the switchover is enabled and VDD fails during an access. The switchover sets BSF in the Status register, which can be cleared only on VDD (3.7). Bracket the time register accesses with it: clear a BSF left by an earlier switchover, read or write the time registers, then read Status again and return -EIO if BSF is set. set_time() then also leaves PORF set. get_time() additionally returns -EIO for a BCD nibble above 9 or a time that rtc_utils_validate_rtc_time() rejects. BSF is written only when it is already set, and the bracket runs under the MFD lock. Retrying is left to the caller. A BSF found set by get_time() can also come from a power loss during a set_time() that never returned. Each byte of a write is stored as it is acknowledged, so such a write leaves a mix of the old and new time. set_time() therefore first writes A0h, a value that is not BCD, to the year register, then 00h-06h in one access as before (4.5). The year is the last byte, so a write cut short leaves A0h. The year counter increments a left mark like a BCD value, so get_time() reports any year from A0h as not set (-ENODATA), except FFh, which is what a read cut short returns. The same write order and year handling are used by MeshCore and ZephCore for RV3028; the three are kept consistent in behaviour. Tested on a RAK4631 with a RAK12002 (RV-3028-C7). A write to 00h cut short by the I2C bus timeout (4.5.1), the interface reset that software can trigger, kept the bytes acknowledged before the timeout, and the year register held A0h. With this change, year 2000 was set and read back, A0h followed by a write cut short by the timeout read as -ENODATA, and a normal set and read succeeded. Across a 31 December rollover the year register went from A0h to A1h, A9h to B0h and FFh to F0h, and each read as -ENODATA. A switchover during an access was not reproduced. Built for native_sim/native and native_sim/native/64 with the RTC build_all I2C test in the ci-base v0.29.4 image. Link: https://www.microcrystal.com/fileadmin/Media/Products/RTC/App.Manual/RV-3028-C7_App-Manual.pdf Link: meshcore-dev/MeshCore#3421 Link: liquidraver/ZephCore#98 Assisted-by: Claude:claude-opus-5.5 Signed-off-by: Pieter Viljoen <pieter@viljoen.com>
c408f88 to
b18d07a
Compare
|
Updated to the revised RV3028 time-write rule, agreed with Zephyr (zephyrproject-rtos/zephyr#121389) and ZephCore (follow-up to liquidraver/ZephCore#98): the year is marked A0h, then the time is written in one 7-byte burst, and a year of A0h-FEh reads as not set. See "Cross-project consistency" in the description for why the earlier year-00 split write was replaced. |
b18d07a to
16a2235
Compare
16a2235 to
0d00235
Compare
0d00235 to
6033e3e
Compare
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 separate 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, inside the existing BSF bracket. The chip keeps each byte as it is acknowledged, so a write cut between the mark and the year byte leaves the mark. On read at an rv3028-eeprom-config descriptor, a year of A0h-FEh gives no time: a New Year counts a left mark on (A0h to A1h, A9h to B0h). FFh stays a failed read, since a read cut short returns 1s and the year is the last byte. The save also refuses a year outside 2000-2099, for every chip, because ZephCore writes the year as two BCD digits and no century bit (the DS3231 and PCF8563 century bits are written as 0 and masked off on read). It encoded y % 100, so a time before 2000, accepted by a node whose clock had no time yet, was stored as a year near the end of the century: `time 900000000` (1998) was restored at the next boot as 2098, past the 2025 floor. At an rv3028-eeprom-config descriptor a refused year is marked A0h instead, so the next boot takes no time rather than the older time the chip still holds; the mark gets the BSF bracket and the same 5 s repeat as a time write, sharing its 13-attempt cap, and a repeat whose run-on time leaves 2000-2099 marks instead. Other chips keep their older time. The header contract and the binding describe both; the Kconfig help and ARCHITECTURE.md describe the 2000-2099 range, and ARCHITECTURE.md the refused-year mark. The same write order, year handling and 2000-2099 range 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>
6033e3e to
81b7549
Compare
|
Updated to 81b7549 after a cross-check with the Zephyr (zephyrproject-rtos/zephyr#121389) and ZephCore (liquidraver/ZephCore#103) RV3028 work. A Status/Control 2 read is now trusted for a write-back only if Control 2's RESET bit, which always reads 0 (manual p. 24), reads 0. Before this, a read cut by an interface reset returns 1s, so the 12_24 clear would have written FDh to Control 2, turning on every interrupt enable and resetting the prescaler. Melopero's unchecked The shared position on 12-hour mode is in the new "12-hour mode" paragraph of the description. |
On a switchover from VDD to VBACKUP the RV-3028-C7 disables and resets its I2C interface, so an access in progress is cut short (Application Manual Rev. 1.4, 4.2 and 5.10). The rest of a read then returns 1s, or a byte that changed meanwhile, and rv3028_get_time() returned whatever it decoded. A write cut short leaves a mix of the old and new time, which the next read returned as valid. This can happen whenever the switchover is enabled and VDD fails during an access. The switchover sets BSF in the Status register, which can be cleared only on VDD (3.7). Bracket the time register accesses with it: clear a BSF left by an earlier switchover, read or write the time registers, then read Status again and return -EIO if BSF is set. set_time() then also leaves PORF set. get_time() additionally returns -EIO for a BCD nibble above 9 or a time that rtc_utils_validate_rtc_time() rejects. BSF is written only when it is already set, and the bracket runs under the MFD lock. Status flags, BSF and PORF, are cleared by writing 0 to the flag and 1 to the others, which leaves them unchanged (3.7), so no read precedes the write: a flag set in between is not lost, and a read cut short is not written back. Retrying is left to the caller. A BSF found set by get_time() can also come from a power loss during a set_time() that never returned. Each byte of a write is stored as it is acknowledged, so such a write leaves a mix of the old and new time. set_time() therefore first writes A0h, a value that is not BCD, to the year register, then 00h-06h in one access as before (4.5). The year is the last byte, so a write cut short leaves A0h. The year counter increments a left mark like a BCD value, so get_time() reports any year from A0h as not set (-ENODATA), except FFh, which is what a read cut short returns. The same write order and year handling are used by MeshCore and ZephCore for RV3028; the three are kept consistent in behaviour. Tested on a RAK4631 with a RAK12002 (RV-3028-C7). A write to 00h cut short by the I2C bus timeout (4.5.1), the interface reset that software can trigger, kept the bytes acknowledged before the timeout, and the year register held A0h. With this change, year 2000 was set and read back, A0h followed by a write cut short by the timeout read as -ENODATA, and a normal set and read succeeded. Across a 31 December rollover the year register went from A0h to A1h, A9h to B0h and FFh to F0h, and each read as -ENODATA. Writing 7Eh and then 01h to a clear Status register left every flag 0, and an update flag set before set_time() was still set after it. A switchover during an access was not reproduced. Built for native_sim/native and native_sim/native/64 with the RTC build_all I2C test in the ci-base v0.29.4 image. Link: https://www.microcrystal.com/fileadmin/Media/Products/RTC/App.Manual/RV-3028-C7_App-Manual.pdf Link: meshcore-dev/MeshCore#3421 Link: liquidraver/ZephCore#98 Assisted-by: Claude:claude-opus-5.5 Signed-off-by: Pieter Viljoen <pieter@viljoen.com>
…bled transfers getCurrentTime() built its DateTime from six Melopero getters, seven I2C transactions read most-significant-first while the counters run, so an hour boundary crossed between the hour and minute reads returned a time an hour in the past. Read the seven clock registers in one burst, which the chip holds coherent (RV-3028-C7 Application Manual Rev 1.4, 4.5), and validate the BCD nibbles, the month and DateTime::isValid(). A switchover to VBACKUP disables and resets the chip's I2C interface (4.2, 5.10), so the rest of a transfer reads as 1s and a byte can still decode to a valid but larger value. A read is used only if BSF reads 0 after it; a set BSF is cleared (3.7) by writing 0 to it alone, since writing 1 leaves a flag unchanged, and the read retried once, which also clears a flag left by an earlier power cut. Control 2 is read in the same access, and 12 hour mode, which begin() leaves unchecked, is cleared the same way (the chip converts Hours itself, manual 02h), so a PM hour cannot decode as a wrong 24 hour one. Control 2's RESET bit always reads 0, so a read showing it set was cut short and is not written back; Melopero's unchecked set24HourMode() is no longer called from begin(). A rejected read is never used: the time runs on from the last accepted or set time, or before there is one comes from the fallback clock, as with no RTC. The field-by-field getters are no longer used, since a failed read decodes as year 2165, past RTClib's month table. setCurrentTime() writes inside the same bracket. It marks the year A0h, which is not BCD, then writes Seconds through Year in one access (4.5). Bytes acked before an interface reset are kept, so a write cut part way leaves the A0h mark rather than a mixed time that passes for the real one. The mark, or the mark counted on by a new year (A0h-FEh), reads as a clock not yet set; FFh stays a failed read, as a read cut by an interface reset returns it. Year 00 reads as 2000. An unconfirmed write is retried every 5 s, at most 13 times in all, so a bus that is down cannot stall each caller of getCurrentTime(). A time outside 2000-2099 is never written, since the chip counts years 00-99 and 2100 would encode as the A0h mark; the clock runs on from it without the RTC. The weekday is now DateTime::dayOfTheWeek(); the formula carried over from dev stored the wrong weekday on most days. This is part of a coordinated RV3028 update, kept consistent in behaviour across Zephyr (zephyrproject-rtos/zephyr#121389), MeshCore and ZephCore (follow-up to liquidraver/ZephCore#98). It replaces the year-00 split write that all three adopted on 2026-10-05. Also log RTC detection for DS3231 and RV3028, which had none, plus a line naming the driver that was bound. Tested on a RAK4631 with a RAK12002: time set and read back, 2000-01-01 set and read back across a reboot, a staged cut write (A0h plus a partial burst) and years A1h and B0h use the fallback clock, FFh is logged as a failed read, a 2100 set leaves the RTC untouched, the stored weekday is right, 12 hour mode is cleared on read and write with the other Control 2 bits kept, and a ~60 s power cut keeps the time. Built for ESP32-S3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
81b7549 to
0f03805
Compare
References used throughout:
dev3e3150c8, the base0f0380541.2.0This PR has grown since it was first opened. It started as the time race below, and now also guards every RV3028 time read and write against a backup switchover, to the position agreed with the ZephCore and Zephyr RV3028 work.
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 (#3421) and ZephCore (liquidraver/ZephCore#103, a follow-up to liquidraver/ZephCore#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.
12-hour mode. Each project ensures the chip is in 24-hour mode before it takes or writes a time. How each project does it: Zephyr's driver owns the chip and writes Control 2 = 00h at every boot, which also disables interrupts it does not handle, then takes the converted time. MeshCore clears 12_24 inside its read bracket, keeping the other bits, and takes the time only from a BSF- and mode-checked re-read. ZephCore selects 24-hour mode at each time write, keeping the other bits, with a read-back, and refuses a time read in 12-hour mode until the next sync, because a boot-time write would sit outside its BSF bracket. MeshCore and ZephCore trust a Control 2 read for a write only if its always-zero RESET bit reads 0; Zephyr's interrupt-enable read-modify-writes do not yet check it (open follow-up). A single-byte write is stored whole when acknowledged; a switchover in the middle of a byte is not covered (5.10).
The defects
1. A time race in the read.
getCurrentTime()builds itsDateTimefrom six Melopero getters. Each one is a separate I2C transaction, andgetHour()is two because it re-reads the register to test the 12 hour flag. That is seven round trips, read most-significant-first, while the counters keep running. An hour boundary crossed between the hour read and the minute read returns a timestamp a full hour in the past:Minute boundaries misread by a minute, day boundaries by a day. The chip only holds the counters still across one access (§4.5, p. 52).
2. A switchover garbles a transfer, and nothing notices.
devruns the RV3028 in Direct Switching Mode (L39; #3545 makes it permanent). On a switchover to VBACKUP the chip disables and resets its I2C interface (§4.2, p. 45; §5.10, p. 95), so the rest of a transfer reads as 1s. A byte can still decode to a valid but larger value (year0x26read as0x27), and Melopero's getters return0xFFon a failed transfer, which decodes as 165. On the write side,setTime()is one 7-byte burst (devL91): a switchover or power loss part way leaves the bytes written before it with the rest unchanged, a mixed time that reads back as valid on every later boot. Each byte is stored as it is acked. The Zephyr RV3028 work tested this on a RAK4631 with a RAK12002 by holding a 3-byte write to 00h past the 950 ms bus timeout (§4.5.1, p. 53): 00h-02h held the new bytes and 03h-06h the old ones, in 6 of 6 repeats, so "the previous time counter values are maintained" does not mean the old time survives a cut write.Only the RV3028 path changes. DS3231 and PCF8563 read through RTClib's
now(), andRTC_RX8130CE::getTime()already does a 7 bytewrite_then_read.The fix
Links are to
0f038054.Reads,
rv3028_read_clock():rv3028_settle(), and the read is retried once (L291). This runs before the fields are validated, since most PM hours do not decode as valid hours. The retry also clears a BSF left by an earlier power cut, without a Status write on every read.DateTime::isValid(), which round-trips throughunixtime()and rejects impossible dates such as 31 February.A rejected read is never used (
getCurrentTime()). The time runs on from the last accepted or set time (rv3028_run_on(), which moves its reference forward on each call so amillis()wrap cannot step it back), or, before there is one, comes from the fallback clock, as on a board with no RTC. The field-by-field getters are no longer used: a failed one decodes as year 2165, past RTClib's month table.Writes,
rv3028_write_time(), inside the same bracket:The weekday is RTClib's
DateTime::dayOfTheWeek(); the formula carried over fromdevstored the wrong weekday on 30325 of the 36525 days from 2000 to 2099. A time outside 2000-2099 is never written (L219): the chip counts years 00-99 only, and 2100 would encode as A0h, the mark. Such a write is not confirmed, so the clock runs on from the set time without the RTC.Year is the last byte of the burst, so a write cut anywhere in it, by a switchover or a power loss the MCU does not survive, leaves the A0h mark, which reads as a clock not yet set rather than a mixed time that passes for the real one. If neither try is confirmed,
setCurrentTime()holds the RTC unread, runs on from the time being set, andgetCurrentTime()retries the write every 5 s, at most 13 attempts in all (L320), so a bus that is down cannot stall each caller; if none is confirmed, the boot runs on without the RTC.The first read failure of a boot is logged once, since
getCurrentTime()runs on every received packet; a clock not yet set is expected and not logged. No log argument has a side effect, so release builds run the same paths.Also adds RTC detection logging for DS3231 and RV3028, which had none, plus a line naming the bound driver.
Portability
The coherence guarantee is the single seven byte read, not the repeated start. Ending the pointer write without a stop avoids releasing the bus where the core honours it, but is not what makes the read atomic.
endTransmission(false)nonStop,requestFromissuesi2cWriteReadNonStopi2c_write_blocking_until(..., !stopBit, ...), 5th arg isnostopI2C_OTHER_FRAMEorUSE_HALV2_DRIVER, so stop-then-startWhere the flag is ignored this degrades to stop-then-start, which is what
Melopero_RV3028::readFromRegister()already does against this part.Notes for review
-ENODATA, ZephCore takes no time, and MeshCore, whose clock has no "not set" value, uses the fallback clock. A chip that was never set is also detected per platform: Zephyr and ZephCore honour the power-on reset flag PORF, which only a confirmed write clears, and ZephCore rejects any year before 2025. MeshCore does not use PORF, because no MeshCore firmware has ever cleared it, so it is still set on deployed nodes whose time is correct; honouring it would drop every updated node to the fallback clock until its time is next set.dev. MeshCore's clock only moves forward throughclock sync,timeand the contact bootstrap, so a node starting at 2000 still syncs.millis()can step back by the MCU clock's drift against the RTC.RV3028_ADDRESSis defined both here and inMelopero_RV3028.has0b1010010. Same value, so it is harmless. Left alone to keep this PR to one fix.Testing
Builds:
RAK_4631_repeater(nRF52840) andheltec_v4_repeater(ESP32-S3) at the final iteration head. Earlier versions also builtHeltec_v3_repeaterandXiao_rp2040_repeater.wio-e5_repeater(STM32) fails on unmodifieddevtoo, with a RadioLib compile error inhal/Stm32duino/Stm32wlHal.cpp.Hardware: a RAK4631 with a RAK12002 (RV3028), USB only, indoors with no GPS fix, so GPS never set the clock. Each build had the throwaway diagnostic CLI from
test/rv3028-diag(290cfc5b, never merged) added; itsrv dumpprints Status 0Eh (and, in the last row, Control 2) and the RTC's own time, andrv setwrites the time registers directly. The last ten rows also addedrv raw(write raw bytes from a register in one access),rv wt(callsetCurrentTime()directly, bypassing the CLI's forward-only check) andrv gt(callgetCurrentTime()).ca636336e54493e3clockgave the fallback 2024-05-15e54493e3timeset at 14:59:37e54493e310after boot, so the BSF a cut sets on this chip had been cleared by the first read6590745765907457rv setyear 00, reboot, thentimeclockgave the fallback 2024-05-15; aftertimeat 15:39:15 the RTC read 15:39:1999e455d9rv wtto host time, then 2000-01-01, then rebootrv gtread 2000-01-01 from the chip, not the fallback99e455d9rv raw 06 A0, thenrv raw 00 11 22 03(the mark plus a 3-byte burst, as a cut write leaves it), rebootrv gtgave the fallback 2024-05-15 with no read failure logged99e455d9rv raw 06 1A(non-BCD, not the mark)RV3028: time read rejected (-1), the first log of that boot9580459brv raw 06 A1, reboot,rv raw 06 B0,rv raw 06 FFtime read rejected (-1); time then restored withrv wtand read back after rebooted51d8d2rv wt2100-01-01, then 2099-12-31 23:59:59, then host time, rebootfe7aef4erv wt2024-05-15, 2026-10-06, 2000-01-01, 2099-12-31 (each 12:00), withrv dumpextended to print the weekday register59e5b0c7rv raw 10 02(12_24 set), thenrv gt; again 12_24 set, thenrv wt21:30bc80fbdfrv gtbegin()no longer touches Control 2; the first read cleared 12_24 only (Control 2 80h, TSE kept) and returned 15:30:23de6156918ba3aacarv gtevery 3 sThis PR's
0f038054has the same tree as8ba3aaca. Not run on hardware: a switchover during a read or write, which needs VDD brought within the DSM threshold of VBACKUP mid-transfer; the BSF paths are covered by review against §3.7 and §4.2.Acceptance run on this PR's exact commit
0f038054(the three projects' shared case list; the diagnostic CLI commit on top changed nothing undersrc/): all pass.time read rejected (-1); seconds 5Ah, read at once since the chip counts it on to a valid value within seconds: rejected, time ran onIteration history and review: ptr727/meshcore-dev-MeshCore#1. Related: #3545, which stores the switchover config in EEPROM.
🤖 Generated with Claude Code