Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Preserve the original EERD state instead of unconditionally clearing it.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Persists RV3028 backup switchover and trickle-charger settings in EEPROM with validation, identity checks, and failure handling.
Changes:
- Adds checked EEPROM read/write and refresh sequencing.
- Preserves calibration bits and verifies configuration.
- Adds bounded retries and RAM fallback.
- Documents blocking configuration behavior.
| File | Summary |
|---|---|
src/helpers/AutoDiscoverRTCClock.h |
Documents blocking configuration and retry behavior. |
src/helpers/AutoDiscoverRTCClock.cpp |
Implements EEPROM configuration, verification, retries, and identity checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8469166 to
aaf190d
Compare
|
On the review overview's "Preserve the original EERD state instead of unconditionally clearing it": declining, deliberately.
|
AutoDiscoverRTCClock::begin() set 35h and 37h with plain register writes, which change only the RAM mirror. With EERD = 0 the RV3028 reloads that mirror from EEPROM every day at midnight (RV-3028-C7 Application Manual Rev 1.4, 4.6.2 and 4.6.9), so on a part still holding the factory EEPROM (BSM = 00, TCE = 0) the backup switchover and trickle charger turned off at the first midnight after boot, and a later power cut lost the time. Writing 0xB4 to 37h also forced bit 7, the LSB of the factory frequency calibration. Store the configuration in EEPROM instead, per the manual: EERD = 1, wait for EEbusy, hold BSM = 00 in RAM while the EEPROM is accessed (3.15.6), compare each byte with a single-byte EEPROM read and write only the bits that differ (CLKOE in 35h; TCE, FEDE, BSM and TCR in 37h), then Refresh, which restores the stored switchover mode, read the config back, and clear EERD. The factory calibration bits are never written. Nothing is written unless a read of the time registers shows no bit an RV3028 always reads as 0 (3.2); a failed read writes nothing and is retried, and only two reads that rule the device out, with BSF clear, end it. The store also waits for a boot read bracketed by BSF, since the EEPROM needs VDD (4.6.8). If EEbusy has not cleared, the switchover is not re-enabled while an EEPROM operation may still be running (3.15.6, 4.6.7). Every I2C transfer is checked, since Melopero's readFromRegister() returns 0xFF on failure. A failed store falls back to the RAM mirror and is retried from getCurrentTime() up to 3 times per boot. Tested on a RAK4631 with a RAK12002: on dev the switchover and trickle charger turned off at RTC midnight and a power cut lost the time; with this change the config is stored once, survives midnight, and a power cut keeps the time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
aaf190d to
23b23b2
Compare
|
Replying to the Copilot overview on Retry path bypassed when another RTC is selected. That's right, and it's deliberate. Clearing EERD while EEPROM activity may be in progress. This one is a judgement, and step 7 of the description states it: "Clearing EERD while EEbusy may still be set is a judgement, not documented behaviour (the §4.6.7 flowchart re-enables refresh after EEbusy = 0)". Clearing EERD starts no EEPROM operation. It only re-enables the automatic refresh, and the next one is at midnight (§4.6.2, p. 54). Leaving EERD set instead would stop every later automatic refresh for as long as the chip stays powered (§3.7, p. 23). What §3.15.6 (p. 39) does require during EEPROM activity is a disabled switchover, and that's kept: when EEbusy may still be set, the saved 37h isn't written back (L139-L142), and the RAM fallback waits for EEbusy = 0 (L244). liquidraver/ZephCore#98 and the Zephyr RV3028 driver handle EERD in the same order. |

