Repository navigation
refactor(radiolib): one template for SX1262, SX1268 and LLCC68 (step 9b, not for merge) - #31
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.
inRange() refused NaN and infinity by testing the bits of the parsed float.
That test is not reliable: under -ffast-math (nRF52 builds use -Ofast) the
compiler may assume a float is never NaN or infinite, and clang does fold the
bit test away (measured: "nan" passed it at -Ofast, while arm-none-eabi-gcc and
GCC 14 kept it). inRange() now takes the text as well and refuses the only
spellings strtof()/atof() turn into NaN or infinity: after optional white
space and sign, text that starts with 'n' or 'i'. An infinity from digits
("1e40") still fails the comparison.
Every other input keeps its old meaning, including garbage that reads as 0
("set rxdelay abc" -> OK, 0), which RadioPrefsChecks pins on purpose. This is
why the integration branch does not take the stricter "must start with a digit
or '.'" check of the upstream pull request: for "set af" and "set dutycycle"
both give the same answers, for the generic float settings only this one
leaves the garbage behaviour alone.
Honest limit: the tests cannot tell this change from the old code or from no
NaN check at all. The comparison inRange() already uses, "v >= 0 && v <= max",
is false for a NaN, and it stayed false with clang and GCC 14 at -O2 and
-Ofast (mutation without the text check: all prefs suites still green). The
text check is what makes that independent of the compiler.
RadioPrefsChecks gains "+nan", "infinity", " nan" for af and dutycycle and
"+nan", "INF" for rxdelay, and its two comments now say text instead of bits.
Verified: all 13 host environments, 354 of 354; GCC 14 in Docker builds
test_common_node_prefs at -O2 and -Ofast with this, the old and the mutated
code, all green; LilyGo_T-Echo_room_server, Heltec_v3_repeater and
LilyGo_T-Echo_companion_radio_usb build.
…n() on the T-Echo
… 11, cut A) The design note arrives as 0008-variant-target.md, but integration already has 0008-forward-policy.md and 0009 and 0010 are taken by the chip adapter and the companion command table, so the note becomes 0011, and the one reference to it (tools/refactoring/variant_duplication.py) follows. The unit test workflow conflicted because both this branch and the chip adapter append their environment to the same line; both are kept.
Both arrived without touching the shared README. The table now names their notes, 0009 and 0011, and the paragraph of planned steps says why the numbers have gaps and that step 10 (note 0010) is still on its own branch.
handleCmdFrame() in examples/companion_radio/MyMesh.cpp is one chain of 62 branches that each state their own minimum frame length. Before it is restructured into a command table (docs/refactoring/0010), its behaviour is pinned on the host. [env:native_companion] builds the real MyMesh.cpp and DataStore.cpp against the mocks in test/mocks. The mocks gain what the companion needs: FSInfo and a (always empty) directory listing in FS.h, and the declaration of radio_new_identity() in target.h. MAX_CONTACTS and MAX_GROUP_CHANNELS are set for the whole build, as every companion environment does; with MAX_CONTACTS only in MyMesh.h, BaseChatMesh.cpp and MyMesh.cpp disagreed about the class layout. The harness drives a node only through the protocol (a fake serial interface and MyMesh::loop()), in zeroed storage, and observes the rest on the collaborators: flash, queued packets, reboot, radio and clock. - test_frame_sweep.cpp: every code 0 to 255 at every length 1 to 176 with five fills, one fresh node per frame, against frame_sweep.golden (replies byte for byte, side effect flags, and a digest of queued packets and stored files per code). Checked to be independent of test order and code order. The stack is scrubbed before each frame because CMD_SET_CHANNEL writes uninitialised stack bytes behind the channel name to /channels2; those bytes are zeroed before the file digest is taken. - test_commands.cpp: 49 tests with the frames an app sends: reply fields, state read back through the protocol, the boundary at each minimum, sub commands. Pins two oddities as they are: a contact added by hand cannot be shared, and a key of 32 zero bytes names one of the reserved anonymous slots, so REMOVE_CONTACT with it removes a slot and the contact count wraps. Mutation checked: 9 of 10 deliberate faults in MyMesh.cpp are caught, the tenth is an equivalent mutant (the len < 4 check of CMD_SEND_CHANNEL_DATA is covered by its later data_type check). No firmware source changes.
companion_sweep.py sends the frames of the host sweep's all zero pass (test/test_companion_frames/frame_sweep.golden) to a USB companion and compares the replies: every frame the host refuses, every length of the commands that only report, and four lengths of every code that is not a command. Error and OK replies are compared byte for byte, report replies by their code. Commands that store, transmit, restart, format or keep session state are never sent at an accepted length. REMOVE_CONTACT and RESET_PATH are among them because a key of zeros names a reserved contact slot; a first run on a T-Echo removed those slots until the next reboot. On techo-2 (integration firmware, USB companion): 6502 frames, all as on the host.
…010) Inventory of every command code (minimum length, further conditions, replies, the answer to a short frame), the table design for step 10b (one row per code with its minimum, a handler method per command that can decline, the central check answering ERR_CODE_UNSUPPORTED_CMD exactly as the chain does today), the rejected alternatives, the cut plan 10a to 10d, the overlap with 19 open upstream pull requests, and the verification of step 10a: host harness, mutation table, hardware sweep on a T-Echo. Also records three findings that are not changed here: a zero key names a reserved contact slot, CMD_SET_CHANNEL stores uninitialised stack bytes behind the channel name, and the len < 4 check of CMD_SEND_CHANNEL_DATA cannot be observed.
handleCmdFrame() was one chain of 62 branches of the form `cmd_frame[0] == CMD_X && len >= N && ...`, each with its own minimum frame length. It is now a table with one row per command code (59 codes): the code, the shortest frame the command accepts, and a handler method. The dispatcher checks the minimum in one place and answers ERR_CODE_UNSUPPORTED_CMD for a frame below it, for a code without a row, and for a handler that returns false, which is exactly what the chain's final else answered in each of those cases. The branch bodies moved unchanged into the handlers (git diff --color-moved shows them as moves); only the branch heads, the final else and three early `return;` (now `return true;`) have no counterpart. A further condition of a branch (a byte value, the reboot and reset words, the control data flag, the trace path's upper bound) becomes the handler's first line, returning false. The three codes that had two branches (39 telemetry, 32 set channel, 54 flood scope key) keep both, in the old order, in one handler; their row carries the smaller minimum. CMD_SEND_CHANNEL_DATA keeps minimum 1 and its own ILLEGAL_ARG checks, because that is its answer today. No byte on the wire changes. The 10a characterization passes unchanged: the frame sweep (225,280 frames against frame_sweep.golden, packets and files included) and the 49 command tests. Eight mutations of the new structure (row minimums, a missing row, the dispatcher ignoring a declining handler or the minimum, handlers declining wrongly) are all caught. See docs/refactoring/0010-companion-command-table.md.
…mages companion_session.py sends 39 frames that the commands accept (every setter with the value the board already holds, declining sub commands, out of range values, stats, self telemetry, a signing round, channels, CLI) and prints the replies, with clock, uptime and battery masked. Run once per image and diff. On techo-2, 10a against 10b: only the build marker in DEVICE_QUERY and the reply to ver differ.
Host suites and mutations aimed at the table, the build matrix with flash and RAM for 13 companion environments on five platform packages plus two repeaters as the control, and the hardware runs on a T-Echo (A/B session and frame sweep).
The unit test workflow and the test_ignore list of [env:native] conflicted because the chip adapter, the variant target and this branch each add their suites at the same place; all of them are kept. The design note keeps its number, 0010, which was left free for it.
The companion command table (note 0010) is merged now, so it gets its row next to steps 9 and 11, and the sentence that called it in progress goes.
The section says what moved into src/helpers/FemRxGain.h and why a class template, how the null store check relates to the open upstream pull request meshcore-dev#3496 (and to meshcore-dev#3453 and meshcore-dev#2682, which touch the same files), the host tests and the mutation table, and the before and after builds of all 50 environments of the four boards plus the 18 of the series: RAM unchanged, flash +16 on T096 and +24 to +72 on the ESP32 boards, measured down to the functions.
…cut B) HeltecV4Board, HeltecV4R8Board, T096Board and HeltecTrackerV2Board now forward attachDynamicPrefs() and handleCommand() to one class template in src/helpers/FemRxGain.h. Replies and store writes are unchanged; `set radio.fem.rxgain` no longer dereferences a missing store (the check of meshcore-dev#3496). The branch records cut B in note 0008, which is note 0011 on this branch since the merge of cut A; the section lands in docs/refactoring/0011-variant-target.md.
…nd without option macros
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
integrationat68e0fcdc(step 11 cut C merged), so the diff againstdevalso shows the earlier steps. This PR's own commits are the last five (629cedb1,568891f9,61d3a4f4,2228a5d6,102c5e77).Step 9b of note 0009:
CustomSX1262,CustomSX1268andCustomLLCC68were copies. A script (tools/refactoring/scripts/sx126x_inventory.py) shows SX1268 and LLCC68 identical in every member, and both differing from SX1262 only instd_init(): theSX126X_USE_REGULATOR_LDOoption, theSX126X_REGISTER_PATCHblock for register 0x8B5 and threeMESH_DEBUGlines. The class body now lives once in the templateCustomSX126x<Base, Traits>(src/helpers/radiolib/CustomSX126x.h), with SX1262'sstd_init(). The three headers keep a real class each (thin subclass, so forward declarations and chip only additions such as upstream meshcore-dev#3638 still have a place) and a traits struct with three flags. SX1268 and LLCC68 set all three to false, so they keep ignoring both macros and print nothing;begin()getsfalseas its ninth argument there, RadioLib's default (source lines in the note). Aligning them is a separate behaviour decision.CustomSTM32WLx, the wrappers andSX126xReset.hare unchanged. Insrc: 339 lines out, 188 in.Evidence:
61d3a4f4): a typed suite over the three classes without option macros (begin arguments, retry with TCXO 0 V on -706/-707, error paths, boosted gain register), and a separate test programtest_radio_chips_initwith all option macros andMESH_DEBUGset, pinning the ordered call log, register 0x8B5 and the debug output per chip. All 17 host environments pass, 605 cases, on the test commit and on the code commit (577 before, 28 new). 14 mutations, all caught, among them each trait flipped for SX1268 or LLCC68 (regulator, register patch, debug) and for SX1262.CustomSX1262(493) andCustomSX1268(12) environment plus 22 controls (all 16 STM32WLx, LR1110, LR1121, LR2021, SX1276). The same 15 fail on both sides, all for reasons outside the radio headers (listed in the note). RAM unchanged in all 512 that build. Flash: nRF52 +0 in 197 of 202 and +16 in 5, RP2040 +0 in 25 of 25, STM32 and the other controls +0, ESP32 mostly +0 or +4 (range -40 to +16). ForHeltec_v3_repeater,nmandsize -Ashow the +4 entirely in the static initializer of the variant globals (radio object built through one more base constructor); every chip and wrapper method has the same size.Upstream collisions (checked 21 October 2026, all 499 open PRs): meshcore-dev#3638 is heavy textually but its additions are SX1262 only and move into the thin subclass; meshcore-dev#2131, meshcore-dev#2869 and meshcore-dev#2912 edit all three copies identically and get smaller with the template. Details in the note.