diff --git a/docs/radio_profiles.md b/docs/radio_profiles.md index 611631ca3..1113da913 100644 --- a/docs/radio_profiles.md +++ b/docs/radio_profiles.md @@ -71,10 +71,15 @@ copies finish, with at least one successfully transmitted. A reply that cannot be queued or whose copies all fail cancels the change. Queued replies have a five-minute deadline; an already-transmitting copy may finish under the normal radio watchdog. This confirms transmission, not reception by the remote client. +An active temporary profile keeps its radio generation until those tracked +reply copies finish or are retired, but its remaining lease continues counting +down during that wait. Permanent remote changes are prepared in an uncommitted file before replying; the saved configuration is replaced only after reply transmission. If that -replacement fails, the old channel remains active and the commit retries. +replacement fails, the requested settings remain pending and the commit retries. +An expired temporary profile returns to the last saved configuration during +those post-reply storage retries; the uncommitted settings do not become active. Local `get radio2.status` reports this pending state. Roles without the tracked remote reply path require local USB for these immediate secondary-profile changes. diff --git a/src/helpers/RadioProfileCLI.cpp b/src/helpers/RadioProfileCLI.cpp index a944bc16f..2f014bc2e 100644 --- a/src/helpers/RadioProfileCLI.cpp +++ b/src/helpers/RadioProfileCLI.cpp @@ -283,11 +283,9 @@ void RadioProfileCLI::publish() { if (external_secondary_override_) return; #endif if (publish_pending_ && (int32_t)(millis() - publish_after_ms_) < 0) return; -#if MESH_ENABLE_MESHTEXT_CHAT || MESH_ENABLE_ETHERNET_TUNNEL - // An accepted remote command owns the old RX generation until its reply has - // completed (or its atomic storage commit has succeeded). - if (hasReplyMutation()) return; -#endif + // Reply packets own the old RX generation until they finish. A post-ACK + // storage retry must not extend an expired temporary lease. + if (hasReplyMutation() && !remote_delivered_) return; publish_pending_ = false; RadioProfileConfig config = temp_active_ ? temporary_ : saved_; #if MESH_ENABLE_MESHTEXT_CHAT || MESH_ENABLE_ETHERNET_TUNNEL @@ -547,9 +545,9 @@ void RadioProfileCLI::loop() { last_ms_ = now; remote_commit_retry_ms_ = elapsed >= remote_commit_retry_ms_ ? 0 : remote_commit_retry_ms_ - elapsed; if (remote_delivered_ && !remote_commit_retry_ms_ && !applyReplyMutation()) { - // Keep the accepted operation pending and the old channel live if the - // post-ACK atomic rename fails. Never report/apparently apply an unsaved - // tuple, and avoid a hot storage retry loop. + // Keep the accepted operation pending if the post-ACK atomic rename fails. + // Saved intent stays authoritative while expired leases can restore it; + // never apply an unsaved tuple or retry storage in a hot loop. remote_commit_retry_ms_ = 1000; } if (!hasReplyMutation() && temp_pending_ && (int32_t)(now - temp_start_ms_) >= 0) { diff --git a/test/test_radio_reply_mutation.py b/test/test_radio_reply_mutation.py index cc86e0ae5..6fc29be1b 100644 --- a/test/test_radio_reply_mutation.py +++ b/test/test_radio_reply_mutation.py @@ -156,6 +156,164 @@ int main() { ''' +LEASE_REPLY = r''' +// Reuse the memory filesystem/radio fixture with the production Dispatcher. +#define main existing_cli_main +#include "test/fixtures/radio_profiles/cli_test.cpp" +#undef main +#include + +class LeaseMillis : public mesh::MillisecondClock { + public: + unsigned long getMillis() override { return g_mock_millis; } +}; +class LeaseDispatcher : public mesh::Dispatcher { + public: + LeaseDispatcher(mesh::Radio& radio, mesh::MillisecondClock& clock, + mesh::PacketManager& manager) : Dispatcher(radio, clock, manager) {} + mesh::DispatcherAction onRecvPacket(mesh::Packet*) override { return ACTION_RELEASE; } + using Dispatcher::isPacketRadioCurrent; +}; + +static void storageRetryLeases() { + unsigned scenarios = 0; + for (bool temporary : {false, true}) for (bool off : {false, true}) + for (bool late_ack : {false, true}) for (bool cancel : {false, true}) + for (bool rollover : {false, true}) { + if (!temporary && late_ack) continue; + Fixture f(true); + f.cmd("set radio2 910.5,500,8,5,rxtx"); f.advance(2000, false); + const auto saved = f.fs.files["/radio_profiles"]; + if (rollover) { g_mock_millis = UINT32_MAX - 30000; f.cli.loop(); } + if (temporary) { + f.cmd("set tempradio2 911.5,500,8,5,rxtx,1"); f.advance(2000, false); + f.advance(59000, false); + } + const uint32_t generation = f.radio.p.generation[1]; + f.cli.beginReplyCommand(); + f.cmd(off ? "set radio2 off" : "set radio2 912.5,500,8,5,rxtx"); + f.cli.endReplyCommand(); + // Fail every final rename, while allowing the previous saved image to roll back. + f.fs.fail_rename_from.push_back("/radio_profiles.reply"); + if (late_ack) { + f.advance(1000, false); + assert(f.cli.secondaryRemainingSeconds() == 0); + assert(f.radio.p.secondary_temporary && f.radio.p.generation[1] == generation); + } + assert(f.cli.finishReplyMutation(true)); + assert(f.radio.p.generation[1] == generation); // callback cannot retune + f.advance(1, false); + assert(f.cli.hasReplyMutation()); + assert(f.radio.p.secondary_temporary == (temporary && !late_ack)); + assert(f.radio.p.secondary.params.freq == (temporary && !late_ack ? 911.5f : 910.5f)); + if (temporary && !late_ack) { + assert(f.cli.secondaryRemainingSeconds() == 1); + f.advance(999, false); + } + assert(!f.radio.p.secondary_temporary && f.radio.p.secondary.params.freq == 910.5f); + assert(f.radio.p.generation[1] == generation + temporary); + + // Persistent storage faults keep the mutation pending, but cannot extend the lease. + for (unsigned minute = 0; minute < 120; ++minute) { + f.advance(60000, false); + assert(f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary); + assert(f.radio.p.secondary.params.freq == 910.5f); + assert(f.cli.savedSecondaryConfig().params.freq == 910.5f); + assert(f.fs.files["/radio_profiles"] == saved); + } + assert(strstr(f.cmd("get radio2.status"), "storage commit (retrying)")); + assert(strstr(f.cmd("set radio2 off", false), "acknowledgement pending")); + const uint32_t restored_generation = f.radio.p.generation[1]; + if (cancel) assert(f.cli.finishReplyMutation(false)); + else f.fs.fail_rename_from.clear(); + f.advance(1000, false); + assert(!f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary); + const auto expected_mode = !cancel && off ? mesh::RadioProfileMode::Off : mesh::RadioProfileMode::RxTx; + const float expected_freq = cancel ? 910.5f : off ? 0 : 912.5f; + assert(f.radio.p.secondary.mode == expected_mode && f.radio.p.secondary.params.freq == expected_freq); + assert(f.radio.p.generation[1] == restored_generation + !cancel); + assert((f.fs.files["/radio_profiles"] == saved) == cancel); + Radio reboot; mesh::RadioProfileCLI restored; + restored.begin(&f.fs, &reboot, &f.clock, true); + assert(reboot.p.secondary.mode == expected_mode && reboot.p.secondary.params.freq == expected_freq); + ++scenarios; + } + std::printf("Storage retry leases: chat=%d ethernet=%d, %u scenarios passed\n", + MESH_ENABLE_MESHTEXT_CHAT, MESH_ENABLE_ETHERNET_TUNNEL, scenarios); +} + +int main() { + unsigned scenarios = 0; + for (bool delivered : {false, true}) for (bool rollover : {false, true}) { + Fixture f(true); + f.cmd("set radio2 910.5,500,8,5,rxtx"); f.advance(2000, false); + const auto saved = f.fs.files["/radio_profiles"]; + if (rollover) { g_mock_millis = UINT32_MAX - 30000; f.cli.loop(); } + f.cmd("set tempradio2 911.5,500,8,5,rxtx,1"); f.advance(2000, false); + f.advance(59000, false); + assert(f.radio.p.secondary_temporary && f.cli.secondaryRemainingSeconds() == 1); + const uint32_t generation = f.radio.p.generation[1]; + + f.cli.beginReplyCommand(); + f.cmd("set radio2 912.5,500,8,5,rxtx"); + f.cli.endReplyCommand(); + assert(f.cli.hasReplyMutation()); + LeaseMillis ms; + StaticPoolPacketManager manager(4); + LeaseDispatcher dispatcher(f.radio, ms, manager); + dispatcher.begin(); + mesh::Packet* reply = dispatcher.obtainNewPacket(); assert(reply); + reply->header = ROUTE_TYPE_DIRECT | (PAYLOAD_TYPE_RESPONSE << PH_TYPE_SHIFT); + reply->payload_len = 1; reply->payload[0] = 42; + reply->radio_profile = 1; reply->radio_generation = generation; + reply->radio_local = false; reply->radio_reply = true; + assert(dispatcher.sendPacket(reply, 0, 5000)); + assert(manager.getOutboundTotal() == 2); + mesh::Packet* replies[2] = {manager.getOutboundByIdx(0), manager.getOutboundByIdx(1)}; + uint8_t targets = 0; + for (auto* packet : replies) { + assert(packet->radio_bound && packet->radio_origin == 1); + assert(packet->radio_origin_generation == generation); + assert(dispatcher.isPacketRadioCurrent(packet)); + targets |= 1U << packet->radio_profile; + } + assert(targets == 3); + + // The lease expires while both acknowledgement copies still own the old generation. + for (uint32_t elapsed : {1000U, 10000U}) { + f.advance(elapsed, false); + assert(f.cli.hasReplyMutation() && f.cli.secondaryRemainingSeconds() == 0); + for (auto* packet : replies) assert(dispatcher.isPacketRadioCurrent(packet)); + assert(f.radio.p.generation[1] == generation && f.radio.p.secondary_temporary); + assert(f.radio.p.secondary.params.freq == 911.5f); + assert(f.fs.files["/radio_profiles"] == saved); + } + + // Completion/cancellation cannot publish while Dispatcher is in its callback. + assert(manager.removeOutboundByIdx(0) == replies[0]); + assert(manager.removeOutboundByIdx(0) == replies[1]); + assert(f.cli.finishReplyMutation(delivered)); + assert(f.radio.p.generation[1] == generation); + for (auto* packet : replies) assert(dispatcher.isPacketRadioCurrent(packet)); + f.advance(1, false); + assert(!f.cli.hasReplyMutation() && !f.radio.p.secondary_temporary); + assert(f.radio.p.generation[1] == generation + 1); + assert(f.radio.p.secondary.params.freq == (delivered ? 912.5f : 910.5f)); + assert((f.fs.files["/radio_profiles"] == saved) == !delivered); + for (auto* packet : replies) { + assert(!dispatcher.isPacketRadioCurrent(packet)); + dispatcher.releasePacket(packet); + } + assert(manager.getFreeCount() == 4); + ++scenarios; + } + std::printf("Lease reply ownership: chat=%d ethernet=%d, %u scenarios passed\n", + MESH_ENABLE_MESHTEXT_CHAT, MESH_ENABLE_ETHERNET_TUNNEL, scenarios); + storageRetryLeases(); +} +''' + + class RadioReplyMutationTest(unittest.TestCase): @staticmethod def compiler_flags(): @@ -185,6 +343,30 @@ class RadioReplyMutationTest(unittest.TestCase): env['ASAN_OPTIONS'] = env.get('ASAN_OPTIONS', '') + ':detect_leaks=0' subprocess.run([str(exe)], check=True, env=env) + def test_temporary_lease_expiry_preserves_reply_generations(self): + compiler = shutil.which('g++') or shutil.which('clang++') + self.assertIsNotNone(compiler) + with tempfile.TemporaryDirectory() as work: + cpp = Path(work) / 'lease_reply.cpp' + cpp.write_text(LEASE_REPLY, encoding='ascii') + env = os.environ.copy() + if os.name != 'nt': + env['ASAN_OPTIONS'] = env.get('ASAN_OPTIONS', '') + ':detect_leaks=0' + for chat, ethernet in ((0, 0), (1, 0), (0, 1)): + with self.subTest(chat=chat, ethernet=ethernet): + exe = Path(work) / f'lease_reply-{chat}-{ethernet}.exe' + result = subprocess.run([compiler, *self.compiler_flags(), + f'-DMESH_ENABLE_MESHTEXT_CHAT={chat}', + f'-DMESH_ENABLE_ETHERNET_TUNNEL={ethernet}', + '-I', str(ROOT/'test/fixtures/radio_profiles/mocks'), + '-I', str(ROOT/'test/mocks'), '-I', str(ROOT/'src'), '-I', str(ROOT), + str(ROOT/'src/helpers/RadioProfileCLI.cpp'), + str(ROOT/'src/Dispatcher.cpp'), str(ROOT/'src/Packet.cpp'), + str(ROOT/'src/helpers/StaticPoolPacketManager.cpp'), str(cpp), + '-o', str(exe)], capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr[-14000:]) + subprocess.run([str(exe)], check=True, env=env) + def test_production_repeater_ack_callbacks_and_deadline(self): source = (ROOT/'examples/simple_repeater/MyMesh.cpp').read_text(encoding='utf-8') methods = '\n'.join(method(source, signature) for signature in (