using a unique low level semaphore to prevent simultaneous access to pixel data

[limpkin]

for large LED strips, with the current code it is possible for the effects to update the pixel data to be sent out before the driver has actually finished pushing it out.
As a consequence I'm using only a single semaphore. In the future it would be best to use double buffering techniques .

Summary by CodeRabbit

• Refactor
  • Unified internal task synchronization into a single coordination mechanism. This simplifies internal timing between low-level tasks, reduces complexity, and slightly improves resource efficiency and reliability while preserving existing external behavior and responsiveness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'review', 'context'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Replaced two binary semaphores (effectSemaphore, driverSemaphore) with a single lowLevelSemaphore. Updated effectTask, driverTask, and setup to use lowLevelSemaphore (setup now gives lowLevelSemaphore at init). Removed old semaphore declarations and all references to them.

Changes

Cohort / File(s)

Summary

Semaphore Consolidation src/main.cpp

Replaced effectSemaphore and driverSemaphore with a single binary semaphore lowLevelSemaphore (volatile xSemaphoreHandle lowLevelSemaphore = xSemaphoreCreateBinary();); updated effectTask to take/give lowLevelSemaphore and conditionally call layerP.loop20ms(); updated driverTask to take/give lowLevelSemaphore and call layerP.loopDrivers(); setup() now gives lowLevelSemaphore at startup; removed old semaphore declarations and all call sites referencing them.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and accurately describes the main change: replacing two separate semaphores with a single shared semaphore to prevent simultaneous access to pixel data, which is the core objective of the PR.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9fa7a3d and bca9991.

📒 Files selected for processing (1)

• src/main.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main.cpp

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main.cpp (1)

117-158: Critical: Single semaphore doesn't guarantee alternation between effect and driver tasks.

The current pattern uses one semaphore with both tasks following take → process → give. This provides mutual exclusion but doesn't ensure strict alternation. After effectTask gives the semaphore (line 142), the FreeRTOS scheduler may allow effectTask to acquire it again before driverTask runs, causing effectTask to update pixel data before driverTask finishes pushing the previous frame—the exact problem this PR aims to solve.

For strict producer-consumer synchronization (effects produce data, driver consumes), use two semaphores:

🔎 Proposed fix: Use two semaphores for guaranteed alternation

-volatile xSemaphoreHandle lowLevelSemaphore = xSemaphoreCreateBinary();
+volatile xSemaphoreHandle effectSemaphore = xSemaphoreCreateBinary();
+volatile xSemaphoreHandle driverSemaphore = xSemaphoreCreateBinary();

In setup() after task creation:

   );
+  
+  // Initialize: allow effectTask to run first
+  xSemaphoreGive(effectSemaphore);
   #endif

In effectTask:

-    if (xSemaphoreTake(lowLevelSemaphore, pdMS_TO_TICKS(100)) == pdFALSE) {
+    if (xSemaphoreTake(effectSemaphore, pdMS_TO_TICKS(100)) == pdFALSE) {
       // EXT_LOGW(ML_TAG, "effectSemaphore wait too long"); 
     }
 
     layerP.loop();
     
     // ... loop20ms code ...
 
-    xSemaphoreGive(lowLevelSemaphore);
+    xSemaphoreGive(driverSemaphore);  // Signal driver to consume

In driverTask:

-    xSemaphoreTake(lowLevelSemaphore, pdMS_TO_TICKS(100));
+    xSemaphoreTake(driverSemaphore, pdMS_TO_TICKS(100));
 
     layerP.loopDrivers();
 
-    xSemaphoreGive(lowLevelSemaphore);
+    xSemaphoreGive(effectSemaphore);  // Signal effect can produce next frame

This ensures: effectTask produces → driverTask consumes → effectTask produces → (repeat)

🧹 Nitpick comments (2)

src/main.cpp (2)

130-132: Update comment to reflect new semaphore name.

The comment references the old effectSemaphore but the code now uses lowLevelSemaphore.

🔎 Proposed fix

     if (xSemaphoreTake(lowLevelSemaphore, pdMS_TO_TICKS(100)) == pdFALSE) {
-      // EXT_LOGW(ML_TAG, "effectSemaphore wait too long"); //happens if no driver!, but let effects continue (for monitor) at 10 fps
+      // EXT_LOGW(ML_TAG, "lowLevelSemaphore wait too long"); //happens if no driver!, but let effects continue (for monitor) at 10 fps
     }

---

154-154: Consider checking semaphore timeout in driverTask.

Unlike effectTask (line 130), driverTask doesn't check the return value of xSemaphoreTake. If the timeout occurs, driverTask proceeds to call loopDrivers() without confirming it has exclusive access to pixel data, potentially violating the synchronization this PR aims to establish.

🔎 Proposed improvement

-    xSemaphoreTake(lowLevelSemaphore, pdMS_TO_TICKS(100));
+    if (xSemaphoreTake(lowLevelSemaphore, pdMS_TO_TICKS(100)) == pdFALSE) {
+      // Timeout occurred - skip this iteration to maintain synchronization
+      continue;
+    }
 
     layerP.loopDrivers();

Or if it's safe to proceed on timeout, add a comment explaining why.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c3733e and 9fa7a3d.

📒 Files selected for processing (1)

• src/main.cpp

[limpkin]

@coderabbitai : are we good?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff to review the changes
gh pr diff 82

Length of output: 118

---

🏁 Script executed:

#!/bin/bash
# Get the full content of the modified file to understand context better
cat src/main.cpp

Length of output: 16090

---

@limpkin The implementation looks solid for preventing simultaneous access to pixel data! Here's my assessment:

✅ What's working well:

• Both tasks properly acquire and release the semaphore around their critical sections
• The 100ms timeout prevents deadlock scenarios
• Both tasks yield with vTaskDelay(1), preventing one from dominating the CPU
• Same priority (3) on the same core (1) ensures fair scheduling
• Initial xSemaphoreGive in setup allows the first task to start

💡 Minor consideration:
The code uses a binary semaphore (xSemaphoreCreateBinary()), which works, but for mutual exclusion a mutex (xSemaphoreCreateMutex()) would be more semantically correct. Mutexes provide:

• Priority inheritance (helps prevent priority inversion)
• Better debugging visibility in FreeRTOS tools
• Clearer intent that this is for mutual exclusion, not signaling

That said, at the same priority level, the binary semaphore approach should work fine in practice.

Questions:

1. Have you tested this with large LED strips under load?
2. Are you seeing the timeout warnings in practice, or do the tasks coordinate smoothly?
3. Any performance impact noticed compared to the previous two-semaphore approach?

If your testing shows it's working as expected, this looks good to merge! 🎉

[limpkin]

no timeouts, all tested, good to go @ewowi !