Skip to content

Adopt an RTC only if its time registers read like that chip 🤖🤖 - #3544

Open
ptr727 wants to merge 1 commit into
meshcore-dev:devfrom
ptr727:fix/rtc-probe-identity
Open

ptr727 wants to merge 1 commit into
meshcore-dev:devfrom
ptr727:fix/rtc-probe-identity

Conversation

@ptr727

@ptr727 ptr727 commented Oct 4, 2026 •

Copy link
Copy Markdown

References used throughout. Each "§", table and "p." number below is the cited document's own, and page numbers are the printed ones.

The defect

#3546 has the full write-up. In short, begin() adopts each RTC on an I2C ACK alone. Any device answering at 0x68, 0x52, 0x51 or 0x32 then becomes the clock:

  • getCurrentTime() reads its registers as the time.
  • setCurrentTime() writes its registers on every sync, which includes GPS every 30 minutes.
  • An RV3028 or RX8130CE impostor also gets written during begin().

Upstream has already worked around this board by board for IMUs at 0x68:

The fix

Each RTC is adopted only if its time registers read like that chip's. Each rule below is taken from that chip's datasheet (L29-L38, table L39-L65). rtcCheck() rules a device out if any of these hold:

  1. Always-zero bit set. A bit the datasheet documents as always 0, in the seven time registers, reads 1:

    Chip Time registers Always-zero bits Source
    DS3231 00h-06h 00 80 80 F8 C0 60 00 (05h bit 7 is Century, so it is allowed; 00h bit 7 is left out for the DS1307, see below) Figure 1, p. 11
    RV3028 00h-06h 80 80 C0 F8 C0 E0 00 §3.2, p. 12
    PCF8563 02h-08h none: unused bits are "x = not relevant", not 0 Table 4, p. 10
    RX8130CE 10h-16h 80 80 C0 80 C0 E0 00, where "'0' means … the read value is always 0" §13.2.1 Table 12, p. 22
  2. All 0xFF. All seven bytes read 0xFF, as an erased 24-series EEPROM does.

  3. Invalid time with the power-loss flag clear. Seconds, minutes, date or month is not valid BCD in range, and the chip's power-loss flag is clear. Each chip sets that flag at power-up, when its time may be undefined:

    • DS3231: OSF, 0Fh bit 7 ("set to logic 1 … the first time power is applied", p. 14)
    • RV3028: PORF, 0Eh bit 0 (§3.7, p. 22)
    • PCF8563: VL, 02h bit 7 (Table 8, p. 13; startup value 1, Table 27, p. 24)

    The RX8130CE is exempt. Its flag VLF (1Dh bit 1) cannot vouch for the fields, because RTC_RX8130CE::begin() clears it on every boot without setting the time.

How the decision is made:

  • rtcRuledOut() rejects a device only if two reads both rule it out. A failed read never does, so it keeps today's behaviour.
  • rtcRead() uses a repeated start. That is the only read form the RX8130CE manual documents (§19.6), and all four documents allow it.
  • The year, hours and weekday are never checked. MeshCore itself can write an out-of-range year: RTClib writes year - 2000 as BCD, so a bad epoch of 2100 or later gives A0+. A chip left in 12-hour mode sets the hours' PM bit. Neither may rule out a real chip.
  • The checks gate all four probes in begin(). The existing DISABLE_DS3231_PROBE opt-outs stay, and compile the DS3231 table out.

Notes for review

  • This cannot catch every foreign device. One whose bytes happen to fit is still adopted, as before. The hardware test shows one: under the DS3231 rule, an LPS22HB's WHO_AM_I (B1 at 0Fh) sits where the DS3231 keeps OSF, so it reads as "power lost". The per-board opt-outs remain the answer for a known conflict.
  • DS1307 at 0x68, still adopted. A DS1307 (hobby RTC modules) answers at 0x68 and works through the same DS3231 code: the time layout matches, and RTClib's adjust() clears its clock-halt bit. It powers up with that bit (00h bit 7, CH) set (DS1307 Table 2 and text, p. 8), so the 0x68 mask leaves 00h bit 7 out. Every other mask bit is documented 0 on the DS3231, the DS3232 (Figure 1, p. 11) and the DS1307. A new DS1307 (seconds 80, date and month 01) then passes the field check. The trade-off: an ICM-20948's WHO_AM_I (EA at 00h) no longer trips the mask. It fails the field check instead, so it is ruled out unless its 0Fh has bit 7 set. Boards with that IMU keep DISABLE_DS3231_PROBE.
  • Interaction with Store RV3028 backup switchover and trickle charger config in EEPROM 🤖🤖 #3545, which stores the RV3028's backup switchover config in EEPROM: both touch the RV3028 branch of begin(), and that PR gates its own EEPROM writes with an RV3028-only always-zero check. Whichever lands second takes the other's begin() hunk.

