Skip to content

Audit: 10 bug fixes, 5 deletions, CI action pinning - #71

Merged
AndrewDemsDS merged 17 commits into
mainfrom
audit/bugs-simplify-ci
Jul 21, 2026
Merged

Audit: 10 bug fixes, 5 deletions, CI action pinning#71
AndrewDemsDS merged 17 commits into
mainfrom
audit/bugs-simplify-ci

Conversation

@AndrewDemsDS

Copy link
Copy Markdown
Owner

Summary

Full four-concern audit of the repo (bugs / simplification / performance / CI-CD), executed one item per commit. Correctness first, then deletions, then CI. All host gates green after every step (host codec/map tests, .zap contiguity, esp32-lint, shellcheck, ruff).

Bug fixes (10)

  • hisense_rs485.h + matter_drivers.cpp + app_main.cpp: shadow setpoint sync wedged in Fahrenheit. The downlink re-sync range-checked the always-Celsius st.setpoint_c against the shadow's previous unit. Once the shadow held F, no Celsius value ever passed the 61-90 F window, so the shadow froze at its first F value and every later command silently re-imposed the stale setpoint (plus a 1 Hz error log). New pure helper hisense_shadow_setpoint_from_status() validates in the wire unit; the ESP32 side moves to the same helper and no longer flips the unit while keeping a stale value on an out-of-range report. Golden host tests pin the F-mode sequence and out-of-range refusal. Ships ameba v1.3.6 + esp32 v1.1.2.
  • ESP32: break-glass listener now starts in BOTH flavours. breakglass_start() sat inside CONFIG_HISENSE_DEBUG_BUILD, so release images never opened :2324 and never logged the fail-closed warning: the exact gap the feature's own contract rules out.
  • ESP32: fault endpoint is created LAST. esp-matter assigns endpoint IDs in creation order, and fault was created before coil/display: the node actually ran fault=8, coil=9, display=10 while every comment, the docs, and hil_display_actuation.py (DISPLAY_EP=9) assumed 8=coil / 9=display / 10=fault. Action needed after the next ESP32 OTA: re-interview node 28 and drop the three orphaned HA entities (coil/display/fault remap to the corrected IDs).
  • ota-release.sh: build forwards all flags. build "${1:-}" dropped everything after the first, so build --bump --debug silently built the release flavour: the same silent-flavour-drop class the script hard-dies on elsewhere.
  • Driver fault map: comments now teach the confirmed base-15 (wire bytes 39/40/64/66), not the falsified base-13 the struct comments and two derivation blurbs still described.
  • Driver: fault length gates were one byte too strict (len > B+1 to read buf[B]); a frame of exactly B+1 silently lost that group's coverage. Boundary tests added.
  • AmebaZ2: no stale pre-loss status on link restore. The link-lost branch left s_status_fresh set, so the restore event could republish pre-loss values for a beat.
  • Docs-site assembler: meta description built from the real line position. lines.index(raw) used the first occurrence of a duplicated line (fenced code blocks were the realistic trigger); also drops the O(n^2) scan. Assembled site verified byte-identical on the current docs.
  • ESP32 brownout comment now matches the deliberate level-0 setting (the block argued for level 4 against its own values; app_main.cpp documents the evidence-gathering decision).
  • ota-release.sh: deterministic-clock shim built fresh per run into a private mktemp dir instead of reusing the predictable /tmp/ota-det-time-<uid>.so, which would LD_PRELOAD whatever sat at that known path into the packaging make.

Simplifications (5)

  • Delete sdk-edits/core-patches/ (byte-for-byte duplicate of a diff already in patches/connectedhomeip.patch, which setup.sh applies).
  • Delete never-called hisense_build_status_request() + hisense_deinit() (zero callers in repo, both ESP32 targets, and the SDK tree).
  • Delete orphaned esp32-matter/smoketest/main/smoketest.cpp (not in CMake; duplicate app_main).
  • Delete superseded ota_go.py (also internally stale: printed "v2", sent version 12).
  • Drop the HTTPS_OTA_URL alias; comment header now counts endpoints 1..10.

Deliberately left alone: the EPM delegate's interface surface (harmonic reads + never-called setters are Delegate overrides / upstream-derived shim; removable mass is small and verification needs a full SDK build), the recon/diag console duplication (two build systems, consoles evolve separately), bench one-shots (mcli/mnodes/drive_ac/matter_scan, flasher utilities), test-only helpers (covered by tests).

