From a7b43f68be8312dc3713b820032fb426597fdc4c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 4 Oct 2026 08:55:43 -0700 Subject: [PATCH 1/5] rtc: identify RTCs by documented always-zero bits; stop skipping real ones rtc_probe() decided whether the device at a declared address was the RTC it expected from one read: a valid BCD block including the hours and the year, or the power-loss flag set (or unreadable). Three ways that went wrong, fork issue #38: - A real RTC was skipped whenever its year byte was not BCD 00-99 or its hours were not 00-23 after masking 0x3F, with the flag clear. Another firmware can leave either: MeshCore's RTClib writes bin2bcd(year-2000), so a bad epoch puts A0 or more in the year, and a DS3231 or RV3028 in 12-hour mode reads PM hours with bit 5 set. - Nothing ruled out a device whose time fields happened to decode, and only one read decided. - An erased EEPROM at the address (every byte 0xFF) read as a lost-power RTC through its 0xFF "status" and was adopted, so its bytes got time writes on every sync. Now, as MeshCore does (meshcore-dev/MeshCore#3544): - An optional zero-mask on each descriptor gives the bits the chip always reads as 0, taken from its data sheet: DS3231 (19-5170 Rev 10, Figure 1, p. 11), RV3028 (RV-3028-C7 App Manual Rev 1.4, 3.2, p. 12) and 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". - A 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 read never does. With no clean read the chip is adopted for write-back but no time is taken from it. - An all-0xFF block 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 hours and year are valid too; otherwise the chip waits for the next sync. The decision is one function, rtc_identify(), so it can be exercised per candidate. Co-Authored-By: Claude Opus 5.5 --- zephcore/adapters/clock/ZephyrRTCDiscover.c | 148 ++++++++++++++---- zephcore/adapters/clock/ZephyrRTCDiscover.h | 13 +- zephcore/boards/common/rtc-i2c.dtsi | 8 + .../dts/bindings/rtc/zephcore,rtc-i2c.yaml | 10 ++ 4 files changed, 143 insertions(+), 36 deletions(-) diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.c b/zephcore/adapters/clock/ZephyrRTCDiscover.c index db373056e..59d62d223 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.c +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.c @@ -17,6 +17,8 @@ #include #include +#include + LOG_MODULE_REGISTER(zephcore_rtc, CONFIG_ZEPHCORE_DATASTORE_LOG_LEVEL); #define RTC_COMPAT zephcore_rtc_i2c @@ -33,9 +35,21 @@ struct rtc_desc { uint8_t date_index; /* day-of-month offset in the 7-byte block */ uint8_t status_reg; /* power-loss flag register, or RTC_STATUS_IN_SECONDS */ uint8_t status_mask; /* "time unreliable" bit within status_reg */ + const uint8_t *zero; /* 7 bytes of always-zero bits, or NULL */ const char *name; }; +#define RTC_ZERO_NAME(node) _CONCAT(rtc_zero_, DT_DEP_ORD(node)) + +#define RTC_ZERO_ARRAY(node) \ + IF_ENABLED(DT_NODE_HAS_PROP(node, zero_mask), \ + (static const uint8_t RTC_ZERO_NAME(node)[] = \ + DT_PROP(node, zero_mask); \ + BUILD_ASSERT(sizeof(RTC_ZERO_NAME(node)) == 7, \ + "zero-mask is one byte per time register");)) + +DT_FOREACH_STATUS_OKAY(RTC_COMPAT, RTC_ZERO_ARRAY) + #define RTC_DESC_ENTRY(node) \ { \ .bus = DEVICE_DT_GET(DT_BUS(node)), \ @@ -44,6 +58,9 @@ struct rtc_desc { .date_index = (uint8_t)DT_PROP(node, date_index), \ .status_reg = (uint8_t)DT_PROP(node, status_reg), \ .status_mask = (uint8_t)DT_PROP(node, status_mask), \ + .zero = COND_CODE_1( \ + DT_NODE_HAS_PROP(node, zero_mask), \ + (RTC_ZERO_NAME(node)), (NULL)), \ .name = DT_NODE_FULL_NAME(node), \ }, @@ -54,14 +71,15 @@ static const struct rtc_desc rtc_descs[] = { /* Chip we'll read/write going forward (first one found present). */ static const struct rtc_desc *s_active; static bool s_probed; +/* A candidate read all 0xFF and was skipped: the first save probes again. */ +static bool s_skipped_ff; +static bool s_reprobed; #define BCD2BIN(x) ((((x) >> 4) & 0x0F) * 10 + ((x) & 0x0F)) #define BIN2BCD(x) ((((x) / 10) << 4) | ((x) % 10)) /* A byte is valid BCD if both nibbles are 0-9, and its decoded value fits the - * field. Used to tell a real RTC apart from an unrelated I2C chip that happens - * to share an address (e.g. an MPU-class IMU at 0x68, same as DS3231) — we must - * never adopt and write time into such a device. */ + * field. */ static bool bcd_field_ok(uint8_t v, unsigned max) { if ((v & 0x0F) > 9 || (v >> 4) > 9) { @@ -95,8 +113,8 @@ static void civil_from_days(int64_t z, int *y, unsigned *m, unsigned *d) *y = yy + (*m <= 2); } -/* Read the chip's power-loss flag. true => held time is unreliable. */ -static bool rtc_time_unreliable(const struct rtc_desc *d, const uint8_t blk[7]) +/* The chip's power-loss flag: 1 set, 0 clear, -1 the status read failed. */ +static int rtc_power_flag(const struct rtc_desc *d, const uint8_t blk[7]) { if (d->status_reg == RTC_STATUS_IN_SECONDS) { return (blk[0] & d->status_mask) != 0; @@ -104,57 +122,116 @@ static bool rtc_time_unreliable(const struct rtc_desc *d, const uint8_t blk[7]) uint8_t st; if (i2c_reg_read_byte(d->bus, d->addr, d->status_reg, &st) != 0) { - return true; /* can't confirm => don't trust it */ + return -1; } return (st & d->status_mask) != 0; } -/* Probe all chips once; cache the first present one in s_active. If a present +/* True if one read shows the device is not this RTC: a bit the chip always + * reads as 0 is set, or a mode-independent field (seconds, minutes, date, + * month) is out of range while the power-loss flag does not read as set. The + * year and hours are not checked: other firmware can leave a year byte past + * 99 or the chip in 12-hour mode, and the chip is still this RTC. */ +static bool rtc_ruled_out(const struct rtc_desc *d, const uint8_t blk[7]) +{ + for (size_t k = 0; d->zero != NULL && k < 7; k++) { + if (blk[k] & d->zero[k]) { + return true; + } + } + + uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F; + bool fields_ok = bcd_field_ok(blk[0] & 0x7F, 59) && bcd_field_ok(blk[1] & 0x7F, 59) && + bcd_field_ok(db, 31) && BCD2BIN(db) >= 1 && + bcd_field_ok(ob, 12) && BCD2BIN(ob) >= 1; + + return !fields_ok && rtc_power_flag(d, blk) != 1; +} + +enum rtc_verdict { RTC_ABSENT, RTC_ERASED, RTC_NOT_THIS, RTC_FOUND, RTC_FOUND_GARBLED }; + +/* Decide whether the device at d is the RTC it declares. A device is ruled + * out only when two reads each rule it out; a failed read never does. An + * all-0xFF block is an erased EEPROM, though a real RTC can power up that + * way, so it is skipped for now. RTC_FOUND leaves the block to decode in blk; + * RTC_FOUND_GARBLED is this RTC with no clean read to take a time from. */ +static enum rtc_verdict rtc_identify(const struct rtc_desc *d, uint8_t blk[7]) +{ + uint8_t again[7]; + bool all_ff = true; + + if (!device_is_ready(d->bus) || + i2c_burst_read(d->bus, d->addr, d->time_reg, blk, 7) != 0) { + return RTC_ABSENT; + } + for (size_t k = 0; k < 7; k++) { + all_ff = all_ff && (blk[k] == 0xFF); + } + if (all_ff) { + return RTC_ERASED; + } + if (!rtc_ruled_out(d, blk)) { + return RTC_FOUND; + } + if (i2c_burst_read(d->bus, d->addr, d->time_reg, again, sizeof(again)) != 0) { + return RTC_FOUND_GARBLED; + } + if (rtc_ruled_out(d, again)) { + return RTC_NOT_THIS; + } + memcpy(blk, again, sizeof(again)); + return RTC_FOUND; +} + +/* Probe all chips once; cache the first one found in s_active. If a found * chip holds a sane time, return it via epoch_out. */ static bool rtc_probe(uint32_t *epoch_out) { + s_skipped_ff = false; + for (size_t i = 0; i < ARRAY_SIZE(rtc_descs); i++) { const struct rtc_desc *d = &rtc_descs[i]; uint8_t blk[7]; + enum rtc_verdict v = rtc_identify(d, blk); - if (!device_is_ready(d->bus)) { + if (v == RTC_ERASED) { + s_skipped_ff = true; continue; } - if (i2c_burst_read(d->bus, d->addr, d->time_reg, blk, sizeof(blk)) != 0) { - continue; /* no ACK => chip absent */ + if (v == RTC_NOT_THIS) { + LOG_INF("%s: device at the address is not this RTC, skipped", d->name); + continue; } - - /* Mask off flag/century bits. We trust this is a real RTC (vs. an - * unrelated chip sharing the address) if EITHER the block is valid - * BCD, OR the chip's power-loss flag is set — the latter is itself - * proof it's an RTC that lost power and whose time registers may be - * garbage. Adopting in the power-loss case is essential: otherwise a - * battery-depleted RTC (garbage registers, VL/OSF/PORF latched) would - * never become the write-back target, so a sync could never - * re-initialise it and the clock would stay blank forever. */ - uint8_t sb = blk[0] & 0x7F, mb = blk[1] & 0x7F, hb = blk[2] & 0x3F; - uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F, yb = blk[6]; - - bool bcd_ok = bcd_field_ok(sb, 59) && bcd_field_ok(mb, 59) && - bcd_field_ok(hb, 23) && bcd_field_ok(db, 31) && - bcd_field_ok(ob, 12) && bcd_field_ok(yb, 99) && - BCD2BIN(db) >= 1 && BCD2BIN(ob) >= 1; - bool unreliable = rtc_time_unreliable(d, blk); - - if (!bcd_ok && !unreliable) { - continue; /* neither valid time nor a lost-power RTC => skip */ + if (v == RTC_ABSENT) { + continue; } if (s_active == NULL) { s_active = d; /* RTC => our write-back target */ } - if (unreliable) { + if (v == RTC_FOUND_GARBLED) { + LOG_WRN("%s present, time unreadable — clock will be set on the " + "next GPS/app/CLI sync", d->name); + continue; + } + if (rtc_power_flag(d, blk) != 0) { LOG_WRN("%s present, power-loss flag set — clock will be set " "on the next GPS/app/CLI sync", d->name); continue; } + uint8_t sb = blk[0] & 0x7F, mb = blk[1] & 0x7F, hb = blk[2] & 0x3F; + uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F, yb = blk[6]; + + /* Identity leaves the hours and year unchecked; a time needs both. */ + if (!bcd_field_ok(hb, 23) || !bcd_field_ok(yb, 99)) { + LOG_WRN("%s present, hours or year unreadable (0x%02x, 0x%02x) — " + "clock will be set on the next GPS/app/CLI sync", + d->name, blk[2], yb); + continue; + } + unsigned sec = BCD2BIN(sb); unsigned min = BCD2BIN(mb); unsigned hour = BCD2BIN(hb); @@ -172,9 +249,9 @@ static bool rtc_probe(uint32_t *epoch_out) hour * 3600 + min * 60 + sec; if (epoch_out) { *epoch_out = (uint32_t)e; + LOG_INF("RTC %s: restored %04u-%02u-%02u %02u:%02u:%02u UTC", + d->name, year, month, day, hour, min, sec); } - LOG_INF("RTC %s: restored %04u-%02u-%02u %02u:%02u:%02u UTC", - d->name, year, month, day, hour, min, sec); return true; } return false; @@ -192,6 +269,11 @@ void zephcore_rtc_save(uint32_t epoch) /* Restore wasn't run (unexpected) — discover now. */ (void)rtc_probe(NULL); s_probed = true; + } else if (s_active == NULL && s_skipped_ff && !s_reprobed) { + /* A running RTC may have counted off all 0xFF since boot; an + * erased EEPROM has not, and is skipped again. */ + s_reprobed = true; + (void)rtc_probe(NULL); } if (s_active == NULL) { return; diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.h b/zephcore/adapters/clock/ZephyrRTCDiscover.h index 4a338625f..3b85cb234 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.h +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.h @@ -25,9 +25,14 @@ extern "C" { /* * Probe all declared RTC chips. If one is present and holds a sane time * (year >= 2025 and its power-loss flag is clear), store the Unix epoch in - * *epoch_out and return true. The present chip (valid time or not) is - * remembered as the write-back target. Returns false if none present or no - * trustworthy time is held. + * *epoch_out and return true. The first chip found (valid time or not) is + * remembered as the write-back target. A device at a declared address is + * passed over if its time block reads all 0xFF (an erased EEPROM; the first + * save probes again), or if two reads each show it is not that RTC: a bit + * the descriptor's zero-mask says reads 0 is set, or seconds, minutes, date + * or month are out of range while the power-loss flag does not read as set. + * A time is taken only from a clean read whose hours and year are valid too. + * Returns false if none present or no trustworthy time is held. */ bool zephcore_rtc_restore(uint32_t *epoch_out); @@ -35,6 +40,8 @@ bool zephcore_rtc_restore(uint32_t *epoch_out); * Persist an authoritative epoch to the discovered RTC chip and clear its * power-loss flag. No-op if no RTC was discovered. Safe to call often, but * intended only for real syncs (GPS/app/CLI), not per-packet clock nudges. + * If restore found nothing but skipped an all-0xFF candidate, the first save + * probes again, once per boot, in the caller's context. */ void zephcore_rtc_save(uint32_t epoch); diff --git a/zephcore/boards/common/rtc-i2c.dtsi b/zephcore/boards/common/rtc-i2c.dtsi index 1dc262ca9..ddad02d00 100644 --- a/zephcore/boards/common/rtc-i2c.dtsi +++ b/zephcore/boards/common/rtc-i2c.dtsi @@ -39,6 +39,8 @@ rtc_ds3231: rtc-ds3231@68 { date-index = <4>; status-reg = <0x0F>; /* control/status */ status-mask = <0x80>; /* OSF — oscillator stopped */ + /* DS3231 data sheet 19-5170 Rev 10, Figure 1, p. 11; 05h bit 7 is Century */ + zero-mask = [80 80 80 f8 c0 60 00]; }; rtc_pcf8563: rtc-pcf8563@51 { @@ -48,6 +50,8 @@ rtc_pcf8563: rtc-pcf8563@51 { date-index = <3>; /* PCF8563 puts date before weekday */ status-reg = <0xFF>; /* flag is in the seconds byte */ status-mask = <0x80>; /* VL — clock integrity not guaranteed */ + /* No zero-mask: the data sheet (Rev. 11.1, Table 4, p. 10) marks the + * unused bits "not relevant", not 0. */ }; rtc_rv3028: rtc-rv3028@52 { @@ -57,6 +61,8 @@ rtc_rv3028: rtc-rv3028@52 { date-index = <4>; status-reg = <0x0e>; /* status */ status-mask = <0x01>; /* PORF — power-on reset */ + /* RV-3028-C7 Application Manual Rev. 1.4, 3.2, p. 12 */ + zero-mask = [80 80 c0 f8 c0 e0 00]; }; rtc_rx8130ce: rtc-rx8130ce@32 { @@ -66,4 +72,6 @@ rtc_rx8130ce: rtc-rx8130ce@32 { date-index = <4>; status-reg = <0x1d>; /* flag */ status-mask = <0x02>; /* VLF — voltage low */ + /* RX8130CE Application Manual ETM50E-10, 13.2.1 Table 12, p. 22 */ + zero-mask = [80 80 c0 80 c0 e0 00]; }; diff --git a/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml b/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml index 92f2e9265..f65109ade 100644 --- a/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml +++ b/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml @@ -41,3 +41,13 @@ properties: type: int required: true description: Bit mask within status-reg that means the held time is unreliable. + + zero-mask: + type: uint8-array + description: > + Seven bytes, one per register of the time block in block order: the + bits this chip always reads as 0, per its data sheet. A device that + shows one of them, or seconds, minutes, date or month out of range + while the power-loss flag does not read as set, on two reads, is not + this RTC and is never adopted. Leave it out for a chip whose unused + bits are not documented as reading 0. From c9e62b963167dd82cdeec5b7432fe42eb7e2d2c4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 4 Oct 2026 09:03:07 -0700 Subject: [PATCH 2/5] rtc: don't refuse a DS1307 on its clock-halt bit; correct three comments From the local passes over a7b43f6 and this commit: - The DS3231 zero-mask checked 00h bit 7. The same 0x68 descriptor also covers the DS1307 and DS3232 (rtc-i2c.dtsi's table). On the DS1307 that bit is CH, clock halt, which a new chip powers up with set, its time at 01/01/00 00:00:00 (DS1307 data sheet Rev 3/15, p. 8), so a new DS1307 was refused on both reads and its clock never started. The bit is left out of the mask. Checked against the DS1307 (Table 2, p. 8: "0 = Always reads back as 0") and the DS3232 (19-5337 Rev 5, Figure 1, p. 11), every other masked bit is a documented 0 in all three parts. The comment also notes that on a DS1307 0Fh is RAM, so the power-loss check reads a user byte there, as on dev. - The DS3231 comment and the binding said "always reads as 0". The DS3231 sheet only shows those bits as 0 (Figure 1, p. 11) and notes the power-up state is undefined; both now say "shows as 0". - The commit and header said a failed read never rules a device out. That holds for the time block; an unreadable power-loss flag counts as not set, so it cannot vouch for out-of-range fields, as on #30. The header and rtc_identify()'s comment now say so. - meshtracker_x1's 0x32 descriptor comment promised the probe "BCD-validates what it reads". It now says a device is passed over when two reads both show seconds, minutes, date or month out of range with VLF clear, and that this part has no zero-mask. Co-Authored-By: Claude Opus 5.5 --- zephcore/adapters/clock/ZephyrRTCDiscover.c | 3 ++- zephcore/adapters/clock/ZephyrRTCDiscover.h | 4 +++- zephcore/boards/common/rtc-i2c.dtsi | 8 ++++++-- .../nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts | 5 +++-- zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml | 2 +- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.c b/zephcore/adapters/clock/ZephyrRTCDiscover.c index 59d62d223..7ba94d006 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.c +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.c @@ -151,7 +151,8 @@ static bool rtc_ruled_out(const struct rtc_desc *d, const uint8_t blk[7]) enum rtc_verdict { RTC_ABSENT, RTC_ERASED, RTC_NOT_THIS, RTC_FOUND, RTC_FOUND_GARBLED }; /* Decide whether the device at d is the RTC it declares. A device is ruled - * out only when two reads each rule it out; a failed read never does. An + * out only when two reads each rule it out; a failed time-block read never + * does, though an unreadable power-loss flag cannot vouch for fields. An * all-0xFF block is an erased EEPROM, though a real RTC can power up that * way, so it is skipped for now. RTC_FOUND leaves the block to decode in blk; * RTC_FOUND_GARBLED is this RTC with no clean read to take a time from. */ diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.h b/zephcore/adapters/clock/ZephyrRTCDiscover.h index 3b85cb234..9c9b21140 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.h +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.h @@ -30,7 +30,9 @@ extern "C" { * passed over if its time block reads all 0xFF (an erased EEPROM; the first * save probes again), or if two reads each show it is not that RTC: a bit * the descriptor's zero-mask says reads 0 is set, or seconds, minutes, date - * or month are out of range while the power-loss flag does not read as set. + * or month are out of range while the power-loss flag does not read as set + * (an unreadable flag counts as not set). A failed time-block read never + * rules a device out. * A time is taken only from a clean read whose hours and year are valid too. * Returns false if none present or no trustworthy time is held. */ diff --git a/zephcore/boards/common/rtc-i2c.dtsi b/zephcore/boards/common/rtc-i2c.dtsi index ddad02d00..b4edf1cd8 100644 --- a/zephcore/boards/common/rtc-i2c.dtsi +++ b/zephcore/boards/common/rtc-i2c.dtsi @@ -39,8 +39,12 @@ rtc_ds3231: rtc-ds3231@68 { date-index = <4>; status-reg = <0x0F>; /* control/status */ status-mask = <0x80>; /* OSF — oscillator stopped */ - /* DS3231 data sheet 19-5170 Rev 10, Figure 1, p. 11; 05h bit 7 is Century */ - zero-mask = [80 80 80 f8 c0 60 00]; + /* Bits shown as 0 in all three parts: DS3231 (19-5170 Rev 10, Figure 1, + * p. 11) and DS3232 (19-5337 Rev 5, Figure 1, p. 11), where 05h bit 7 is + * Century, and DS1307 (Rev 3/15, Table 2, p. 8, "always reads back as + * 0"). 00h bit 7 is left out: it is the DS1307's clock-halt bit, set on + * a new chip. On a DS1307, 0Fh is RAM, so status-reg reads a user byte. */ + zero-mask = [00 80 80 f8 c0 60 00]; }; rtc_pcf8563: rtc-pcf8563@51 { diff --git a/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts b/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts index 3f7de1ef7..9ef329f92 100644 --- a/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts +++ b/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts @@ -335,8 +335,9 @@ * The part is an RX8900 second-source (same 3.2x2.5x1.0 package, same * 32.768 kHz TCXO grades), so it uses the RX8900 register layout rather than * the RX8130CE one that rtc-i2c.dtsi assumes for address 0x32: time at 0x00, - * FLAG at 0x0E, VLF in bit 1. Boot-time discovery BCD-validates what it reads, - * so a wrong guess here is detected and skipped, not silently trusted. */ + * FLAG at 0x0E, VLF in bit 1. Boot-time discovery passes a device over when + * two reads both show seconds, minutes, date or month out of range with VLF + * clear, so most wrong guesses are skipped; there is no zero-mask here. */ &i2c1 { compatible = "nordic,nrf-twim"; status = "okay"; diff --git a/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml b/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml index f65109ade..7e6643c25 100644 --- a/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml +++ b/zephcore/dts/bindings/rtc/zephcore,rtc-i2c.yaml @@ -46,7 +46,7 @@ properties: type: uint8-array description: > Seven bytes, one per register of the time block in block order: the - bits this chip always reads as 0, per its data sheet. A device that + bits the chip's data sheet shows as reading 0. A device that shows one of them, or seconds, minutes, date or month out of range while the power-loss flag does not read as set, on two reads, is not this RTC and is never adopted. Leave it out for a chip whose unused From 87b86d6ea9566c52f43685f95e93bee0e5f7d86e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 4 Oct 2026 10:07:25 -0700 Subject: [PATCH 3/5] rtc: re-check the waived fields before restoring; four comment fixes From the local pass over c9e62b9; the maintainer approved this third round of edits: - Identity can let out-of-range seconds, minutes, date or month pass on a power-loss flag read as set, and restore read the flag again. Had it read clear the second time, restore checked only the hours and year and decoded the rest, e.g. a month up to 25 into an epoch years off. Restore now re-checks all of them through rtc_fields_ok(), which identity uses too, and logs "time unreadable" otherwise. - Two comments in ZephyrRTCDiscover.c still said the masked bits are ones the chip "always reads as 0"; they say "shows as 0", as the binding and dtsi do. - The header and rtc_identify() said a failed time-block read never rules a device out. A failed first read means nothing is there, as on dev; only a failed second read does not count. - meshtracker_x1's comment now says an unreadable FLAG also counts as VLF not set. Left as is, matching meshcore-dev/MeshCore#3544: a set power-loss flag does not waive the zero-mask. The DS3231 and DS3232 sheets leave the power-up state undefined, so a new part could in principle show a 1 in a masked bit and be refused. The sheets show those bits as 0 and give no way to write them, so this is listed rather than changed. Co-Authored-By: Claude Opus 5.5 --- zephcore/adapters/clock/ZephyrRTCDiscover.c | 48 +++++++++++-------- zephcore/adapters/clock/ZephyrRTCDiscover.h | 4 +- .../meshtracker_x1_nrf52840.dts | 3 +- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.c b/zephcore/adapters/clock/ZephyrRTCDiscover.c index 7ba94d006..0418d217c 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.c +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.c @@ -35,7 +35,7 @@ struct rtc_desc { uint8_t date_index; /* day-of-month offset in the 7-byte block */ uint8_t status_reg; /* power-loss flag register, or RTC_STATUS_IN_SECONDS */ uint8_t status_mask; /* "time unreliable" bit within status_reg */ - const uint8_t *zero; /* 7 bytes of always-zero bits, or NULL */ + const uint8_t *zero; /* 7 bytes of bits the data sheet shows as 0, or NULL */ const char *name; }; @@ -127,11 +127,22 @@ static int rtc_power_flag(const struct rtc_desc *d, const uint8_t blk[7]) return (st & d->status_mask) != 0; } -/* True if one read shows the device is not this RTC: a bit the chip always - * reads as 0 is set, or a mode-independent field (seconds, minutes, date, - * month) is out of range while the power-loss flag does not read as set. The - * year and hours are not checked: other firmware can leave a year byte past - * 99 or the chip in 12-hour mode, and the chip is still this RTC. */ +/* Seconds, minutes, date and month in range: the fields that read the same in + * 12- and 24-hour mode. */ +static bool rtc_fields_ok(const struct rtc_desc *d, const uint8_t blk[7]) +{ + uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F; + + return bcd_field_ok(blk[0] & 0x7F, 59) && bcd_field_ok(blk[1] & 0x7F, 59) && + bcd_field_ok(db, 31) && BCD2BIN(db) >= 1 && + bcd_field_ok(ob, 12) && BCD2BIN(ob) >= 1; +} + +/* True if one read shows the device is not this RTC: a bit the data sheet + * shows as 0 is set, or rtc_fields_ok() fails while the power-loss flag does + * not read as set. The year and hours are not checked: other firmware can + * leave a year byte past 99 or the chip in 12-hour mode, and the chip is + * still this RTC. */ static bool rtc_ruled_out(const struct rtc_desc *d, const uint8_t blk[7]) { for (size_t k = 0; d->zero != NULL && k < 7; k++) { @@ -139,20 +150,15 @@ static bool rtc_ruled_out(const struct rtc_desc *d, const uint8_t blk[7]) return true; } } - - uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F; - bool fields_ok = bcd_field_ok(blk[0] & 0x7F, 59) && bcd_field_ok(blk[1] & 0x7F, 59) && - bcd_field_ok(db, 31) && BCD2BIN(db) >= 1 && - bcd_field_ok(ob, 12) && BCD2BIN(ob) >= 1; - - return !fields_ok && rtc_power_flag(d, blk) != 1; + return !rtc_fields_ok(d, blk) && rtc_power_flag(d, blk) != 1; } enum rtc_verdict { RTC_ABSENT, RTC_ERASED, RTC_NOT_THIS, RTC_FOUND, RTC_FOUND_GARBLED }; -/* Decide whether the device at d is the RTC it declares. A device is ruled - * out only when two reads each rule it out; a failed time-block read never - * does, though an unreadable power-loss flag cannot vouch for fields. An +/* Decide whether the device at d is the RTC it declares. A first read that + * fails means nothing is there. Otherwise it is ruled out only when two reads + * each rule it out: a failed second read does not, and an unreadable + * power-loss flag cannot vouch for the fields. An * all-0xFF block is an erased EEPROM, though a real RTC can power up that * way, so it is skipped for now. RTC_FOUND leaves the block to decode in blk; * RTC_FOUND_GARBLED is this RTC with no clean read to take a time from. */ @@ -225,11 +231,11 @@ static bool rtc_probe(uint32_t *epoch_out) uint8_t sb = blk[0] & 0x7F, mb = blk[1] & 0x7F, hb = blk[2] & 0x3F; uint8_t db = blk[d->date_index] & 0x3F, ob = blk[5] & 0x1F, yb = blk[6]; - /* Identity leaves the hours and year unchecked; a time needs both. */ - if (!bcd_field_ok(hb, 23) || !bcd_field_ok(yb, 99)) { - LOG_WRN("%s present, hours or year unreadable (0x%02x, 0x%02x) — " - "clock will be set on the next GPS/app/CLI sync", - d->name, blk[2], yb); + /* Identity leaves the hours and year unchecked, and may have let the + * other fields pass on a flag read since; a time needs them all. */ + if (!rtc_fields_ok(d, blk) || !bcd_field_ok(hb, 23) || !bcd_field_ok(yb, 99)) { + LOG_WRN("%s present, time unreadable — clock will be set on the " + "next GPS/app/CLI sync", d->name); continue; } diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.h b/zephcore/adapters/clock/ZephyrRTCDiscover.h index 9c9b21140..787177497 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.h +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.h @@ -31,8 +31,8 @@ extern "C" { * save probes again), or if two reads each show it is not that RTC: a bit * the descriptor's zero-mask says reads 0 is set, or seconds, minutes, date * or month are out of range while the power-loss flag does not read as set - * (an unreadable flag counts as not set). A failed time-block read never - * rules a device out. + * (an unreadable flag counts as not set). A failed second read never rules + * a device out; a failed first read means nothing is there. * A time is taken only from a clean read whose hours and year are valid too. * Returns false if none present or no trustworthy time is held. */ diff --git a/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts b/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts index 9ef329f92..514578e07 100644 --- a/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts +++ b/zephcore/boards/nrf52840/meshtracker_x1/meshtracker_x1_nrf52840.dts @@ -337,7 +337,8 @@ * the RX8130CE one that rtc-i2c.dtsi assumes for address 0x32: time at 0x00, * FLAG at 0x0E, VLF in bit 1. Boot-time discovery passes a device over when * two reads both show seconds, minutes, date or month out of range with VLF - * clear, so most wrong guesses are skipped; there is no zero-mask here. */ + * not read as set (clear, or the FLAG read failed), so most wrong guesses + * are skipped; there is no zero-mask here. */ &i2c1 { compatible = "nordic,nrf-twim"; status = "okay"; From c947e26fc2b5a7de54c1d4051ea73e192c26ca11 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 4 Oct 2026 10:18:50 -0700 Subject: [PATCH 4/5] rtc: rule out a device whose second read is all 0xFF; name an unreadable flag From Copilot's review of #40 at 87b86d6, and the local pass over this change: - A device whose first read was ruled out but not all 0xFF, and whose confirming read came back all 0xFF, sent that second block to rtc_ruled_out(). A descriptor with a zero-mask rules it out, but the PCF8563 has none and keeps its VL flag in the seconds byte, so 0xFF read as "power lost" and the device was adopted, then written on every sync. Two reads that disagree like that mean an unstable device, not an erased one (an erased part reads all 0xFF the first time and is skipped there), so the all-0xFF second read now rules it out, the same for every descriptor, via rtc_all_ff(). - An unreadable status register was logged as "power-loss flag set". The log now says "unreadable" in that case; the handling is unchanged. Co-Authored-By: Claude Opus 5.5 --- zephcore/adapters/clock/ZephyrRTCDiscover.c | 40 +++++++++++++-------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.c b/zephcore/adapters/clock/ZephyrRTCDiscover.c index 0418d217c..a3c3f9c01 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.c +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.c @@ -155,26 +155,33 @@ static bool rtc_ruled_out(const struct rtc_desc *d, const uint8_t blk[7]) enum rtc_verdict { RTC_ABSENT, RTC_ERASED, RTC_NOT_THIS, RTC_FOUND, RTC_FOUND_GARBLED }; +static bool rtc_all_ff(const uint8_t blk[7]) +{ + for (size_t k = 0; k < 7; k++) { + if (blk[k] != 0xFF) { + return false; + } + } + return true; +} + /* Decide whether the device at d is the RTC it declares. A first read that - * fails means nothing is there. Otherwise it is ruled out only when two reads - * each rule it out: a failed second read does not, and an unreadable - * power-loss flag cannot vouch for the fields. An - * all-0xFF block is an erased EEPROM, though a real RTC can power up that - * way, so it is skipped for now. RTC_FOUND leaves the block to decode in blk; - * RTC_FOUND_GARBLED is this RTC with no clean read to take a time from. */ + * fails means nothing is there, and one that is all 0xFF is an erased EEPROM, + * though a real RTC can power up that way, so it is skipped for now. + * Otherwise the device is ruled out only when two reads each rule it out: a + * failed second read does not, an all-0xFF second read does, and an + * unreadable power-loss flag cannot vouch for the fields. RTC_FOUND leaves + * the block to decode in blk; RTC_FOUND_GARBLED is this RTC with no clean + * read to take a time from. */ static enum rtc_verdict rtc_identify(const struct rtc_desc *d, uint8_t blk[7]) { uint8_t again[7]; - bool all_ff = true; if (!device_is_ready(d->bus) || i2c_burst_read(d->bus, d->addr, d->time_reg, blk, 7) != 0) { return RTC_ABSENT; } - for (size_t k = 0; k < 7; k++) { - all_ff = all_ff && (blk[k] == 0xFF); - } - if (all_ff) { + if (rtc_all_ff(blk)) { return RTC_ERASED; } if (!rtc_ruled_out(d, blk)) { @@ -183,7 +190,7 @@ static enum rtc_verdict rtc_identify(const struct rtc_desc *d, uint8_t blk[7]) if (i2c_burst_read(d->bus, d->addr, d->time_reg, again, sizeof(again)) != 0) { return RTC_FOUND_GARBLED; } - if (rtc_ruled_out(d, again)) { + if (rtc_all_ff(again) || rtc_ruled_out(d, again)) { return RTC_NOT_THIS; } memcpy(blk, again, sizeof(again)); @@ -222,9 +229,12 @@ static bool rtc_probe(uint32_t *epoch_out) "next GPS/app/CLI sync", d->name); continue; } - if (rtc_power_flag(d, blk) != 0) { - LOG_WRN("%s present, power-loss flag set — clock will be set " - "on the next GPS/app/CLI sync", d->name); + int flag = rtc_power_flag(d, blk); + + if (flag != 0) { + LOG_WRN("%s present, power-loss flag %s — clock will be set " + "on the next GPS/app/CLI sync", d->name, + flag > 0 ? "set" : "unreadable"); continue; } From 0a4618d7599254eb2019377513c13a7ac509b747 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 4 Oct 2026 11:32:54 -0700 Subject: [PATCH 5/5] rtc: state the all-0xFF second read in the probe's contract From Copilot's review of liquidraver/ZephCore#99: the header's zephcore_rtc_restore() contract did not say that an all-0xFF second read rules a device out, which rtc_identify() has done since c947e26, nor that, unlike a first-read all-0xFF skip, it does not itself cause the first save to probe again. The restore paragraph now lists the cases as the code checks them: a failed first read (nothing there), an all-0xFF first read (skipped), then the two-read rule-out, whose list now includes the all-0xFF second read. The save paragraph now says a first-read all-0xFF skip makes the first save probe again when nothing was adopted, and that a device ruled out on its second read is not probed again for that reason (it is re-identified only if another candidate's skip triggers the re-probe). The same paragraphs, and two comments in the .c, also now match the code where they over-claimed: probing runs in order and stops at the first sane time; masked bits are ones the data sheet shows as 0, as the binding and dtsi say; a time needs its fields, hours and year each in range, since restore re-checks them (87b86d6), with 12-hour mode not decoded (fork issue #34). Co-Authored-By: Claude Opus 5.5 --- zephcore/adapters/clock/ZephyrRTCDiscover.c | 7 ++--- zephcore/adapters/clock/ZephyrRTCDiscover.h | 30 +++++++++++---------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.c b/zephcore/adapters/clock/ZephyrRTCDiscover.c index a3c3f9c01..3fb81843a 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.c +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.c @@ -71,7 +71,8 @@ static const struct rtc_desc rtc_descs[] = { /* Chip we'll read/write going forward (first one found present). */ static const struct rtc_desc *s_active; static bool s_probed; -/* A candidate read all 0xFF and was skipped: the first save probes again. */ +/* A candidate's first read was all 0xFF and it was skipped: if nothing was + * adopted, the first save probes again. */ static bool s_skipped_ff; static bool s_reprobed; @@ -197,8 +198,8 @@ static enum rtc_verdict rtc_identify(const struct rtc_desc *d, uint8_t blk[7]) return RTC_FOUND; } -/* Probe all chips once; cache the first one found in s_active. If a found - * chip holds a sane time, return it via epoch_out. */ +/* Probe the chips in order; cache the first one found in s_active. Stop at + * the first that holds a sane time, returned via epoch_out. */ static bool rtc_probe(uint32_t *epoch_out) { s_skipped_ff = false; diff --git a/zephcore/adapters/clock/ZephyrRTCDiscover.h b/zephcore/adapters/clock/ZephyrRTCDiscover.h index 787177497..ba82da2ce 100644 --- a/zephcore/adapters/clock/ZephyrRTCDiscover.h +++ b/zephcore/adapters/clock/ZephyrRTCDiscover.h @@ -23,18 +23,19 @@ extern "C" { #endif /* - * Probe all declared RTC chips. If one is present and holds a sane time - * (year >= 2025 and its power-loss flag is clear), store the Unix epoch in - * *epoch_out and return true. The first chip found (valid time or not) is - * remembered as the write-back target. A device at a declared address is - * passed over if its time block reads all 0xFF (an erased EEPROM; the first - * save probes again), or if two reads each show it is not that RTC: a bit - * the descriptor's zero-mask says reads 0 is set, or seconds, minutes, date - * or month are out of range while the power-loss flag does not read as set - * (an unreadable flag counts as not set). A failed second read never rules - * a device out; a failed first read means nothing is there. - * A time is taken only from a clean read whose hours and year are valid too. - * Returns false if none present or no trustworthy time is held. + * Probe the declared RTC chips in order, stopping at the first that holds a + * sane time (year >= 2025 and its power-loss flag clear): store its Unix + * epoch in *epoch_out and return true. The first chip found (valid time or + * not) is remembered as the write-back target. A failed first read means + * nothing is there, and a first read of all 0xFF (an erased EEPROM) is + * skipped. Otherwise a device is passed over only if two reads each show it + * is not that RTC: a bit set that the data sheet shows as 0 (the + * descriptor's zero-mask), seconds, minutes, date or month out of range + * while the power-loss flag does not read as set (an unreadable flag counts + * as not set), or, on the second read, all 0xFF. A failed second read never + * rules a device out. A time is taken only from a clean read whose fields, + * hours and year are each in range (12-hour mode is not decoded). Returns + * false if none present or no trustworthy time is held. */ bool zephcore_rtc_restore(uint32_t *epoch_out); @@ -42,8 +43,9 @@ bool zephcore_rtc_restore(uint32_t *epoch_out); * Persist an authoritative epoch to the discovered RTC chip and clear its * power-loss flag. No-op if no RTC was discovered. Safe to call often, but * intended only for real syncs (GPS/app/CLI), not per-packet clock nudges. - * If restore found nothing but skipped an all-0xFF candidate, the first save - * probes again, once per boot, in the caller's context. + * If restore adopted nothing but skipped a candidate whose first read was all + * 0xFF, the first save probes again, once per boot, in the caller's context. + * A device ruled out on its second read is not probed again for that reason. */ void zephcore_rtc_save(uint32_t epoch);