Testing

Builds: release builds of RAK_4631_repeater (nRF52840), heltec_v4_repeater (ESP32-S3), RAK_11310_repeater (RP2040), Heltec_t1_repeater (with DISABLE_DS3231_PROBE) and R1Neo_repeater (the RX8130CE board), plus MESH_DEBUG=1 on the first three. No new warnings.

Hardware (read-only). A RAK4631 with a RAK12002 (RV3028), a RAK1902 (ST LPS22HB) and a RAK1906 (Bosch BME680) on the I2C bus, and nothing else, per an ACK scan. Its RAK12501 GNSS (Quectel L76K) is on the UART, not the I2C bus. The build is test/rtc-probe-diag (cc4bf1fa): this change as of 0aacafa4 on the iteration branch, plus a throwaway serial CLI. The later change that leaves 00h bit 7 out of the 0x68 mask was recomputed against the raw blocks below and changes no verdict. It ACK-scans the bus, then runs each chip's check against each address found, using register reads only.

Address Device Registers 00h-06h DS3231 rule RV3028 rule PCF8563 rule RX8130CE rule
0x52 RAK12002 RV3028 (the real RTC) 06 09 12 01 08 01 00 ok ok, adopted ruled out ok
0x5C RAK1902 LPS22HB (0Fh = WHO_AM_I B1) 00 00 00 00 00 00 00 ok (0Fh reads as OSF) ruled out ruled out ok (no field check)
0x76 RAK1906 BME680 (D0h chip ID = 61) 33 AA 16 49 13 01 39 ruled out ruled out ruled out ruled out

ZephCore, a separate firmware, has an independent implementation of the same identity rules: ptr727/liquidraver-ZephCore#40 (now upstream as liquidraver/ZephCore#99), for its issue #38. Run read-only on this same board at a7b43f6 (raw log), it gave the same verdicts at every address, with one recorded exception. ZephCore keeps a field check for the RX8130CE, so it also rules out the LPS22HB under that rule. MeshCore cannot keep that check, because its RX8130CE driver clears VLF on every boot.

The two implementations agree on the rules and the masks. Where they differ, the difference is deliberate and has a recorded reason:

  • No clean read: ZephCore adopts the chip but takes no time from it. MeshCore keeps today's getCurrentTime() behaviour.
  • RX8130CE field check: ZephCore keeps it. MeshCore drops it, for the driver reason above.
  • Unreadable power-loss flag: ZephCore treats it as clear, following its existing convention. MeshCore adopts the device, because a failed read proves nothing.
  • All 0xFF: ZephCore skips the device on one read and re-probes once, on its first save. MeshCore rules it out only on two agreeing reads.

An earlier build that checked only the always-zero bits let the LPS22HB's all-zero block pass three of the four rules. That result is why the field check with the power-loss exception was added.

Fixes #3546

Iteration history and review: ptr727/meshcore-dev-MeshCore#14.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Hardware-specific RTC detection changes warrant final human validation.

Review effort: Lite
Findings: None

What changed in this PR

This pull request hardens RTC auto-discovery by validating chip-specific register contents before adopting an I²C device as the system clock.

Changes:

  • Adds per-chip masks, time validation, and power-loss handling.
  • Uses repeated-start reads and consecutive rejection checks.
  • Applies validation to all four RTC probes while preserving opt-outs.
File Summary
src/​helpers/​AutoDiscoverRTCClock.cpp Adds RTC identity validation and gates device adoption.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

AutoDiscoverRTCClock::begin() adopted each RTC on an I2C ACK alone, so
any device answering at 0x68, 0x52, 0x51 or 0x32 (an IMU, a 24-series
EEPROM) became the clock: read as the time, and written on every time
sync. Upstream already worked around this per board with
DISABLE_DS3231_PROBE for IMUs at 0x68.

Read the seven time registers and skip the device if two reads both show
it cannot be that chip, per its datasheet:
- a bit documented as always 0 is set (DS3231, RV3028, RX8130CE; the
  PCF8563 documents none), or all seven read 0xFF (an erased EEPROM);
