diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp index c30bd9ae..5cbde8fb 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp @@ -658,14 +658,23 @@ void ChatScreen::on_send_clicked(lv_event_t* event) { String message(text); if (message.length() > 0 && screen->_send_message_callback) { - if (screen->_send_message_callback(message)) { - // Clear only after persistence and queue admission succeed. - lv_textarea_set_text(screen->_text_area, ""); - lv_group_focus_obj(screen->_text_area); - } + // Publish to the main loop; the composer is cleared only after + // persistence and queue admission succeed (clear_composer() from + // UIManager::apply_outbound_result). A rejected send (busy router, + // full queue, storage error) keeps the text for a normal re-send, so + // no typed input is ever lost to a failed send. + screen->_send_message_callback(message); } } +void ChatScreen::clear_composer() { + // Recursive lock: apply_outbound_result() calls this while already + // holding the LVGL lock. + LVGL_LOCK(); + lv_textarea_set_text(_text_area, ""); + lv_group_focus_obj(_text_area); +} + void ChatScreen::format_timestamp(double timestamp, char* buf, size_t buf_size) { // Convert to time_t for formatting time_t time = (time_t)timestamp; diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.h b/lib/tdeck_ui/UI/LXMF/ChatScreen.h index 6a9b5e66..2cbfd206 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.h +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.h @@ -90,6 +90,15 @@ public: */ void add_message(const ::LXMF::LXMessage& message, bool outgoing); + /** + * Clear the composer text input and refocus it. Called from the main + * loop once an outgoing send has been persisted and admitted (the LVGL + * send callback no longer clears synchronously — it defers to this). + * Takes the LVGL lock internally; call with the lock already held + * (recursive) or from the main loop before UI work. + */ + void clear_composer(); + /** * Update delivery status of a message * @param message_hash Hash of message to update diff --git a/lib/tdeck_ui/UI/LXMF/ComposeScreen.cpp b/lib/tdeck_ui/UI/LXMF/ComposeScreen.cpp index 2f546471..f64b2207 100644 --- a/lib/tdeck_ui/UI/LXMF/ComposeScreen.cpp +++ b/lib/tdeck_ui/UI/LXMF/ComposeScreen.cpp @@ -290,8 +290,10 @@ void ComposeScreen::on_send_clicked(lv_event_t* event) { dest_hash.assignHex(dest_hash_str.c_str()); if (screen->_send_callback && screen->_send_callback(dest_hash, message)) { - // Clear only after persistence and queue admission succeed. - screen->clear(); + // Clearing is deferred to the main loop: on acceptance the Compose + // route is replaced with Chat (render_route re-clears the form on + // next entry), so a rejected send leaves both fields intact for a + // normal re-send. } } diff --git a/lib/tdeck_ui/UI/LXMF/OutgoingSendMailbox.h b/lib/tdeck_ui/UI/LXMF/OutgoingSendMailbox.h new file mode 100644 index 00000000..026d6edd --- /dev/null +++ b/lib/tdeck_ui/UI/LXMF/OutgoingSendMailbox.h @@ -0,0 +1,75 @@ +// Copyright (c) 2024 microReticulum contributors +// SPDX-License-Identifier: MIT + +#ifndef UI_LXMF_OUTGOINGSENDBOARD_H +#define UI_LXMF_OUTGOINGSENDBOARD_H + +#include +#include +#include +#include + +namespace UI { +namespace LXMF { + +// Single-slot handoff for one outgoing message from the LVGL task to the +// Arduino main loop. The LVGL task only publishes (destination, content, +// source); the main loop performs identity recall, signing/pack, the +// RouterLock-scoped router admission, and the LittleFS persistence there, so +// a multi-second save can never hold the LVGL mutex past the 5s deadlock +// guard (LVGLLock.h:45) and trip the assert. The user's input is retained on +// the screen until the main loop confirms acceptance (see +// UIManager::apply_outbound_result). +// +// One producer (LVGL task), one consumer (main loop). The mutex covers a +// small copy (strings are moved out on take). A second send while one is +// pending is rejected and its input is retained for retry — the same UX as +// the router-busy path. +class OutgoingSendMailbox { +public: + enum class Source : uint8_t { + None, + Chat, + Compose, + }; + + struct Slot { + Source source = Source::None; + std::string destination; // raw peer-hash bytes (binary-safe) + std::string content; // UTF-8 message text + }; + + // Returns false when a send is already pending (caller retains input). + bool request(Source source, const void* destination, size_t destinationSize, + const char* content, size_t contentSize) { + std::lock_guard guard(_mutex); + if (_slot.source != Source::None) return false; + _slot.source = source; + _slot.destination.assign(static_cast(destination), destinationSize); + _slot.content.assign(content, contentSize); + return true; + } + + // Returns false when nothing is pending; the slot is consumed on success. + bool take(Slot& slot) { + std::lock_guard guard(_mutex); + if (_slot.source == Source::None) return false; + slot = std::move(_slot); + _slot = Slot{}; + return true; + } + + bool hasPending() const { + std::lock_guard guard(_mutex); + return _slot.source != Source::None; + } + +private: + mutable std::mutex _mutex; + Slot _slot{}; +}; + +} // namespace LXMF +} // namespace UI + +#endif // UI_LXMF_OUTGOINGSENDBOARD_H diff --git a/lib/tdeck_ui/UI/LXMF/UIManager.cpp b/lib/tdeck_ui/UI/LXMF/UIManager.cpp index fbab7761..eb633f31 100644 --- a/lib/tdeck_ui/UI/LXMF/UIManager.cpp +++ b/lib/tdeck_ui/UI/LXMF/UIManager.cpp @@ -726,6 +726,11 @@ void UIManager::update() { // Settings Save only publishes a snapshot from the LVGL event. Persistence // and interface changes execute here on the main owner loop, before LVGL. if (_settings_screen) _settings_screen->service_pending_save(); + // Outgoing sends only publish (destination, content, source) from the LVGL + // event; identity recall, pack/sign, RouterLock admission, and the + // LittleFS persistence execute here, before LVGL_LOCK. A multi-second + // save can therefore never hold the render lock (LVGLLock.h:45 guard). + service_pending_sends(); // Flush display-name write-throughs the last conversation-list refresh // deferred. Done here, BEFORE LVGL_LOCK, so the microStore/LittleFS I/O // never runs under the render lock (same reason as on_message_received). @@ -1442,10 +1447,10 @@ void UIManager::on_location_from_chat() { bool UIManager::on_send_message_from_compose(const Bytes& dest_hash, const String& message) { if (!send_message(dest_hash, message)) return false; - // Replace Compose with Chat so Back returns to Messages instead of - // reopening a cleared compose form. - _current_peer_hash = dest_hash; - replace_route(Route::CHAT); + // Clearing the compose form and replacing it with Chat are deferred to + // apply_outbound_result() (COMPOSE_NAVIGATED) once the main loop + // confirms persistence + admission. A rejected send keeps the user on + // the compose screen with both fields intact for a normal re-send. return true; } @@ -1573,23 +1578,54 @@ void UIManager::set_rns_status(bool connected, const String& server_name) { } bool UIManager::send_message(const Bytes& dest_hash, const String& content) { - std::string hash_hex = dest_hash.toHex().substr(0, 8); - std::string msg = "Sending message to " + hash_hex + "..."; - INFO(msg.c_str()); + if (dest_hash.size() == 0 || content.length() == 0) return false; - // Pre-graft: Identity::mark_persistent(dest_hash) — fork-only API for - // the 5s fast-flush semantics. Vanilla upstream relies on microStore's - // dirty-tracking + reticulum->should_persist_data() to decide what - // gets written. If we observe lost contacts after crashes, revisit - // microStore flush cadence rather than re-adding the fork API. - // (void)Identity::mark_persistent(dest_hash); + std::string hash_hex = dest_hash.toHex().substr(0, 8); + INFO(("Sending message to " + hash_hex + "...").c_str()); + + // This runs on LVGL's 8 KiB task under the LVGL lock, so it must stay + // allocation-light and lock-free: no identity recall, no message + // construction, no router lock or admission. The canonical build, + // signing/pack, router admission, and LittleFS persistence all execute + // in service_pending_sends() on the main loop. + // + // While this work used to run here, a multi-second save held the LVGL + // mutex past the 5s deadlock guard and rebooted the device (assert at + // LVGLLock.h:45) — the same failure the receive path already fixed + // (see on_message_received). + const OutgoingSendMailbox::Source source_kind = + (_navigation.current() == Route::COMPOSE) + ? OutgoingSendMailbox::Source::Compose + : OutgoingSendMailbox::Source::Chat; + if (!_outgoing_sends.request(source_kind, + dest_hash.data(), dest_hash.size(), + content.c_str(), content.length())) { + WARNING("Outgoing send already pending; message retained for retry"); + return false; + } + + INFO(" Outgoing message handed to main loop"); + return true; +} + +void UIManager::service_pending_sends() { + // Runs in update() BEFORE the big LVGL_LOCK(), alongside the other + // mailbox servicing (settings save, conversation-list flushes, call + // starts). It is the only place in the send path that may take the + // router lock, block on router admission, or wait on LittleFS — + // mirroring on_message_received()'s receive-path discipline ("Don't + // take LVGL_LOCK across the LittleFS write"). + OutgoingSendMailbox::Slot slot; + if (!_outgoing_sends.take(slot)) return; + + Bytes dest_hash(slot.destination.data(), slot.destination.size()); + Bytes content_bytes((const uint8_t*)slot.content.data(), slot.content.size()); + const OutboundSource source = (slot.source == OutgoingSendMailbox::Source::Compose) + ? OutboundSource::COMPOSE + : OutboundSource::CHAT; // Get our source destination (needed for signing) - Destination source = _router.delivery_destination(); - - // Create message content - Bytes content_bytes((const uint8_t*)content.c_str(), content.length()); - Bytes title; // Empty title + Destination source_dest = _router.delivery_destination(); // Look up destination identity Identity dest_identity = Identity::recall(dest_hash); @@ -1605,13 +1641,13 @@ bool UIManager::send_message(const Bytes& dest_hash, const String& content) { // UI messages prefer single-packet opportunistic delivery on LoRa. The // router automatically promotes messages that exceed the LoRa packet MDU - // to DIRECT, so this preserves large-message support without forcing every - // short message through the heavier link/resource path. + // to DIRECT, so this preserves large-message support without forcing + // every short message through the heavier link/resource path. ::LXMF::LXMessage message( destination, - source, + source_dest, content_bytes, - title, + Bytes{}, ::LXMF::Type::Message::OPPORTUNISTIC ); @@ -1621,19 +1657,19 @@ bool UIManager::send_message(const Bytes& dest_hash, const String& content) { DEBUG(" Set destination hash manually"); } - // Pack the message to generate hash and signature before saving - message.pack(); - // Reject router contention or queue exhaustion before persistence. The // admission guard commits the final packed/stamped message immediately // before queue ownership transfer while RouterLock prevents a concurrent - // producer from consuming the checked capacity. This callback runs on - // LVGL's 8 KiB task, so MessageStore keeps its rollback snapshot - // object-owned rather than local to save_message(). + // producer from consuming the checked capacity. RouterLock router_lock(0); + OutboundCommit commit; + commit.source = source; + commit.dest_hash = dest_hash; if (!router_lock.acquired()) { WARNING("Router busy; outgoing message retained for retry"); - return false; + commit.result = OutboundResult::RETRY; + apply_outbound_result(commit); + return; } OutboundPersistenceContext persistence_context{&_store, &message}; @@ -1643,25 +1679,79 @@ bool UIManager::send_message(const Bytes& dest_hash, const String& content) { message, persistOutgoingMessage, &persistence_context); } catch (const std::exception& error) { WARNINGF("Outgoing message preparation failed: %s", error.what()); - return false; + admission = ::LXMF::OutboundAdmissionResult::QUEUE_FULL; } if (admission == ::LXMF::OutboundAdmissionResult::GUARD_REJECTED) { ERROR("Outgoing message persistence failed; message not queued"); - show_storage_error("Storage is unavailable. The message was not sent."); - return false; - } - if (admission != ::LXMF::OutboundAdmissionResult::ACCEPTED) { + commit.result = OutboundResult::STORAGE_ERROR; + } else if (admission != ::LXMF::OutboundAdmissionResult::ACCEPTED) { WARNING("Outbound queue full; outgoing message retained for retry"); - return false; + commit.result = OutboundResult::RETRY; + } else { + INFO(" Message queued for delivery"); + commit.result = (source == OutboundSource::COMPOSE) + ? OutboundResult::COMPOSE_NAVIGATED + : OutboundResult::ADDED; + // The admitted, packed form (post-stamp) is what the router will + // send and what delivery callbacks will address; retain it for the + // UI commit. + commit.packed = message.packed(); } + apply_outbound_result(commit); +} - if (_navigation.current() == Route::CHAT && _current_peer_hash == dest_hash) { - _chat_screen->add_message(message, true); +// UI commit for a serviced outgoing send. The LVGL_LOCK here is short — no +// I/O — so it cannot approach the 5s deadlock guard. Composer input is only +// cleared on acceptance; a RETRY/STORAGE_ERROR keeps the user's text for a +// normal re-send (same contract as the old "clear only after persistence and +// queue admission succeed"). +void UIManager::apply_outbound_result(const OutboundCommit& commit) { + LVGL_LOCK(); + switch (commit.result) { + case OutboundResult::STORAGE_ERROR: + show_storage_error("Storage is unavailable. The message was not sent."); + break; + case OutboundResult::COMPOSE_NAVIGATED: + // Replace Compose with Chat so Back returns to Messages instead + // of reopening a cleared compose form. Guarded on the current + // route: if the user already navigated away while the main loop + // was persisting (possible on a slow filesystem), leave their + // current location alone — the message is persisted and queued + // either way. The conversation reload from storage shows the + // just-persisted outgoing message. + if (_navigation.current() == Route::COMPOSE) { + _current_peer_hash = commit.dest_hash; + replace_route(Route::CHAT); + } + break; + case OutboundResult::ADDED: + if (_chat_screen && _current_peer_hash == commit.dest_hash) { + if (_navigation.current() == Route::CHAT && + !commit.packed.empty()) { + // Reconstruct the display view of the admitted message. + // unpack_from_bytes restores content/timestamp/hash and + // the flags are re-set below, matching the live object + // the old synchronous path displayed (outgoing, queued). + try { + ::LXMF::LXMessage message = + ::LXMF::LXMessage::unpack_from_bytes( + commit.packed, + ::LXMF::Type::Message::OPPORTUNISTIC, true); + message.incoming(false); + message.state(::LXMF::Type::Message::OUTBOUND); + _chat_screen->add_message(message, true); + } catch (const std::exception& error) { + WARNINGF("Failed to render sent message: %s", error.what()); + } + } + _chat_screen->clear_composer(); + } + break; + case OutboundResult::RETRY: + default: + break; // input retained; user re-sends } - - INFO(" Message queued for delivery"); - return true; } void UIManager::on_message_received(::LXMF::LXMessage& message) { diff --git a/lib/tdeck_ui/UI/LXMF/UIManager.h b/lib/tdeck_ui/UI/LXMF/UIManager.h index f2eeabbc..ec2a7500 100644 --- a/lib/tdeck_ui/UI/LXMF/UIManager.h +++ b/lib/tdeck_ui/UI/LXMF/UIManager.h @@ -37,8 +37,9 @@ #include "SettingsScreen.h" #include "PropagationNodesScreen.h" #include "CallScreen.h" -#include "CallCommandMailbox.h" #include "CallStartMailbox.h" +#include "CallCommandMailbox.h" +#include "OutgoingSendMailbox.h" #include "CallGenerationGuard.h" #include "CallLinkOwnership.h" #include "CallLivenessWatchdog.h" @@ -530,6 +531,26 @@ private: // LXMF message handling bool send_message(const RNS::Bytes& dest_hash, const String& content); + // Outgoing-send threading split. send_message (LVGL task) does only + // in-memory work and hands the packed message to update(), which + // persists and admits it off the LVGL lock; apply_outbound_result + // commits the UI under a short LVGL_LOCK. See OutgoingSendMailbox.h. + enum class OutboundResult { + RETRY, // retained for retry; keep the user's input + ADDED, // persisted + queued; append to the viewed chat + COMPOSE_NAVIGATED, // persisted + queued; replace Compose with Chat + STORAGE_ERROR, // persistence failed; show storage dialog + }; + enum class OutboundSource { CHAT, COMPOSE }; + struct OutboundCommit { + OutboundResult result = OutboundResult::RETRY; + OutboundSource source = OutboundSource::CHAT; + RNS::Bytes dest_hash; + RNS::Bytes packed; // admitted packed form, for the UI commit + }; + void service_pending_sends(); + void apply_outbound_result(const OutboundCommit& commit); + // UI updates void refresh_current_screen(); @@ -601,6 +622,9 @@ private: LXSTAudio* _lxst_audio; CallStartMailbox _call_starts; CallCommandMailbox _call_commands; + // One packed outgoing message at a time, handed from the LVGL task to + // the main loop for persistence + admission (see OutgoingSendMailbox.h). + OutgoingSendMailbox _outgoing_sends; CallGenerationGuard _call_generation_guard; CallLinkOwnership _call_link_ownership; CallLivenessWatchdog _call_liveness; diff --git a/tests/build_scripts/test_location_live_integration.py b/tests/build_scripts/test_location_live_integration.py index dd8f190a..223c5886 100644 --- a/tests/build_scripts/test_location_live_integration.py +++ b/tests/build_scripts/test_location_live_integration.py @@ -82,24 +82,28 @@ def test_live_and_chat_outbound_share_a_router_mutex(): cpp.index("class LiveLocationEnvelopeRouter") : cpp.index("UIManager::UIManager") ] - send = cpp[cpp.index("bool UIManager::send_message") : cpp.index("void UIManager::on_message_received")] + # The durable send path (admission guard + router admission) moved to + # the main-loop worker service_pending_sends(); send_message now only + # publishes to the mailbox. + send = cpp[cpp.index("bool UIManager::send_message") : cpp.index("void UIManager::service_pending_sends")] + service = cpp[cpp.index("void UIManager::service_pending_sends") : cpp.index("void UIManager::apply_outbound_result")] + apply = cpp[cpp.index("void UIManager::apply_outbound_result") : cpp.index("void UIManager::on_message_received")] assert "RouterLock" in router_block - assert "RouterLock" in send - assert "RouterLock router_lock(0)" in send - assert "_router.try_handle_outbound(" in send - assert "persistOutgoingMessage" in send - assert "_store.save_message(message)" not in send - assert send.index("RouterLock router_lock(0)") < send.index("_router.try_handle_outbound(") - assert send.index("_router.try_handle_outbound(") < send.index( - "_chat_screen->add_message(message, true)" - ) + assert "_outgoing_sends.request(" in send + assert "RouterLock" not in send + assert "RouterLock router_lock(0)" in service + assert "_router.try_handle_outbound(" in service + assert "persistOutgoingMessage" in service + assert "_store.save_message(message)" not in service + assert service.index("RouterLock router_lock(0)") < service.index("_router.try_handle_outbound(") + # UI commit (append to the viewed chat) happens only after admission. + assert "_chat_screen->add_message(message, true)" in apply assert "return context.store->save_message(*context.message);" in cpp network_pump = main[ main.index("// Process Reticulum") : main.index("LOOP_STEP(8); // Memory monitor") ] assert "RouterLock" in network_pump - assert "RouterLock router_lock(0)" in send def test_ble_ingress_and_ui_router_mutators_follow_nonblocking_lock_order(): diff --git a/tests/build_scripts/test_message_persistence_contract.py b/tests/build_scripts/test_message_persistence_contract.py index 6b3be09d..6360e3c1 100644 --- a/tests/build_scripts/test_message_persistence_contract.py +++ b/tests/build_scripts/test_message_persistence_contract.py @@ -16,28 +16,44 @@ def function_body(source: str, signature: str, next_signature: str) -> str: return source[start:end] -def test_outgoing_message_is_committed_before_display_and_send(): +def test_outgoing_send_is_published_off_lock_and_persisted_on_main_loop(): + # Regression: the LVGL send callback used to run identity recall, + # RouterLock admission, and the LittleFS persistence under the LVGL lock. + # A multi-second save then tripped the 5s deadlock guard (LVGLLock.h:45) + # and rebooted the device. send_message must now stay allocation-light + # and lock-free; the durable work belongs in service_pending_sends, + # which update() services before LVGL_LOCK. source = UI_MANAGER.read_text() - body = function_body( + send_body = function_body( source, "bool UIManager::send_message(", - "void UIManager::on_message_received(", + "void UIManager::service_pending_sends(", + ) + service_body = function_body( + source, + "void UIManager::service_pending_sends(", + "void UIManager::apply_outbound_result(", ) - display = body.index("_chat_screen->add_message(message, true)") - if "_router.try_handle_outbound(" in body: - lock = body.index("RouterLock router_lock(0)") - admission = body.index("_router.try_handle_outbound(") - assert lock < admission < display - assert "persistOutgoingMessage" in body - assert "_store.save_message(message)" not in body - assert "return context.store->save_message(*context.message);" in source - else: - save = body.index("if (!_store.save_message(message))") - send = body.index("_router.handle_outbound(message)") - assert save < display < send - assert "Outgoing message persistence failed; message not queued" in body - assert "The message was not sent" in body + # LVGL-task side: publish only. No router lock, no persistence, no + # router admission on this path. + assert "_outgoing_sends.request(" in send_body + assert "RouterLock" not in send_body + assert "_router.try_handle_outbound(" not in send_body + assert "_store.save_message(" not in send_body + + # Main-loop side: admission guard commits the final packed/stamped + # message immediately before queue ownership transfer. + lock = service_body.index("RouterLock router_lock(0)") + admission = service_body.index("_router.try_handle_outbound(") + assert lock < admission + assert "persistOutgoingMessage" in service_body + assert "Outgoing message persistence failed; message not queued" in service_body + assert "The message was not sent" in source + + # update() services the mailbox before acquiring LVGL_LOCK. + update_body = function_body(source, "void UIManager::update()", "bool UIManager::send_message(") + assert update_body.index("service_pending_sends();") < update_body.index("LVGL_LOCK();") def test_ui_messages_prefer_lora_safe_opportunistic_delivery(): @@ -109,8 +125,18 @@ def test_delivery_state_is_committed_before_ui_update(): def test_rejected_outgoing_message_keeps_retryable_input(): chat = CHAT_SCREEN.read_text() compose = COMPOSE_SCREEN.read_text() - assert "if (screen->_send_message_callback(message))" in chat + ui = UI_MANAGER.read_text() + # Both send callbacks publish through the main-loop handoff; the LVGL + # handler itself no longer clears the input. + assert "screen->_send_message_callback(message)" in chat + assert "lv_textarea_set_text(screen->_text_area" not in chat.split( + "void ChatScreen::on_send_clicked(" + )[1].split("}")[0] assert "if (screen->_send_callback && screen->_send_callback(dest_hash, message))" in compose + # The composer is cleared only after persistence + admission succeed, + # from the main-loop commit. + apply_body = ui[ui.index("void UIManager::apply_outbound_result("):] + assert "_chat_screen->clear_composer();" in apply_body def test_storage_error_dialogs_are_coalesced():