feat(health): lifetime wear counters + health-warning board (/api/health) - #115
feat(health): lifetime wear counters + health-warning board (/api/health)#115iliabaranov wants to merge 5 commits into
Conversation
…lth) Persistent counters to decide when a remote is retired: button presses, lockstep mismatch events, cumulative uptime, boots, flashes (cable + OTA) and OTAs. Stored as one NVS blob (dcs_app/health); survives reboot, idf.py flash and OTA; lost only on erase-flash / NVS auto-erase. Press counting hooks the existing dcs_publish_core_tick sink (three lines): a press is the rising edge of BOTH cores reading STOP on the same tick; a mismatch event is the rising edge of the cores disagreeing on the same tick. The detector baselines on the first aligned observation, so the boot-priming STOP the cores emit for their first ticks, or a button latched at power-on, is never counted. main.c and estop_verdict.c are untouched. Remote only; machn gets uptime/boots/flashes. Flush task: 1 Hz, internal stack (writes NVS), flushes when dirty for >= 60 s or every 600 s, plus best-effort on esp_restart(). Uptime is esp_timer-based, no timekeeping libs; loses at most the last interval. dcs_health_publish(src, level, msg): any task can raise/clear a named warning; /api/health lists them and /state.json carries the worst level as `health`. Built-in publishers: button_wear (NKK FF01 datasheet: 100,000 operations mechanical/electrical life; WARN at 80 %, CRITICAL at 100 %) and loop_mismatch (> 20 per 1000 presses after 50 events). All thresholds are Kconfig (Protective Stop: lifetime health). HTTP: GET /api/health (unauthenticated, like /state.json); POST /api/health/reset?what=button|all&confirm=1 (admin) for a switch replacement or a refurbished unit. Dashboard gets a health chip. max_user_uri_handlers 30 -> 36. Pure logic (codec, edge detectors, thresholds, flush policy) is in dcs_health_logic.c with a host test in firmware/test (41 checks). Docs: API.md, MONITORING.md (glossary + "retire when"), TROUBLESHOOTING, hardware/README BOM row, QUICKSTART verify line.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
HIL bench results (PSTOP06 /
|
| Step | Expected | Observed |
|---|---|---|
| First boot after OTA push | boots 1, flashes 1, otas 1, health 0 in /state.json |
as expected; fw_sha matches /state.json |
| 3 press/release cycles (relays 1+2 off, then on) | presses +3 |
presses 3 |
| Same, mismatch side | skewed relays (~270 ms between loops) show as one-loop-open on press and release | mismatch_events 6 (2 per cycle). The comparator's pstop_mismatch stayed 0 (no bonded machine, nothing to compare), so this counter is the more sensitive, machine-independent signal |
| Dirty flush | within 60 s | nvs_flushes 3, last_flush_age_s 0 |
| Hard power cut (relay 3 off 4 s) | all counters persist, boots +1, uptime loss < flush interval |
presses 3, mismatch 6, boots 2, uptime 121 → 140 (≈19 s of real time, 0 lost from base) |
| Single-loop fault (relay 1 off only, 2 s) | mismatch +1, no press |
mismatch 7, presses 3; l0=146 l1=85 during |
POST /api/health/reset?what=button&confirm=1 (admin) |
zero presses/mismatch, button_swaps 1, immediate flush |
as expected |
Reset without confirm, bad what, no auth |
400 / 400 / 401 | as expected |
Unflushed press → POST /admin/api/restart |
shutdown-handler flush saves the delta | presses 1, mismatch 2 survived, boots 3 |
Throwaway build RATED_OPS=3, MISMATCH_WARN_MIN=2 (direct OTA, not registry) |
after 3 presses: level 2, button_wear CRITICAL, /state.json health 2; loop_mismatch quiet (6 < 20 allowed) |
{"src":"button_wear","level":2,"msg":"100% of rated 3 ops; replace switch"}, health: 2; no loop_mismatch |
| Dashboard | health chip present in / |
present, wired to j.health |
| Push registry image back | flashes 3, otas 3, warning clears (3/100000 = 0 %) |
as expected, level 0 |
Finished with reset?what=all so PSTOP06's history starts clean on the real image (boots 1, flashes 0, presses 0); relays left bench-normal; fleet console shows v1.2-17-g53c371c online.
Not exercised: nvs_flush_fails path, dropped (table full), and a real cable flash (would show flashes +1, otas +0).
Follow-up bench pass: power relay + software reset
Still not exercisable from here: Branch now includes #114's second commit (merge, no rewrite) so |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether dcs_health_reset_button()/reset_all() zeroing the press/mismatch delta atomics with atomic_store (line 365-366, 388-389) instead of atomic_exchange could lose a concurrent press from dcs_health_note_core_tick()'s lock-free atomic_fetch_add — it can drop at most one lifetime-counter increment during a reset window, not a safety-relevant race, so I did not flag it as a separate finding.
Extended reasoning...
This run reported three new confirmed findings (unauthenticated-heap allocation choice in dcs_admin_pages.c, the reset-vs-flush-failure inconsistency in dcs_health.c, and the Kconfig min/max flush-interval ordering issue) that are already being posted as inline comments, so no additional top-level restatement of those is needed. I additionally traced the atomic_store usage in dcs_health_reset_button()/reset_all() against the lock-free atomic_fetch_add in dcs_health_note_core_tick() (confirmed at dcs_health.c:236/240 vs 365-366/388-389) to check for a lost-update race during a reset; it is real but bounded to losing at most a single wear-counter increment, not a safety-path or data-corruption issue, so it doesn't rise to the level of a new inline finding. Given three confirmed findings already surfaced this run plus four unresolved inline threads from my prior review (dcs_health.c:322, :133, :479, :192) with no visible code changes addressing them in this diff, a human should still look at this PR before merge — my note here is only to record the extra race I checked and ruled out as low-severity, not to suggest anything is resolved.
# Conflicts: # docs/QUICKSTART.md
…PSRAM buffers - flush_locked: clear the dirty flag BEFORE taking the deltas. An edge from the lock-free core-tick hook that landed between exchange and clear lost its <=FLUSH_MIN_S guarantee and slid to the FLUSH_MAX_S boundary. - reset_all: realign s_uptime_in_base_s so uptime_s is actually 0 after the reset (it came back as the seconds since the previous flush). - reset_button/reset_all: mutate a copy; restore RAM if the NVS write fails so /api/health never shows a reset the handler reported as 500. - health_task: take s_lock around the periodic snapshot; the admin resets rewrite s_base wholesale and a torn read could publish a bogus level. - init: an OTA rollback (last-invalid partition carries the SHA on record) changes the running image without anyone flashing; do not count it as a flash or an OTA, so flashes - otas stays an honest cable-flash count. - dcs_health_flush_due: MIN > MAX Kconfig misconfiguration no longer lets the MAX rule defeat the MIN throttle (MAX stretches to MIN). Host test. - /api/health: PSRAM-first buffers like page_state; the route is unauthenticated and the internal heap is tight. Bench (PSTOP06): OTA -> flashes/otas +1; reset all -> uptime_s 0, reads 3 s later; press -> dirty flush within 60 s. Host tests 44/44, both roles build.
|
All seven review items addressed in
Rollback path itself is not bench-testable from here (no route boots the other slot), so that one is reasoned from the |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked dcs_health_get_snapshot()'s best-effort lock (dcs_health.c:361-384): when the 100ms xSemaphoreTake times out it still calls snapshot_counters() unconditionally, so a concurrent admin reset can produce a torn read on the unauthenticated GET /api/health path. That's the same best-effort-consistency tradeoff already accepted elsewhere in this file (e.g. the documented "momentarily inconsistent but monotone" comment), not a new correctness issue, so I'm not flagging it separately.
Extended reasoning...
This run's new commit (8b49dd1, "fix(health): address review — reset/flush races, rollback mis-count, PSRAM buffers") addresses the previously-raised concern that dcs_health_reset_button()/reset_all() left s_base mutated in RAM even when the NVS flush failed: the code now snapshots s_base into saved and restores it (and s_uptime_in_base_s) on a non-OK flush result, matching what was requested. One remaining, narrower issue was found in the same reset paths — the pending press/mismatch delta atomics are zeroed before the flush attempt and are not restored alongside s_base if the flush fails — and that is being posted as the sole new inline finding. I confirmed the get_snapshot lock-timeout behavior described above by reading the current code and consider it an acceptable, pre-existing pattern rather than a new bug worth a separate comment.
The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.
| char msg[DCS_HEALTH_MSG_LEN]; | ||
| (void)snprintf( | ||
| msg, | ||
| sizeof(msg), | ||
| "%lu loop mismatches over %lu presses; check switch wiring", | ||
| (unsigned long)c->mismatch_events, | ||
| (unsigned long)c->presses); |
There was a problem hiding this comment.
🟡 (optional) check_loop_mismatch()'s snprintf into msg[DCS_HEALTH_MSG_LEN=64] can silently truncate: the literal template "%lu loop mismatches over %lu presses; check switch wiring" is 51 chars before digits, so once mismatch_events+presses digit counts exceed 12 (e.g. presses in the tens of millions with mismatch_events six digits — exactly a long-neglected, never-retired unit this feature targets, since the check reruns every 60s with the live presses total) the trailing advice ("check switch wiring") gets cut off with no error, degrading the exact operator warning the feature exists to deliver. Fix: size DCS_HEALTH_MSG_LEN (or this message's format) for the worst-case digit width of both %lu fields, not just the shorter button_wear message.
Extended reasoning...
dcs_health.h:50 defines DCS_HEALTH_MSG_LEN=64 (63 usable bytes + NUL); it was sized to fit check_button_wear's message (max ~44 chars with a 10-digit rated_ops) but not checked against check_loop_mismatch's format. check_loop_mismatch() (dcs_health.c:481-504) builds "%lu loop mismatches over %lu presses; check switch wiring" — literal characters alone total 51 (' loop mismatches over '=22 + ' presses; check switch wiring'=29), so full length = 51 + digits(mismatch_events) + digits(presses). health_task calls this every 60s with the live c->presses/c->mismatch_events (dcs_health.c:529-538), so on a unit whose presses has grown into 8 digits (tens of millions) while mismatch_events is in the 5-6 digit range — plausible for a long-running or automated e-stop cycling rig that nobody retired — combined digits exceed 12, snprintf(msg, 64, ...) truncates, and dcs_health_publish() stores/serves the truncated string via /api/health warnings[].msg with no indication anything was cut.
Verification: nit. The truncation is real but purely cosmetic (safe snprintf, no overflow/crash/data loss). At dcs_health.c:496-501 the message is built with snprintf into char msg[DCS_HEALTH_MSG_LEN] where DCS_HEALTH_MSG_LEN=64 (dcs_health.h:50). The literal portion of "%lu loop mismatches over %lu presses; check switch wiring" is exactly 51 chars (" loop mismatches over "=22 + " presses; check switch…
1 participant
Stacked on #114 (base =
docs/quickstart); retarget tomainonce that merges.Why
Know when a remote should be retired for wear. The stop switch on the BOM (NKK FF0126BBCAEA01) is rated 100,000 operations (mechanical and electrical life, FF01 datasheet). Nothing on the chip counted presses, and nothing persisted across power cycles.
What
Counters (NVS blob
dcs_app/health, survive reboot /idf.py flash/ OTA; lost onerase-flashor the boot-time NVS auto-erase):pressesmismatch_eventsuptime_sesp_timer; no timekeeping libs; loses <= the last flush interval on power pullbootsboot_count, never auto-cleared)flashes/otasPENDING_VERIFY. Cable flashes =flashes - otasbutton_swapsWarning board —
dcs_health_publish(src, level, msg)from any task; one entry per source with count/age;/api/healthlists them,/state.jsongets"health": 0|1|2. First publishers:button_wear: WARN atCONFIG_DCS_BUTTON_WARN_PCT(80 %) ofCONFIG_DCS_BUTTON_RATED_OPS(100000), CRITICAL at 100 %.loop_mismatch: WARN when mismatches exceed 20 per 1000 presses after 50 lifetime events.HTTP:
GET /api/health(unauthenticated, like/state.json);POST /api/health/reset?what=button|all&confirm=1(admin). Dashboard health chip.max_user_uri_handlers30 -> 36.Safety-path impact: none
Three lines added to the existing
dcs_publish_core_tick()telemetry sink.main.c,estop_verdict.candpstop_care untouched. The hook is a short spinlock critical section: no logging, allocation or NVS. The flush task uses an internal stack (NVS writes forbidden from PSRAM stacks perdcs_internal.h), prio 1, flushes when dirty >= 60 s or every 600 s, plus best-effort onesp_restart().Edge detector baselines only on the first aligned (same-tick, both cores) observation, so the boot-priming STOP the cores emit for their first ticks, or a button latched at power-on, is never a phantom press. The host test models that boot sequence explicitly.
Remote only: on machn the per-core "verdict" is robot state, so press/mismatch fields are compiled out; uptime/boots/flashes still apply.
Verification
idf.py buildremote + machn: clean, no new warnings.firmware/test:test_dcs_health_logic41 checks (codec, edge detectors incl. boot priming and tick skew, thresholds incl. u32 overflow, flush policy).tools/misra_check.sh: new-file findings all within the existing deviation register (15.5 / 8.4 / 8.9 / 21.6).pre-commit run --all-filespasses.Bench (to do on hardware): press 3x ->
presses+3 within 60 s;POST /admin/api/restart->boots+1,uptime_smonotone; OTA ->flashes+1,otas+1; setCONFIG_DCS_BUTTON_WARN_PCT=0... actually setDCS_BUTTON_RATED_OPS=3for a warning after one press.Docs
API.md(endpoint + field table),MONITORING.md(glossary, traps, "retire when"),TROUBLESHOOTING.md,hardware/README.mdBOM row,QUICKSTART.mdverify line. Note the existingSAFETY_CHAIN.md:116("no automatic factory reset") disagrees withdcs_support.c:374, which auto-erases NVS onNO_FREE_PAGES; documented as a data-loss caveat, not changed here.