- seconds, minutes, date or month is not valid BCD in range while the
  chip's power-loss flag (OSF, PORF, VL), set at power-up when its time
  may be undefined, is clear. Not applied to the RX8130CE, whose driver
  clears VLF in begin() without setting the time.
The year and hours are not checked, as MeshCore can write an
out-of-range year and 12-hour mode sets the hours' PM bit. A failed read
keeps today's behaviour. Reads use a repeated start.

Tested read-only on a RAK4631: the RV3028 is adopted, and two
environment sensors on the same bus are ruled out under the rules that
can tell them apart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

No unresolved review comments remain, and the changes are fully reviewed.

Review effort: Lite
Findings: None

ptr727 added a commit to ptr727/liquidraver-ZephCore that referenced this pull request Oct 4, 2026
… ones

rtc_probe() decided from one read whether the device at a declared
address was the RTC its descriptor expects: a valid BCD block including
hours and year, or the power-loss flag set or unreadable. That skipped
real RTCs whose year byte or hours another firmware left out of range
(a year byte of A0 or more; 12-hour mode reads PM hours with bit 5 set),
ruled nothing out by identity, and adopted an erased EEPROM through its
0xFF "status".

- An optional zero-mask descriptor property holds the time-block bits a
  part's data sheet shows as 0. rtc-i2c.dtsi sets it for 0x68 (DS3231
  19-5170 Rev 10 and DS3232 19-5337 Rev 5, Figure 1, p. 11; DS1307 Rev
  3/15, Table 2, p. 8; 00h bit 7 left out, the DS1307's clock-halt bit),
  the RV3028 (App Manual Rev 1.4, 3.2, p. 12) and the RX8130CE
  (ETM50E-10, 13.2.1 Table 12, p. 22). The PCF8563 gets none: its data
  sheet (Rev 11.1, Table 4, p. 10) marks unused bits "not relevant".
- One read rules a device out if a masked bit is set, or if seconds,
  minutes, date or month are out of range while the power-loss flag does
  not read as set. The year and hours are no longer identity checks.
- A device is passed over only when two reads each rule it out. A failed
  first read means nothing is there; a failed second read does not count,
  and with no clean read the chip is adopted for write-back but gives no
  time. An all-0xFF second read rules the device out.
- An all-0xFF first read is skipped, and the first save probes once more,
  since a real RTC can power up that way.
- A time is restored only from a clean read whose fields, hours and year
  are all valid.

The decision is one function, rtc_identify(). The same rules, with a
few agreed differences, are in meshcore-dev/MeshCore#3544.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mikecarper added a commit to mikecarper/MeshCore that referenced this pull request Oct 10, 2026
Reject truncated anonymous reply paths and advert metadata before copying,
while preserving timestamp-only empty-password logins. Limit room posting to
read-write/admin ACL roles, retain inbox frames until the requesting transport
accepts them, and lock raw LittleFS traversal against concurrent writes.

Separate ISR event generations from main-loop radio state. Confirm RX/TX IRQs,
detach concrete wrapper callbacks safely, and discard inconsistent LR2021 FIFO
snapshots before rearming. Preserve bridge RX frames through packet-pool
exhaustion and bound ESP-NOW retries and missing-callback recovery. Screen RTC
register layouts without losing DS1307 startup, and ignore poisoned future
contact timestamps during clock bootstrap.

Keep the nRF52 loop stack at 8 KiB while reducing the OTA listing call chain
from 7,832 to 6,056 static bytes. Retain all target names through lossless Full
Companion compression and the canonical size optimizer. Preserve constrained
STM32 filesystems and bind corrected ESP32 UART/OLED budgets to source size
assertions. Document the separately measured preexisting W12 package limits.

Adapted for this branch from meshcore-dev/MeshCore PRs meshcore-dev#3521, meshcore-dev#3340, meshcore-dev#3362,
meshcore-dev#2062, meshcore-dev#3503, meshcore-dev#3038, meshcore-dev#3534, meshcore-dev#3544 and meshcore-dev#3512.

Validation: 1,829 native tests; sanitized parser, inbox, LittleFS, ACL, clock,
RTC, IRQ/FIFO and bridge fault tests; 332 additional Companion cases; real
RAK4631 Full and both repeater profiles, G2 Full/standard, V3 Full and both
constrained STM32 release recipes. Radio-family compile coverage continues
separately; W12 full/portable package guards remain enforced.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants