Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/activities/ActivityManager.cpp (1)

43-46: ⚠️ Potential issue | 🔴 Critical

Use a real shared spinlock instead of taskENTER_CRITICAL(nullptr).

waitingTaskHandle protection is currently tied to taskENTER_CRITICAL(nullptr) (Line [43], Line [243]). On common FreeRTOS/ESP-IDF ports, this should use a valid shared portMUX_TYPE*; otherwise atomicity is not guaranteed (and may assert).

🔧 Suggested fix

+namespace {
+portMUX_TYPE waitingTaskMux = portMUX_INITIALIZER_UNLOCKED;
+}
+
 // Notify any task blocked in requestUpdateAndWait() that the render is done.
 TaskHandle_t waiter = nullptr;
-taskENTER_CRITICAL(nullptr);
+taskENTER_CRITICAL(&waitingTaskMux);
 waiter = waitingTaskHandle;
 waitingTaskHandle = nullptr;
-taskEXIT_CRITICAL(nullptr);
+taskEXIT_CRITICAL(&waitingTaskMux);
 if (waiter) {
   xTaskNotify(waiter, 1, eIncrement);
 }
@@
-  taskENTER_CRITICAL(nullptr);
+  taskENTER_CRITICAL(&waitingTaskMux);
   auto currTaskHandler = xTaskGetCurrentTaskHandle();
   auto mutexHolder = xSemaphoreGetMutexHolder(renderingMutex);
   bool isRenderTask = (currTaskHandler == renderTaskHandle);
   bool alreadyWaiting = (waitingTaskHandle != nullptr);
   bool holdingRenderLock = (mutexHolder == currTaskHandler);
   if (!alreadyWaiting && !isRenderTask && !holdingRenderLock) {
     waitingTaskHandle = currTaskHandler;
   }
-  taskEXIT_CRITICAL(nullptr);
+  taskEXIT_CRITICAL(&waitingTaskMux);

#!/bin/bash
set -euo pipefail

echo "=== Call sites using nullptr critical sections ==="
rg -n --type cpp 'taskENTER_CRITICAL\s*\(\s*nullptr\s*\)|taskEXIT_CRITICAL\s*\(\s*nullptr\s*\)' src

echo
echo "=== FreeRTOS critical-section API definitions in repo headers ==="
fd -t f 'task.h|FreeRTOS.h|portmacro.h' . | xargs -r rg -n 'taskENTER_CRITICAL|taskEXIT_CRITICAL|portENTER_CRITICAL|portMUX_TYPE'

Expected: confirm whether this toolchain requires a non-null portMUX_TYPE* and identify all affected call sites.

Also applies to: 243-253

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/ActivityManager.cpp` around lines 43 - 46, The code uses
taskENTER_CRITICAL(nullptr)/taskEXIT_CRITICAL(nullptr) around accesses to
waitingTaskHandle (e.g., in the block that sets waiter = waitingTaskHandle;
waitingTaskHandle = nullptr;) which can be invalid on some FreeRTOS/ESP-IDF
ports; replace the nullptr critical sections with a real shared spinlock: add or
reuse a portMUX_TYPE (or equivalent) mutex (e.g., declare a static portMUX_TYPE
activity_mutex or use an existing shared portMUX_TYPE pointer), and replace
taskENTER_CRITICAL(nullptr)/taskEXIT_CRITICAL(nullptr) calls with the
port-specific macros that accept that mutex
(portENTER_CRITICAL(&activity_mutex)/portEXIT_CRITICAL(&activity_mutex) or
taskENTER_CRITICAL(&activity_mutex)/taskEXIT_CRITICAL(&activity_mutex)),
ensuring every other call site that touches waitingTaskHandle (including the
ones around lines ~243-253) uses the same mutex.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/activities/ActivityManager.h (1)

106-108: Document the one-waiter limit in the public method contract.

The internal member comment states single-waiter behavior, but the public API comment doesn’t. Adding that here will reduce misuse by callers.

✏️ Suggested doc tweak

-  // Trigger a render and block until it completes.
-  // Must NOT be called from the render task or while holding a RenderLock.
+  // Trigger a render and block until it completes.
+  // Supports only one waiting task at a time.
+  // Must NOT be called from the render task or while holding a RenderLock.
   void requestUpdateAndWait();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/ActivityManager.h` around lines 106 - 108, Update the public
comment for requestUpdateAndWait() to explicitly state the single-waiter
limitation: document that at most one caller may block waiting for the render to
complete (concurrent callers will fail or deadlock), and reiterate that it must
not be called from the render task or while holding a RenderLock; mention
expected behavior if multiple callers attempt to wait (e.g., only the first
waiter is honored or others will return/raise).

src/activities/ActivityManager.cpp (1)

244-248: Use currTaskHandle naming for consistency.

Minor readability nit: Line 244 uses currTaskHandler, while the type and surrounding code use “Handle”.

✏️ Suggested rename

-  auto currTaskHandler = xTaskGetCurrentTaskHandle();
+  auto currTaskHandle = xTaskGetCurrentTaskHandle();
   auto mutexHolder = xSemaphoreGetMutexHolder(renderingMutex);
-  bool isRenderTask = (currTaskHandler == renderTaskHandle);
+  bool isRenderTask = (currTaskHandle == renderTaskHandle);
   bool alreadyWaiting = (waitingTaskHandle != nullptr);
-  bool holdingRenderLock = (mutexHolder == currTaskHandler);
+  bool holdingRenderLock = (mutexHolder == currTaskHandle);
   if (!alreadyWaiting && !isRenderTask && !holdingRenderLock) {
-    waitingTaskHandle = currTaskHandler;
+    waitingTaskHandle = currTaskHandle;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/activities/ActivityManager.cpp` around lines 244 - 248, Rename the local
variable currTaskHandler to currTaskHandle for consistency with surrounding code
and types: change the declaration that assigns xTaskGetCurrentTaskHandle() and
all subsequent uses (including comparisons with renderTaskHandle,
waitingTaskHandle checks, and the mutexHolder comparison used to set
holdingRenderLock) to currTaskHandle; ensure the variable name is updated
wherever referenced (e.g., in the xSemaphoreGetMutexHolder result comparison and
isRenderTask/alreadyWaiting/holdingRenderLock assignments) so naming is
consistent.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro