mirror of
https://github.com/torlando-tech/pyxis.git
synced 2026-09-25 06:04:51 +00:00
fix(lxmf): move conversation-open store I/O off the LVGL task
Opening a conversation crashed the same way sending did. load_conversation()
(LVGL task, under the LVGL lock held by replace_route) ran the full open
pipeline synchronously: identity recall (ustore), display-name read, the
message-index read, and the per-message metadata reads. On this device's
degraded LittleFS each op is 0.4-2s, so a cold open of a dozen-message
conversation held the LVGL mutex past the 5s deadlock guard and asserted at
LVGLLock.h:45. The send path already got the mailbox fix; the open path never
did.
Restructure with the same pattern:
- load_conversation() (LVGL task) now only navigates + resets the list and
shows the truncated hash in the header. Same-peer re-opens return early
with zero store I/O (rows are still built).
- prepare_conversation() (main loop, called from update()) does the store
I/O between a short guard lock and a short commit lock, then commits the
header name + initial bubbles + background-fill arming under a brief
LVGL_LOCK. A generation counter discards a stale in-flight prepare when
the conversation changes mid-I/O.
- refresh() re-arms the prepare instead of re-reading under the lock.
The 1Hz store 'not found in index' fetch is pre-existing (present on
2527c6d) and is being tracked separately as a flash-wear follow-up.
Build tdeck SUCCESS, 170/170 contract tests pass.
This commit is contained in:
@@ -168,29 +168,93 @@ void ChatScreen::create_input_area() {
|
||||
|
||||
void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore& store) {
|
||||
LVGL_LOCK();
|
||||
_peer_hash = peer_hash;
|
||||
_message_store = &store;
|
||||
|
||||
// Same-peer re-open (back to the list and re-tap): the content is
|
||||
// already committed by prepare_conversation() and the rows are still
|
||||
// built — nothing to do. This keeps re-opens free of any store I/O.
|
||||
if (_peer_hash == peer_hash && _prepared_peer_hash == peer_hash) {
|
||||
return;
|
||||
}
|
||||
|
||||
_peer_hash = peer_hash;
|
||||
|
||||
// A peer change cancels any in-flight background fill from the
|
||||
// previous conversation (it would otherwise prepend the previous
|
||||
// conversation's older rows into the new one). refresh() re-arms the
|
||||
// fill for the new peer.
|
||||
// conversation's older rows into the new one). prepare_conversation()
|
||||
// re-arms the fill for the new peer once its metadata is gathered.
|
||||
if (_bg_fill_active.exchange(false)) {
|
||||
_keep_bottom_during_background_fill.store(false);
|
||||
_bg_fill_target = _display_start_idx; // fill is a no-op now
|
||||
}
|
||||
|
||||
// LVGL-task side of a conversation open: navigation + list reset only.
|
||||
// The store reads (identity recall, display name, message index, per-
|
||||
// message metadata) moved to prepare_conversation() on the main loop —
|
||||
// a cold open does dozens of LittleFS/ustore reads that take seconds on
|
||||
// a degraded filesystem, and running them here (under the LVGL lock,
|
||||
// held by replace_route) held the mutex past the 5s deadlock guard and
|
||||
// rebooted the device (assert at LVGLLock.h:45). Same defect class as
|
||||
// the send path; same fix (see OutgoingSendMailbox.h / UIManager::
|
||||
// service_pending_sends).
|
||||
{
|
||||
char log_buf[64];
|
||||
snprintf(log_buf, sizeof(log_buf), "Loading conversation with peer %.8s...",
|
||||
snprintf(log_buf, sizeof(log_buf), "Opening conversation with peer %.8s...",
|
||||
peer_hash.toHex().c_str());
|
||||
INFO(log_buf);
|
||||
}
|
||||
|
||||
// Clear existing messages and row tracking (rows are rebuilt by
|
||||
// prepare_conversation()'s commit once the metadata is gathered).
|
||||
lv_obj_clean(_message_list);
|
||||
_messages.clear();
|
||||
_message_rows.clear();
|
||||
_all_message_hashes.clear();
|
||||
_display_start_idx = 0;
|
||||
|
||||
// Header shows the truncated hash immediately; prepare_conversation()
|
||||
// upgrades it to the resolved display name once recall completes.
|
||||
lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label
|
||||
{
|
||||
char hash_buf[20];
|
||||
snprintf(hash_buf, sizeof(hash_buf), "%.12s...", peer_hash.toHex().c_str());
|
||||
lv_label_set_text(label_peer, hash_buf);
|
||||
}
|
||||
|
||||
// Arm a prepare for this peer. Bumping the generation discards any
|
||||
// in-flight prepare for a previous (or same) peer: its commit re-checks
|
||||
// the generation before touching the UI.
|
||||
_prepared_peer_hash = Bytes();
|
||||
_prepare_generation++;
|
||||
}
|
||||
|
||||
void ChatScreen::prepare_conversation() {
|
||||
// Called from UIManager::update() on the main loop. The guard and the
|
||||
// commit are short locked sections; the store I/O between them (identity
|
||||
// recall, display name, message index, per-message metadata) runs OFF the
|
||||
// LVGL lock, so a slow cold open can't hold the mutex past the 5s
|
||||
// deadlock guard.
|
||||
Bytes peer_hash;
|
||||
uint32_t generation = 0;
|
||||
::LXMF::MessageStore* store = nullptr;
|
||||
{
|
||||
LVGL_LOCK();
|
||||
if (!_message_store || _peer_hash.size() == 0) {
|
||||
return;
|
||||
}
|
||||
if (_prepared_peer_hash == _peer_hash) {
|
||||
return; // already prepared for this peer
|
||||
}
|
||||
peer_hash = _peer_hash;
|
||||
generation = _prepare_generation;
|
||||
store = _message_store;
|
||||
}
|
||||
|
||||
// ── slow I/O, off the LVGL lock ────────────────────────────────────────
|
||||
// Three-tier display name resolution (mirrors ConversationListScreen):
|
||||
// 1. Live announce cache (Identity::recall_app_data)
|
||||
// 2. MessageStore-persisted name (survives reboots)
|
||||
// 3. Truncated hash (last resort)
|
||||
// 3. Truncated hash (already shown by load_conversation)
|
||||
// When (1) hits, write through to the persistent cache so future
|
||||
// cold boots get the name back without waiting for a re-announce.
|
||||
String peer_name;
|
||||
@@ -198,97 +262,101 @@ void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore&
|
||||
if (app_data && app_data.size() > 0) {
|
||||
peer_name = parse_display_name(app_data);
|
||||
if (peer_name.length() > 0) {
|
||||
store.set_display_name(peer_hash, std::string(peer_name.c_str()));
|
||||
store->set_display_name(peer_hash, std::string(peer_name.c_str()));
|
||||
}
|
||||
}
|
||||
if (peer_name.length() == 0) {
|
||||
std::string cached = store.get_display_name(peer_hash);
|
||||
std::string cached = store->get_display_name(peer_hash);
|
||||
if (!cached.empty()) {
|
||||
peer_name = String(cached.c_str());
|
||||
}
|
||||
}
|
||||
if (peer_name.length() == 0) {
|
||||
char hash_buf[20];
|
||||
snprintf(hash_buf, sizeof(hash_buf), "%.12s...", peer_hash.toHex().c_str());
|
||||
peer_name = hash_buf;
|
||||
|
||||
// Load all message hashes from store (sorted by timestamp).
|
||||
std::vector<Bytes> all_hashes = store->get_messages_for_conversation(peer_hash);
|
||||
|
||||
// Gather only the few NEWEST messages (the rest of the page is streamed
|
||||
// in by tick_background_fill() a couple per main-loop tick).
|
||||
size_t display_start_idx = 0;
|
||||
if (all_hashes.size() > INITIAL_RENDER) {
|
||||
display_start_idx = all_hashes.size() - INITIAL_RENDER;
|
||||
}
|
||||
|
||||
// Update header with peer info
|
||||
lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label
|
||||
lv_label_set_text(label_peer, peer_name.c_str());
|
||||
|
||||
refresh();
|
||||
}
|
||||
|
||||
void ChatScreen::refresh() {
|
||||
LVGL_LOCK();
|
||||
if (!_message_store) {
|
||||
return;
|
||||
}
|
||||
|
||||
INFO("Refreshing chat messages");
|
||||
|
||||
// Clear existing messages and row tracking
|
||||
lv_obj_clean(_message_list);
|
||||
_messages.clear();
|
||||
_message_rows.clear();
|
||||
|
||||
// Reserve capacity for message hashes to reduce fragmentation
|
||||
_all_message_hashes.reserve(200);
|
||||
|
||||
// Load all message hashes from store (sorted by timestamp)
|
||||
_all_message_hashes = _message_store->get_messages_for_conversation(_peer_hash);
|
||||
|
||||
// Render only the few NEWEST messages synchronously (under the LVGL lock) so
|
||||
// the conversation opens fast. The rest of the page is streamed in by
|
||||
// tick_background_fill() a couple per main-loop tick, so the UI never freezes.
|
||||
// (This runs on the main loop, not a task: the MessageStore shares one
|
||||
// _json_doc between save + load and isn't safe for concurrent access.)
|
||||
if (_all_message_hashes.size() > INITIAL_RENDER) {
|
||||
_display_start_idx = _all_message_hashes.size() - INITIAL_RENDER;
|
||||
} else {
|
||||
_display_start_idx = 0;
|
||||
}
|
||||
|
||||
{
|
||||
char log_buf[80];
|
||||
snprintf(log_buf, sizeof(log_buf), " Found %zu messages, displaying last %zu",
|
||||
_all_message_hashes.size(), _all_message_hashes.size() - _display_start_idx);
|
||||
INFO(log_buf);
|
||||
}
|
||||
|
||||
for (size_t i = _display_start_idx; i < _all_message_hashes.size(); i++) {
|
||||
const auto& msg_hash = _all_message_hashes[i];
|
||||
|
||||
// Use fast metadata loader (cache hit: O(1) in-memory; miss: one
|
||||
std::vector<MessageItem> items;
|
||||
items.reserve(INITIAL_RENDER);
|
||||
for (size_t i = display_start_idx; i < all_hashes.size(); i++) {
|
||||
// Fast metadata loader (cache hit: O(1) in-memory; miss: one
|
||||
// LittleFS read that warms the cache for every later touch).
|
||||
::LXMF::MessageStore::MessageMetadata meta = _message_store->load_message_metadata(msg_hash);
|
||||
::LXMF::MessageStore::MessageMetadata meta =
|
||||
store->load_message_metadata(all_hashes[i]);
|
||||
if (!meta.valid) {
|
||||
continue;
|
||||
}
|
||||
|
||||
MessageItem item;
|
||||
item.message_hash = msg_hash;
|
||||
item.message_hash = all_hashes[i];
|
||||
item.content = String(meta.content.c_str());
|
||||
format_timestamp(meta.timestamp, item.timestamp_str, sizeof(item.timestamp_str));
|
||||
item.outgoing = !meta.incoming;
|
||||
item.delivered = (meta.state == static_cast<int>(::LXMF::Type::Message::DELIVERED));
|
||||
item.failed = (meta.state == static_cast<int>(::LXMF::Type::Message::FAILED));
|
||||
|
||||
_messages.push_back(item);
|
||||
create_message_bubble(item);
|
||||
items.push_back(item);
|
||||
}
|
||||
|
||||
// Queue the rest of the first page to stream in on the main loop. Set the
|
||||
// target before activating so tick sees a consistent target.
|
||||
_bg_fill_target = (_all_message_hashes.size() > MESSAGES_PER_PAGE)
|
||||
? _all_message_hashes.size() - MESSAGES_PER_PAGE
|
||||
: 0;
|
||||
const bool initial_fill_active = _display_start_idx > _bg_fill_target;
|
||||
_keep_bottom_during_background_fill.store(initial_fill_active);
|
||||
_bg_fill_active.store(initial_fill_active);
|
||||
{
|
||||
char log_buf[80];
|
||||
snprintf(log_buf, sizeof(log_buf), " Found %zu messages, displaying %zu",
|
||||
all_hashes.size(), items.size());
|
||||
INFO(log_buf);
|
||||
}
|
||||
// ───────────────────────────────────────────────────────────────────────
|
||||
|
||||
scroll_to_bottom();
|
||||
// ── commit, brief LVGL lock ────────────────────────────────────────────
|
||||
{
|
||||
LVGL_LOCK();
|
||||
// The conversation changed (or this peer was re-opened) while the
|
||||
// I/O ran; the newer open owns the UI now.
|
||||
if (_peer_hash != peer_hash || _prepare_generation != generation) {
|
||||
return;
|
||||
}
|
||||
if (peer_name.length() > 0) {
|
||||
lv_obj_t* label_peer = lv_obj_get_child(_header, 1); // Second child is peer label
|
||||
lv_label_set_text(label_peer, peer_name.c_str());
|
||||
}
|
||||
_all_message_hashes = std::move(all_hashes);
|
||||
for (const auto& item : items) {
|
||||
_messages.push_back(item);
|
||||
create_message_bubble(item);
|
||||
}
|
||||
_display_start_idx = display_start_idx;
|
||||
|
||||
// Queue the rest of the first page to stream in on the main loop. Set
|
||||
// the target before activating so tick sees a consistent target.
|
||||
_bg_fill_target = (_all_message_hashes.size() > MESSAGES_PER_PAGE)
|
||||
? _all_message_hashes.size() - MESSAGES_PER_PAGE
|
||||
: 0;
|
||||
const bool initial_fill_active = _display_start_idx > _bg_fill_target;
|
||||
_keep_bottom_during_background_fill.store(initial_fill_active);
|
||||
_bg_fill_active.store(initial_fill_active);
|
||||
|
||||
_prepared_peer_hash = peer_hash;
|
||||
scroll_to_bottom();
|
||||
}
|
||||
}
|
||||
|
||||
void ChatScreen::refresh() {
|
||||
// Request a full re-gather of this conversation's content: the main
|
||||
// loop's prepare_conversation() does the store reads off the LVGL lock
|
||||
// and commits the result under a brief lock. (The old synchronous
|
||||
// implementation re-read the index + every page of metadata here under
|
||||
// the lock — the same stall class this fix removes.) Bumping the
|
||||
// generation discards any in-flight prepare so the re-gather is fresh.
|
||||
LVGL_LOCK();
|
||||
if (!_message_store || _peer_hash.size() == 0) {
|
||||
return;
|
||||
}
|
||||
INFO("Refreshing chat messages");
|
||||
_prepared_peer_hash = Bytes();
|
||||
_prepare_generation++;
|
||||
}
|
||||
|
||||
// Stream older messages in a few at a time, called from UIManager::update() on
|
||||
|
||||
@@ -83,6 +83,21 @@ public:
|
||||
*/
|
||||
void load_conversation(const RNS::Bytes& peer_hash, ::LXMF::MessageStore& store);
|
||||
|
||||
/**
|
||||
* Prepare the current conversation's content. Call from
|
||||
* UIManager::update() on the main loop after the chat route is active.
|
||||
*
|
||||
* load_conversation() (LVGL task) only navigates and clears the list;
|
||||
* the store reads (identity recall, display-name lookup, message index,
|
||||
* per-message metadata) are slow on a degraded LittleFS — a cold open
|
||||
* can take several seconds — so running them under the LVGL lock (as the
|
||||
* old synchronous path did) held the mutex past the 5s deadlock guard
|
||||
* and rebooted the device (assert at LVGLLock.h:45). This method does
|
||||
* that I/O on the main loop, then commits header + initial bubbles
|
||||
* under a brief LVGL_LOCK. No-op when this peer is already prepared.
|
||||
*/
|
||||
void prepare_conversation();
|
||||
|
||||
/**
|
||||
* Add a new message to the chat
|
||||
* @param message LXMF message to add
|
||||
@@ -185,6 +200,14 @@ private:
|
||||
// Map message hash to bubble row for targeted updates
|
||||
std::map<RNS::Bytes, lv_obj_t*> _message_rows;
|
||||
|
||||
// Conversation-prepare state (main-loop I/O + LVGL commit). Both fields are
|
||||
// only read/written while holding the LVGL lock (load_conversation, the
|
||||
// prepare guard, and the commit are all locked sections), so they need no
|
||||
// atomics. _prepare_generation disambiguates a same-peer re-open that
|
||||
// happens while a prepare's I/O is in flight.
|
||||
RNS::Bytes _prepared_peer_hash;
|
||||
uint32_t _prepare_generation = 0;
|
||||
|
||||
BackCallback _back_callback;
|
||||
SendMessageCallback _send_message_callback;
|
||||
CallCallback _call_callback;
|
||||
|
||||
@@ -818,6 +818,12 @@ void UIManager::update() {
|
||||
// loop, so each small batch only briefly holds the LVGL lock instead of the
|
||||
// whole page blocking it past LVGLLock's 5s timeout.
|
||||
if (_navigation.current() == Route::CHAT && _chat_screen) {
|
||||
// Conversation open: the LVGL task only navigated + cleared the list
|
||||
// (load_conversation); the store reads + bubble build run here on the
|
||||
// main loop so a slow cold open can't hold the LVGL mutex past the 5s
|
||||
// deadlock guard (same fix class as the send path). No-op once this
|
||||
// peer is prepared; load_conversation() re-arms it per peer open.
|
||||
_chat_screen->prepare_conversation();
|
||||
_chat_screen->tick_background_fill();
|
||||
// Long-press full-message view: the LVGL event handler only records
|
||||
// the hash; the disk read + modal build run here on the main loop.
|
||||
|
||||
@@ -16,7 +16,7 @@ def function_body(source: str, signature: str, next_signature: str) -> str:
|
||||
def test_initial_background_fill_stays_at_newest_message():
|
||||
source = CHAT_CPP.read_text()
|
||||
header = CHAT_H.read_text()
|
||||
refresh = function_body(source, "void ChatScreen::refresh()", "void ChatScreen::tick_background_fill()")
|
||||
prepare = function_body(source, "void ChatScreen::prepare_conversation()", "void ChatScreen::refresh()")
|
||||
tick = function_body(source, "void ChatScreen::tick_background_fill()", "void ChatScreen::load_more_messages(")
|
||||
on_scroll = function_body(source, "void ChatScreen::on_scroll(", "void ChatScreen::create_message_bubble(")
|
||||
assert "void ChatScreen::scroll_to_bottom()" in source
|
||||
@@ -28,10 +28,26 @@ def test_initial_background_fill_stays_at_newest_message():
|
||||
"lv_obj_scroll_to_y(_message_list, LV_COORD_MAX, LV_ANIM_OFF)"
|
||||
)
|
||||
|
||||
keep = refresh.index("_keep_bottom_during_background_fill.store(initial_fill_active)")
|
||||
activate = refresh.index("_bg_fill_active.store(initial_fill_active)")
|
||||
bottom = refresh.index("scroll_to_bottom()")
|
||||
assert keep < activate < bottom
|
||||
# Cold-open I/O (identity recall, display name, message index, per-message
|
||||
# metadata) runs on the main loop in prepare_conversation, between the
|
||||
# guard lock and the commit lock — never inside a held LVGL lock, so a
|
||||
# slow LittleFS open can't hold the mutex past the 5s deadlock guard.
|
||||
i_io = prepare.index("Identity::recall_app_data")
|
||||
i_index = prepare.index("get_messages_for_conversation")
|
||||
i_meta = prepare.index("load_message_metadata")
|
||||
i_commit_lock = prepare.index("commit, brief LVGL lock")
|
||||
i_rows = prepare.index("create_message_bubble(item)")
|
||||
i_target = prepare.index("_bg_fill_target =")
|
||||
i_keep = prepare.index("_keep_bottom_during_background_fill.store(initial_fill_active)")
|
||||
i_activate = prepare.index("_bg_fill_active.store(initial_fill_active)")
|
||||
i_bottom = prepare.index("scroll_to_bottom()")
|
||||
assert i_commit_lock < i_rows < i_target < i_keep < i_activate < i_bottom
|
||||
assert i_io < i_index < i_meta < i_commit_lock
|
||||
# refresh() must not do the slow store reads itself (it just re-arms the
|
||||
# prepare; the main loop does the I/O off-lock).
|
||||
refresh = function_body(source, "void ChatScreen::refresh()", "void ChatScreen::tick_background_fill()")
|
||||
assert "load_message_metadata" not in refresh
|
||||
assert "get_messages_for_conversation" not in refresh
|
||||
|
||||
load = tick.index("load_more_messages(")
|
||||
keep_check = tick.index("if (_keep_bottom_during_background_fill.load())")
|
||||
|
||||
Reference in New Issue
Block a user