feat(audio,task): Worker primitive + Processor::setCoreAffinity offload#2357
Closed
feat(audio,task): Worker primitive + Processor::setCoreAffinity offload#2357
Conversation
Phase 2 of issue #2308 option (B) — give portable user code a way to move audio pump work off the core that runs FastLED.show(), without forcing the core-pinning concept into every sketch on platforms where it can't be expressed. New: fl::task::Worker One-shot functor worker. Caller calls worker.run(fn); it runs fn on the worker and blocks the caller until it returns. Platform behaviour: - ESP32 dual-core (FL_HAS_MULTICORE_AFFINITY) — spawns a persistent FreeRTOS task pinned to the requested core, wakes it on a binary semaphore, runs fn, signals completion on a second semaphore. The caller blocks during run() but the scheduler can service OS/WiFi on the caller's host core in the meantime, so audio cost lands on the other physical core. - Single-core ESP32 / AVR / ARM / host — run() is synchronous. Same API, same calling code; no-op on pinning. Keeps setCoreAffinity() compilable and meaningful everywhere. Audio integration: Processor::setCoreAffinity(int core) Portable: accepts 0/1/-1. On dual-core ESP32 creates a pinned Worker and routes the per-tick pump closure through it. On single-core / non-ESP32 it's a silent no-op for core==0 and an error for positive others. The scheduler still drives every_ms(1); only the inner pump body hops to the worker. Audio task remains a one-shot functor — we're not running a long coroutine loop. The coroutine (as FreeRTOS task) is the pinning mechanism, nothing more, which matches the clarification in issue #2308 review comments. Threading caveat is documented on setCoreAffinity: when offloaded, onBass/onBeat/onVibeLevels callbacks fire on the pinned core, so user code must not call FastLED.show() or touch LED arrays from inside. Autoresearch validation - testCoroutineCoreAffinity (AutoResearchRemote.cpp): spawns coroutines with core_id=0..FL_CPU_CORES-1 and asserts xPortGetCoreID() inside the lambda matches the request. Validates the phase 1 plumbing. - testWorkerAffinity: end-to-end check that fl::task::Worker actually runs work on the requested core. Reports isPinned() and observed core per worker. On single-core platforms verifies synchronous fallback runs the functor. - testFftBackendTiming: times the currently-compiled FFT backend (kiss_fftr by default, ESP-DSP with -DFL_FFT_USE_ESP_DSP=1) against a single-tone sine, reports us-per-call + peak bin for side-by-side comparison across builds. This is the runtime half of the hw-vs-software FFT comparison in issue #2308 option (A). All three are registered in testCoroutineAll so the existing --coroutine autoresearch mode picks them up. Test plan verified locally - bash test fl_task_worker --debug pass - bash test fl_audio_audio_processor --debug pass - bash test fl_audio_audio_reactive --debug pass - bash compile esp32dev --examples Blink pass - bash compile esp32s3 --examples Blink pass - bash compile esp32c3 --examples Blink pass (synchronous fallback) - bash compile esp32s3 --examples AutoResearch --no-filter pass - bash lint pass Refs #2308 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds CPU core affinity support for audio processor tasks on ESP32 platforms via a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Proc as Processor
participant Worker as fl::task::Worker
participant FreeRTOS as FreeRTOS Kernel
participant Core as Target CPU Core
App->>Proc: setCoreAffinity(core_id)
activate Proc
Proc->>Worker: create Worker(core_id)
activate Worker
Worker->>FreeRTOS: xTaskCreatePinnedToCore(...)
FreeRTOS->>Core: spawn pinned task
Worker-->>Proc: return Worker instance
deactivate Worker
Proc->>Proc: buildAutoPumpTask()
Proc-->>App: success
deactivate Proc
Note over Proc,Core: Audio pump execution
App->>Proc: audio update (every 1ms)
activate Proc
Proc->>Proc: pump_fn: read samples, call update()
Proc->>Worker: run(pump_fn)
activate Worker
Worker->>Worker: acquire mutex
Worker->>FreeRTOS: xSemaphoreGive(mWorkAvailable)
Core->>Core: execute pump_fn on pinned core
FreeRTOS->>FreeRTOS: xSemaphoreGive(mWorkDone)
Worker->>Worker: release mutex
Worker-->>Proc: return
deactivate Worker
Proc-->>App: complete
deactivate Proc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
📊 Header Compilation PerformanceTotal Time: 786.84ms Top 5 Slowest Headers
Warnings
Full Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/AutoResearch/AutoResearchRemote.cpp`:
- Around line 2357-2530: The help output currently omits the newly-bound RPCs;
update the help handler (the mRemote->bind("help", ...) response) to include
entries for testCoroutineCoreAffinity, testFftBackendTiming, and
testWorkerAffinity with short descriptions and indicate any platform-specific
skips (e.g., ESP32/multicore conditions) so operators can discover these RPCs;
ensure the exact symbol names match the bound handlers
testCoroutineCoreAffinity, testFftBackendTiming, and testWorkerAffinity when
adding them to the help JSON/listing.
In `@src/fl/task/worker.cpp.hpp`:
- Around line 60-72: Worker objects created with core == -1 or invalid cores are
being reported as pinned because mImpl/mTask is non-null even when affinity ==
tskNO_AFFINITY; update the WorkerImpl construction logic (around
WorkerImpl::taskEntry / the code that calls xTaskCreatePinnedToCore in Worker or
WorkerImpl) to record actual pinning state and make isPinned() check that
recorded flag (or affinity != tskNO_AFFINITY) rather than just mImpl != nullptr;
only set the "pinned" flag (or mark the instance as pinned) when affinity !=
tskNO_AFFINITY and the task creation succeeds, and ensure the alternative path
for unpinned creation leaves the pinned flag false so isPinned() returns correct
result.
In `@src/fl/task/worker.h`:
- Around line 83-86: Replace the owning raw pointer mImpl with
fl::unique_ptr<WorkerImpl> to ensure RAII ownership: change the declaration of
mImpl in class Worker to fl::unique_ptr<WorkerImpl>, update construction sites
in worker.cpp.hpp to use fl::make_unique<WorkerImpl>(...) instead of new, remove
manual delete calls and replace any explicit reset logic with mImpl.reset() or
rely on destructor, and keep the Worker destructor definition out-of-line (after
WorkerImpl is fully defined) so unique_ptr can be destructed correctly; update
any usage that assumed raw pointer (e.g., mImpl->...) to work unchanged since
unique_ptr supports operator->.
In `@tests/fl/task/worker.cpp`:
- Around line 40-46: The test unconditionally asserts Worker(0).isPinned() is
false which breaks on dual-core ESP32 where Worker may create a pinned task;
guard the assertion so it only runs for host/non-multicore builds. Modify the
FL_TEST_CASE for fl::task::Worker to wrap the FL_CHECK_FALSE(worker.isPinned())
with a preprocessor guard (e.g. `#ifndef` FL_HAS_MULTICORE_AFFINITY / `#endif`) or
an equivalent build-time check so the assertion is compiled only when
FL_HAS_MULTICORE_AFFINITY is not defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0158bec8-6229-415d-9fa9-8fe58557b18a
📒 Files selected for processing (7)
examples/AutoResearch/AutoResearchRemote.cppsrc/fl/audio/audio_processor.cpp.hppsrc/fl/audio/audio_processor.hsrc/fl/task/_build.cpp.hppsrc/fl/task/worker.cpp.hppsrc/fl/task/worker.htests/fl/task/worker.cpp
Comment on lines
+2357
to
+2530
| // Test: CoroutineConfig::core_id actually pins the FreeRTOS task. | ||
| // Validates the plumbing shipped in issue #2308 phase 1 (#2337) and used | ||
| // by Processor::setCoreAffinity() in phase 2. ESP32-only because | ||
| // uxTaskGetCoreID() and multi-core affinity are FreeRTOS-SMP features. | ||
| mRemote->bind("testCoroutineCoreAffinity", [](const fl::json& args) -> fl::json { | ||
| (void)args; | ||
| fl::json r = fl::json::object(); | ||
|
|
||
| #if defined(FL_IS_ESP32) && defined(FL_HAS_MULTICORE_AFFINITY) && FL_HAS_MULTICORE_AFFINITY | ||
| // Drive one test per valid core; report the observed core for each. | ||
| fl::json per_core = fl::json::array(); | ||
| bool all_passed = true; | ||
| for (int requested = 0; requested < FL_CPU_CORES; ++requested) { | ||
| fl::atomic<int> observed(-1); | ||
| fl::atomic<bool> task_ran(false); | ||
| fl::task::CoroutineConfig cfg; | ||
| cfg.func = [&observed, &task_ran]() { | ||
| observed.store(static_cast<int>(xPortGetCoreID())); | ||
| task_ran.store(true); | ||
| }; | ||
| cfg.name = "affinity_probe"; | ||
| cfg.core_id = requested; | ||
| auto t = fl::task::coroutine(cfg); | ||
|
|
||
| uint32_t start = millis(); | ||
| while (!task_ran.load() && (millis() - start) < 2000) { | ||
| delay(10); | ||
| } | ||
|
|
||
| fl::json entry = fl::json::object(); | ||
| entry.set("requested", static_cast<int64_t>(requested)); | ||
| entry.set("observed", static_cast<int64_t>(observed.load())); | ||
| bool ok = task_ran.load() && (observed.load() == requested); | ||
| entry.set("ok", ok); | ||
| per_core.push_back(entry); | ||
| if (!ok) { | ||
| all_passed = false; | ||
| } | ||
| } | ||
| r.set("success", all_passed); | ||
| r.set("cores", per_core); | ||
| r.set("cpuCores", static_cast<int64_t>(FL_CPU_CORES)); | ||
| #else | ||
| // Single-core platform (ESP32-S2/C2/C3/C5/C6/H2) or non-ESP32. | ||
| // Report skip as a pass so the suite doesn't red on these variants. | ||
| r.set("success", true); | ||
| r.set("skipped", true); | ||
| r.set("reason", "FL_HAS_MULTICORE_AFFINITY is 0 on this platform"); | ||
| #endif | ||
| return r; | ||
| }); | ||
|
|
||
| // Test: FFT backend timing — hardware (ESP-DSP) vs software (kiss_fftr). | ||
| // Runs the currently-compiled backend against a known single-tone sine | ||
| // signal, reports µs-per-call and peak bin magnitudes so autoresearch | ||
| // can compare two runs (one with -DFL_FFT_USE_ESP_DSP=1, one without). | ||
| // The comparison lives in the post-processor, not here — this handler | ||
| // only measures ONE backend per build. | ||
| mRemote->bind("testFftBackendTiming", [](const fl::json& args) -> fl::json { | ||
| (void)args; | ||
| fl::json r = fl::json::object(); | ||
|
|
||
| constexpr int N = 512; | ||
| constexpr int SAMPLE_RATE = 44100; | ||
| constexpr int TONE_HZ = 1000; | ||
| constexpr int ITERATIONS = 100; | ||
|
|
||
| // Generate a single-tone sine signal at TONE_HZ, 50 % full-scale. | ||
| fl::vector<fl::i16> samples(N); | ||
| for (int i = 0; i < N; ++i) { | ||
| float phase = 2.0f * 3.14159265358979323846f * | ||
| static_cast<float>(TONE_HZ) * | ||
| static_cast<float>(i) / | ||
| static_cast<float>(SAMPLE_RATE); | ||
| samples[i] = static_cast<fl::i16>(16000.0f * sinf(phase)); | ||
| } | ||
|
|
||
| fl::audio::fft::FFT fft; | ||
| fl::audio::fft::Args fft_args; | ||
| fft_args.samples = N; | ||
| fft_args.bands = 16; | ||
| fft_args.sample_rate = SAMPLE_RATE; | ||
|
|
||
| fl::audio::fft::Bins bins(fft_args.bands); | ||
|
|
||
| // Warm-up: prime the FFT kernel cache so the first measured call | ||
| // doesn't pay for ImplCache allocation. | ||
| fft.run(fl::span<const fl::i16>(samples.data(), N), &bins, fft_args); | ||
|
|
||
| uint32_t t_start = micros(); | ||
| for (int iter = 0; iter < ITERATIONS; ++iter) { | ||
| fft.run(fl::span<const fl::i16>(samples.data(), N), &bins, fft_args); | ||
| } | ||
| uint32_t t_end = micros(); | ||
|
|
||
| float total_us = static_cast<float>(t_end - t_start); | ||
| float us_per_call = total_us / static_cast<float>(ITERATIONS); | ||
|
|
||
| // Find the peak CQ bin and its neighbours for a sanity signal. | ||
| fl::span<const float> raw_bins = bins.raw(); | ||
| int peak_bin = 0; | ||
| float peak_mag = 0.0f; | ||
| for (int b = 0; b < static_cast<int>(raw_bins.size()); ++b) { | ||
| if (raw_bins[b] > peak_mag) { | ||
| peak_mag = raw_bins[b]; | ||
| peak_bin = b; | ||
| } | ||
| } | ||
|
|
||
| r.set("success", us_per_call > 0.0f); | ||
| r.set("usPerCall", static_cast<double>(us_per_call)); | ||
| r.set("iterations", static_cast<int64_t>(ITERATIONS)); | ||
| r.set("samples", static_cast<int64_t>(N)); | ||
| r.set("bands", static_cast<int64_t>(fft_args.bands)); | ||
| r.set("sampleRate", static_cast<int64_t>(SAMPLE_RATE)); | ||
| r.set("toneHz", static_cast<int64_t>(TONE_HZ)); | ||
| r.set("peakBin", static_cast<int64_t>(peak_bin)); | ||
| r.set("peakMagnitude", static_cast<double>(peak_mag)); | ||
| #if FL_FFT_ESP_DSP_ACTIVE | ||
| r.set("backend", "esp-dsp"); | ||
| #else | ||
| r.set("backend", "kiss_fftr"); | ||
| #endif | ||
| return r; | ||
| }); | ||
|
|
||
| // Test: fl::task::Worker end-to-end. | ||
| // Validates that Worker::run(fn) executes fn on the requested core (on | ||
| // dual-core ESP32) or synchronously (everywhere else). Report observed | ||
| // core per core_id so the autoresearch post-processor can confirm | ||
| // phase 2 of #2308 actually offloads across cores. | ||
| mRemote->bind("testWorkerAffinity", [](const fl::json& args) -> fl::json { | ||
| (void)args; | ||
| fl::json r = fl::json::object(); | ||
|
|
||
| #if defined(FL_IS_ESP32) && defined(FL_HAS_MULTICORE_AFFINITY) && FL_HAS_MULTICORE_AFFINITY | ||
| fl::json per_core = fl::json::array(); | ||
| bool all_passed = true; | ||
| for (int requested = 0; requested < FL_CPU_CORES; ++requested) { | ||
| fl::task::Worker worker(requested, "ar_probe"); | ||
| fl::atomic<int> observed(-1); | ||
| fl::atomic<bool> ran(false); | ||
| worker.run([&observed, &ran]() { | ||
| observed.store(static_cast<int>(xPortGetCoreID())); | ||
| ran.store(true); | ||
| }); | ||
|
|
||
| fl::json entry = fl::json::object(); | ||
| entry.set("requested", static_cast<int64_t>(requested)); | ||
| entry.set("observed", static_cast<int64_t>(observed.load())); | ||
| entry.set("isPinned", worker.isPinned()); | ||
| bool ok = ran.load() && (observed.load() == requested); | ||
| entry.set("ok", ok); | ||
| per_core.push_back(entry); | ||
| if (!ok) { | ||
| all_passed = false; | ||
| } | ||
| } | ||
| r.set("success", all_passed); | ||
| r.set("cores", per_core); | ||
| r.set("cpuCores", static_cast<int64_t>(FL_CPU_CORES)); | ||
| #else | ||
| // Single-core / non-ESP32: Worker::run is synchronous. Verify that | ||
| // the fallback actually executes the functor. | ||
| fl::task::Worker worker(0); | ||
| fl::atomic<bool> ran(false); | ||
| worker.run([&ran]() { ran.store(true); }); | ||
| r.set("success", ran.load()); | ||
| r.set("skipped", true); | ||
| r.set("reason", "FL_HAS_MULTICORE_AFFINITY is 0 — verified synchronous fallback"); | ||
| r.set("isPinned", worker.isPinned()); | ||
| #endif | ||
| return r; | ||
| }); |
There was a problem hiding this comment.
Add the new RPCs to help.
testCoroutineCoreAffinity, testFftBackendTiming, and testWorkerAffinity are now bound, but the custom help response still omits them, so operators using the sketch’s help output won’t discover the new autoresearch hooks.
Proposed fix outline
+ fl::json testCoroutineCoreAffinity_fn = fl::json::object();
+ testCoroutineCoreAffinity_fn.set("name", "testCoroutineCoreAffinity");
+ testCoroutineCoreAffinity_fn.set("phase", "Phase 4: Utility");
+ testCoroutineCoreAffinity_fn.set("args", "[]");
+ testCoroutineCoreAffinity_fn.set("returns", "{success, skipped?, cores?, cpuCores?}");
+ testCoroutineCoreAffinity_fn.set("description", "Validate CoroutineConfig core affinity plumbing");
+ functions.push_back(testCoroutineCoreAffinity_fn);
+
+ fl::json testFftBackendTiming_fn = fl::json::object();
+ testFftBackendTiming_fn.set("name", "testFftBackendTiming");
+ testFftBackendTiming_fn.set("phase", "Phase 4: Utility");
+ testFftBackendTiming_fn.set("args", "[]");
+ testFftBackendTiming_fn.set("returns", "{success, usPerCall, backend, peakBin, peakMagnitude}");
+ testFftBackendTiming_fn.set("description", "Measure the active FFT backend timing");
+ functions.push_back(testFftBackendTiming_fn);
+
+ fl::json testWorkerAffinity_fn = fl::json::object();
+ testWorkerAffinity_fn.set("name", "testWorkerAffinity");
+ testWorkerAffinity_fn.set("phase", "Phase 4: Utility");
+ testWorkerAffinity_fn.set("args", "[]");
+ testWorkerAffinity_fn.set("returns", "{success, skipped?, cores?, cpuCores?, isPinned?}");
+ testWorkerAffinity_fn.set("description", "Validate Worker core pinning or synchronous fallback");
+ functions.push_back(testWorkerAffinity_fn);🧰 Tools
🪛 GitHub Actions: linux_native
[error] 2431-2431: C++ compilation failed: use of undeclared identifier 'sinf'; did you mean 'sin8'? (examples/AutoResearch/AutoResearchRemote.cpp:2431:58)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/AutoResearch/AutoResearchRemote.cpp` around lines 2357 - 2530, The
help output currently omits the newly-bound RPCs; update the help handler (the
mRemote->bind("help", ...) response) to include entries for
testCoroutineCoreAffinity, testFftBackendTiming, and testWorkerAffinity with
short descriptions and indicate any platform-specific skips (e.g.,
ESP32/multicore conditions) so operators can discover these RPCs; ensure the
exact symbol names match the bound handlers testCoroutineCoreAffinity,
testFftBackendTiming, and testWorkerAffinity when adding them to the help
JSON/listing.
Comment on lines
+60
to
+72
| BaseType_t affinity = (core >= 0 && core < FL_CPU_CORES) | ||
| ? static_cast<BaseType_t>(core) | ||
| : tskNO_AFFINITY; | ||
| // Slightly elevated priority so the worker preempts idle work but | ||
| // doesn't starve user tasks on the same core. | ||
| BaseType_t rc = xTaskCreatePinnedToCore( | ||
| &WorkerImpl::taskEntry, | ||
| name, | ||
| static_cast<configSTACK_DEPTH_TYPE>(stack_size), | ||
| this, | ||
| tskIDLE_PRIORITY + 5, | ||
| &mTask, | ||
| affinity); |
There was a problem hiding this comment.
Don’t report unpinned workers as pinned.
core == -1 and invalid core IDs are converted to tskNO_AFFINITY, but a real task is still created; then isPinned() returns true because mImpl != nullptr. That makes Worker() look pinned on dual-core ESP32 even though it is explicitly “no pinning”.
Proposed fix
- BaseType_t affinity = (core >= 0 && core < FL_CPU_CORES)
- ? static_cast<BaseType_t>(core)
- : tskNO_AFFINITY;
+ if (core < 0 || core >= FL_CPU_CORES) {
+ mValid = false;
+ return;
+ }
+ BaseType_t affinity = static_cast<BaseType_t>(core);Also applies to: 187-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fl/task/worker.cpp.hpp` around lines 60 - 72, Worker objects created with
core == -1 or invalid cores are being reported as pinned because mImpl/mTask is
non-null even when affinity == tskNO_AFFINITY; update the WorkerImpl
construction logic (around WorkerImpl::taskEntry / the code that calls
xTaskCreatePinnedToCore in Worker or WorkerImpl) to record actual pinning state
and make isPinned() check that recorded flag (or affinity != tskNO_AFFINITY)
rather than just mImpl != nullptr; only set the "pinned" flag (or mark the
instance as pinned) when affinity != tskNO_AFFINITY and the task creation
succeeds, and ensure the alternative path for unpinned creation leaves the
pinned flag false so isPinned() returns correct result.
Comment on lines
+83
to
+86
| private: | ||
| int mCoreId; | ||
| WorkerImpl* mImpl; ///< Owned; platform-specific payload. Null on | ||
| ///< synchronous-fallback platforms. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use RAII for the owned worker implementation.
mImpl is an owning raw pointer, which forces manual allocation/deletion in worker.cpp.hpp. Prefer fl::unique_ptr<WorkerImpl> here and keep the destructor out-of-line after WorkerImpl is complete. As per coding guidelines, “Use proper RAII patterns with smart pointers (fl::shared_ptr, fl::unique_ptr) or moveable wrapper objects instead of raw pointers.”
Proposed direction
+#include "fl/stl/unique_ptr.h"
@@
- WorkerImpl* mImpl; ///< Owned; platform-specific payload. Null on
- ///< synchronous-fallback platforms.
+ fl::unique_ptr<WorkerImpl> mImpl; ///< Platform-specific payload. Null on
+ ///< synchronous-fallback platforms.Then replace manual new/delete in worker.cpp.hpp with fl::make_unique<WorkerImpl>(...) and mImpl.reset().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private: | |
| int mCoreId; | |
| WorkerImpl* mImpl; ///< Owned; platform-specific payload. Null on | |
| ///< synchronous-fallback platforms. | |
| private: | |
| int mCoreId; | |
| fl::unique_ptr<WorkerImpl> mImpl; ///< Platform-specific payload. Null on | |
| ///< synchronous-fallback platforms. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fl/task/worker.h` around lines 83 - 86, Replace the owning raw pointer
mImpl with fl::unique_ptr<WorkerImpl> to ensure RAII ownership: change the
declaration of mImpl in class Worker to fl::unique_ptr<WorkerImpl>, update
construction sites in worker.cpp.hpp to use fl::make_unique<WorkerImpl>(...)
instead of new, remove manual delete calls and replace any explicit reset logic
with mImpl.reset() or rely on destructor, and keep the Worker destructor
definition out-of-line (after WorkerImpl is fully defined) so unique_ptr can be
destructed correctly; update any usage that assumed raw pointer (e.g.,
mImpl->...) to work unchanged since unique_ptr supports operator->.
Comment on lines
+40
to
+46
| FL_TEST_CASE("Worker: isPinned() is false on host / non-ESP32 builds") { | ||
| // Host tests build without FL_HAS_MULTICORE_AFFINITY so the worker | ||
| // falls through to the synchronous path and does not spawn a pinned | ||
| // FreeRTOS task. This encodes that contract. | ||
| fl::task::Worker worker(0); | ||
| FL_CHECK_FALSE(worker.isPinned()); | ||
| } |
There was a problem hiding this comment.
Gate the host-only isPinned() assertion.
On dual-core ESP32, Worker(0) is expected to create a pinned implementation, so this unconditional FL_CHECK_FALSE will fail if the worker tests run on hardware.
Proposed fix
FL_TEST_CASE("Worker: isPinned() is false on host / non-ESP32 builds") {
@@
fl::task::Worker worker(0);
+#if defined(FL_IS_ESP32) && defined(FL_HAS_MULTICORE_AFFINITY) && FL_HAS_MULTICORE_AFFINITY
+ FL_CHECK(worker.isPinned());
+#else
FL_CHECK_FALSE(worker.isPinned());
+#endif
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FL_TEST_CASE("Worker: isPinned() is false on host / non-ESP32 builds") { | |
| // Host tests build without FL_HAS_MULTICORE_AFFINITY so the worker | |
| // falls through to the synchronous path and does not spawn a pinned | |
| // FreeRTOS task. This encodes that contract. | |
| fl::task::Worker worker(0); | |
| FL_CHECK_FALSE(worker.isPinned()); | |
| } | |
| FL_TEST_CASE("Worker: isPinned() is false on host / non-ESP32 builds") { | |
| // Host tests build without FL_HAS_MULTICORE_AFFINITY so the worker | |
| // falls through to the synchronous path and does not spawn a pinned | |
| // FreeRTOS task. This encodes that contract. | |
| fl::task::Worker worker(0); | |
| `#if` defined(FL_IS_ESP32) && defined(FL_HAS_MULTICORE_AFFINITY) && FL_HAS_MULTICORE_AFFINITY | |
| FL_CHECK(worker.isPinned()); | |
| `#else` | |
| FL_CHECK_FALSE(worker.isPinned()); | |
| `#endif` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/fl/task/worker.cpp` around lines 40 - 46, The test unconditionally
asserts Worker(0).isPinned() is false which breaks on dual-core ESP32 where
Worker may create a pinned task; guard the assertion so it only runs for
host/non-multicore builds. Modify the FL_TEST_CASE for fl::task::Worker to wrap
the FL_CHECK_FALSE(worker.isPinned()) with a preprocessor guard (e.g. `#ifndef`
FL_HAS_MULTICORE_AFFINITY / `#endif`) or an equivalent build-time check so the
assertion is compiled only when FL_HAS_MULTICORE_AFFINITY is not defined.
Member
Author
|
Closing unmerged. Parked pending option (A) of #2308 landing first and a hardware-measured FPS gap to justify this work. Full lessons-learned writeup and unparking criteria in the tracking issue: #2359. Phase 1 (#2337, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 of #2308 option (B) — portable way to offload audio pump work off the core running `FastLED.show()`. Builds on the `CoroutineConfig::core_id` plumbing shipped in #2337.
Two new pieces:
1. `fl::task::Worker` — one-shot functor worker
Semaphore handshake: binary semaphore to wake the worker on a pending functor pointer, a second binary semaphore to signal the caller on completion. Caller blocks during run() but the scheduler can service OS/WiFi on the caller's host core in the meantime — so the audio cost lands on the other physical core.
The audio pump functor stays one-shot (no resume, no coroutine loop). The coroutine / FreeRTOS task is the pinning mechanism, nothing more — matching the clarification in the issue thread.
2. `Processor::setCoreAffinity(int core)`
Portable opt-in. `0` / `1` / `-1` accepted. Silently a no-op on platforms that can't express the concept so user code compiles unchanged everywhere:
```cpp
auto proc = AudioManager::instance().add(config);
proc->setCoreAffinity(0); // offload audio to core 0 when dual-core
```
Scheduler still drives `every_ms(1)`; only the inner pump body hops to the worker. Threading caveat documented: when offloaded, `onBass` / `onBeat` / `onVibeLevels` callbacks fire on the pinned core, so user code must not call `FastLED.show()` or touch LED arrays from inside those callbacks.
Autoresearch hooks for hardware validation
Three new RPC handlers in `examples/AutoResearch/AutoResearchRemote.cpp`, all added to `testCoroutineAll`:
Test plan — local verification
Refs #2308
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests