From 42b122b8fd53b6c48804d2d34a143581acf11eb6 Mon Sep 17 00:00:00 2001 From: Zach Nelson Date: Sat, 28 Feb 2026 04:10:14 -0600 Subject: [PATCH] docs: ActivityManager migration guide (#1222) ## Summary **What is the goal of this PR?** Added overview and migration guide for ActivityManager changes in #1016. Thanks for the suggestion, @drbourbon! --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**YES, fully written by Claude 4.6**_ --- docs/activity-manager.md | 472 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 472 insertions(+) create mode 100644 docs/activity-manager.md diff --git a/docs/activity-manager.md b/docs/activity-manager.md new file mode 100644 index 00000000..c1c3adc5 --- /dev/null +++ b/docs/activity-manager.md @@ -0,0 +1,472 @@ +# Activity & ActivityManager Migration Guide + +This document explains the refactoring from the original per-activity render task model to the centralized `ActivityManager` introduced in [PR #1016](https://github.com/crosspoint-reader/crosspoint-reader/pull/1016). It covers the architectural differences, what changed for activity authors, and the FreeRTOS task and locking model that underpins the system. + +## Overview of Changes + +| Aspect | Old Model | New Model | +|--------|-----------|-----------| +| Render task | One per activity (8KB stack each) | Single shared task in `ActivityManager` | +| Render mutex | Per-activity `renderingMutex` | Single global mutex in `ActivityManager` | +| `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. | +| Subactivity results | Callback lambdas stored in parent | `startActivityForResult()` / `setResult()` / `finish()` | +| `requestUpdate()` | Notifies activity's own render task | Delegates to `ActivityManager` (immediate or deferred) | + +## Architecture + +### Old Model: Per-Activity Render Tasks + +Each activity created its own FreeRTOS render task on entry and destroyed it on exit: + +```text +┌─────────────────────────────────────────────────────────┐ +│ Main Task (Arduino loop) │ +│ ┌───────────────────────────────────────────────────┐ │ +│ │ currentActivity->loop() │ │ +│ │ ├── handle input │ │ +│ │ ├── update state (under RenderLock) │ │ +│ │ └── requestUpdate() ──notify──► Render Task │ │ +│ │ (per-activity)│ │ +│ │ 8KB stack │ │ +│ │ owns mutex │ │ +│ └───────────────────────────────────────────────────┘ │ +│ │ +│ ActivityWithSubactivity: │ +│ ┌──────────────┐ ┌──────────────┐ │ +│ │ Parent │────►│ SubActivity │ │ +│ │ (has render │ │ (has own │ │ +│ │ task) │ │ render task) │ │ +│ └──────────────┘ └──────────────┘ │ +└─────────────────────────────────────────────────────────┘ +``` + +Problems with this approach: + +- **8KB per render task**: Each activity allocated an 8KB FreeRTOS stack for its render task, even though only one renders at a time +- **Dangerous deletion patterns**: `exitActivity()` + `enterNewActivity()` in callbacks led to `delete this` situations where the caller was destroyed while its code was still on the stack +- **Subactivity coupling**: Parents stored callbacks to child results, creating tight coupling and lifetime hazards + +### New Model: Centralized ActivityManager + +A single `ActivityManager` owns the render task and manages an activity stack: + +```text +┌──────────────────────────────────────────────────────────┐ +│ Main Task (Arduino loop) │ +│ │ +│ activityManager.loop() │ +│ │ │ +│ ├── currentActivity->loop() │ +│ │ ├── handle input │ +│ │ ├── update state (under RenderLock) │ +│ │ └── requestUpdate() │ +│ │ │ +│ ├── process pending actions (Push / Pop / Replace) │ +│ │ │ +│ └── if requestedUpdate: ──notify──► Render Task │ +│ (single, shared) │ +│ 8KB stack │ +│ global mutex │ +│ │ +│ Activity Stack: │ +│ ┌──────────┬──────────┬──────────┐ ┌──────────┐ │ +│ │ Home │ Settings │ Wifi │ │ Keyboard │ │ +│ │ (stack) │ (stack) │ (stack) │ │ (current)│ │ +│ └──────────┴──────────┴──────────┘ └──────────┘ │ +│ stackActivities[] currentActivity │ +└──────────────────────────────────────────────────────────┘ +``` + +## Migration Checklist + +### 1. Change Base Class + +If your activity extended `ActivityWithSubactivity`, change it to extend `Activity`: + +```cpp +// BEFORE +class MyActivity final : public ActivityWithSubactivity { + MyActivity(GfxRenderer& r, MappedInputManager& m, std::function goBack) + : ActivityWithSubactivity("MyActivity", r, m), goBack(goBack) {} +}; + +// AFTER +class MyActivity final : public Activity { + MyActivity(GfxRenderer& r, MappedInputManager& m) + : Activity("MyActivity", r, m) {} +}; +``` + +Note that navigation callbacks like `goBack` are no longer stored — use `finish()` or `activityManager.goHome()` instead. + +### 2. Replace Navigation Functions + +The free functions `exitActivity()` / `enterNewActivity()` in `main.cpp` are gone. Use `ActivityManager` methods: + +```cpp +// BEFORE (in main.cpp or via stored callbacks) +exitActivity(); +enterNewActivity(new SettingsActivity(renderer, mappedInput, onGoHome)); + +// AFTER (from any Activity method) +activityManager.goToSettings(); +// or for arbitrary navigation: +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.). + +### 3. Replace Subactivity Pattern + +The `enterNewActivity()` / `exitActivity()` subactivity pattern is replaced by a stack with typed results: + +```cpp +// BEFORE +void MyActivity::launchWifi() { + enterNewActivity(new WifiSelectionActivity(renderer, mappedInput, + [this](bool connected) { onWifiDone(connected); })); +} +// Child calls: onComplete(true); // triggers callback, which may call exitActivity() + +// AFTER +void MyActivity::launchWifi() { + startActivityForResult( + std::make_unique(renderer, mappedInput), + [this](const ActivityResult& result) { + if (result.isCancelled) return; + auto& wifi = std::get(result.data); + onWifiDone(wifi.connected); + }); +} +// Child calls: +// setResult(WifiResult{.connected = true, .ssid = ssid}); +// finish(); +``` + +Key differences: + +- **`startActivityForResult()`** pushes the current activity onto the stack and launches the child +- **`setResult()`** stores a typed result on the child activity +- **`finish()`** signals the manager to pop the child, call the result handler, and resume the parent +- The parent is never deleted during this process — it's safely stored on the stack + +### 4. Update `render()` Signature + +The `RenderLock` type changed from `Activity::RenderLock` (inner class) to standalone `RenderLock`: + +```cpp +// BEFORE +void render(Activity::RenderLock&&) override; + +// AFTER +void render(RenderLock&&) override; +``` + +Include `RenderLock.h` if not transitively included via `Activity.h`. + +### 5. Update `onEnter()` / `onExit()` + +Activities no longer create or destroy render tasks: + +```cpp +// BEFORE +void MyActivity::onEnter() { + Activity::onEnter(); // created render task + logged + // ... allocate resources + requestUpdate(); +} +void MyActivity::onExit() { + // ... free resources + Activity::onExit(); // acquired RenderLock, deleted render task +} + +// AFTER +void MyActivity::onEnter() { + Activity::onEnter(); // just logs + // ... allocate resources + requestUpdate(); +} +void MyActivity::onExit() { + // ... free resources + Activity::onExit(); // just logs +} +``` + +The render task lifecycle is handled entirely by `ActivityManager::begin()`. + +### 6. Update `requestUpdate()` Calls + +The signature changed to accept an `immediate` flag: + +```cpp +// BEFORE +void requestUpdate(); // always immediate notification to per-activity render task + +// AFTER +void requestUpdate(bool immediate = false); +// immediate=false (default): deferred until end of current loop iteration +// immediate=true: sends notification to render task right away +``` + +**When to use `immediate`**: Almost never. Deferred updates are batched — if `loop()` triggers multiple state changes that each call `requestUpdate()`, only one render happens. Use `immediate` only when you need the render to start before the current function returns (e.g., before a blocking network call). + +**`requestUpdateAndWait()`**: Blocks the calling task until the render completes. Use sparingly — it's designed for cases where you need the screen to reflect new state before proceeding (e.g., showing "Checking for update..." before calling a network API). + +### 7. Remove Stored Navigation Callbacks + +Old activities often stored `std::function` callbacks for navigation: + +```cpp +// BEFORE +class SettingsActivity : public ActivityWithSubactivity { + const std::function goBack; // stored callback + const std::function goHome; // stored callback +public: + SettingsActivity(GfxRenderer& r, MappedInputManager& m, + std::function goBack, std::function goHome) + : ActivityWithSubactivity("Settings", r, m), goBack(goBack), goHome(goHome) {} +}; + +// AFTER +class SettingsActivity : public Activity { +public: + SettingsActivity(GfxRenderer& r, MappedInputManager& m) + : Activity("Settings", r, m) {} + // Use finish() to go back, activityManager.goHome() to go home +}; +``` + +This removes `std::function` overhead (~2-4KB per unique signature) and eliminates lifetime risks from captured `this` pointers. + +## Technical Details + +### FreeRTOS Task Model + +The firmware runs on an ESP32-C3, a single-core RISC-V microcontroller. FreeRTOS provides cooperative and preemptive multitasking on this single core — only one task executes at any moment, and the scheduler switches between tasks at yield points (blocking calls, `vTaskDelay`, `taskYIELD`) or when a tick interrupt promotes a higher-priority task. + +There are two tasks relevant to the activity system: + +```text +┌──────────────────────┐ ┌──────────────────────────┐ +│ Main Task │ │ Render Task │ +│ (Arduino loop) │ │ (ActivityManager-owned) │ +│ Priority: 1 │ │ Priority: 1 │ +│ │ │ │ +│ Runs: │ │ Runs: │ +│ - gpio.update() │ │ - ulTaskNotifyTake() │ +│ - activity->loop() │ │ (blocks until notified)│ +│ - pending actions │ │ - RenderLock (mutex) │ +│ - sleep/power mgmt │ │ - activity->render() │ +│ - requestUpdate →────┼─────┼─► xTaskNotify() │ +│ (end of loop) │ │ │ +└──────────────────────┘ └──────────────────────────┘ +``` + +Both tasks run at priority 1. Since the ESP32-C3 is single-core, they alternate execution: the main task runs `loop()`, then at the end of the loop iteration, notifies the render task if an update was requested. The render task wakes, acquires the mutex, calls `render()`, releases the mutex, and blocks again. + +Do not use `xTaskCreate` inside activities. If you have a use case that seems to require a background task, open a discussion to propose a lifecycle-aware `Worker` abstraction first. + +### The Render Mutex and RenderLock + +A single FreeRTOS mutex (`renderingMutex`) protects shared state between `loop()` and `render()`. Since these run on different tasks, any state read by `render()` and written by `loop()` must be guarded. + +`RenderLock` is an RAII wrapper: + +```cpp +// Standalone class (not tied to any specific activity) +class RenderLock { + bool isLocked = false; +public: + explicit RenderLock(); // acquires activityManager.renderingMutex + explicit RenderLock(Activity&); // same — Activity& param kept for compatibility + ~RenderLock(); // releases mutex if still held + void unlock(); // early release +}; +``` + +**Usage patterns:** + +```cpp +// In loop(): protect state mutations that render() reads +void MyActivity::loop() { + if (somethingChanged) { + RenderLock lock; + state = newState; // safe — render() can't run while lock is held + } + requestUpdate(); // trigger render after lock is released +} + +// In render(): lock is passed in, held for duration of render +void MyActivity::render(RenderLock&&) { + // Lock is held — safe to read shared state + renderer.clearScreen(); + renderer.drawText(..., stateString, ...); + renderer.displayBuffer(); + // Lock released when RenderLock destructor runs +} +``` + +**Critical rule**: Never call `requestUpdateAndWait()` while holding a `RenderLock`. The render task needs the mutex to call `render()`, so holding it while waiting for the render to complete is a deadlock: + +```text +Main Task Render Task +────────── ─────────── +RenderLock lock; (blocked on mutex) +requestUpdateAndWait(); + → notify render task + → block waiting for + render to complete → wakes up + → tries to acquire mutex + → DEADLOCK: main holds mutex, + waits for render; render + waits for mutex +``` + +### requestUpdate() vs requestUpdateAndWait() + +```text +requestUpdate(false) requestUpdate(true) +───────────────── ───────────────── +Sets flag only. Notifies render task +Render happens after immediately. +loop() returns and Render may start +ActivityManager checks before the calling +the flag. function returns. + (Does NOT wait for + render to complete.) + +requestUpdateAndWait() +────────────────────── +Notifies render task AND +blocks calling task until +render is done. Uses +FreeRTOS direct-to-task +notification on the +caller's task handle. +``` + +`requestUpdateAndWait()` flow in detail: + +```text +Calling Task Render Task +──────────── ─────────── +requestUpdateAndWait() + ├─ assert: not render task + ├─ assert: not holding RenderLock + ├─ store waitingTaskHandle + ├─ xTaskNotify(renderTask) → wakes render task + └─ ulTaskNotifyTake() ─┐ + (blocked) │ RenderLock lock; + │ activity->render(); + │ // render complete + │ taskENTER_CRITICAL + │ waiter = waitingTaskHandle + │ waitingTaskHandle = nullptr + │ taskEXIT_CRITICAL + │ xTaskNotify(waiter) ───┐ + │ │ + ┌──────────────────────┘ │ + │ (woken by notification) ◄────────────────────────┘ + └─ return +``` + +### Activity Lifecycle Under ActivityManager + +```text +activityManager.replaceActivity(make_unique(...)) + │ + ▼ +╔═══════════════════════════════════════════════════╗ +║ pendingAction = Replace ║ +║ pendingActivity = MyActivity ║ +╚═══════════════════════════════════════════════════╝ + │ + ▼ (next loop iteration) + ActivityManager::loop() + │ + ├── currentActivity->loop() // old activity's last loop + │ + ├── process pending action: + │ ├── RenderLock lock; + │ ├── oldActivity->onExit() // cleanup under lock + │ ├── delete oldActivity + │ ├── clear stack + │ ├── currentActivity = MyActivity + │ ├── lock.unlock() + │ └── MyActivity->onEnter() // init new activity + │ + └── if requestedUpdate: + └── notify render task +``` + +For push/pop (subactivity) navigation: + +```text +Parent calls: startActivityForResult(make_unique(...), handler) + │ + ▼ +╔══════════════════════════════════════╗ +║ pendingAction = Push ║ +║ pendingActivity = Child ║ +║ parent->resultHandler = handler ║ +╚══════════════════════════════════════╝ + │ + ▼ (next loop iteration) + ├── Parent moved to stackActivities[] + ├── currentActivity = Child + └── Child->onEnter() + + ... child runs ... + +Child calls: setResult(MyResult{...}); finish(); + │ + ▼ +╔══════════════════════════════════════╗ +║ pendingAction = Pop ║ +║ child->result = MyResult{...} ║ +╚══════════════════════════════════════╝ + │ + ▼ (next loop iteration) + ├── result = child->result + ├── Child->onExit(); delete Child + ├── currentActivity = Parent (popped from stack) + ├── Parent->resultHandler(result) + └── requestUpdate() // automatic re-render for parent +``` + +### Common Pitfalls + +**Calling `finish()` and continuing to access `this`**: `finish()` sets `pendingAction = Pop` but does not immediately destroy the activity. The activity is destroyed on the next `ActivityManager::loop()` iteration. It's safe to access member variables after `finish()` within the same function, but don't rely on the activity surviving past the current `loop()` call. + +**Modifying shared state without `RenderLock`**: If `render()` reads a variable and `loop()` writes it, the write must be under a `RenderLock`. Without it, `render()` could see a half-written value (e.g., a partially updated string or struct). + +**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. + +**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 +// WRONG — blocks render for the entire network call +void MyActivity::doNetworkStuff() { + RenderLock lock; + state = LOADING; + auto result = http.get(url); // blocks for seconds with lock held + state = DONE; +} + +// CORRECT — release lock before blocking +void MyActivity::doNetworkStuff() { + { + RenderLock lock; + state = LOADING; + } + requestUpdate(true); // render "Loading..." immediately, before we block + auto result = http.get(url); // lock is not held + { + RenderLock lock; + state = DONE; + } + requestUpdate(); +} +```