From c650278fa7a680d097d73cdae5e7ede8454cb898 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Tue, 27 Oct 2026 08:17:23 -0700 Subject: [PATCH] Share constrained STM32 code while preserving firmware features Omit unbound forwarding-region callbacks only for exclusive Companion source roles. Reuse the compact neighbor sorter in repeaters for every RPC order and the CLI list, removing duplicate sort implementations while preserving storage and size gates. --- examples/simple_repeater/MyMesh.cpp | 23 +++--- scripts/client_acl_role.py | 43 +++++++++- src/helpers/RadioProfileCLI.cpp | 4 +- src/helpers/RadioProfileCLI.h | 6 +- test/test_client_acl_role.py | 30 +++++++ test/test_radio_profile_cli.py | 19 +++-- test/test_small_table_sort.py | 119 ++++++++++++++++++++++++++++ 7 files changed, 216 insertions(+), 28 deletions(-) diff --git a/examples/simple_repeater/MyMesh.cpp b/examples/simple_repeater/MyMesh.cpp index 5d770a7a2..00afb61de 100644 --- a/examples/simple_repeater/MyMesh.cpp +++ b/examples/simple_repeater/MyMesh.cpp @@ -60,9 +60,10 @@ mesh::FloodAdvertLimiter::Entry* repeaterRtcAdvertSlowBank() { #endif #include #include +#include #include #include // std::nothrow (heap-allocated flood rule table) -#include // for qsort() +#include #include #include #if defined(NRF52_POWER_MANAGEMENT) @@ -707,7 +708,7 @@ void MyMesh::sendDefaultPasswordWarning(ClientInfo* client, } } -// Comparison functions for qsort() - defined at file scope to avoid heap allocations +// Shared neighbour ordering; equally ranked entries have no guaranteed tie order. static int cmp_neighbours_newest_to_oldest(const void* a, const void* b) { const NeighbourInfo* na = *(const NeighbourInfo**)a; const NeighbourInfo* nb = *(const NeighbourInfo**)b; @@ -942,21 +943,20 @@ int MyMesh::handleRequest(ClientInfo *sender, uint32_t sender_timestamp, uint8_t } } - // Sort neighbours based on order using qsort() - standard C library function - // qsort() doesn't allocate heap memory (uses stack-based recursion) and is O(n log n) - // This matches the pattern used elsewhere in the codebase (e.g., BaseChatMesh) + // Share the bounded O(n log n) sorter instead of retaining separate C + // and C++ sort implementations on flash-constrained boards. if (order_by == 0) { // sort by newest to oldest - qsort(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_newest_to_oldest); + mesh::sortSmallTable(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_newest_to_oldest); } else if (order_by == 1) { // sort by oldest to newest - qsort(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_oldest_to_newest); + mesh::sortSmallTable(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_oldest_to_newest); } else if (order_by == 2) { // sort by strongest to weakest - qsort(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_strongest_to_weakest); + mesh::sortSmallTable(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_strongest_to_weakest); } else if (order_by == 3) { // sort by weakest to strongest - qsort(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_weakest_to_strongest); + mesh::sortSmallTable(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), cmp_neighbours_weakest_to_strongest); } // build results buffer @@ -6023,9 +6023,8 @@ void MyMesh::formatNeighborsReply(char *reply) { } // sort neighbours newest to oldest - std::sort(sorted_neighbours, sorted_neighbours + neighbours_count, [](const NeighbourInfo* a, const NeighbourInfo* b) { - return a->heard_timestamp > b->heard_timestamp; // desc - }); + mesh::sortSmallTable(sorted_neighbours, neighbours_count, sizeof(NeighbourInfo*), + cmp_neighbours_newest_to_oldest); for (int i = 0; i < neighbours_count && dp - reply < 134; i++) { NeighbourInfo *neighbour = sorted_neighbours[i]; diff --git a/scripts/client_acl_role.py b/scripts/client_acl_role.py index e7dc98786..995884297 100644 --- a/scripts/client_acl_role.py +++ b/scripts/client_acl_role.py @@ -1,12 +1,14 @@ -"""Use the smaller ClientInfo layout only for an effective repeater source role. +"""Derive feature-preserving role definitions from effective application sources. -One shared compiler definition covers application and helper translation units. +Shared compiler definitions cover application and helper translation units. Unknown/custom roles keep the complete layout; room and sensor session fields -are never removed from builds that use them. +are never removed from builds that use them. Companions omit only unbound +forwarding-region callbacks because that role has no RegionMap. """ import importlib.util from pathlib import Path +import re SCRIPT_DIR = Path(__file__).resolve().parent if "__file__" in globals() else Path("scripts").resolve() @@ -18,6 +20,39 @@ DEFINITION = "MESH_CLIENT_REPEATER_ONLY" REPEATER_SOURCES = ("../examples/simple_repeater/main.cpp", "../examples/simple_repeater/MyMesh.cpp") OTHER_ROLES = ("simple_room_server", "simple_sensor", "companion_radio", "simple_secure_chat", "kiss_modem", "partition_expander", "esp32_partition_migrator", "esp32_partition_legacy_seed") +COMPANION_DEFINITION = "MESH_ROLE_COMPANION" +COMPANION_SOURCES = ("../examples/companion_radio/main.cpp", "../examples/companion_radio/MyMesh.cpp") + + +def companion_role_policy(src_filter, flags=(), cppdefines=()): + """Exclude unbound forwarding-scope callbacks, retaining all radio commands. + + Companions have no RegionMap. Select the complete exclusive source role, + never an environment-name alias, so every translation unit shares its ABI. + Unknown and mixed roles retain the general callback implementation. + """ + values = SOURCE_POLICY.flag_defines(flags, cppdefines) + selected = SOURCE_POLICY.source_selected + other_roles = ("simple_repeater", *(role for role in OTHER_ROLES + if role != "companion_radio")) + filter_text = src_filter if isinstance(src_filter, str) else " ".join(map(str, src_filter)) + example_includes = re.findall(r"\+<\.\./examples/([^>]+)>", filter_text.replace("\\", "/")) + known_companion_only = all(path == "companion_radio" or path.startswith("companion_radio/") + for path in example_includes) + qualified = (known_companion_only + and all(selected(src_filter, source) for source in COMPANION_SOURCES) + and not any(selected(src_filter, "../examples/" + role + "/" + source) + for role in other_roles + for source in ("main.cpp", "MyMesh.cpp", "SensorMesh.cpp"))) + if COMPANION_DEFINITION in values: + value = values[COMPANION_DEFINITION] + text = "0" if value is None else str(value).strip("\"'() ") + if text not in ("0", "1"): + raise ValueError(COMPANION_DEFINITION + " override must be 0 or 1") + if text == "1" and not qualified: + raise ValueError(COMPANION_DEFINITION + "=1 requires only the Companion source role") + return {} + return {COMPANION_DEFINITION: 1} if qualified else {} def role_policy(src_filter, flags=(), cppdefines=()): @@ -46,6 +81,8 @@ def install(build_env): src_filter = build_env.GetProjectOption("build_src_filter", "") definitions = role_policy(src_filter, build_env.get("BUILD_FLAGS", ()), build_env.get("CPPDEFINES", ())) + definitions.update(companion_role_policy(src_filter, build_env.get("BUILD_FLAGS", ()), + build_env.get("CPPDEFINES", ()))) if definitions: build_env.AppendUnique(CPPDEFINES=list(definitions.items())) diff --git a/src/helpers/RadioProfileCLI.cpp b/src/helpers/RadioProfileCLI.cpp index 5127ab7ea..4b790912b 100644 --- a/src/helpers/RadioProfileCLI.cpp +++ b/src/helpers/RadioProfileCLI.cpp @@ -92,7 +92,7 @@ bool secondaryOverlaps(uint32_t primary_hz, float secondary_mhz, uint32_t cleara return (primary_hz > secondary_hz ? primary_hz - secondary_hz : secondary_hz - primary_hz) <= clearance_hz; } -#if !defined(COMPANION_RADIO_FULL) +#if !defined(COMPANION_RADIO_FULL) && !MESH_ROLE_COMPANION bool isMediaMeshProfile(const RadioProfileConfig& config) { return config.mode != RadioProfileMode::Off && config.params.freq >= 911.1495f && config.params.freq <= 911.1505f @@ -102,7 +102,7 @@ bool isMediaMeshProfile(const RadioProfileConfig& config) { #endif } -#if !defined(COMPANION_RADIO_FULL) +#if !defined(COMPANION_RADIO_FULL) && !MESH_ROLE_COMPANION void RadioProfileCLI::setMediaMeshScopeHandler(MediaMeshScopeHandler handler, void* context) { media_scope_handler_ = handler; media_scope_context_ = context; diff --git a/src/helpers/RadioProfileCLI.h b/src/helpers/RadioProfileCLI.h index 41e1b1dbb..5fe0bae40 100644 --- a/src/helpers/RadioProfileCLI.h +++ b/src/helpers/RadioProfileCLI.h @@ -61,7 +61,7 @@ class RadioProfileCLI { // readable backup. Read/I/O failures and an intact backup remain read-only. bool recoverable_corrupt_store_ = false; bool publish_pending_ = false; -#if !defined(COMPANION_RADIO_FULL) +#if !defined(COMPANION_RADIO_FULL) && !MESH_ROLE_COMPANION MediaMeshScopeHandler media_scope_handler_ = nullptr; void* media_scope_context_ = nullptr; uint32_t media_scope_retry_ms_ = 0; @@ -71,7 +71,7 @@ class RadioProfileCLI { void retryMediaMeshScope(); bool mediaMeshScopePending() const { return media_scope_pending_; } #else - // Full Companions have no forwarding RegionMap. Do not retain unreachable + // Companions have no forwarding RegionMap. Do not retain unreachable // infrastructure callbacks, state or failure text in their tight images. bool mediaMeshScopeReady(const RadioProfileConfig&) const { return true; } void updateMediaMeshScope() {} @@ -141,7 +141,7 @@ class RadioProfileCLI { #endif void begin(FILESYSTEM* fs, Radio* radio, RTCClock* rtc, bool infrastructure_replies = false); // Bind only after RegionMap::load(), so boot reconciliation is not discarded. -#if !defined(COMPANION_RADIO_FULL) +#if !defined(COMPANION_RADIO_FULL) && !MESH_ROLE_COMPANION void setMediaMeshScopeHandler(MediaMeshScopeHandler handler, void* context); #else void setMediaMeshScopeHandler(MediaMeshScopeHandler, void*) {} diff --git a/test/test_client_acl_role.py b/test/test_client_acl_role.py index 7f23fa0dc..46be48bdc 100644 --- a/test/test_client_acl_role.py +++ b/test/test_client_acl_role.py @@ -19,6 +19,33 @@ FIXTURE = ROOT / "test/fixtures/client_acl_spiffs" class ClientAclRoleTests(unittest.TestCase): + def test_complete_exclusive_companion_role_uses_one_global_definition(self): + definition = POLICY.COMPANION_DEFINITION + for role in ("+<../examples/companion_radio>", + "+<../examples/companion_radio/*.cpp>", + ["+<*.cpp>", "+<..\\examples\\companion_radio\\*.cpp>"]): + self.assertEqual(POLICY.companion_role_policy(role), {definition: 1}) + for source in ("", "+<*>", "+<../examples/companion_radio/main.cpp>", + "+<../examples/companion_radio> -<../examples/companion_radio/MyMesh.cpp>", + "+<../examples/companion_radio> +<../examples/custom_role>", + "+<../examples/simple_repeater>"): + self.assertEqual(POLICY.companion_role_policy(source), {}) + for other in ("simple_repeater", *POLICY.OTHER_ROLES): + if other == "companion_radio": + continue + self.assertEqual(POLICY.companion_role_policy( + "+<../examples/companion_radio> +<../examples/" + other + ">"), {}) + role = "+<../examples/companion_radio>" + for flag in ("-D" + definition + "=0", "-D" + definition + "=1", "-U" + definition): + self.assertEqual(POLICY.companion_role_policy(role, flag), {}) + self.assertEqual(POLICY.companion_role_policy(role, cppdefines=[(definition, 1)]), {}) + for value in ("true", "2", "-1"): + with self.assertRaises(ValueError): + POLICY.companion_role_policy(role, "-D" + definition + "=" + value) + with self.assertRaises(ValueError): + POLICY.companion_role_policy("+<../examples/simple_repeater>", + "-D" + definition + "=1") + def test_only_complete_repeater_role_qualifies(self): for value in ("+<../examples/simple_repeater>", "+<../examples/simple_repeater/*.cpp>", ["+<*.cpp>", "+<..\\examples\\simple_repeater\\*.cpp>"]): @@ -57,6 +84,9 @@ class ClientAclRoleTests(unittest.TestCase): del env["appended"] POLICY.install(env) self.assertNotIn("appended", env) + env["SRC_FILTER"] = "+<../examples/companion_radio/*.cpp>" + POLICY.install(env) + self.assertEqual(env["appended"], {"CPPDEFINES": [(POLICY.COMPANION_DEFINITION, 1)]}) def test_every_platform_base_installs_role_hook(self): ini = (ROOT / "platformio.ini").read_text() diff --git a/test/test_radio_profile_cli.py b/test/test_radio_profile_cli.py index 01d6d49b6..4166d7512 100644 --- a/test/test_radio_profile_cli.py +++ b/test/test_radio_profile_cli.py @@ -9,7 +9,7 @@ import unittest ROOT = Path(__file__).resolve().parents[1] class RadioProfileCLITest(unittest.TestCase): - def test_full_companion_keeps_rf_presets_without_unused_region_code(self): + def test_all_companions_keep_rf_presets_without_unused_region_code(self): compiler = shutil.which('g++') or shutil.which('clang++') self.assertIsNotNone(compiler) code = r''' @@ -23,7 +23,7 @@ int main() { f.cmd("set radio2 mediamesh"); f.advance(2000); assert(f.radio.p.enabled() && fabsf(f.radio.p.secondary.params.freq - 911.150f) < 0.0001f); -#if defined(COMPANION_RADIO_FULL) +#if defined(COMPANION_RADIO_FULL) || MESH_ROLE_COMPANION assert(!scope.commits && !scope.checks); #else assert(scope.enabled && scope.durable && scope.commits); @@ -37,9 +37,11 @@ int main() { with tempfile.TemporaryDirectory(prefix='profile-region-role-') as work: source = Path(work)/'role.cpp' source.write_text(code, encoding='ascii') - for full in (False, True): - binary = Path(work)/('companion' if full else 'infrastructure') - flags = ['-DCOMPANION_RADIO_FULL=1'] if full else [] + roles = {'infrastructure': [], 'full': ['-DCOMPANION_RADIO_FULL=1'], + 'ordinary': ['-DMESH_ROLE_COMPANION=1']} + for role, flags in roles.items(): + companion = role != 'infrastructure' + binary = Path(work)/role compiled = subprocess.run([ compiler, '-std=c++17', '-Os', '-ffunction-sections', '-fdata-sections', '-Wl,--gc-sections', @@ -53,11 +55,12 @@ int main() { self.assertEqual(compiled.returncode, 0, compiled.stderr) run = subprocess.run([str(binary)], capture_output=True, text=True, timeout=10) self.assertEqual(run.returncode, 0, run.stdout+run.stderr) - sizes[full] = int(run.stdout) + sizes[role] = int(run.stdout) image = binary.read_bytes() for marker in (b'MediaMesh scope save pending', b'MediaMesh region unavailable or full'): - self.assertEqual(marker in image, not full) - self.assertLess(sizes[True], sizes[False]) + self.assertEqual(marker in image, not companion) + for role in ('full', 'ordinary'): + self.assertLess(sizes[role], sizes['infrastructure']) def test_commands_persistence_and_timers(self): compiler = shutil.which('g++') or shutil.which('clang++') diff --git a/test/test_small_table_sort.py b/test/test_small_table_sort.py index de868fb17..91b73f58c 100644 --- a/test/test_small_table_sort.py +++ b/test/test_small_table_sort.py @@ -8,6 +8,8 @@ import sys import tempfile import unittest +from cpp_source import body + ROOT = Path(__file__).resolve().parents[1] HARNESS = r''' @@ -140,8 +142,125 @@ void sortElsewhere(void* items, size_t count, size_t width, } ''' +NEIGHBOUR_HARNESS = r''' +#include +#include +#include +#include +#include +#include +#include +#include +#include +struct NeighbourInfo { + uint32_t heard_timestamp; + int8_t snr; + uint16_t identity; +}; +@COMPARATORS@ +static void rpcSort(NeighbourInfo** sorted_neighbours, int16_t neighbours_count, + uint8_t order_by) { + @RPC_SORT@ +} +static void cliSort(NeighbourInfo** sorted_neighbours, int16_t neighbours_count) { + @CLI_SORT@ +} +static bool before(const NeighbourInfo* a, const NeighbourInfo* b, unsigned order) { + if (order == 0) return a->heard_timestamp > b->heard_timestamp; + if (order == 1) return a->heard_timestamp < b->heard_timestamp; + if (order == 2) return a->snr > b->snr; + return a->snr < b->snr; +} +int main() { + std::mt19937 random(0x18180002); + const uint32_t timestamps[] = {1, 42, 42, UINT32_MAX, 1, UINT32_MAX - 1}; + const int8_t snrs[] = {INT8_MIN, -16, -1, 0, 0, 7, 7, INT8_MAX}; + int (*const compare[])(const void*, const void*) = { + cmp_neighbours_newest_to_oldest, cmp_neighbours_oldest_to_newest, + cmp_neighbours_strongest_to_weakest, cmp_neighbours_weakest_to_strongest + }; + for (size_t count : {0U, 1U, 2U, 16U, 50U, 100U}) { + std::vector records(count); + for (size_t i = 0; i < count; ++i) { + records[i].heard_timestamp = timestamps[i % 6]; + records[i].snr = snrs[i % 8]; + records[i].identity = static_cast(i); + } + const auto original = records; + for (unsigned order = 0; order < 4; ++order) { + std::vector actual(count), expected(count); + for (size_t i = 0; i < count; ++i) actual[i] = &records[i]; + std::shuffle(actual.begin(), actual.end(), random); + expected = actual; + std::sort(expected.begin(), expected.end(), [order](const NeighbourInfo* a, + const NeighbourInfo* b) { + return before(a, b, order); + }); + rpcSort(actual.data(), static_cast(count), order); + std::vector seen(count); + for (size_t i = 0; i < count; ++i) { + const auto index = actual[i]->identity; + assert(index < count && !seen[index] && actual[i] == &records[index]); + seen[index] = true; + assert(!before(actual[i], expected[i], order)); + assert(!before(expected[i], actual[i], order)); + assert(compare[order](&actual[i], &expected[i]) == 0); + if (i) assert(!before(actual[i], actual[i - 1], order)); + } + // Unsupported order values retain the caller's original pointer order. + const auto unchanged = actual; + rpcSort(actual.data(), static_cast(count), 4); + assert(actual == unchanged); + cliSort(actual.data(), static_cast(count)); + for (size_t i = 1; i < count; ++i) { + assert(actual[i - 1]->heard_timestamp >= actual[i]->heard_timestamp); + } + for (size_t i = 0; i < count; ++i) { + assert(records[i].heard_timestamp == original[i].heard_timestamp); + assert(records[i].snr == original[i].snr); + assert(records[i].identity == original[i].identity); + } + } + } + puts("PASS: real neighbour RPC orders and CLI, duplicate keys, negative SNR and pointer records"); +} +''' + class SmallTableSortTests(unittest.TestCase): + def test_real_neighbour_rpc_and_cli_orderings(self): + compiler = os.environ.get("CXX") or shutil.which("g++") or shutil.which("clang++") + self.assertIsNotNone(compiler, "a host C++17 compiler is required") + production = (ROOT / "examples/simple_repeater/MyMesh.cpp").read_text() + names = ("newest_to_oldest", "oldest_to_newest", + "strongest_to_weakest", "weakest_to_strongest") + comparators = "\n".join(body(production, "static int cmp_neighbours_" + name + "(") + for name in names) + start = production.index("if (order_by == 0) {") + rpc_sort = production[start:production.index("// build results buffer", start)] + formatter = body(production, "void MyMesh::formatNeighborsReply(") + start = formatter.index("// sort neighbours newest to oldest") + cli_sort = formatter[start:formatter.index("for (int i = 0;", start)] + source = NEIGHBOUR_HARNESS.replace("@COMPARATORS@", comparators) + source = source.replace("@RPC_SORT@", rpc_sort).replace("@CLI_SORT@", cli_sort) + for platform in ("STM32_PLATFORM", "NRF52_PLATFORM", "ESP32_PLATFORM", "RP2040_PLATFORM"): + with self.subTest(platform=platform), tempfile.TemporaryDirectory( + prefix="neighbour-sort-") as directory: + work = Path(directory) + (work / "test.cpp").write_text(source) + binary = work / "sort" + command = [compiler, "-std=c++17", "-O2", "-Wall", "-Wextra", "-Werror", + "-D" + platform + "=1", "-I" + str(ROOT / "src"), + str(work / "test.cpp"), "-o", str(binary)] + if sys.platform.startswith("linux"): + command[1:1] = ["-fsanitize=address,undefined", "-fno-sanitize-recover=all", + "-fno-pie", "-no-pie"] + built = subprocess.run(command, capture_output=True, text=True, timeout=60) + self.assertEqual(built.returncode, 0, built.stdout + built.stderr) + tested = subprocess.run([str(binary)], capture_output=True, text=True, timeout=10) + self.assertEqual(tested.returncode, 0, tested.stdout + tested.stderr) + self.assertIn("negative SNR and pointer records", tested.stdout) + def test_sort_on_all_platforms(self): compiler = os.environ.get("CXX") or shutil.which("g++") or shutil.which("clang++") self.assertIsNotNone(compiler, "a host C++17 compiler is required")