References used throughout:
dev3e3150c8, the base.AutoDiscoverRTCClockis unchanged sincefad0ffb7, where thedevhardware rows below ran23b23b241.2.0The defect
AutoDiscoverRTCClock::begin()configured the RV3028 with two plain register writes (L38-L39):Melopero's
writeToRegister()is a plain I2C write, so these set only the RAM mirror of EEPROM-backed configuration registers:0xB4also forces 37h bit 7, which is EEOffset[0], the LSB of the factory frequency calibration (§3.15.6, p. 39).The mode itself is right. §7.3 (p. 105) specifies DSM with the trickle charger for a capacitor backup, and adds: "Power Management settings have to be stored in EEPROM for permanent configuration".
The fix
The config is stored in EEPROM following the manual, including §3.15.6 (p. 39): BSM must be 00 or 10 for any EEPROM read or write. Links are to
23b23b24.rv3028Configure()writes nothing unlessrv3028Identify()gets a read of the time registers 00h-06h with no bit an RV3028 always reads as 0 (§3.2, p. 12). A device at 0x52 that is not an RV3028, such as a 24-series EEPROM, gets none of the config writes.rv3028OnVdd()clears BSF (§3.7, p. 22), reads 00h-06h again, and requires BSF still 0. A switchover in between means VDD is unstable, and an EEPROM write needs VDD (§4.6.8, p. 57), so the store waits for a retry. BSF is cleared first because a power cut before this boot leaves it set.rv3028StoreConfig()sets EERD = 1, then waits for EEbusy = 0 (§4.6.7, p. 56). This also covers the ~66 ms POR refresh (§4.6.1, p. 54).rv3028_config, it reads the EEPROM copy with a single-byte read (EECMD 22h, §4.6.6, p. 55). It writes with a single-byte write (21h, §4.6.5, p. 55) only if the byte differs (rv3028EepromRead/Write()). The bits written:0xB4set it too.Waits and transfers:
rv3028EepromCommand()waits per §4.6.7 (p. 56): 1 ms after a read or Refresh, 10 ms after a write. Each gets an extra 1 ms, because Arduinodelay()can return early on some cores.rv3028EepromIdle()polls for about 100 ms, and ends the wait on the first failed status read, so a dead bus is not polled through each transfer's timeout.TwoWiredirectly (rv3028Read/Write()). Melopero'sreadFromRegister()ignores the I2C results and returns0xFFon a failed transfer, and a failed read must never be written back.Failure handling: if the store fails,
rv3028Configure()sets the RAM mirror as before with a checked read-modify-write (rv3028SetRam()), but only once EEbusy reads 0 (L244). A RAM-only config lasts only until the next refresh, sogetCurrentTime()re-runs the whole attempt, identification included, every 10 minutes, at most 3 times per boot (L207-L214). The cap is there because a password-locked chip never succeeds, and each attempt writes to it again. Failures are logged withMESH_DEBUG_PRINTLN, whose arguments carry no side effects, so release builds run the same paths.An already-configured chip costs two identification reads, a BSF clear, two EEPROM byte reads and a Refresh per boot, and no EEPROM write.
Notes for review
getCurrentTime(), ZephCore from a work-queue timer, and Zephyr leaves retry to the caller.drivers/mfd/mfd_rv3028.conmainrun a Refresh and a whole-block Update (§4.6.3, p. 54) with BSM live. That was this branch's first version too,f0af008a. Testing on two boards found no spurious switchovers either way (see Testing), so §3.15.6 was chosen on the manual:0x00also cleared CLKSY, PORIE and FD in RAM. Those now keep the chip's values (§3.15.4, p. 37). With CLKOE = 0 the pin is held low, so CLKSY and FD have no effect. PORIE stays at its factory 0. Zephyr stores FD = 111 as well, which MeshCore accepts as found: §7.3 note 5 (p. 105) allows either way of turning CLKOUT off.begin()still adopts anything that ACKs at 0x52 as the clock, reading it as the time and writing it on every sync, and does the same for the other three RTC addresses. That is fixed separately in Adopt an RTC only if its time registers read like that chip 🤖🤖 #3544. Here only the new config writes are gated by identification.set24HourMode()still uses Melopero's unchecked read-modify-write on Control2, on any device at 0x52.Testing
Builds:
RAK_4631_repeater(nRF52840) andheltec_v4_repeater(ESP32-S3) at the final iteration head;RAK_11310_repeater(RP2040) ataaf190db. Release andMESH_DEBUG=1, with no new warnings.Hardware setup:
test/rv3028-diag(290cfc5b, never merged). Itsrv dumpprints RAM and EEPROM 35h/37h, Control 1 and status 0Eh. Itsrv setwrites the time registers, so the RTC could be stepped to 23:59:50 to pass midnight in seconds. Itsrv factoryrestores EEPROM 35h/37h to their factory values, keeping EEOffset[0].devfad0ffb700/B4: bit 7 forced to 1 against the chip's 0. EEPROM stillC0/10devfad0ffb7C0/10: switchover and trickle charger off, CLKOUT ondevfad0ffb711(PORF = 1)f0af008a(vendor Update sequence)40/34written once. Held past midnight. No write on reboot. Time kept to the second (the RTC advanced 4:47 against 4:47 wall clock), 0Eh =30(BSF = 1, PORF = 0)4f6de86240/34written once, RAM BSM restored. Held past midnight. No write on reboot. Time kept to the second (4:07 against 4:07), 0Eh =306faac3bc743bdae534, Control 120(EERD = 0), 0Eh10(BSF = 0)743bdae5rv factory, then rebootC0/10→40/34. Control 120(EERD = 0), 0Eh10(BSF cleared by the boot read)This PR's
23b23b24differs from743bdae5only by a comment. The power-cut retention rows ran on earlier commits of the same store sequence; what changed since is the identification gate, the BSF-bracketed boot read and the EEbusy gate, which decide whether the store runs, not what it stores. The chip's stored config, which is what keeps the time across a cut, was confirmed on743bdae5above.Spurious switchovers: status 0Eh was polled every 30 s for 31 minutes on the
f0af008abuild, with DSM and the trickle charger stored, across 3 reboots. BSF never set. Spot reads across the session's serial-DFU flashes showed it set only after the deliberate power cuts.Second implementation, same hardware. ZephCore carries the same §3.15.6 sequence, now upstream as liquidraver/ZephCore#98 (iteration and raw logs in ptr727/liquidraver-ZephCore#35). It was tested on a second RAK4631 + RAK12002, on USB only, and with the power again pulled by hand:
devf9d01cf, no RTC used72bfc7b11(PORF = 1, BSF = 0)334d9f7C0/10→40/34. Switchover off for 52.2 ms during the store and 14.3 ms on a reboot that wrote nothing. Held past an RTC midnight. Time kept, 0Eh =30(BSF = 1, PORF = 0)7b6a82aFixes #3547
Iteration history and review: ptr727/meshcore-dev-MeshCore#11. Related: #3544, which stops
begin()adopting a device that only ACKs at an RTC address, and #3421, which guards the RV3028 time reads and writes against the same switchover.🤖 Generated with Claude Code