diff --git a/docs/activity-manager.md b/docs/activity-manager.md index c1c3adc5..2f0756a0 100644 --- a/docs/activity-manager.md +++ b/docs/activity-manager.md @@ -11,6 +11,7 @@ This document explains the refactoring from the original per-activity render tas | `RenderLock` | Inner class of `Activity` | Standalone class, acquires global mutex | | Subactivities | `ActivityWithSubactivity` base class | Activity stack managed by `ActivityManager` | | Navigation | Free functions in `main.cpp` | `activityManager.goHome()`, `goToReader()`, etc. | +| Forward flow | Parent stays on stack (`pushActivity`) | Parent destroyed on forward flow (`replaceWith*` + `ReturnHint`) | | Subactivity results | Callback lambdas stored in parent | `startActivityForResult()` / `setResult()` / `finish()` | | `requestUpdate()` | Notifies activity's own render task | Delegates to `ActivityManager` (immediate or deferred) | @@ -99,7 +100,7 @@ class MyActivity final : public Activity { }; ``` -Note that navigation callbacks like `goBack` are no longer stored — use `finish()` or `activityManager.goHome()` instead. +Note that navigation callbacks like `goBack` are no longer stored — use `finish()`, `onGoHome()`, or a direct `activityManager.goHome()` / `goTo*()` / `replaceWith*()` call instead. ### 2. Replace Navigation Functions @@ -116,7 +117,7 @@ activityManager.goToSettings(); activityManager.replaceActivity(std::make_unique(renderer, mappedInput)); ``` -`replaceActivity()` destroys the current activity and clears the stack. Use it for top-level navigation (home, reader, settings, etc.). +`replaceActivity()` destroys the current activity and clears the stack. Use it for top-level navigation (home, reader, settings, etc.). See the "Navigation Flow" section after the checklist for when to use replace vs push, and how the `ReturnHint` mechanism restores a parent's prior state without keeping it resident. ### 3. Replace Subactivity Pattern @@ -234,12 +235,82 @@ class SettingsActivity : public Activity { public: SettingsActivity(GfxRenderer& r, MappedInputManager& m) : Activity("Settings", r, m) {} - // Use finish() to go back, activityManager.goHome() to go home + // Use finish() to go back (pops the stack, or returns via ReturnHint if empty), + // onGoHome() to exit the flow ("up and out"), or activityManager.goHome() for a hard reset. }; ``` This removes `std::function` overhead (~2-4KB per unique signature) and eliminates lifetime risks from captured `this` pointers. +## Navigation Flow + +### Push vs Replace: Memory Matters + +On an ESP32-C3, heap fragmentation is a real constraint — especially when the next activity is heavy (EPUB reader, TLS sync). There are two ways to launch a new activity, and the choice matters: + +| Call | Current activity | Stack | When to use | +|----------------------------------------------------|---------------------------------|------------------|-------------------------------------------------------------| +| `replaceActivity()` / `goTo*()` / `replaceWith*()` | **destroyed** (onExit + delete) | cleared | Forward flow: the caller has no reason to stay resident | +| `pushActivity()` / `startActivityForResult()` | kept alive on stack | parent preserved | Modal result flow: the caller needs to resume with a result | + +**Default to replace.** Push is only correct when you need to deliver a result back to a still-living parent (keyboard entry, confirmation dialog, chapter picker, etc.). For plain one-way transitions — opening a book, going to settings, switching tabs — use replace so the parent's memory is freed before the next activity runs. + +### The `ReturnHint` Pattern + +Replace destroys the parent, but the user still expects Back to return "where they came from" — not always Home. To bridge that, `ActivityManager` holds a small `ReturnHint`: + +```cpp +enum class ReturnTo : uint8_t { Home, FileBrowser, RecentBooks }; + +struct ReturnHint { + ReturnTo target = ReturnTo::Home; + std::string path; // FileBrowser directory to restore + std::string selectName; // item to re-focus (file name, book path) + int selectIndex = -1; // combined-list index (Home selector, Recents row) +}; +``` + +A parent records a hint before launching a forward flow. When the launched activity (or anything it chains to) eventually exits with an empty stack, `ActivityManager::returnFromChild()` consumes the hint and routes to the correct parent — restoring its prior selection. If no hint is set, it falls back to `goHome()`. + +Two ways to set a hint: + +**1. Dedicated wrappers** — for the common book-open paths: + +```cpp +// FileBrowserActivity::onFileOpen +ReturnHint hint; +hint.target = ReturnTo::FileBrowser; +hint.path = basepath; // "/books/fiction" +hint.selectName = entry; // "war_and_peace.epub" +activityManager.replaceWithReader(fullPath, std::move(hint)); + +// RecentBooksActivity::onSelect +ReturnHint hint; +hint.target = ReturnTo::RecentBooks; +hint.selectIndex = selectorIndex; +activityManager.replaceWithReader(path, std::move(hint)); +``` + +**2. `setReturnHint()` + any `goTo*()`** — for arbitrary transitions where a dedicated wrapper would be overkill: + +```cpp +// HomeActivity::dispatchMenuAction +ReturnHint hint; +hint.target = ReturnTo::Home; +hint.selectIndex = selectorIndex; // restore focus on the same menu entry +activityManager.setReturnHint(std::move(hint)); +activityManager.goToSettings(); // parent destroyed; hint survives the round trip +``` + +`goTo*()` helpers do **not** clear the hint — only `goHome()` (explicit hard-reset) and the `replaceWith*()` helpers (which overwrite it with their own hint) do. This lets a hint survive chained transitions: Home → Reader → KOReaderSync → Reader → back to Home, hint intact. + +How `finish()` interacts with the hint: + +- **Non-empty stack**: `finish()` pops to the parent on the stack (classic modal result flow). Hint is untouched. +- **Empty stack**: `finish()` falls through to `returnFromChild()` automatically. An activity launched via a `replaceWith*()` helper has no stack — so its Back-button `finish()` naturally routes via the hint. + +`onGoHome()` is now semantically "up and out" — it calls `returnFromChild()`, so long-press Back in a reader returns to whichever view opened the book, not always Home. For an explicit hard-reset, call `activityManager.goHome()` directly. + ## Technical Details ### FreeRTOS Task Model @@ -444,6 +515,10 @@ Child calls: setResult(MyResult{...}); finish(); **Creating background tasks that outlive the activity**: Any FreeRTOS task created in `onEnter()` must be deleted in `onExit()` before the activity is destroyed. The `ActivityManager` does not track or clean up background tasks. +**Using push for forward navigation**: `pushActivity()` / `startActivityForResult()` keeps the parent alive on the stack. For a heavy child (EPUB reader, TLS sync) on a fragmented heap, the parent's resident allocations can be the difference between a successful launch and OOM. Only push when you need the parent to receive a result — otherwise use `replaceActivity()` / `goTo*()` / `replaceWith*()` and let the parent be freed first. If you do need "back to where I came from" semantics, record a `ReturnHint` before the replace instead of pushing. + +**Stale `ReturnHint`**: A hint set by one activity persists until either `returnFromChild()` / `goHome()` clears it, or a `replaceWith*()` helper overwrites it. If you record a hint but the flow aborts down an unusual path (error screen, boot transition), the next unrelated `finish()` could consume it. Prefer setting the hint immediately before the transition, and call `activityManager.clearReturnHint()` if you abort the flow without launching the intended target. + **Holding `RenderLock` across blocking calls**: The render task is blocked on the mutex while you hold the lock. Keep critical sections short — acquire, mutate state, release, then do blocking work. ```cpp diff --git a/src/activities/Activity.cpp b/src/activities/Activity.cpp index 17ad577d..91122af1 100644 --- a/src/activities/Activity.cpp +++ b/src/activities/Activity.cpp @@ -10,7 +10,9 @@ void Activity::requestUpdate(bool immediate) { activityManager.requestUpdate(imm void Activity::requestUpdateAndWait() { activityManager.requestUpdateAndWait(); } -void Activity::onGoHome() { activityManager.goHome(); } +// "Up and out" — return to whichever parent launched this flow. If no return hint +// is set (typical for activities launched via a plain goTo*()), falls back to Home. +void Activity::onGoHome() { activityManager.returnFromChild(); } void Activity::startActivityForResult(std::unique_ptr&& activity, ActivityResultHandler resultHandler) { this->resultHandler = std::move(resultHandler); diff --git a/src/activities/ActivityManager.cpp b/src/activities/ActivityManager.cpp index 7aa1f239..92036049 100644 --- a/src/activities/ActivityManager.cpp +++ b/src/activities/ActivityManager.cpp @@ -22,6 +22,10 @@ #include "util/FullScreenMessageActivity.h" #include "weather/WeatherActivity.h" +#ifndef DEBUG_MEMORY_CONSUMPTION +#define DEBUG_MEMORY_CONSUMPTION 0 +#endif + void ActivityManager::begin() { xTaskCreate(&renderTaskTrampoline, "ActivityManagerRender", 8192, // Stack size @@ -32,12 +36,16 @@ void ActivityManager::begin() { assert(renderTaskHandle != nullptr && "Failed to create render task"); } +#if DEBUG_MEMORY_CONSUMPTION static void logActivityStackState(const char* stage, Activity* currentActivity, size_t stackSize) { const uint32_t freeHeap = esp_get_free_heap_size(); const uint32_t contigHeap = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT | MALLOC_CAP_DEFAULT); LOG_DBG("ACT", "%s: current=%s stackSize=%zu free=%lu contig=%lu", stage, currentActivity ? currentActivity->getName().c_str() : "", stackSize, freeHeap, contigHeap); } +#else +static inline void logActivityStackState(const char*, Activity*, size_t) {} +#endif void ActivityManager::renderTaskTrampoline(void* param) { auto* self = static_cast(param); @@ -159,7 +167,9 @@ void ActivityManager::loop() { RenderLock lock; if (pendingAction == PendingAction::Replace) { +#if DEBUG_MEMORY_CONSUMPTION logActivityStackState("replace_before", currentActivity.get(), stackActivities.size()); +#endif // Destroy the current activity exitActivity(lock); // Clear the stack @@ -167,13 +177,21 @@ void ActivityManager::loop() { stackActivities.back()->onExit(); stackActivities.pop_back(); } +#if DEBUG_MEMORY_CONSUMPTION logActivityStackState("replace_after_clear", nullptr, stackActivities.size()); +#endif } else if (pendingAction == PendingAction::Push) { +#if DEBUG_MEMORY_CONSUMPTION logActivityStackState("push_before", currentActivity.get(), stackActivities.size()); +#endif // Move current activity to stack stackActivities.push_back(std::move(currentActivity)); +#if DEBUG_MEMORY_CONSUMPTION LOG_DBG("ACT", "Pushed to activity stack, new size = %zu", stackActivities.size()); logActivityStackState("push_after", currentActivity.get(), stackActivities.size()); +#else + LOG_DBG("ACT", "Pushed to activity stack, new size = %zu", stackActivities.size()); +#endif } pendingAction = PendingAction::None; currentActivity = std::move(pendingActivity); @@ -215,8 +233,10 @@ void ActivityManager::replaceActivity(std::unique_ptr&& newActivity) { if (currentActivity) { // Defer launch if we're currently in an activity, to avoid deleting the current activity // leading to the "delete this" problem +#if DEBUG_MEMORY_CONSUMPTION LOG_DBG("ACT", "replaceActivity requested: current=%s stackSize=%zu", currentActivity->getName().c_str(), stackActivities.size()); +#endif pendingActivity = std::move(newActivity); pendingAction = PendingAction::Replace; } else { @@ -233,12 +253,10 @@ void ActivityManager::goToFileTransfer() { void ActivityManager::goToSettings() { replaceActivity(std::make_unique(renderer, mappedInput)); } void ActivityManager::goToFileBrowser(std::string path, std::string focusName) { - hasReturnHint = false; replaceActivity(std::make_unique(renderer, mappedInput, std::move(path), std::move(focusName))); } void ActivityManager::goToRecentBooks(int focusIndex) { - hasReturnHint = false; replaceActivity(std::make_unique(renderer, mappedInput, focusIndex)); } @@ -303,7 +321,7 @@ void ActivityManager::returnFromChild() { break; case ReturnTo::Home: default: - goHome(std::move(hint.selectName)); + goHome(std::move(hint.selectName), hint.selectIndex); break; } } @@ -321,9 +339,9 @@ void ActivityManager::goToFullScreenMessage(std::string message, EpdFontFamily:: void ActivityManager::goToWeather() { replaceActivity(std::make_unique(renderer, mappedInput)); } -void ActivityManager::goHome(std::string focusBookPath) { +void ActivityManager::goHome(std::string focusBookPath, int focusSelectorIndex) { hasReturnHint = false; - replaceActivity(std::make_unique(renderer, mappedInput, std::move(focusBookPath))); + replaceActivity(std::make_unique(renderer, mappedInput, std::move(focusBookPath), focusSelectorIndex)); } void ActivityManager::pushActivity(std::unique_ptr&& activity) { @@ -332,8 +350,10 @@ void ActivityManager::pushActivity(std::unique_ptr&& activity) { LOG_ERR("ACT", "pendingActivity while pushActivity is not expected"); pendingActivity.reset(); } +#if DEBUG_MEMORY_CONSUMPTION LOG_DBG("ACT", "pushActivity requested: current=%s stackSize=%zu", currentActivity ? currentActivity->getName().c_str() : "", stackActivities.size()); +#endif pendingActivity = std::move(activity); pendingAction = PendingAction::Push; } diff --git a/src/activities/ActivityManager.h b/src/activities/ActivityManager.h index b108e988..f0e6ec28 100644 --- a/src/activities/ActivityManager.h +++ b/src/activities/ActivityManager.h @@ -116,7 +116,7 @@ class ActivityManager { void goToBoot(); void goToFullScreenMessage(std::string message, EpdFontFamily::Style style = EpdFontFamily::REGULAR); void goToWeather(); - void goHome(std::string focusBookPath = {}); + void goHome(std::string focusBookPath = {}, int focusSelectorIndex = -1); // Replace-with-hint helpers: destroy the current activity before launching the new // one (freeing its memory) and record where to route control when the new activity @@ -130,6 +130,19 @@ class ActivityManager { // no hint is set, defaults to goHome(). void returnFromChild(); + // Record a ReturnHint before calling any plain goTo*() helper. Allows an activity + // (e.g. Home) to declare "when this flow ends, come back here with this state" for + // transitions where we don't want a dedicated replaceWith*() wrapper. + // Cleared by returnFromChild() or by an explicit goHome()/replaceWith*() call. + void setReturnHint(ReturnHint hint) { + returnHint = std::move(hint); + hasReturnHint = true; + } + void clearReturnHint() { + returnHint = {}; + hasReturnHint = false; + } + // This will move current activity to stack instead of deleting it void pushActivity(std::unique_ptr&& activity); diff --git a/src/activities/home/HomeActivity.cpp b/src/activities/home/HomeActivity.cpp index fe9ad3d0..a41b349c 100644 --- a/src/activities/home/HomeActivity.cpp +++ b/src/activities/home/HomeActivity.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -211,15 +212,27 @@ void HomeActivity::onEnter() { recentsLoaded = true; } + // Apply focus: book path takes priority, else combined selector index (covers + // "return to the menu entry I was on"). + bool focused = false; if (!focusBookPath.empty()) { for (size_t i = 0; i < recentBooks.size(); ++i) { if (recentBooks[i].path == focusBookPath) { selectorIndex = static_cast(i); + focused = true; break; } } focusBookPath.clear(); } + if (!focused && focusSelectorIndex >= 0) { + rebuildMenuEntries(); // need menu count to clamp; rebuild is idempotent + const int combinedSize = static_cast(recentBooks.size() + menuEntries.size()); + if (combinedSize > 0) { + selectorIndex = std::min(focusSelectorIndex, combinedSize - 1); + } + } + focusSelectorIndex = -1; // Trigger first update menuEntriesDirty = true; @@ -366,6 +379,13 @@ void HomeActivity::onSelectBook(const std::string& path) { } void HomeActivity::dispatchMenuAction(MenuAction action) { + // Record where the menu entry was focused so that when the launched activity exits + // (via returnFromChild() or an empty-stack finish()), we come back to the same row. + ReturnHint hint; + hint.target = ReturnTo::Home; + hint.selectIndex = selectorIndex; + activityManager.setReturnHint(std::move(hint)); + switch (action) { case MenuAction::FileBrowser: activityManager.goToFileBrowser(); diff --git a/src/activities/home/HomeActivity.h b/src/activities/home/HomeActivity.h index 32cece1f..dbf29f17 100644 --- a/src/activities/home/HomeActivity.h +++ b/src/activities/home/HomeActivity.h @@ -44,7 +44,8 @@ class HomeActivity final : public Activity { std::vector menuEntries; bool menuEntriesDirty = true; - std::string focusBookPath; // book path to re-select on first render, if present in recents + std::string focusBookPath; // book path to re-select on first render, if present in recents + int focusSelectorIndex = -1; // fallback combined-selector index when focusBookPath doesn't match void onSelectBook(const std::string& path); void dispatchMenuAction(MenuAction action); @@ -57,8 +58,11 @@ class HomeActivity final : public Activity { void loadRecentCovers(int coverHeight); public: - explicit HomeActivity(GfxRenderer& renderer, MappedInputManager& mappedInput, std::string focusBookPath = {}) - : Activity("Home", renderer, mappedInput), focusBookPath(std::move(focusBookPath)) {} + explicit HomeActivity(GfxRenderer& renderer, MappedInputManager& mappedInput, std::string focusBookPath = {}, + int focusSelectorIndex = -1) + : Activity("Home", renderer, mappedInput), + focusBookPath(std::move(focusBookPath)), + focusSelectorIndex(focusSelectorIndex) {} void onEnter() override; void onExit() override; void loop() override; diff --git a/src/activities/reader/EpubReaderActivity.cpp b/src/activities/reader/EpubReaderActivity.cpp index 7c3494e3..4421cc6b 100644 --- a/src/activities/reader/EpubReaderActivity.cpp +++ b/src/activities/reader/EpubReaderActivity.cpp @@ -1,3 +1,7 @@ +#ifndef DEBUG_MEMORY_CONSUMPTION +#define DEBUG_MEMORY_CONSUMPTION 0 +#endif + #include "EpubReaderActivity.h" #include @@ -35,11 +39,15 @@ constexpr unsigned long skipChapterMs = 700; // pages per minute, first item is 1 to prevent division by zero if accessed constexpr int PAGE_TURN_LABELS[] = {1, 1, 3, 6, 12}; +#if DEBUG_MEMORY_CONSUMPTION void logReaderMemSnapshot(const char* stage) { const uint32_t freeHeap = esp_get_free_heap_size(); const uint32_t contigHeap = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT | MALLOC_CAP_DEFAULT); LOG_DBG("ERS", "Reader mem[%s]: free=%lu contig=%lu", stage, freeHeap, contigHeap); } +#else +inline void logReaderMemSnapshot(const char*) {} +#endif bool writeReaderProgressCache(const std::string& cachePath, const int spineIndex, const int currentPage, const int pageCount) { diff --git a/src/activities/reader/ReaderActivity.cpp b/src/activities/reader/ReaderActivity.cpp index 031bd78b..69f572a0 100644 --- a/src/activities/reader/ReaderActivity.cpp +++ b/src/activities/reader/ReaderActivity.cpp @@ -20,12 +20,20 @@ #include "components/UITheme.h" #include "fontIds.h" +#ifndef DEBUG_MEMORY_CONSUMPTION +#define DEBUG_MEMORY_CONSUMPTION 0 +#endif + namespace { +#if DEBUG_MEMORY_CONSUMPTION void logReaderLaunchMemSnapshot(const char* stage) { const uint32_t freeHeap = esp_get_free_heap_size(); const uint32_t contigHeap = heap_caps_get_largest_free_block(MALLOC_CAP_8BIT | MALLOC_CAP_DEFAULT); LOG_DBG("READER", "Reader mem[%s]: free=%lu contig=%lu", stage, freeHeap, contigHeap); } +#else +inline void logReaderLaunchMemSnapshot(const char*) {} +#endif } // namespace std::string ReaderActivity::extractFolderPath(const std::string& filePath) {