Performance

Baselines: host suite 0.75 s, lint 0.78 s, QA job ~15 s, docs site ~60 s, AmebaZ2 tag build ~3.5 min, ESP32 tag build ~10.5 min. No code changes recommended: the QA gate is already fast and correctly ordered, and the full-clean firmware builds are slow by design. One hypothesis for the operator to trial on the runner (not a repo change): IDF_CCACHE_ENABLE=1 for the ESP32 tag builds.

CI/CD (1)

  • SHA-pin every action in pages.yaml (was all floating major tags) and checkout in the Gitea build, to the newest release of the major each already used. Matches the repo's stated policy and the other three workflows; Dependabot keeps the pins current. Verify on the next pages run: same ~60 s job, pinned refs in the log.

Test plan

  • firmware/test/run_tests.sh (375 codec/map checks incl. new shadow-sync + fault-boundary tests)
  • ota-release.sh lint (host tests + .zap contiguity + version monotonicity)
  • esp32-lint.sh (PROJECT_VER 1.1.2 <-> sdkconfig 10102)
  • shellcheck / ruff
  • Docs-site output byte-identical to main after the assembler fix
  • Gitea sdk-builder full AmebaZ2 build (runs on push of this branch's mirror)
  • Post-merge: next AmebaZ2 + ESP32 OTA cycles exercise the shipped fixes; re-interview node 28 for the endpoint remap

…a v1.3.6 + esp32 v1.1.2

The downlink shadow re-sync on the AmebaZ2 range-checked the always-Celsius
st.setpoint_c against the shadow's PREVIOUS unit. Once the shadow held
Fahrenheit, no Celsius value could pass the 61..90 F window, so every poll
failed the check: the shadow froze at its first F value, the log filled at
1 Hz, and each later command silently re-imposed the stale setpoint.

New pure helper hisense_shadow_setpoint_from_status() converts to the wire
unit and validates against it. The AmebaZ2 downlink uses it; the ESP32
shadow sync moves to the same helper, which also stops it flipping the
shadow unit while keeping a stale Celsius value on an out-of-range report
(both fields now keep their last good values together).

Host tests pin the F-mode sequence (first F sync + follow-up F sync), the
full C range in both units, and out-of-range refusal leaving the shadow
untouched.

Assisted-by: AI
breakglass_start() sat inside #ifdef CONFIG_HISENSE_DEBUG_BUILD, so a
release image never opened the :2324 OTA trigger and never logged the
fail-closed warning -- the exact gap the function's own contract rules
out ('NOT a diag-console command: that console is debug-flavour only,
so a trigger there would be missing from precisely the images most
likely to need it'). The listener is authenticated and fails closed, so
starting it unconditionally costs nothing when no token is configured.

Assisted-by: AI
…the .zap

esp-matter assigns endpoint IDs in creation order (min_unused_endpoint_id++),
and fault was created BEFORE coil and display: the live node actually ran
fault=8, coil=9, display=10 while every comment, the header map, the docs
(coil ep8 / display ep9 / fault ep10, 1:1 with the AmebaZ2 .zap) and
hil_display_actuation.py's DISPLAY_EP=9 assumed the documented order. On the
ESP32 node the HIL display test therefore addressed the coil endpoint and
could never pass. The boot log also omitted s_ep_fault, masking the swap.

Reorder so creation is coil -> display -> fault (8/9/10 as documented) and
add fault to the boot log. Behaviour change on next OTA: node 28's coil,
display and fault entities remap to the corrected endpoint IDs -- re-interview
the node and drop the three orphaned entities in HA.

Assisted-by: AI
…lently dropped

'build) build "${1:-}"' forwarded one argument, so
'ota-release.sh build --bump --debug' bumped but built the RELEASE
flavour: the silent-flavour-drop class the script itself hard-dies on
in the release path. Usage always advertised both flags. "$@" expands
to nothing when no flags are given, so bare 'build' is unchanged.

Assisted-by: AI
The struct field comments and both derivation blurbs still said the wire
byte was payload+13 (frame[37]/[38]/[62]/[64]) while the defines they
describe settled on base 15 (39/40/64/66), confirmed by five independent
hardware-verified bit agreements (hisense_rs485.h:591). A reader trusting
the comments would probe the wrong bytes -- in a fault table, the worst
place for that. Comments only; no code change.

Assisted-by: AI
Reading buf[B] only needs len > B, but every gate demanded len > B+1, so
a frame of exactly B+1 silently lost that fault group's coverage. Never
an over-read, just a quiet blind spot in a diagnostics feature. Boundary
tests pin both sides of each group's cutoff (decodes at byte+1, does not
decode when the frame ends on the byte).

Assisted-by: AI
The link-lost branch nulled the liveness attributes but left
s_status_fresh alone. Sequence: a frame lands (fresh=true) -> the bus
goes silent before the downlink drains -> the link-lost event and the
queued status event both take the nulling path, fresh still set -> on
restore the link-up event consumes the flag and republishes the STALE
pre-loss values for a beat until the next poll. Clear the flag in the
link-lost branch so attributes stay null until fresh data arrives.

Assisted-by: AI
… not the first match

first_heading_and_para continued the paragraph from lines.index(raw),
the FIRST occurrence of the line's content. A first-prose line that
also appears earlier (typically inside a fenced code block) made the
description absorb the earlier block's next line. Iterate with
enumerate and continue from the real position; also drops the O(n^2)
scan. Verified: the assembled site is byte-identical on the current
docs, and the fence-duplicate case now yields the right description.

Assisted-by: AI
The block contradicted itself: the header said 'REVERTED to level 0',
the body argued level 0 is the worst setting and 'Level 4 rather than
the maximum on purpose', and the values are level 0. app_main.cpp
documents the actual decision: level 0 is kept on purpose while the
reset-reason logging collects evidence. Rewrite the comment to say so.
Config values unchanged.

Assisted-by: AI
The shim lived at the predictable /tmp/ota-det-time-<uid>.so and was
reused whenever it was newer than the script: a stale or foreign file
at that known path would be LD_PRELOADed into the packaging make. It is
a ~100 ms gcc call, so build it into a private mktemp dir every run and
remove the dir after the packaging make. Verified: shim builds, preloads
cleanly, no dir left behind.

Assisted-by: AI
core-patches/fan-control-fanmode-lowmedhigh.patch is byte-for-byte the
first diff of patches/connectedhomeip.patch, which scripts/setup.sh
applies. Unreferenced by any script, doc or CI; two captures of one fix
is how they drift.

Assisted-by: AI
…hisense_deinit

Zero callers in the repo, in both ESP32 targets, or anywhere in the SDK
tree (the bus task sends the HISENSE_STATUS_REQUEST template directly,
and the firmware never deinitialises). Dead public API is a liability in
a shared driver header: someone eventually calls it expecting it to be
the live path. Host suite still green.

Assisted-by: AI
smoketest/main/CMakeLists.txt registers ONLY busmon.cpp; smoketest.cpp
defines a second app_main and is in no build. It is the pre-busmon
iteration of the same on-target check, kept compiling against nothing.

Assisted-by: AI
ota-release.sh flash covers its job (update_node with retries + on-device
verification), and the script is internally stale: it prints 'update_node
-> v2' while sending software_version 12. Unreferenced by CI, hooks and
docs.

Assisted-by: AI
…rectly

One macro aliasing another at the same scope only made the OTA URL
look like a second configuration knob.

Assisted-by: AI
Said 1..8 despite ten endpoints; left over from before the display and
fault endpoints existed.

Assisted-by: AI
pages.yaml floated every action on major tags (checkout@v4,
configure-pages@v5, jekyll-build-pages@v1, upload-pages-artifact@v3,
deploy-pages@v4) and the Gitea build used checkout@v4, while qa.yaml
and both release workflows pin SHAs -- the repo's stated supply-chain
policy (dependabot.yml). Pin each to the newest release of the major it
already used (checkout gets the same v7.0.0 the other workflows pin),
so this is a no-op in behaviour. Dependabot keeps the pins current.
Verify on the next pages run: same ~60 s job, pinned refs in the log.

Assisted-by: AI
@AndrewDemsDS
AndrewDemsDS merged commit 5afc628 into main Jul 21, 2026
7 checks passed
@AndrewDemsDS
AndrewDemsDS deleted the audit/bugs-simplify-ci branch July 21, 2026 07:52
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.

1 participant