Repository navigation
Conversation
…cut off
Two defects in one table, each of which the tasklist suspected and neither of
which anything checked.
**31 German causes contained characters the font cannot draw.** The 5x7 font
in kiln_hmi carries the 95 printable ASCII glyphs and nothing else. The
labels had always been transliterated -- VERSCHWEISST, KEINE WAERME -- and the
long causes had not: 68 non-ASCII characters, every one reaching the panel as
a blank. A German operator read a fault screen with holes in the instruction.
All of them are transliterated now, and a test holds the whole table to
0x20-0x7E so the next umlaut fails the build.
**27 causes were longer than the screen can show.** They run to 279
characters against five rows of 21, and what falls off the bottom of a 128x64
panel is the end of the sentence. In an instruction the end of the sentence is
the instruction: for KILN_FAULT_CONTACTOR_WELDED the dropped part was
"ISOLATE THE KILN AT ITS SUPPLY NOW", and for the supervisor's reasons it was
"press the supervisor's clear button on the panel" -- the one piece of
information distinguishing the two acknowledgements this system has.
Shortening the causes would have been the wrong fix: the same strings are what
/api/status sends, and a browser renders a paragraph well. So each entry may
carry a panel form as well -- same meaning, action first, trimmed to what the
screen holds -- and carries none where the full cause already fits, so there
is no second string to drift from the first. kiln_fault_panel_in and its two
siblings fall back to the cause, so the display asks for one thing and always
gets something it can render completely.
Two gates, both written to name the offender rather than merely fail, because
a bound that does not say which string is a bound nobody can fix:
- every panel form fits five rows under the screen's own word wrap;
- no word is longer than a row. That is a real constraint in German --
"Thermoelement-Messeingang" is 25 characters -- and draw.cpp's own comment
says why cutting one is unacceptable: a word split in a hurry reads as a
different word.
Writing the short forms turned up a misassignment worth noting: the text about
current disagreeing with the commanded state belongs to KILN_WARN_RELAY_SUSPECT,
not to KILN_WARN_CURRENT_DEV, which is about drift from the run's reference.
Two different warnings with two different remedies.
26/26 host suites, the same under ASan/UBSan, clang-tidy 20 clean.
G4. The safety cycle has read the interlock pin every 100 ms since
SWR-SAF-31 was implemented and shown it nowhere, so an operator standing in
front of an idle kiln could see that HEAT was absent and had to guess why.
It is published in the snapshot, taken from the input the rule actually used
rather than re-read from the port: a second sample of a safety input at a
different instant is how two parts of one device come to disagree about
whether the door is open.
Three states and not two, in all three places. "open" and "shut" both
presuppose a switch, and a kiln with no interlock fitted must not read as one
whose door is shut -- that is precisely the case warning 113 exists for and
the one wrong answer available here. So:
/api/status carries `door` as a string, not a boolean, so no client can
accidentally treat an unmonitored door as a safe one.
the panel spells out all three on the diagnostics screen, and puts DOOR
in the slot that would say HEAT. The two cannot both be true,
so the badge that says why the kiln is hot says why it is not.
the browser raises a pill only for "open". "unmonitored" is a standing
condition with warning 113 already in the banner, and
repeating it on every idle kiln without an interlock would
train the operator to ignore the row.
Making room on the diagnostics screen cost the power and energy row labels,
which now share one row with the units doing the naming -- kW is a rate and
kWh is a total, a distinction a kiln owner makes before a firmware engineer
does. The two strings left the table rather than staying in it unused.
Four tests, including that an unmonitored door never renders as a shut one on
the panel, and that the API reports all three states as the simulator's
switch is opened, closed and removed.
Also closed, both already true: L6, the hardware build has been in CI since
its first build turned up two -Wformat-truncation errors; and N1, whose live
weakness was an access point that would start without a passphrase -- the
access point is gone, so the surface is deleted rather than regulated, which
is the only kind of fix that cannot be misconfigured back. N4's example of a
client joining that access point is reworded with it.
26/26 host suites, the same under ASan/UBSan, clang-tidy 20 clean on host and
target, hardware image builds, StrictDoc exports clean.
C5 and C13. Three architecture decisions held by discipline alone, and the grep standing in for the first of them said so in its own comment. **SWA-14, the core includes no platform header.** A grep over include lines misses the only two ways this has a real chance of going wrong: a platform header reached indirectly through a project header, and one inside `#if defined(ESP_PLATFORM)`, which the host build never compiles -- so the core could stop being portable while every gate stayed green. The check now reads the preprocessor's own record of every header it opened, from the target build where those headers are on the path and a mistake would actually resolve. Ninja keeps that in a binary log rather than in .d files, so it comes from `ninja -t deps`: 202 headers on the current build, none of them an IDF component the core may not reach. The direct grep stays as well, because that is what a reviewer means by the question. **SWA-03, no mutable state in the core, and SWA-02, time is injected**, are checked from the object code, which is stronger than any grep over sources. A mutable file-scope object lands in .data or .bss, so nm finds it whatever it is called and wherever it was declared -- including a function-local static, which is the form that gets past a reviewer looking for globals. A direct clock read leaves an undefined symbol for time, clock_gettime, esp_timer_get_time and the rest. Both hold today: zero writable symbols and zero clock references in libkiln_core.a. What those two buy is the 168-hour firing a test exercises in milliseconds and the 15-minute runaway timer it does the same to. Both would have broken silently: the tests keep passing, they just stop testing what they claim to. **SWA-18, the 20-byte log record**, gains static_asserts beside the encoder: the size, the CRC as its last byte, and architecture 10.4's 204 records per sector following from it. It guards a persisted format, so changing the number without changing the sector header's format_version would make every log already written decode as plausible nonsense -- plausible because the CRC would be recomputed over whatever the new layout produced. The component graph is checked too, from each component's REQUIRES, for layering against architecture section 4 and for cycles: seven components, layered and acyclic. The script carries a --self-test that builds a deliberate global, a function-local static and a time() call and proves each check sees them, because a gate nobody has seen fail is a gate nobody should trust. It also says which checks it skipped rather than reporting a pass it did not make, so CI runs the object-code half in the layering job and the include half in the hardware-build job, where each has what it needs. 26/26 host suites, layercheck clean, 4/4 self-test checks.
C7. Decodes a kilnlog partition dump to CSV, walking the ring in sequence order rather than address order so a wrap does not interleave two firings, and reads what esptool read_flash produces -- which needs no cooperation from a device that will not boot, this being exactly when a log matters. It would have been less code to link kiln_core and call kiln_logrec_decode. That is precisely what it must not do. A log is the only evidence of what a kiln did before it failed, and the question an investigator has is "did the firmware record this correctly" -- which the firmware's own decoder cannot answer, because a codec that encodes and decodes with the same wrong idea round-trips perfectly and tells you nothing. So the decoder is written from the format as logrec.h and architecture 10.2 document it: offsets, scales, CRC-8 poly 0x07 and CRC-16/CCITT-FALSE, the state nibble and the flags above it. The honest cost is stated in its header: the format now lives in two places and the two can drift. Which is what the fixture is for. firmware/controller/test/host/fixtures/ logring.bin is two log sectors the C++ encoder produced, committed, with three records chosen for what a decoder gets wrong -- a negative temperature, the segment sentinel, every flag at once, a non-sample event, each field at the edge of its scale -- and the sectors deliberately out of address order so that anything reading the ring by address is caught. Both implementations are pinned to it. test_logrec re-encodes the same samples and compares bytes, reporting the first differing offset rather than "the images differ", because an offset sends the reader to a field and a hex dump sends them nowhere. logdump's self-test decodes the same file and checks every field of all three records. The fixture's README says what to do when it fails: ask which side moved, and if the format changed deliberately then KILN_LOG_FORMAT_VERSION changes with it so logs already on devices stay readable. 55 self-test checks, run in CI beside the other tool self-tests. 26/26 host suites, clang-tidy 20 clean.
… than NVS
SWR-CFG-09 and SEC-14, from tasklist N4. The owner can now erase everything
from the device itself: the configuration and the WiFi credentials inside it,
the latched fault, every run record, the sample log and the wear counters. It
restarts the run numbering, because a kiln whose first firing is number 94
tells its new owner something false about its history, and it leaves the
production data block and the firmware alone -- those identify the unit for
its whole life and are not the owner's data.
Last entry of the display's menu, behind the confirmation an abort uses and
defaulting to "no", because the knob's first position must not be the
destructive one. Refused while running, paused or autotuning. It reports
counts rather than only success and does as much as it can rather than
stopping at the first failure: a reset that gave up half way would leave an
owner with some of their data erased and no idea which.
The implementation overwrites the credential with zeros and commits BEFORE
erasing the key, because on NVS an erase delists an entry and leaves its bytes
in the page until a garbage collection that may never come -- and "the API
says it is not configured" is not the question somebody selling a kiln is
asking.
Which is the part worth reading. The requirement says the test must read the
raw store back and look for the secret rather than asking the API, and it was
right to be that specific: the first version of this test passed with the
overwrite deleted. Twice, for two different reasons, both of them the host
key/value fake being kinder than the medium it stands in for:
- erase() memset the whole entry, so the bytes went whether or not anything
had overwritten them;
- set() then reused the delisted slot, so the next write destroyed the
evidence even after that was fixed.
The fake now models NVS: an erase delists the entry and keeps its bytes, a
write never lands on a delisted slot, and collection happens only when a test
asks for it. The flash fake has enforced NOR's refusal to un-clear a bit for
the same reason -- a fake that cannot reproduce the failure cannot be used to
prove it does not happen.
With the overwrite removed from kiln_app_factory_reset the test now fails with
"it was delisted rather than erased"; with it restored it passes. That
experiment is the evidence, and the test says so beside the assertion.
Three tests: the raw-bytes check including across a reboot, the history and
counters going with it, and the refusal mid-firing leaving a report that does
not read like a partial success.
26/26 host suites, the same under ASan/UBSan, clang-tidy 20 clean, hardware
image builds, StrictDoc exports clean.
…d to vanish SWR-LOG-16, the half that does not depend on the update path, plus a defect it exposed in the log ring. Three event codes are appended to the persisted log -- an abnormal reset, a factory reset, a network join commanded at the display -- and log.security_events is the owner's opt-out, default on, because the default has to be the one that helps an owner who has not thought about it. UR-REG-004 asks for that opt-out as explicitly as it asks for the log: a device that records access to itself regardless of its owner's wishes is a surveillance feature rather than a security one. The opt-out is applied in the one place every event passes through, so no emitter can forget it, and it removes only the security events. A fault, a state change and the periodic sample are the evidence of what a kiln did, which is a safety record rather than a record of access to the device, and no configuration item switches those off. Details worth knowing: a reset cause the adapter cannot identify counts as abnormal, because a reset nobody can account for is exactly what a log should carry and calling UNKNOWN normal would hide an adapter that is wrong. The factory reset logs itself last, so the record is the first entry of the new owner's log and explains why the rest is missing. The join records no SSID -- a log of which network a kiln joined is a log of where it lives. What is NOT here, and why: the update-path events this requirement lists need a version string and a refusal reason, and neither fits a 20-byte record (SWA-18). They arrive with the update path (H2) and will need a second record type or their own channel. Codes that could carry no detail would have looked like progress and recorded nothing. **The defect this found.** A record appended outside a run was lost. Until these events existed nothing ever appended outside one: every emitter ran inside a firing and begin_run had always claimed a sector first. A boot-time event went into a sector with no header -- which iterate() skips, because it selects runs from headers, and which the next begin_run() erases. Written, stored, counted, lost. The ring now claims a sector lazily, tagged run 0 for "not part of a run", which costs no extra erase: on a fresh ring it claims the sector begin_run would have claimed, and on a device with history it never fires. The bookkeeping was the part that bit. claim_sector writes a header and nothing else, so the first version left head_seq at 0; begin_run then issued the SAME sequence number to the run's sector, two sectors tied for highest, and since mount resolves order from the sequence number alone (architecture 10.3) the tie made an idle record the newest in the ring. SWA-09's power-loss journal then read the wrong record, and three recovery tests failed -- which is how I found it rather than shipping it. A test now covers the property directly, including the sequence numbers not tying, and logdump learned the three new names while asserting the old codes still mean what every existing log says they mean. Config schema 3: the opt-out is appended at the END of the struct, which is the case SWR-CFG-05's migration was designed for -- an older blob is shorter, the prefix copies, the new item takes its default. Schema 2's mid-struct removal needed a splice; this needs nothing. 26/26 host suites, the same under ASan/UBSan, 57 logdump checks, layercheck clean, clang-tidy 20 clean on host and target, hardware image builds, StrictDoc exports clean.
…tasks **A hardware branch at level 3.** SYS-HW-* says what the system needs: two switching devices in series, a buzzer, an interlock input. What the board has to achieve to deliver that lived in 22 task-list entries, which is a list of fixes rather than a specification: it said what was wrong today and not what right looks like. docs/03_hardware_req.sdoc is HWR-01 to HWR-23, each refining a SYS-HW-* or SYS-SAF-*, with the numbers and the verification method. The rule for what belongs there: a requirement is a hardware requirement when getting it wrong is a board change rather than a firmware one. The V-model's level table gains the branch, which was the classic V all along and merely implicit here. Content worth knowing it now carries: the charge pump's 1 s release with the two numbers that need establishing rather than assuming (a pumped level of about 2.7 V against a MOSFET characterised at 4.5 V, and a 155 ms decay that is a slide rather than an edge); the CT's full scale, anti-alias corner below Nyquist, open-input bias and clamp; separate SPI buses for the two front ends, because a supervisor whose chip select a crashed controller can drive is not independent of it. **The task list is a third of its length.** 1173 lines to 313, 10 979 words to 2 451. Every completed item is gone -- what was done and why is in the commit that did it, which is what the file's own header has always said -- and the remaining 49 are one to four lines each. All 49 identifiers are preserved, since they are cited from commit messages. Section A now points at the HWR requirements rather than repeating their numbers, so the board's specification has one home. The state table is re-measured and the stale rows are gone: it claimed the hardware configuration was not in CI and that the phase strap and extra CT inputs were missing against requirements that no longer exist.
My own gate failed CI on correct code, which is the worst way for a gate to behave: the next person deletes it. SWA-03 was read from nm's one-letter class, where 'd' means initialised writable data. A `const` table of POINTERS has to have its pointers relocated at load time, so on ELF it lands in .data.rel.ro -- and nm calls that 'd' too. On Mach-O such tables stay in __const and get 's', which is why this passed locally and flagged eight legitimate tables in kiln_core the moment it ran on Linux: k_items, k_examples, k_sup_reasons and the five name tables. Now read from the SECTION rather than the class. GNU nm's sysv format carries a Section column; Apple's leaves it empty, so the Mach-O branch asks `nm -m` instead. Both reject .data, .bss, their thread-local forms, a C `*COM*` common symbol, and a bare .data.rel -- and neither rejects .data.rel.ro, which is the whole point. The sections are named rather than pattern-matched, so one nobody anticipated is reported instead of silently allowed. The self-test gains the case that would have caught this: a const table of pointers must NOT be reported as mutable state. It also now calls writable_syms itself instead of a copy of the nm invocation -- duplicating the command under test is exactly why the first self-test could not see the false positive, which is a lesson worth more than the fix.
The licence-hygiene job checks every tracked .sdoc, and the new one carried no header: StrictDoc files cannot take a leading comment, so the other documents put theirs in the METADATA block and this one now does too. Its OPTIONS block is also brought into line with the rest. StrictDoc's grammar fixes the order of those keys, and REQUIREMENT_STYLE after NODE_IN_TOC is a parse error rather than an ignored field.
Contributor
Author
|
Landed on |
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
1 participant
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.
A review of the whole tasklist, implementing what could be implemented and closing what was already true. 57 open items before, 47 after, and three of the closures were defects rather than paperwork.
Defects found and fixed
The German fault screen was unreadable, and the instruction was cut off. 68 non-ASCII characters across 31 German cause strings, against a font that carries ASCII only — so the cause line had holes in it. And the causes run to 279 characters against a panel that holds five rows of 21, so 27 of them were truncated: for
KILN_FAULT_CONTACTOR_WELDEDthe dropped part was "ISOLATE THE KILN AT ITS SUPPLY NOW". Each entry can now carry a panel form as well, and two gates name the offender rather than merely failing.A log record appended outside a run was silently lost. Found while adding boot-time security events. It went into a sector with no header, which
iterate()skips and the nextbegin_run()erases: written, stored, counted, lost. The first fix then lefthead_seqat 0, which made two sectors tie for newest and madeSWA-09's power-loss journal read the wrong record — caught by three existing recovery tests.Two test fakes were kinder than the media they stand in for. The key/value fake cleared a whole entry on erase and reused the delisted slot on the next write, either of which made SWR-CFG-09's "the secret is gone" assertion pass with no implementation behind it. It now delists and keeps the bytes, as NVS does. With the overwrite removed from the implementation the test fails; with it restored it passes.
Implemented
/api/status, the OLED and the dashboard, as three states rather than two: "open" and "shut" both presuppose a switch, and a kiln with none fitted must not read as one whose door is shut.tools/layercheck.sh. The core's platform-freedom is checked from the preprocessor's own record (vianinja -t deps), not from include lines, so an indirect orESP_PLATFORM-guarded include is caught; no-mutable-state and no-clock-reads are checked from the object code, which also catches a function-localstatic. Plusstatic_asserts on the 20-byte log record, and a--self-testthat proves each check can fail.tools/logdump.py, with a decoder written from the format's documentation rather than fromkiln_core, and a committed wire-format fixture both implementations are pinned to. A codec that encodes and decodes with the same wrong idea round-trips perfectly and proves nothing.H2: a version string and a refusal reason do not fit a 20-byte record.Closed as already done
L6 (the hardware build has been in CI since its first build turned up two
-Wformat-truncationerrors), N1 (the access point whose missing passphrase was the live CRA weakness is gone, so the surface is deleted rather than regulated), J1/J2, M4.Verification
26/26 host suites, the same under ASan/UBSan,
kiln_coreline coverage 91.8 % against the 90 % floor, clang-tidy 20 clean on host and target, layercheck clean, 57 logdump self-test checks, both IDF configurations build (hardware image 54 % free), StrictDoc exports clean.