Cut verbosity across the firmware for less code, same behaviour #63
rob
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/cut-verbosity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A whole-codebase quality pass aimed only at less code for the same behaviour.
Wire format, hardware behaviour and the public shape the tests use are
unchanged;
pio test -e nativepasses at every commit, andpio runforboth node envs and
host_simstill build.Structural changes, one per commit:
sim::DimmerintoLevelEndpoint: the ramp/lockout/command logicnow lives once in
level_endpoint.cpp, unguarded, with only the LEDCoutput and pin setup behind
#if defined(ARDUINO).sim::Dimmeris now athin
ctrl::LevelEndpointsubclass. This forced a small follow-up fix tohost_sim's circuit table, sincesim::Dimmerandsim::Switchno longershare a base beyond
ctrl::Endpoint.Endpoint::onCommanddefaults toreturn false; delete the read-onlyoverrides it made redundant.
ctrl::UpdateStackfor the WiFi/update/boot object graph both nodesbuilt by hand, one
ctrl::printBusEvent, and theCTRL_BUILD_IDfallbackcentralised in
firmware_version.h.WifiManager::onFrame's validate-then-dispatch switches merged into one.detail::framehelper for the codec encode boilerplate inlib/can-protocol/src/codecs.h(header-only, no behaviour change; theshared test vectors confirm the bytes are unchanged).
memmovefor the two backends' pending TX queues;Node::notePeerwalksthe peer table once instead of three times.
Endpoint::failsafe(),TouchSensor::baseline(),TouchSensor::stdDev()) with no caller.Then a comment sweep, keeping only bare spec pointers, genuine hardware/protocol
traps and non-obvious constant reasons; narration, rationale essays and
restatements of CLAUDE.md or the spec are gone.
lib/: 237 insertions, 431 deletions across 40 files.src/: 68 insertions,370 deletions across 5 files.
Tested:
pio test -e native(436 cases) andpio runfornode_lighting,node_bathroomandhost_simafter every commit.Reviewed against the task list and CLAUDE.md invariants. All 436 native tests pass,
node_lighting,node_bathroomandhost_simall build.test/,vectors/,tools/vector_cases.h,docs/,README.md,CONTRIBUTING.mdand CI are untouched. Traced the wire-affecting bits by hand (LevelEndpoint/Dimmer collapse, codecs.hdetail::frame, the name-table array lookups against their enum declaration orders, thememmovequeue shifts,notePeer's single-pass rework,host_sim'sCircuitfunction-pointer table) and none change behaviour. Noponytail:comments, no scope creep, commit messages are subject-only with British spelling.One minor finding, not a correctness bug:
Centralising the two nodes'
onBusEventintoctrl::printBusEventdropped a genuine hardware-trap comment that was specific tonode_lighting: "A bus-off here means nothing this node sent reached the wire, so suspect the run from twaiTxPin to the transceiver, or the transceiver itself." That's exactly the kind of one-line hardware-trap comment the sweep says to keep, and it doesn't survive anywhere else (checkedcan_bus_twai.h/.cppand the newnode.h/update_stack.h). The equivalent MCP2515 fact ("this backend reports no bus-off yet") did survive, incan_bus.handcan_bus_mcp2515.cpp, so only the TWAI side lost its diagnostic note. Worth a one-liner back neartwaiTxPin/busConfiginsrc/node_lighting/main.cpp, sincectrl::printBusEventis necessarily generic and can't carry it.@ -40,3 +19,4 @@// (WCMCU-230): no SPI bus and no MCP2515 here.constexpr uint8_t twaiTxPin = 25;constexpr uint8_t twaiRxPin = 26;constexpr ctrl::CanBusConfig busConfig = {0, twaiTxPin, twaiRxPin};The old
onBusEventhere carried a TWAI-specific diagnostic hint on bus-off ("suspect the run from twaiTxPin to the transceiver, or the transceiver itself") that's now gone along with the function, sincectrl::printBusEventis shared and generic. That was a genuine hardware-trap comment (category b of the sweep), not narration, and it isn't preserved anywhere else. Worth putting back as a one-liner nearbusConfig/twaiTxPin.Re-reviewed at
70c25b6. The restored bus-off note in src/node_lighting/main.cpp checks out — correct content, sits next to twaiTxPin/busConfig as described. No other regressions found against the rest of the diff. pio test -e native (436 cases), pio run -e node_lighting -e node_bathroom, and pio run -e host_sim all pass. Clean to merge.@ -38,2 +22,2 @@return "?";}static const char* const names[] = {"off", "connecting", "connected", "ap-active", "updating", "failed", "update-failed"};return state < (sizeof(names) / sizeof(names[0])) ? names[state] : "?";I'm seeing that pattern quite a lot over a few files. Could it be abstracted?
Good catch — abstracted into one
template <size_t N> const char* nameOf(const char* const (&names)[N], size_t i)inlib/node-runtime/src/update_log.h, used bywifiStateName/wifiActionName,busEventName(node.h),twaiStateName(can_bus_twai.cpp) andclassName(host_sim/main.cpp). Also swept the remainingsizeof(x)/sizeof(x[0])in lib/ and src/ tostd::size. Pushed at6a4a578; tests and both node envs plus host_sim all still pass.@ -18,14 +19,20 @@namespace ctrl {// A name table indexed by an enum or a small integer; out of range reads "?".Remove the comment
@ -20,1 +21,4 @@// A name table indexed by an enum or a small integer; out of range reads "?".template <size_t N>inline const char* nameOf(const char* const (&names)[N], size_t i) {Pull this out in to its own file. We shouldn't be pulling in this file just for this method. Also, not a fan of single letter parameters
Re "Remove the comment" (#4045): done.
Re "Pull this out in to its own file... not a fan of single letter parameters" (#4046): moved
nameOfintolib/node-runtime/src/name_table.h(just<stddef.h>and the template);update_log.hincludes it forwifiStateName/wifiActionName, andnode.h,can_bus_twai.cppandhost_sim/main.cppnow includename_table.hdirectly instead ofupdate_log.h. Renamed the parameter toindex. Pushed atc8d12d8.