Repository navigation
test(mocks): a deterministic AES128 mock (note 0010, open finding 3) - #32
Merged
Merged
Conversation
… with host tests
What changes
- NodePrefs of the server roles (repeater, room server, sensor) moves verbatim
from helpers/CommonCLI.h into the new helpers/CommonNodePrefs.h, together
with the ADVERT_LOC_* and LOOP_DETECT_* constants. CommonCLI.h includes the
new header, so every existing #include <helpers/CommonCLI.h> keeps working
unchanged (roles, bridges, UI, downstream forks).
- New characterization tests for both NodePrefs classes (server roles and
companion radio). They pin down today's behaviour:
* the exact /prefs.json text written for a fully populated NodePrefs,
* loading that text restores every persisted field,
* every radio CLI get/set (CommonRadioPrefs::handleCommand): reply text,
the field it writes, the dirty flag, and the TX power / RX gain push to
the radio driver,
* FEM gain access through the "custom" key/value store used by boards.
The expected values were captured from the unchanged code.
- Native test env: also builds CommonRadioPrefs.cpp and TxtDataHelpers.cpp.
New test mocks: target.h (a recording stand-in for the global
radio_driver), ltoa() and constrain() in Arduino.h, and PrefsStreams.h
(stream helpers that were local to test_companion_node_prefs).
Why
- CommonCLI.h pulls in Mesh.h, IdentityStore.h, ClientACL.h and RegionMap.h,
so the server NodePrefs could not be compiled in the native env and had no
tests at all. The companion NodePrefs test only contained a disabled case.
- The next change merges the radio and GPS settings that are duplicated
between the two NodePrefs classes. These tests are the proof that it keeps
the on-flash format and the CLI behaviour byte for byte.
No functional change: the moved block is identical to the original
(diff of the extracted ranges is empty), and all native suites pass.
…companion NodePrefs What changes - New RadioPrefsFields (helpers/CommonRadioPrefs.h) and GPSPrefsFields (helpers/CommonGPSPrefs.h) hold the 17 radio and 3 GPS settings once, with their types, defaults and unit comments. Their fields are ordered by size so the two structs carry no internal padding. - Both NodePrefs classes (server roles: helpers/CommonNodePrefs.h, companion radio: examples/companion_radio/NodePrefs.h) inherit these two structs instead of declaring the same 20 fields each. Inheritance (rather than a member) keeps every existing access such as prefs.freq or _prefs.sf compiling unchanged in roles, UI, bridges, boards and downstream forks. - CommonRadioPrefs becomes the one concrete binding of RadioPrefsFields: it owns the "radio" JSON structure, the radio CLI commands and the key/value access boards use. The two private RadioPrefs classes that implemented its 34 pure virtual getters/setters by forwarding to the parent are removed. - New CommonGPSPrefs replaces the two identical private GPSPrefs classes. Why - The two NodePrefs classes carried verbatim copies of the radio and GPS settings and of their JSON/CLI adapters (the companion copy was marked "COPIED from CommonCLI (for now)"). Copies drift: in this release cycle the companion copy needed three separate fixes for defects the server copy did not have (6dad3d5: setSpreadFactor() did not assign; e3d0f9f and 5d82ed3: getters returned constants, setters were no-ops). - Adding one radio setting meant editing both copies plus the 34-method interface. Now it is one field and one def() line, shared by every role. Compatibility - /prefs.json is unchanged byte for byte: same keys, same order, same number formatting (proven by the characterization tests added in the previous commit, which pass unchanged before and after this change). - CLI replies and side effects are unchanged (same tests). - Legacy binary prefs loaders read field by field, so the changed in-memory layout of NodePrefs does not affect them; no code copies NodePrefs as a raw block. - Source compatible for all in-tree users. CommonRadioPrefs (introduced in this release cycle) is now a concrete class constructed from a RadioPrefsFields; code that subclassed it to implement the pure virtuals would have to drop that subclass.
Adds an "About this fork" box at the top of the README of the integration branch: what the fork does, where the refactoring series is documented, how the branches are organised (one refactor/... branch per step based on upstream dev, each prepared as a separate pull request against upstream; integration combines the finished steps) and where the status is tracked. The upstream README below the box is unchanged. This commit exists only on the integration branch, never on a step branch.
- docs/refactoring/README.md: what the series is for, the rules every step follows (characterization tests first, no behaviour change, full build matrix with size comparison, hardware upgrade and downgrade tests, one concern per pull request) and the status of each step. - docs/refactoring/architecture-review-v1.18.md: the measured findings the series is based on, each with file and line references or a git command to reproduce it, plus issues noticed while reading that are not part of the series. - docs/refactoring/0001-shared-radio-gps-prefs.md: design note for the shared radio and GPS settings: context, decision, rejected alternatives, compatibility and the verification evidence (host tests, mutation tests, build sizes for 18 environments, hardware run on two boards).
…ing steps nodetool.py addresses nRF52 boards by USB serial number and runs the upgrade/downgrade check of the design notes in docs/refactoring: text CLI for the server roles, CMD_RUN_CLI_COMMAND frames for the companion, self info, reboot and serial DFU through adafruit-nrfutil. scripts/ holds the get/set command lists used for step 1. Only on the integration branch; the step branches stay free of tooling.
…hooks What changes - New native test environment native_core that builds the real src/Dispatcher.cpp and src/Mesh.cpp for the host. The suite test_core_tuning drives a mesh::Mesh with nothing overridden against fakes for the radio, clock, RNG, packet manager and tables. - Link-time stand-ins for the four mesh::Identity / LocalIdentity members that Mesh.cpp references (constructors, verify, sign, calcSharedSecret). The real ones need the board crypto libraries; the tuning tests never call them, and a call would fail the test. - The tests pin what the core uses when no role overrides the tuning hooks: airtime budget factor 1.0, RX delay with base 10, retransmit delay from the raw packet length, no direct retransmit delay, no extra ACKs, interference threshold, CAD and AGC reset off. - native ignores test_core_tuning (it needs the extra sources), and the unit test workflow also runs native_core. Why - The next change lets the core read these values from the shared radio settings instead of a virtual getter per role. These tests prove that the core keeps its current defaults when no settings are attached, which is what every downstream user that does not opt in gets. No firmware code changes in this commit.
What changes - RadioPrefsFields moves from helpers/CommonRadioPrefs.h into the new core header src/RadioPrefsFields.h (no dependencies), so the core can use it. CommonRadioPrefs.h includes it; nothing else changes for its users. - Dispatcher gets useRadioPrefs(const RadioPrefsFields*). Once a node attaches its settings, the default implementations of the tuning hooks read them on every call: getAirtimeBudgetFactor(), calcRxDelay(), getInterferenceThreshold(), getCADEnabled(), getAGCResetInterval() in Dispatcher, getRetransmitDelay(), getDirectRetransmitDelay() and getExtraAckTransmitCount() in Mesh. Without settings they return exactly the built-in defaults they returned before. All hooks stay virtual. - Repeater, room server, sensor and companion call useRadioPrefs(&_prefs) in their constructor and drop the overrides that only forwarded a setting: 29 overrides that were identical in all roles, implemented in the core once instead. - The sensor keeps its two retransmit delay overrides (it draws from six discrete steps, the other roles from a continuous range) and now states explicitly that it sends no extra ACKs, which it never did: the CLI accepted multi.acks there but nothing read it. Changing either is a behaviour change and is left to a separate change. Why - Every role re-implemented the same eight one-line getters to hand a setting to the core, and a new core setting needed a new virtual plus an override in every role (CAD in 7c8e092 touched 15 files that way). Now a new tuning value is one field the core reads. Compatibility - Behaviour is unchanged for every role: the formulas moved verbatim from the role overrides into the core, and test_core_tuning checks that the core returns exactly what the v1.18.0 role overrides returned, that settings changed at run time apply immediately, and that the core keeps its old defaults when no settings are attached. - Downstream code that overrides any of these hooks keeps working; code that does not call useRadioPrefs() gets the old defaults.
…g values - docs/refactoring/0002-core-reads-radio-prefs.md: context (31 role overrides, 29 identical), decision, rejected alternatives (constructor argument, copying values on change, removing the hooks), compatibility and the verification: host tests on the real Dispatcher and Mesh, mutation tests, sizes for 18 environments against step 1 and dev, the pow/powf check on every object file, and the hardware run with a diagnostic build on a room server and a companion. - README.md: step 2 in the status table. - architecture-review-v1.18.md: which part of finding F1 step 2 addresses.
…ware run diag-tuning.patch adds a "diag tuning" CLI command to the room server and the companion that prints the values the running core uses for its tuning hooks. Applied identically to dev and to step 2, it showed the same core values for the same settings on both. For testing only; it lives on the integration branch, not on a step branch.
Adds RadioCliEdgeCases for both NodePrefs classes: boundaries and out of range values for every radio command, values that do not fit the field (int.thresh 300 is stored as 44, tx -5 reads back as 251), garbage input (atof/atoi read it as 0), NaN and infinity (rejected by "set rxdelay" and "set txdelay"), prefix look-alikes ("get tx foo" is "get tx", "set afx" is not handled), commands owned by other handlers ("set freq") and the key/value store exposing only the FEM gains. The expected values are what the current code returns, including the odd ones, which are kept on purpose.
"set af nan" is accepted by IEEE builds. nRF52 builds compile with -Ofast, whose -ffast-math lets float comparisons assume that NaN never occurs, so the result there depends on the compiler; that case is only checked when __FAST_MATH__ is not defined. NaN is detected through its bits, not std::isnan, for the same reason.
Why: the next change makes the radio CLI table driven. These cases are where a generic parser would most likely differ from the hand-written branches.
What changes - CommonRadioPrefs.cpp gets a table with one row per radio setting: its /prefs.json key, its field in RadioPrefsFields, its type, its CLI name and how the CLI treats it (get only, float with an upper limit, uint8_t with an optional upper limit, on/off), the reply for out of range values, and whether boards may read and write it through the key/value store. - The table drives the "radio" object of /prefs.json (structure() moves from the header into the .cpp and loops over the rows), the key/value access (fem_rxgain, fem_txgain), and the CLI of the eight settings that fit a generic pattern: freq (get only), cad, int.thresh, rxdelay, txdelay, direct.txdelay, path.hash.mode, multi.acks. - The float range check of the table recognises NaN and infinity by their bits. nRF52 builds compile with -Ofast, whose -ffast-math lets float comparisons assume that NaN never occurs; on a T-Echo the first version of this change accepted "set rxdelay nan" while the old branch rejected it. The bit check rejects NaN and infinity on every platform and with any flags, which is what the old branches did on the boards. - New native environment native_ofast runs the prefs tests with -Ofast, so host tests see the optimisation the nRF52 firmware is built with. The unit test workflow runs it. - radio, af, dutycycle, radio.rxgain, tx and agc.reset.interval keep their hand-written branches unchanged (composite value, derived value, radio driver calls, unit conversion, prefix quirk). They were moved verbatim. Why - A radio setting used to be spelled out in three places in this file alone: a def() line, a get branch and a set branch with its own parsing, range check and reply. A new setting of a common kind is now one row. Compatibility - /prefs.json, CLI replies, dirty marking and the key/value store are unchanged: the characterization and edge case tests (including garbage input, values that do not fit the field, NaN, prefix look-alikes) pass unchanged for both NodePrefs classes. - The public interface of CommonRadioPrefs is unchanged except that structure() is no longer defined inline.
- docs/refactoring/0003-radio-settings-table.md: context (27 branches for 14 settings), the table and its CLI kinds, the six settings that keep hand-written branches and why, alternatives, compatibility, and the verification: about 70 edge cases pinned on the old code, mutation tests in IEEE and -Ofast host builds plus the GCC check in CI, sizes for 18 environments, and the hardware run of 58 commands on a room server and a companion, including the NaN difference it found in the first version. - README.md: step 3 in the status table. - architecture-review-v1.18.md: the -Ofast / NaN observation as issue 9.
Both notes carried the 'Hardware' heading twice: an empty one directly followed by the real section. The stray line rendered as an empty section in the verification chapter. Remove the empty one in each note, so each has a single Hardware section. No text changes otherwise.
What this adds - 0004-shared-server-base.md: the measurement behind review finding F2 and the cut into steps 4a to 4f. Every top level member function of the three role implementations was extracted and compared after stripping comments, whitespace and the class name: 73 lines per role are identical in all three, 83 more differ only by cosmetics or one role value, and 50 more are identical in the repeater and the room server while the sensor lacks them. That is 312 lines of pure copies, about 420 counting the logging block and about 500 with the boot sequence. - The three differences that are behaviour rather than formatting, with the check that neither peer search can overflow (ClientACL caps num_clients at MAX_CLIENTS) and that the room server's missing STM32 guard is latent because no STM32 room server environment exists. - Ledger: why the sensor drifted, with the commit for each of the four deviations, and the exclusive upper bound of RNG::nextInt() that explains the two retransmit formulas and the divide by zero fix. Why - The review estimated 350 to 400 duplicated lines in finding F2. The series needs a measured cut and named role hooks before any code moves, and the sensor decision needed the upstream history first. Status - Proposal. No code moved yet, and the characterization depth is still open.
RCC-SKIPPED: defect and cause established by reading the source; the root cause chain was waived for this fix. What changes - IdentityStore::save() returns the result of writing the identity instead of always returning true. Both overloads were affected: the two argument form computed the result, logged it and then discarded it, and the form that also stores a display name ignored the result of both writes. - In that second form the display name write is now part of the result, so a medium that fills up between the keypair and the name is a failure, and it gets the debug line the other form already had. - New native environment native_identity_store covers IdentityStore against a mock filesystem whose file opens and then runs out of write budget, which is how a full or worn flash behaves. The unit test workflow runs it. - test/mocks gains that filesystem (Adafruit_LittleFS.h) and the serialisation of mesh::Identity and mesh::LocalIdentity, copied from src/Identity.cpp so that a short write is a failure in the double as well. test/mocks/Utils.h gains the no-op MESH_DEBUG macros that src/MeshCore.h defines for firmware builds, since the mock stands in for that header's constants. Why - Every role generates a keypair when the load at boot finds no file and then saves it: simple_repeater/main.cpp:94, simple_room_server/main.cpp:75, simple_sensor/main.cpp:101, kiss_modem/main.cpp:56, simple_secure_chat/main.cpp:327, companion_radio/MyMesh.cpp:955. When that write fails the node keeps running under an identity that never reached the medium and comes back with a different public key after the next boot. - One caller already acts on the result and is written for an honest one: CMD_IMPORT_PRIVATE_KEY in companion_radio/MyMesh.cpp:1544 adopts the imported key and answers OK when save() reports success, and answers ERR_CODE_FILE_IO_ERROR when it does not. While save() always reported success, a failed import was answered with OK and the old key came back at the next boot. Compatibility - The success path is unchanged, so nothing differs while the filesystem accepts the write. - All nine call sites were checked for what a false now triggers. Eight discard the result and are unchanged; three of those reach save() through void wrappers (simple_repeater and simple_room_server MyMesh::saveIdentity, SensorMesh::saveIdentity) that cannot propagate it anyway. The only consumer is CMD_IMPORT_PRIVATE_KEY above, which already has its error path. No boot path reads the result, so a failing write adds no error cascade at startup. - The overload that stores a display name has no caller in this repository. Evidence - Red before the change, green after: ReportsFailureWhenNoByteCanBeWritten, ReportsFailureWhenThePrivateKeyIsCutOff, ReportsFailureWhenTheKeypairIsCutOff and ReportsFailureWhenOnlyTheNameIsCutOff each reported "Actual: true, Expected: false" on the old code; 9 of 9 pass with the change. - pio test -e native -e native_kiss_modem -e native_identity_store: 63 of 63. - Five deliberate mutations of the new code were each caught by the suite: returning true again in either overload, dropping the check of the name write, not reading the result of writeTo(), and joining the two writes with OR instead of AND.
…radio What this adds - A host harness for the server roles. The three role implementations could not be compiled natively before: they need a file system, an RTC and the globals a variant's target.h declares. test/mocks gains an in-memory file system (File derives from Stream and takes void* buffers, as the Arduino cores have it), a DateTime stand-in, the LittleFS global, Arduino's random API and a capturing Serial; target.h gains the board, the RTC and a radio driver that is now a mesh::Radio, so a role can be built exactly as its main.cpp builds it. - A native_server environment that compiles the real room server role against those mocks. RP2040_PLATFORM selects the plain string file modes the mock implements. One role per environment, because the repeater and the room server both define MyMesh. - test_server_base: six characterization tests for the temporary radio parameters, which step 4c moves into the shared base. They pin what the CLI command schedules, that the pending values reach the radio once, and that the node reverts to its own settings afterwards. - Identity stubs for the suite. Unlike the tuning tests, these are deterministic rather than refusing: a role signs every advertisement it schedules, so a refusing stub turns ordinary operation into a failure. They are not cryptographic and no test here asserts a cryptographic property. Measured while writing this - A timer set for T fires at T + 1, not at T: Dispatcher::millisHasNowPassed() is '(long)(now - timestamp) > 0'. Three tests pin that edge, since it is the observable boundary of every scheduled action in the roles, and the signed difference is what carries it across the millis() rollover. - The role's fallback radio defaults show up as 915 MHz and 250 kHz on the host, where the root platformio.ini values are absent. That is review issue 8, now visible in a test rather than only by reading. Verification - pio test -e native -e native_core -e native_kiss_modem -e native_ofast -e native_server: 88 of 88 test cases pass (82 before, 6 new). The shared mocks that other suites use (Stream.h, SHA256.h, Arduino.h, target.h) were changed, and all four pre-existing environments still pass. - Seven deliberately introduced faults each turned the suite red: applying with no reply delay, neither timer being cleared, the timeout read as seconds, the frequency and bandwidth swapped when applying, the revert restoring the temporary values, and millisHasNowPassed relaxed to >=. Both patched files were restored afterwards and the suite is green again.
The cut plan listed the characterization depth as an open decision, between leaning on the diff for the identical moves and building a harness for the real role classes. Probing the compiler settled it: the native build lacked three headers and a handful of mock methods, not a deep rework, so the harness exists and the question is closed. Also notes the state of review finding F6 for the server roles, since the harness is the first time any of that code runs on the host.
Adds GroupMessageTest in a new native_chat environment: the lengths that must keep working (empty text, short text, one byte across a cipher block, the longest text that fits, and a length beyond that, which is clamped and not refused), and the two lengths a short companion frame produces (-1 and -4). Each case is observed through the payload length of the packet that reaches the packet manager, so "refused before sending" and "sent" are told apart by whether a packet was queued at all. The last case characterises sendGroupData(), the sibling that already refuses a negative length. The environment builds the real Utils.cpp, Packet.cpp, Dispatcher.cpp, Mesh.cpp, BaseChatMesh.cpp, AdvertDataHelpers.cpp and TxtDataHelpers.cpp on the host, with fakes for radio, clock, RNG and packet manager and with the same link-time identity stubs the board crypto would otherwise provide. test/mocks/Arduino.h gains cstdio, cstdlib, cstring and ltoa(), which the Arduino API provides and the host C library does not. The native environment ignores the new folder because it does not build the chat layer. This commit is red on purpose, and that is the point of it: $ pio test -e native_chat Testing... Program received signal SIGBUS (Bus error: 10) native_chat test_chat_group_msg ERRORED 00:00:07.591 $ pio test -e native_chat -a "--gtest_filter=-GroupMessageTest.Refuses*" GroupMessageTest.SendsEmptyText [PASSED] GroupMessageTest.SendsShortText [PASSED] GroupMessageTest.SendsTextAcrossABlockBoundary [PASSED] GroupMessageTest.ClampsTextThatIsTooLong [PASSED] GroupMessageTest.SendsTheLongestTextThatFits [PASSED] GroupMessageTest.GroupDataAlreadyRefusesNegativeLength [PASSED] 6 test cases: 6 succeeded in 00:00:03.471 The six cases that describe today's behaviour pass, so the expectations are right; only the two negative lengths bring the process down, in the memcpy of sendGroupMessage. The next commit makes them pass. Why: a companion frame shorter than 7 bytes makes MyMesh.cpp compute a negative text length for this call, and a T-Echo running dev stops answering for good when it arrives. Without a test at this level the fix would rest on the frame handler alone, and every other caller of sendGroupMessage would stay unguarded.
RCC-SKIPPED: defect and cause established by reading the source; the root cause chain was waived for this fix. What changes - The sanitising that the binary prefs loader has always done moves out of loadPrefsInt() into CommonCLI::sanitisePrefs(), and the /prefs.json path calls it too. The fields are the ones the binary loader already covered. - One limit is corrected while moving it: airtime_factor is clamped to 0..99, not 0..9. "set dutycycle 1" stores 100/1 - 1 = 99, so 9 would throw away any duty cycle below 10 percent at the next boot. The limit of 9 was written in 01d84d5 (2025-03-05) and "set dutycycle" was added in 0aa0ec1 (2026-03-07), so the limit was never adjusted for it. Every other limit is unchanged. "set tx" and "set multi.acks" have no check in the CLI at all, so for those the loader's limit is the only guard and it stays as it was. - ConfigSerializer no longer stores a float or double that is not a finite number. A token such as nan, inf or 1e40 leaves the field at the value it already had, which is the default the firmware set up before reading the file. The check looks at the bits, not at a comparison, and for floats it runs after the narrowing, because a value that is finite as a double can overflow to infinity as a float. - loadPrefs() logs it when loadSerial() reports a parse error. The result was discarded before. Behaviour is otherwise unchanged: a partially parsed file is still applied, and now sanitised. Why - Server roles read /prefs.json through _prefs->loadSerial(file) and used whatever was in it. The file is plain text on a filesystem that a user can write, and values out of range reach the radio setup directly: freq, bw, sf, cr, tx_power_dbm, the delay factors, the bridge settings. - The binary loader sanitised all of this and had done so before /prefs.json existed, with the comment "sanitise bad pref values". The JSON path lost that, so the protection depended on which file a node happened to boot from. - Non-finite values need their own handling because constrain() cannot catch them: constrain(NaN, 0, 9) returns NaN, since both of its comparisons are false for a NaN. Infinity is clamped, NaN is not. - Clamping a NaN into range has no sensible answer, which is why it is rejected at the parser instead. The firmware sets its defaults before reading the file (for the repeater at MyMesh.cpp:891 onwards, with loadPrefs at :949), so leaving the field untouched keeps the build default. Clamping to the nearest bound would put a node whose file says freq:nan on 150 MHz. Compatibility - The binary loader keeps its behaviour except for airtime_factor, which it now leaves alone up to 99 instead of cutting it to 9. - On the /prefs.json path a value outside the limits is now clamped at boot where it used to be taken as is. The limits match what the CLI accepts, with one looser spot: the CLI takes a bandwidth from 7.0, while the loader clamps to 7.8, which is the smallest bandwidth the radios offer. - A file holding a non-finite number now leaves that one field at its default instead of poisoning it. Every other key in the file is still applied. - The change to ConfigSerializer also covers the companion NodePrefs, which uses the same serializer. Evidence - Five new cases in test_config_serializer. Three are red on upstream/dev 428a95c and green with the change: a file with nan, a file with inf and -inf, and a float that overflows to infinity. Two pin behaviour that must not change: finite values are stored, and garbage still parses as zero. - pio test -e native -e native_kiss_modem: 59 of 59. - Five deliberate mutations of the parser check were each caught: dropping the check in either path, checking before the narrowing to float, inverting the bit comparison, and using the float mask for doubles. - sanitisePrefs() itself has no host test: no native environment compiles CommonCLI.cpp on this branch. It is covered by the build of every role and by a board test with a hand written /prefs.json holding out of range values.
What changes - IdentityStore::save() returns the result of writing the identity instead of always returning true. Both overloads were affected: the two argument form computed the result, logged it and then discarded it, and the form that also stores a display name ignored the result of both writes. - In that second form the display name write is now part of the result, so a medium that fills up between the keypair and the name is a failure, and it gets the debug line the other form already had. - New native environment native_identity_store covers IdentityStore against a mock filesystem whose file opens and then runs out of write budget, which is how a full or worn flash behaves. The unit test workflow runs it. - test/mocks gains that filesystem (Adafruit_LittleFS.h) and the serialisation of mesh::Identity and mesh::LocalIdentity, copied from src/Identity.cpp so that a short write is a failure in the double as well. test/mocks/Utils.h gains the no-op MESH_DEBUG macros that src/MeshCore.h defines for firmware builds, since the mock stands in for that header's constants. Why - Every role generates a keypair when the load at boot finds no file and then saves it: simple_repeater/main.cpp:94, simple_room_server/main.cpp:75, simple_sensor/main.cpp:101, kiss_modem/main.cpp:56, simple_secure_chat/main.cpp:327, companion_radio/MyMesh.cpp:955. When that write fails the node keeps running under an identity that never reached the medium and comes back with a different public key after the next boot. - One caller already acts on the result and is written for an honest one: CMD_IMPORT_PRIVATE_KEY in companion_radio/MyMesh.cpp:1544 adopts the imported key and answers OK when save() reports success, and answers ERR_CODE_FILE_IO_ERROR when it does not. While save() always reported success, a failed import was answered with OK and the old key came back at the next boot. Compatibility - The success path is unchanged, so nothing differs while the filesystem accepts the write. - All nine call sites were checked for what a false now triggers. Eight discard the result and are unchanged; three of those reach save() through void wrappers (simple_repeater and simple_room_server MyMesh::saveIdentity, SensorMesh::saveIdentity) that cannot propagate it anyway. The only consumer is CMD_IMPORT_PRIVATE_KEY above, which already has its error path. No boot path reads the result, so a failing write adds no error cascade at startup. - The overload that stores a display name has no caller in this repository. Evidence - Red before the change, green after: ReportsFailureWhenNoByteCanBeWritten, ReportsFailureWhenThePrivateKeyIsCutOff, ReportsFailureWhenTheKeypairIsCutOff and ReportsFailureWhenOnlyTheNameIsCutOff each reported "Actual: true, Expected: false" on the old code; 9 of 9 pass with the change. - pio test -e native -e native_kiss_modem -e native_identity_store: 63 of 63. - Five deliberate mutations of the new code were each caught by the suite: returning true again in either overload, dropping the check of the name write, not reading the result of writeTo(), and joining the two writes with OR instead of AND.
What changes
- init_bme680_bsec() now returns 0 unless bsec_iaq.bme68xStatus is BME68X_OK.
The check sits before the existing bsecStatus check, so a device that is not
a BME68x is rejected before BSEC is configured, subscribed or handed a saved
state blob.
Why
- Bsec::beginCommon() fills two independent status fields. bsecStatus comes
from bsec_init(), which is a pure software init of the Bosch library and
reports BSEC_OK even with an empty I2C bus. bme68xStatus comes from
bme68x_init(), which is the only part that talks to the chip: it reads the
chip id and returns BME68X_E_DEV_NOT_FOUND when the id is not 0x61.
- init_bme680_bsec() looked at bsecStatus only, so it reported success for any
Bosch sensor that shares the address: BME280 and BMP280 default to 0x76 just
like BME680, and the SDO pin only moves them to 0x77.
- Until 1.17.1 that false positive was cosmetic. The detection loop kept
walking the table after a hit, so BME280 was still initialized and only an
empty LPP channel was left over. 1.18.0 marks the address as consumed after
a successful init ("detected[def.address] = false"), and it probes the
alternate Bosch address as well, so the false positive now takes both 0x76
and 0x77 away from the real driver. A RAK4631 with a BME280 reports no
environment telemetry at all: query_bme680_bsec() returns early forever,
because bsec_data_ready is never set without a BME68x. Issue meshcore-dev#3634.
- ENV_INCLUDE_BME680_BSEC is set by variants/rak4631/platformio.ini only, so
RAK4631 builds are the ones affected.
- There is no configuration workaround for a user: moving the sensor to 0x77
does not help now that both addresses are probed.
Compatibility
- A real BME680 is unaffected: bme68x_init() returns BME68X_OK for chip id
0x61, and nothing else in the function changed.
- On a board with no BME68x, BSEC stays inactive, which is what the code
already intended. bsec_active keeps its value of false, so the loop() hook
does not run BSEC and no bsec_state.bin is written.
- The same defect was reported upstream in meshcore-dev#2746, which fixes it the same way
and additionally checks the INA226 die id. That PR is still open; this
change is the minimal part that the issue needs.
Evidence
- Bosch sources, as installed by the pinned dependency
"BSEC Software Library @ ^1.8.1492" (resolved to 1.8.1492):
src/bsec.cpp Bsec::beginCommon() sets bsecStatus from bsec_init() and
bme68xStatus from bme68x_init(); src/bme68x/bme68x.c bme68x_init() returns
BME68X_E_DEV_NOT_FOUND unless the chip id equals BME68X_CHIP_ID.
- The regression window was established by diffing repeater-v1.17.1 against
repeater-v1.18.0: the variant flags are identical in both, and the sensor
manager gained the consumed-address line and the alternate Bosch addresses.
- pio run -e RAK_4631_repeater: SUCCESS, flash 440952 bytes.
- Mutation check that the new line is really compiled in that env: renaming
bme68xStatus to a non-existent member fails the build at
EnvironmentSensorManager.cpp:506, and restoring it builds green with the
identical flash figure.
- Not verified on hardware: no RAK4631 with a BME280 was run for this change.
EnvironmentSensorManager.cpp is in no native environment's build_src_filter,
so there is no host test for it either.
CMD_SEND_CHANNEL_TXT_MSG reads txt_type, channel_idx and a 4 byte timestamp, so the frame needs 7 bytes before the text starts, and the branch had no length guard. For a shorter frame "len - i" is computed in size_t, wraps, and reaches sendGroupMessage() as a negative int. The clamp there tests "text_len + prefix_len > MAX_TEXT_LEN", which a negative value passes, and the following memcpy() converts the length back to a size_t: a 3 byte frame asks for 4294967292 bytes into a 197 byte stack buffer. Two guards, because there are two mistakes. The branch now states its minimum like the 35 other branches of handleCmdFrame do, so a frame below 7 bytes is answered through the existing path instead of being parsed. And sendGroupMessage() refuses a negative length the way sendGroupData() in the same file already does, which protects every other caller and is the reason the fix can be tested at all. A frame of exactly 7 bytes keeps working and sends an empty message, as before. Evidence. On a T-Echo (techo-3dcf) running dev 428a95c, built with FIRMWARE_BUILD_DATE "f1-dev" and confirmed by "ver => v1.18.0 (Build: f1-dev)": CH_TXT len=7 -> RESP_CODE_OK GET_STATS core -> uptime=78s CH_TXT len=3 -> NO REPLY DEVICE_QUERY -> NO REPLY (three times over 8 s, node never came back) On the host, the same sources in the native_chat environment: before: Program received signal SIGBUS (Bus error: 10) after: 8 test cases: 8 succeeded native test_mesh_tables PASSED native test_config_serializer PASSED native test_utils PASSED native test_utf8_helpers PASSED native test_routing_policy PASSED native_chat test_chat_group_msg PASSED native_kiss_modem test_kiss_modem PASSED 62 test cases: 62 succeeded in 00:00:29.696 test_companion_node_prefs is reported SKIPPED, which it also is on unmodified dev (46 of 46 there), so that is not a change from this commit.
What changes - New class ServerMesh (src/helpers/ServerMesh.h, .cpp) between mesh::Mesh and the three server roles. It holds the fields all three declared separately (the transport key store, the Region map and its temp instance, the Region load stack, the Region a packet arrived on, the node's default scope, the load flag and the file system pointer) and six methods: onRecvPacket(), sendFloodReply(), sendFloodScoped(), startRegionsLoad(), saveRegions() and onDefaultRegionChanged(). - Repeater, room server and sensor derive from it and drop their copies. Each role gains exactly three lines: the include, the base in the class declaration and the constructor forwarding to it. 266 lines deleted, 10 inserted. - The shared constructor initialisation moved too: recv_pkt_region to NULL, region_load_active to false and the default scope zeroed, which all three did identically at the end of their own constructor. Why - The six methods were byte for byte identical in all three roles, and the Region and scope fields were declared three times with the same types. A change to the reply scope rules had to be made in three places, which is how the roles drifted apart in the first place (see the ledger). This is the first of the steps in docs/refactoring/0004-shared-server-base.md. Compatibility - Behaviour is unchanged. The bodies moved unaltered: each one was extracted from all three role implementations and compared after normalising only the class name and the pointer star spacing, and all six were identical, 60 lines per role. Nothing in the move depends on the open sensor questions. - The members are protected rather than private, because the role code reaches them directly as it did when they were its own fields. Verification - pio test -e native -e native_core -e native_kiss_modem -e native_ofast -e native_server: 88 of 88 test cases pass, unchanged from before the move. - All three role implementations compile for the host harness with no errors, and the preprocessor structure of each header is intact (the #endif count per file is unchanged). - NOT YET VERIFIED: the firmware build matrix, and the moved methods have no direct host test of their own yet; for this step the diff proof and the compilation carry it. A characterization test for them follows.
What this adds - Ten characterization tests for ServerMesh::onRecvPacket(), the resolution that step 4a moved into the shared base. It decides which Region the packet being handled arrived on, and that field drives the scope of every flooded reply, so nothing else in the firmware reports it. The tests reach it through a subclass of the real room server role that opens up the base's state. - Covered: a direct packet leaves no Region; an un-scoped flood arrives on the wildcard, or on none when the wildcard denies flood; a scoped flood with transport code 0 arrives on the wildcard; one with an unknown code arrives on none; one carrying a Region's own code arrives on that Region, and on none when that Region denies flood; and the Region does not survive into the next packet. Measured while writing this - RegionMap::putRegion() sets REGION_DENY_FLOOD on a new Region, so a freshly added Region is invisible to the flood lookup until the flag is cleared. Pinned in its own test, since the others depend on it. - The two flood route types disagree about a denied Region: the un-scoped branch applies the flag itself, the scoped branch delegates to findMatch(), whose transport-code-0 shortcut returns the wildcard before the mask is applied. So denying flood on the wildcard stops an un-scoped flood but not a scoped one carrying no code. Recorded as observed behaviour and added to the ledger as issue 10; not changed here, because step 4a is a move. Verification - pio test -e native -e native_core -e native_kiss_modem -e native_ofast -e native_server: 98 of 98 test cases pass (88 before, 10 new). - Six deliberately introduced faults in the moved method each turned the suite red: the Region not being cleared for a non-flood packet, the wildcard's flood denial ignored and inverted, a scoped flood resolving to the wildcard instead of going through the lookup, the two flood route types swapped, and the lookup mask dropped. Two of them were not caught by the first version of these tests, which is what added the cases carrying a real transport code. The file was restored afterwards and the suite is green again. - NOT YET VERIFIED: the firmware build matrix for step 4a.
… wrap Adds src/helpers/MillisDeadline.h with mesh::millisHasPassed(now, deadline) and test_millis_deadline covering it: the strict comparison, agreement with the (int32_t)(now - deadline) > 0 idiom of Dispatcher::millisHasNowPassed over 70 pairs including differences of 0, 1, 2^31 - 1, 2^31 and 2^32 - 1, a deadline that has already wrapped while millis() has not, the period over three windows (quiet, across 2^31, across the 2^32 wrap), and the documented horizon of half the counter. The helper takes uint32_t and not long on purpose. long is 32 bits on every firmware target and 64 bits on the test host, and the two widths do not behave the same at the wrap: 32 bits over-fires briefly, 64 bits stalls for a whole counter period. A test written with long would measure the host instead of the target. The helper compares the wrapped difference against half the counter rather than casting a difference above LONG_MAX to long, which the standard leaves to the implementation before C++20. Both spellings agree on every input and compile to identical instructions on nRF52 with -Ofast and on ESP32-S3 with -Os. Written against the shape the variants ship: with "return now > deadline" as the body, four of the five tests fail. Six mutations of the helper were each caught (4, 3, 2, 5, 4 and 2 failing tests), including computing in unsigned long instead of uint32_t, which is invisible on a 32-bit target and wrong on a 64-bit host. Compiled on its own with -Wall -Wextra -Wconversion -Wsign-conversion -Wshadow -Wold-style-cast -Wpedantic, the header is warning free on arm-none-eabi gcc 7.2.1, 12.3.1, 12.3.1 with -Ofast and 14.2.1 and on xtensa-esp32 and xtensa-esp32s3 gcc 8.4.0; the firmware builds with -w, where a warning would not surface. Why: the next change points seven variants at this function. Seven hand-written copies of the idiom are what produced the defect, and variants/*/target.cpp does not build on the host, so only a shared function can be covered by a test.
…nd without option macros
…mpanion handlers Measured inventory of the cmd_frame accesses in the 59 handlers (static script and an AddressSanitizer probe of the host harness), the reader API, why the answers stay byte for byte the same, one pull request in four commits, rejected alternatives, and the upstream overlap on dec8ad6 (including the ByteReader of meshcore-dev#2864).
A catalog of 98 real frames (every command at its minimum, both sides of every optional field, the measured reads past the end) goes to a fresh node once as it is and once behind each of four tails a previous frame leaves in cmd_frame. Replies, queued packets, files, collaborator calls and what later loops and a flood show must be identical. A second test pins the answer one byte below each of the 44 minimums. Characterization for step 10c, on the unchanged handlers.
…ever past its end Header only, not used by firmware code yet. u8/u16/u32/i32 copy with memcpy as the handlers do, bytes(n) returns a pointer into the frame, has(n) tests an optional field, restAsCString() writes the terminator at buf[len] as five companion handlers do today. A read past the end returns 0 or nullptr and clears ok() for good. Host builds can count such reads (FRAME_READER_COUNT_OVERRUNS); native_companion does, and a new test allows them only for an announced path in SEND_RAW_DATA and SEND_CHANNEL_DATA.
The handlers that read one field after the other without a length test of their own: offsets counted by hand become u8(), u32(), bytes(n), the pointer plus len - n handed to a callee becomes remaining() and bytes(remaining()), and the five in place terminators become restAsCString(), which writes the same byte. Every read lies inside the row's minimum, so the reader never runs out here. Frame sweep, command tests and the frame bounds tests pass unchanged.
…ength tests read through FrameReader Optional fields test r.has(n) for the bytes they need, which is the old len test with the same value. The path of SEND_RAW_DATA and SEND_CHANNEL_DATA is taken with bytes() before it is copied, so a path length byte that announces more than the frame holds no longer copies the previous frame's bytes; it fails the length test that follows, as it did before, with the same ILLEGAL_ARG. SET_DEFAULT_FLOOD_SCOPE bounds its strlen() to the frame: a name without terminator was refused before and still is. SET_AUTOADD_CONFIG reads the hop count into a local first, since min() may be a macro.
Found by a mutation of step 10c (has(17) instead of has(16)) that no test caught: the key's effect only shows in the next flood, whose route type carries a transport code or not. Passes on the handlers before and after 10c.
SEND_CHANNEL_DATA and SEND_RAW_DATA with a path longer than the frame, and SET_DEFAULT_FLOOD_SCOPE with an unterminated name, each behind four tails that would complete the path or end the name: ILLEGAL_ARG, nothing queued, no file changed. Passes on the handlers before and after 10c. The note lists the open findings outside the handlers with a way to reproduce each.
The first CI run failed NoCommandReadsThePreviousFrame on Linux only: the payload of a queued channel message held bytes of earlier frames. The host's AES128 mock writes nothing, so an encrypted packet's payload, MAC and transport code are whatever its buffer held before, which depends on the allocator. With a mock that copies its input the test passes on Linux; the firmware is not involved. Encrypted packets are now compared by header, route, path and payload length; the other types stay byte for byte. Checked on macOS and in a Linux container, against the handlers before and after 10c, and the mutations caught before are still caught. The note records the cause in place of the earlier guess.
…and input The mock's encryptBlock() wrote nothing, and Utils::encrypt() encrypts straight into the packet buffer, so encrypted packets on the host carried whatever that buffer held before. The frame sweep pinned those leftovers in the packets digests of every sending command that encrypts and was stable only because macOS and the CI's Linux allocate alike. Each block is now XORed with the key. The golden file changes in exactly 20 packets digests (codes 02 03 1a 1b 27 32 34 39 3e); no reply line and no files digest changes.
With a deterministic cipher mock the payload of an encrypted packet no longer depends on the allocator, so NoCommandReadsThePreviousFrame can compare every queued packet in full instead of its shape.
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
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.
CI only, not for merge into integration. Based on
integrationate0d05fde(step 10c merged), so the diff againstdevalso shows those commits. This PR's own commits are the last three, from12d453bb.Fixes open finding 3 of note 0010: the host's
AES128mock (test/mocks/AES.h) wrote nothing inencryptBlock(), andUtils::encrypt()encrypts straight into the packet buffer, so encrypted packets on the host carried whatever that buffer held before. The mock now XORs each block with the key: the output depends only on key and input, anddecryptBlock()undoesencryptBlock(). Test code only, no firmware change.Evidence:
mallocblock prefilled (macOS: interposedmalloc, filla5,3c; Linux container python:3.12-slim, glibc 2.41:MALLOC_PERTURB_=165,60), the frame sweep differs in 20 lines, allpacketsdigests, for every sending command that encrypts:02 03 1a 1b 27 32 34 39 3e. Unfilled, both systems match the old golden file.abort()in it: onlynative_companionandnative_chat(the latter asserts payload lengths only).frame_sweep.goldenregenerated withCOMPANION_SWEEP_UPDATE=1: exactly those 20packetsdigests change; code, pass andfilesdigest unchanged in each, no reply line touched.a5 3c 00 ff, LinuxMALLOC_PERTURB_165, 60, 1).test_frame_bounds.cppcompares every queued packet byte for byte again (packetLine()removed). That comparison sees the leftovers: with the old mock it failsNoCommandReadsThePreviousFramein the Linux container, as the first CI run of 10c did. With the new mock it passes on both systems and under every fill.