Repository navigation
Conversation
ptr727
marked this pull request as ready for review
October 4, 2026 15:47
This was referenced Oct 4, 2026
There was a problem hiding this comment.
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>
ptr727
force-pushed
the
fix/rtc-probe-identity
branch
from
October 4, 2026 16:19
e2ad35c to
9d0d5e3
Compare
This was referenced Oct 4, 2026
Merged
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>
This was referenced Oct 5, 2026
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
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.
References used throughout. Each "§", table and "p." number below is the cited document's own, and page numbers are the printed ones.
dev3e3150c89d0d5e312.1.4, the version MeshCore buildsThe 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.begin().Upstream has already worked around this board by board for IMUs at 0x68:
e7e97ec4, with lilygo_techo_card0908b034, with heltec_t1: "producing a bogus, unfixable clock"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:Always-zero bit set. A bit the datasheet documents as always 0, in the seven time registers, reads 1:
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)80 80 C0 F8 C0 E0 0080 80 C0 80 C0 E0 00, where "'0' means … the read value is always 0"All 0xFF. All seven bytes read 0xFF, as an erased 24-series EEPROM does.
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:
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.year - 2000as BCD, so a bad epoch of 2100 or later givesA0+. A chip left in 12-hour mode sets the hours' PM bit. Neither may rule out a real chip.begin(). The existingDISABLE_DS3231_PROBEopt-outs stay, and compile the DS3231 table out.Notes for review
B1at 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.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 (seconds80, date and month01) then passes the field check. The trade-off: an ICM-20948's WHO_AM_I (EAat 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 keepDISABLE_DS3231_PROBE.begin(), and that PR gates its own EEPROM writes with an RV3028-only always-zero check. Whichever lands second takes the other'sbegin()hunk.Testing
Builds: release builds of
RAK_4631_repeater(nRF52840),heltec_v4_repeater(ESP32-S3),RAK_11310_repeater(RP2040),Heltec_t1_repeater(withDISABLE_DS3231_PROBE) andR1Neo_repeater(the RX8130CE board), plusMESH_DEBUG=1on 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 of0aacafa4on 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.06 09 12 01 08 01 00B1)00 00 00 00 00 00 0061)33 AA 16 49 13 01 39ZephCore, 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:
getCurrentTime()behaviour.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