Back to Fastled

C++ Code Standards

agents/docs/cpp-standards.md

3.10.437.8 KB
Original Source

C++ Code Standards

Namespace and Headers

  • Use fl:: namespace instead of std::
  • **If you want to use a stdlib header like <type_traits>, look check for equivalent in fl/type_traits.h
  • Vector type usage: Use fl::vector<T> for dynamic arrays. The implementation is in src/fl/stl/vector.h.

fl::net Namespace Convention

Files in src/fl/net/ follow a two-level namespace pattern:

  • Primary user-facing type (if a single one exists) → lives directly in fl::net
    • Example: fl::net::OTA
  • Supporting types (enums, transport types, options) → live in fl::net::<module> sub-namespace
    • Example: fl::net::ota::Service
  • Facade / collection modules (no single primary type) → all types in fl::net::<module>
    • Example: fl::net::http::*, fl::net::ble::*

When moving a type into a sub-namespace, drop the module prefix from the name since the namespace already provides context:

  • OTAServicefl::net::ota::Service
  • BleStatusInfofl::net::ble::StatusInfo
  • BleTransportStatefl::net::ble::TransportState
FilePrimary type (fl::net)Sub-namespace (fl::net::<mod>)
ota.hOTAota::Service
rpc.h(none — transport aliases)rpc::StreamClient, rpc::StreamServer
http.h(none — facade)http::Server, http::Response, http::fetch_get, …
ble.h(none — collection)ble::TransportState, ble::StatusInfo, ble::createTransport, …

API Object Pattern

An API object is a public-facing wrapper header that lives alongside a directory of the same name containing implementation details. Users only ever #include the API header.

File layout:

src/fl/stl/fixed_point.h        ← API object (public interface)
src/fl/stl/fixed_point/          ← implementation directory
    s16x16.h                     ← concrete type
    base.h                       ← shared internals
    ...

Rules:

  1. One header, one directory, same name. foo.h is the public API; foo/ holds everything behind it.
  2. The API object wraps, it doesn't implement. It inherits from or delegates to concrete types in the directory. It adds uniformity (common interface, operator forwarding) and convenience (type promotion, free functions) but contains no core logic itself.
  3. Concrete types are self-contained. Each file in the directory is independently functional. The API object composes them — it doesn't modify them.
  4. A trait maps parameters to concrete types. A dispatch mechanism (e.g. fixed_point_impl<IntBits, FracBits, Sign>) selects the right concrete type at compile time. Invalid combinations fail via an undefined primary template.
  5. The API object re-exposes everything through a uniform interface. Operators, math functions, conversions — all forwarded. The wrapper adds no new logic, just type-safe forwarding.
  6. Free functions live in the API header. ADL-enabled free functions (e.g. fl::sin(), fl::floor()) are SFINAE-gated to the wrapper type, giving users a natural calling convention.
  7. Cross-type interactions live in the API header. Operations spanning multiple concrete types (e.g. s0x32 * s16x16 → s16x16) are defined at the bottom, after all types are visible.

Exemplar: src/fl/stl/fixed_point.h wrapping src/fl/stl/fixed_point/

Platform Dispatch Headers

  • FastLED uses dispatch headers in src/platforms/ (e.g., int.h, io_arduino.h) that route to platform-specific implementations via coarse-to-fine detection. See src/platforms/README.md for details.
  • Platform-specific headers (src/platforms/**): Header files typically do NOT need platform guards (e.g., #ifdef ESP32). Only the .cpp implementation files require guards. When the .cpp file is guarded from compilation, the header won't be included. This approach provides better IDE code assistance and IntelliSense support.
    • Correct pattern:
      • header.h: No platform guards (clean interface)
      • header.cpp: Has platform guards (e.g., #ifdef ESP32 ... #endif)
    • Avoid: Adding #ifdef ESP32 to both header and implementation files (degrades IDE experience)

Sparse Platform Dispatch Pattern (.cpp.hpp files)

A .cpp.hpp file is a compile-time component router that assembles a complete feature from sparse, per-platform fragments with null/no-op fallbacks.

Key properties:

  1. Sparse — No platform implements everything. Each platform contributes only the pieces it supports (e.g., ESP32 provides FreeRTOS tasks but not cooperative coroutines; Teensy provides cooperative context switching but not OS tasks).
  2. Fallback — Missing pieces get a null/no-op implementation automatically. If a platform doesn't provide a component, the system still works — those features are simply inert.
  3. Component routing — The .cpp.hpp file examines platform defines and #includes the right fragments. It composes a complete system from whatever pieces the current platform offers.

Why .cpp.hpp instead of .cpp or .h?

  • Contains function definitions (like .cpp) — so it can't be a normal .h (would cause multiple-definition linker errors).
  • Designed to be #included from exactly one translation unit (like .hpp) — not compiled on its own.
  • Marked // IWYU pragma: private to enforce single inclusion.
  • The extension signals: "I contain implementations, I'm meant to be included, and I must only be included once."

Naming convention (future standard):

  • component.impl.cpp.hpp — The router file. Contains the #if/#elif dispatch logic that selects which platform fragment to include. Included from exactly one translation unit.
  • component_<platform>.impl.hpp — Platform-specific implementation fragments that the router includes (e.g., coroutine_esp32.impl.hpp, coroutine_wasm.impl.hpp).

The names make roles explicit: .impl.cpp.hpp = "implementation router, include me once." _<platform>.impl.hpp = "platform fragment, the router includes me."

Structure of a .impl.cpp.hpp router:

cpp
// IWYU pragma: private

// Platform detection
#include "platforms/arm/is_arm.h"
#include "platforms/esp/is_esp.h"
#include "platforms/wasm/is_wasm.h"

#if defined(FL_IS_WASM)
#include "platforms/wasm/feature_wasm.impl.hpp"
#elif defined(FASTLED_STUB_IMPL)
#include "platforms/stub/feature_stub.impl.hpp"
#elif defined(FL_IS_ESP32)
#include "platforms/esp/32/feature_esp32.impl.hpp"
#else
// Fallback: null/no-op implementation
#include "platforms/shared/feature_null.impl.hpp"
#endif

Current exemplar: src/platforms/coroutine.impl.cpp.hpp.

No-op Fallback (_noop.hpp)

The dispatcher's #else branch needs a target — that's the no-op implementation. Platforms without hardware support for a feature get an implementation that satisfies the same API signature with empty / zero-returning bodies, so user code compiles unconditionally on any platform.

Naming and location:

  • Lives in src/platforms/shared/ with the suffix _noop.hpp — e.g., pin_noop.hpp, memory_noop.hpp, simd_noop.hpp, codec/h264_noop.hpp.
  • Never call it _null.hpp, _stub.hpp, or _dummy.hpp. The keyword is noop — this is what to grep for to find the convention.

File contents:

  • Mark // IWYU pragma: private (header is only included via the dispatcher, never directly).
  • Functions are inline to stay ODR-safe if the header is reached from multiple TUs.
  • Live in the fl::platforms:: namespace (not fl:: — keeps the public surface clean).
  • Body convention: return a safe API-defined inert default — commonly 0, false, nullptr, or empty span, but documented sentinel values are also valid when the API contract requires them (e.g., pin_noop.hpp::needsPwmIsrFallback returns true because the no-op platform genuinely needs the ISR fallback path; setPwmFrequencyNative returns -4 as the documented "not supported" error code). Never assert, throw, or call FL_WARN. A no-op is a no-op — the whole point is that user code keeps running on unsupported platforms.
  • For multi-tier APIs that ship FL_<COMPONENT>_HAS_<FEATURE> capability macros (see Type 3 below), the no-op header must not define any of the boolean capability flags — so #ifdef FL_<COMPONENT>_HAS_<FEATURE> evaluates to false on the unsupported platform. It must still define any numeric properties (e.g., #define FL_WATCHDOG_PERSIST_BYTES 0) so user code that reads them compiles.

Stub-only variant (_stub_noop.h): When the no-op only makes sense for the host/stub build (because it pretends to be a real OS primitive that no real MCU exposes), put it in src/platforms/stub/ with the _stub_noop.h suffix instead — e.g., mutex_stub_noop.h, thread_stub_noop.h, semaphore_stub_noop.h. Rule of thumb:

  • shared/<x>_noop.hpp — fallback that any unsupported platform may include via the dispatcher.
  • stub/<x>_stub_noop.h — only the host/stub build consumes it.

Current exemplars: src/platforms/shared/memory_noop.hpp, src/platforms/shared/pin_noop.hpp, src/platforms/shared/simd_noop.hpp, src/platforms/shared/codec/h264_noop.hpp.

Span Usage

  • Automatic span conversion: fl::span<T> has implicit conversion constructors - you don't need explicit fl::span<T>(...) wrapping in function calls. Example:
    • Correct: verifyPixels8bit(output, leds) (implicit conversion)
    • Verbose: verifyPixels8bit(output, fl::span<const CRGB>(leds, 3)) (unnecessary explicit wrapping)
  • Container auto-conversion: fl::span auto-converts from containers with data()/size(). Pass the container directly:
    • AudioSample(data, timestamp) or fl::span<const fl::i16> s = myVector;
    • AudioSample(fl::span<const fl::i16>(data.data(), data.size()), timestamp)
  • Prefer passing and returning by span: Use fl::span<T> or fl::span<const T> for function parameters and return types unless a copy of the source data is required:
    • Preferred: fl::span<const uint8_t> getData() (zero-copy view)
    • Preferred: void process(fl::span<const CRGB> pixels) (accepts arrays, vectors, etc.)
    • Avoid: std::vector<uint8_t> getData() (unnecessary copy)
    • Use fl::span<const T> for read-only views to prevent accidental modification

Macro Definition Patterns

Type 1: Platform/Feature Detection Macros (defined/undefined pattern)

Platform Identification Naming Convention:

  • MUST follow pattern: FL_IS_<PLATFORM><_OPTIONAL_VARIANT>
  • Correct: FL_IS_STM32, FL_IS_STM32_F1, FL_IS_STM32_H7, FL_IS_ESP_32S3
  • Wrong: FASTLED_STM32_F1, FASTLED_STM32 (missing FL_IS_ prefix)
  • Wrong: FL_STM32_F1, IS_STM32_F1 (incorrect prefix pattern)

Detection and Usage:

  • Platform defines like FL_IS_ARM, FL_IS_STM32, FL_IS_ESP32 and their variants
  • Feature detection like FASTLED_STM32_HAS_TIM5, FASTLED_STM32_DMA_CHANNEL_BASED (not platform IDs)
  • These are either defined (set) or undefined (unset) - NO values
  • Correct: #ifdef FL_IS_STM32 or #ifndef FL_IS_STM32
  • Correct: #if defined(FL_IS_STM32) or #if !defined(FL_IS_STM32)
  • Define as: #define FL_IS_STM32 (no value)
  • Wrong: #if FL_IS_STM32 or #if FL_IS_STM32 == 1
  • Wrong: #define FL_IS_STM32 1 (do not assign values)

Type 2: Configuration Macros with Defaults (0/1 or numeric values)

  • Settings that have a default when undefined (e.g., FASTLED_USE_PROGMEM, FASTLED_ALLOW_INTERRUPTS)
  • Numeric constants (e.g., FASTLED_STM32_GPIO_MAX_FREQ_MHZ 100, FASTLED_STM32_DMA_TOTAL_CHANNELS 14)
  • These MUST have explicit values: 0, 1, or numeric constants
  • Correct: #define FASTLED_USE_PROGMEM 1 or #define FASTLED_USE_PROGMEM 0
  • Correct: #define FASTLED_STM32_GPIO_MAX_FREQ_MHZ 100
  • Check as: #if FASTLED_USE_PROGMEM or #if FASTLED_USE_PROGMEM == 1
  • Wrong: #define FASTLED_USE_PROGMEM (missing value - ambiguous default behavior)

Type 3: Component Capability Flags (per-subsystem, defined/undefined for booleans, numeric for properties)

For a subsystem (e.g., Watchdog, Audio, Codec) that has multi-tier platform support, document each platform's capabilities with FL_<COMPONENT>_HAS_<FEATURE> flags. These let user code compile-time gate optional features without doing platform detection itself.

Spelled-out names — no acronyms in macro identifiers. The macro name and the public API name share a single vocabulary. If the API is FastLED.watchdog() then the macros are FL_WATCHDOG_*, not FL_WDT_*. If the API is FastLED.audio() then the macros are FL_AUDIO_*. Acronyms in macro names cost grepability and force readers to learn a second name for the same thing.

  • ✅ Correct: FL_WATCHDOG_HAS_WINDOW_MODE, FL_WATCHDOG_PERSIST_BYTES, FL_AUDIO_HAS_I2S, FL_CODEC_HAS_H264
  • ❌ Wrong: FL_WDT_HAS_WINDOW_MODE (abbreviation hides the component), FASTLED_WATCHDOG_WINDOW_MODE (use FL_ for newer per-component flags), WATCHDOG_HAS_WINDOW_MODE (missing prefix)

Boolean capability flags:

  • Defined or undefined (same shape as Type 1) — NO values
  • Define as: #define FL_WATCHDOG_HAS_WINDOW_MODE (no value)
  • Check as: #if defined(FL_WATCHDOG_HAS_WINDOW_MODE) or #ifdef FL_WATCHDOG_HAS_WINDOW_MODE

Numeric properties (sizes, limits, capacities):

  • Explicit numeric values (same shape as Type 2)
  • Define as: #define FL_WATCHDOG_PERSIST_BYTES 16
  • Check as: #if FL_WATCHDOG_PERSIST_BYTES >= 8
  • User code may static_assert(FL_WATCHDOG_PERSIST_BYTES >= 8, "...") to enforce minimums the unified API promises.

Where they live: Defined in the per-platform *.impl.hpp (or the component's public header) that implements the feature. The _noop.hpp fallback defines none of the boolean flags (so #ifdef checks correctly evaluate false on unsupported platforms) and always defines a zero/default for every numeric property the unified API exposes (so #if-on-numeric-value still compiles).

Distinction from Type 1 (FL_IS_*): Type 1 says "I am running on platform X." Type 3 says "this component has feature Y on this platform." A single platform may carry many Type 3 flags from independent components.

Warning and Debug Output

  • Use proper warning macros from fl/stl/compiler_control.h
  • Use FL_DBG("message" << var) for debug prints (easily stripped in release builds)
  • Use FL_WARN("message" << var) for warnings (persist into release builds)
  • Avoid fl::printf, fl::print, fl::println - prefer FL_DBG/FL_WARN macros instead
  • Note: FL_DBG and FL_WARN use stream-style << operator, NOT printf-style formatting

Teensy 3.x __cxa_guard Conflicts

  • Problem: Function-local statics with non-trivial constructors generate implicit __cxa_guard_* function calls. If Teensy's <new.h> is included after the compiler sees the static, the signatures conflict.
  • Preferred Solution (.cpp.hpp files): Include <new.h> early on Teensy 3.x to declare the guard functions before use:
    cpp
    // Teensy 3.x compatibility: Include new.h before function-local statics
    #if defined(__MK20DX128__) || defined(__MK20DX256__) || defined(__MK64FX512__) || defined(__MK66FX1M0__)
        #include <new.h>
    #endif
    
    • Example: src/fl/async.cpp.hpp (includes <new.h> early to prevent conflicts)
    • This ensures the compiler uses the correct __guard* signature
  • Alternative Solution (header files): Move static initialization to corresponding .cpp file
    • Example: See src/platforms/shared/spi_hw_1.{h,cpp} for the correct pattern
  • Exception: Statics inside template functions are allowed (each template instantiation gets its own static, avoiding conflicts)
  • Linter: Enforced by ci/lint_cpp/test_no_static_in_headers.py for critical directories (src/platforms/shared/, src/fl/, src/fx/)
  • Suppression: Add // okay static in header comment if absolutely necessary (use sparingly)

Channel Engine DMA Wait Pattern

  • onBeginFrame() / show() must wait for poll() == READY before starting a new frame — use a simple while (poll() != READY) loop
  • Yield in wait loops with fl::task::run() — never busy-spin without yielding. Use fl::task::run(250, fl::task::ExecFlags::SYSTEM) inside wait loops to yield to the OS scheduler (FreeRTOS vTaskDelay(0) on ESP32, std::this_thread::yield() on host). This prevents watchdog timeouts and starvation of WiFi/BT/system tasks. Include fl/task/executor.h.
    • Do NOT use bare fl::yield(), vTaskDelay(), or taskYIELD()fl::task::run() is the unified API
    • For tight DMA polling use ExecFlags::SYSTEM (OS yield only, minimal overhead)
    • For longer waits use ExecFlags::ALL (also pumps coroutines and scheduled tasks)
  • Do NOT branch on DRAINING or other intermediate states inside the wait loop in show() or onBeginFrame(). The poll() method drives the state machine to READY; callers just wait for it.
  • onEndFrame() may wait for READY or DRAINING — after show() kicks off DMA, it's fine to return once DMA is running (DRAINING). onBeginFrame() will ensure READY before the next frame.
  • Rationale: Branching on intermediate states in the "wait for previous frame" path splits logic across multiple places and makes the code harder to reason about.
  • Reference: See src/fl/channels/README.md → "DMA Wait Pattern" section

Naming Conventions

  • Member variable naming: All member variables in classes and structs MUST use mCamelCase (prefix with 'm'):
    • Correct: int mCount;, fl::string mName;, bool mIsEnabled;
    • Wrong: int count;, fl::string name;, bool isEnabled;
    • The 'm' prefix clearly distinguishes member variables from local variables and parameters
  • Follow existing code patterns and naming conventions

Public Settings Pattern (Global Configuration via CFastLED)

Core Principle: Any new global / library-wide configuration setter MUST be exposed as a public method on the CFastLED god instance in src/FastLED.h. The implementation may live as a free function in the fl:: namespace for ADL / testability, but the documented user-facing entry point is FastLED.setX(...), not fl::set_x(...).

Rationale: CFastLED is the discoverable surface — users see FastLED.setBrightness(), FastLED.setMaxRefreshRate(), FastLED.setPowerModel() in every sketch and expect every other knob to live there too. Free-function-only configuration ratchets the API surface into a second, undocumented place that users won't find and that drifts out of sync with the god instance.

Exemplar (src/FastLED.h:1455):

cpp
// Free function in fl:: namespace — does the work, testable in isolation
namespace fl { void set_power_model(const PowerModelRGB& m) noexcept; }

// Public entry point on CFastLED — thin delegator, this is what users call
class CFastLED {
    inline void setPowerModel(const PowerModelRGB& model) {
        set_power_model(model);
    }
};

Rules:

  1. NEVER ship a new fl::set_* / fl::enable_* / fl::disable_* / fl::use_* free function that mutates library-wide state without a matching CFastLED::setX() / enableX() wrapper.
    • ❌ Bad: fl::set_input_gamut(&profile, fl::InputGamut::Rec709); as the documented call site
    • ✅ Good: FastLED.setInputGamut(&profile, fl::InputGamut::Rec709); (god instance), with a fl::set_input_gamut(...) free function backing it
  2. The wrapper is a inline one-liner that delegates — no logic, no validation, no error handling. The free function holds all behavior.
  3. Per-object configuration (e.g. one strip's diode profile, one controller's correction) MAY live on the per-object API. This rule targets library-wide / process-wide / default-profile state.
  4. Documentation, examples, and PR descriptions reference the god-instance form. The free function is an implementation detail.
  5. ⚠️ Strict for new code; transitional allowlist for legacy names only. Every new public global setter under fl:: must ship with a CFastLED wrapper. A small transitional allowlist (GRANDFATHERED_NAMES in ci/lint_cpp/public_settings_pattern_checker.py) exempts pre-existing bare setters (e.g. fl::set_input_gamut #2710, fl::enable_rgbw_colorimetric_lut, fl::set_rgbww_colorimetric_profile) until their wrappers land. Entries are removed as each name is wrapped — the goal is an empty allowlist. New additions do NOT get grandfathered.

Check Process:

  1. For every new public function in src/fl/**/*.h whose name matches ^set_|^enable_|^disable_|^use_ and that mutates a static / global / namespace-scope variable, grep src/FastLED.h for a CFastLED method that delegates to it.
  2. If none exists: violation — add the wrapper in the same PR.
  3. Enforced by ci/lint_cpp/public_settings_pattern_checker.py. The checker carries a shrinking GRANDFATHERED_NAMES allowlist for legacy bare setters; remove a name from the list once its CFastLED wrapper is merged.

Where the rule does NOT apply:

  • Helpers, constructors, factory functions — these are not setters of global state.
  • Functions in fl:: that operate on caller-owned objects without touching global state (fl::fill_solid(span, color)).
  • Internal fl::detail:: / anonymous-namespace functions — not public API.

Singleton-Stored Configuration: Own by Value, Never by Borrowed Pointer

Core Principle: Any public fl::set_* API that stores a configuration profile / settings object in process-wide (or static / namespace-scope) state MUST store by value (copy / move) and never retain a caller-owned pointer. The parameter SHOULD be const T& (or T&&); a const T* parameter is permitted only as a signal for nullable-reset (nullptr means "revert to default") and only if the implementation copies the pointed-to value into a value-typed slot on non-null and clears that slot on null. Storing the raw pointer (sActive = profile) is always a deterministic use-after-scope bug when callers pass a stack temporary like auto p = make_profile(); fl::set_x(&p); /* p goes out of scope */.

Rules:

  1. NEVER store a borrowed pointer to a caller-owned value in a singleton.
    • ❌ Bad:
      cpp
      namespace { const RgbcctProfile* sActive = nullptr; }
      void set_rgbww_colorimetric_profile(const RgbcctProfile* p) FL_NOEXCEPT {
          sActive = p;  // caller's lifetime, BOOM if they pass a stack temporary
      }
      
    • ✅ Good (value semantics, preferred):
      cpp
      namespace { RgbcctProfile sActive = kRgbwwDefaultProfile; }
      void set_rgbww_colorimetric_profile(const RgbcctProfile& p) FL_NOEXCEPT {
          sActive = p;  // copy, lifetime owned by library
      }
      
    • ✅ Good (nullable-reset exception — const T* allowed when null means "reset"):
      cpp
      namespace { RgbcctProfile sActive = kRgbwwDefaultProfile; }
      void set_rgbww_colorimetric_profile(const RgbcctProfile* p) FL_NOEXCEPT {
          sActive = (p != nullptr) ? *p : kRgbwwDefaultProfile;  // copy on non-null, reset on null
      }
      
  2. Profile getters return const T& to the owned copy, not a pointer that could be nullptr.
  3. For polymorphic configuration (rare), use fl::shared_ptr<T> or fl::unique_ptr<T> — never a raw const T*.

Rationale: The colorimetric / power-model / driver-config setters are the only place this rule fires in the current codebase, but every one of them got a use-after-scope CodeRabbit finding (#2554, #2560, #2588, #2683, #2682) before the rule was written. The Public Settings Pattern (above) tells you where the setter lives (god instance); this rule tells you how it stores the value. The nullable-reset exception above accommodates the established set_rgbww_colorimetric_profile(const T*) shape — pointer parameter, value-typed storage — which is correct and idiomatic for "pass nullptr to revert."

Check Process:

  1. For each fl::set_* setter that stores into a static / namespace-scope variable, verify the storage is value-typed (T, not const T*).
  2. If the parameter is const T* AND the storage is const T*, flag as HIGH severity (pointer storage = use-after-scope).
  3. If the parameter is const T* but the storage is T and the body copies on non-null + resets on null, that's the allowed nullable-reset shape — no violation.

In-Place Profile Mutation Bumps a Version (Cache Invalidation Contract)

Core Principle: If set_X(T* obj, ...) writes into the fields of a caller-owned *obj, and any subsystem caches values derived from *obj keyed only on (obj_ptr, ...), then the setter MUST bump obj->mCacheVersion (or call obj->invalidate()) and the cache key MUST include that version. Otherwise the cache returns stale data the next time the same pointer is passed in.

Rules:

  1. NEVER mutate a profile field in place without bumping a version counter when a cache exists keyed on the profile pointer.
    • ❌ Bad:
      cpp
      void set_input_gamut(DiodeProfile* p, InputGamut g) FL_NOEXCEPT {
          apply_gamut(p, g);  // rewrites p->input_xy_*
          // …no version bump; ProfileCache keyed on (p, cct) still returns stale M_src
      }
      
    • ✅ Good:
      cpp
      void set_input_gamut(DiodeProfile* p, InputGamut g) FL_NOEXCEPT {
          apply_gamut(p, g);
          ++p->mCacheVersion;  // forces any derived-value cache to rebuild
      }
      // and:
      struct CacheKey { const DiodeProfile* p; u32 version; int cct; };
      
  2. Document the cache-invalidation contract in the setter's doxygen: "Mutates *p in place; bumps p->mCacheVersion so derived caches rebuild on next access."
  3. Prefer immutable profiles when possible. A DiodeProfile that ships as const to all consumers and is replaced wholesale (set_profile(const T&)) eliminates this entire class of bug — see Singleton-Stored Configuration rule above.

Rationale: Drove four CodeRabbit findings in the survey window (#2554, #2589, #2707, #2711) and is the root cause of the colorimetric "looks right in tests but stale in production" class. Tests typically construct one profile per test case so the cache never hits — the bug only shows up in real applications that mutate one shared profile across frames.

Check Process:

  1. For each fl::set_*(T* obj, ...) where the body mutates *obj, search the codebase for a cache keyed on obj (typical names: ProfileCache, kCache, sCachedDerived).
  2. If a cache exists and the setter doesn't bump a version field, flag as HIGH severity.

Contract Change Fanout (API Surface Changes Touch All Layers in One PR)

Core Principle: When a PR changes a numeric bound, enum range, struct shape, or semantic of a public API surface, it MUST update every consumer of that contract in the same PR: (a) the producer / setter, (b) the validator, (c) unit tests, (d) docs (agents/docs/*, AutoResearch JSON-RPC reference, RPC handlers), and (e) any matching debug-metrics / stats struct. Partial sweeps cause silent producer/consumer skew.

Rules:

  1. NEVER widen a bound without sweeping the validator and tests.
    • ❌ Bad: examples/AutoResearch/AutoResearchRemote.cpp bumps laneSizes max from 8 to 16, but src/fl/channels/validation.cpp.hpp still rejects > 8 — silent rejection at runtime.
    • ✅ Good: Same PR updates the example, the validator, the test fixture asserting the new max, and the JSON-RPC schema doc.
  2. NEVER add a struct field without updating the debug-metrics dump and the parser.
  3. NEVER change available() / empty() / size() semantics without updating the docstring AND every caller that compares against constants.

Rationale: This is a broadening of the existing "API Unit Change" rule (which was scoped to _ms / _us / _bytes suffixes). The recurring CodeRabbit pattern (#2621, #2648, #2669, #2682) is more general — any contract change, not just unit changes, needs the same fanout.

Check Process (PR-level):

  1. If the diff modifies a public setter, enum, struct field, or named constant that defines a contract — grep for the contract name across src/, examples/, tests/, and agents/docs/.
  2. If any consumer doesn't appear in the diff, either update it in the same PR or document why it doesn't need updating (with a comment in the PR description).

fl::span for Callback Typedefs and Small Fixed-Size Arrays

Core Principle: The general fl::span rule (above) covers function parameters but is repeatedly missed for two specific shapes: (a) fl::function<...> callback / handler typedefs, and (b) small fixed-size array parameters like const float xy[2]. These slip through because they don't look like the canonical void f(const u8*, size_t) pattern.

Rules:

  1. NEVER declare a callback typedef with a (ptr, size) shape.
    • ❌ Bad: using write_bytes_handler_t = fl::function<size_t(const u8*, size_t)>;
    • ✅ Good: using write_bytes_handler_t = fl::function<size_t(fl::span<const u8>)>;
  2. NEVER take small fixed-size arrays as raw pointers.
    • ❌ Bad: void set_input_gamut(DiodeProfile* p, InputGamut g, const float white_xy[2]);
    • ✅ Good: void set_input_gamut(DiodeProfile* p, InputGamut g, fl::span<const float, 2> white_xy);
    • ✅ Good (alternative — by-value pair): void set_white_point(DiodeProfile* p, fl::vec2f white_xy);
  3. For RGBW / RGBWW multi-channel output, prefer a fl::span<u8, 4> (or 5) over five separate u8* parameters.

Scoped exception — deferred legacy migrations: A (ptr, size) callback typedef MAY be temporarily retained in a legacy layer when migrating it would force a synchronized rewrite of every caller and the migration is being deferred to a follow-up PR. The exception requires (a) a comment at the typedef site referencing the migration plan / tracking issue, (b) naming the legacy location (current example: src/fl/stl/cstdio.h retains raw pointer+length handler signatures intentionally), and (c) the exception is timeboxed — it must be revisited at the next layer-wide migration window. Permanent divergence is not permitted; the goal is "all-or-nothing per-layer sweep when the engineering budget allows."

Rationale: The base span rule prevents most of the pattern but CodeRabbit caught these two shapes in #2560, #2683, #2711. Just adding the two shapes to the canonical examples will close most remaining gaps. The scoped exception accommodates the real-world case where touching every caller in one PR isn't tractable; without it the rule pushes engineers toward unprincipled mixed APIs.

ISR-Shared State: fl::atomic or Critical Section, Never Bare volatile

Core Principle: Any field that is written from an ISR callback context and read from task context (or vice versa), or written from two task contexts without other locking, MUST be either an fl::atomic<T> with explicit memory_order, or guarded by portENTER_CRITICAL_ISR/portEXIT_CRITICAL_ISR (ISR side) and portENTER_CRITICAL/portEXIT_CRITICAL (task side). Plain volatile only prevents the compiler from caching the load — it does NOT provide atomicity, ordering, or visibility across cores.

Rules:

  1. NEVER use int / bool / volatile int for state shared with an ISR.
    • ❌ Bad:
      cpp
      volatile int mPendingTransmits = 0;
      // ISR: --mPendingTransmits;          (not atomic on dual-core, no ordering)
      // task: while (mPendingTransmits) {} (no acquire fence)
      
    • ✅ Good (atomic):
      cpp
      fl::atomic<int> mPendingTransmits{0};
      // ISR: mPendingTransmits.fetch_sub(1, fl::memory_order_release);
      // task: while (mPendingTransmits.load(fl::memory_order_acquire) > 0) {}
      
    • ✅ Good (critical section, when the state is more complex than a single word):
      cpp
      // ISR side:
      portENTER_CRITICAL_ISR(&mLock);
      mTransmitting = false;
      mNextDescriptor = next;
      portEXIT_CRITICAL_ISR(&mLock);
      // task side:
      portENTER_CRITICAL(&mLock);
      bool tx = mTransmitting;
      portEXIT_CRITICAL(&mLock);
      
  2. NEVER use std::atomic<T> — FastLED targets AVR / older embedded toolchains that don't have <atomic>. Use fl::atomic<T> from fl/stl/atomic.h. (See the project-wide rule: prefer fl:: over std:: except for low-level metaprogramming traits.)
  3. NEVER clear an ISR-shared flag in task context before all in-flight ISR work has drained.
    • This is the underrun-race pattern — clearing mTransmitting = false in refill() while a chunk-done ISR is still pending corrupts the next frame's state.

Rationale: ESP32 / RP2040 / dual-core MCUs make this a hardware-correctness issue, not just a style preference. Caught only twice in the survey window (#2682, #2703), but both were high-severity races that would have shipped without manual review.

Check Process:

  1. For each field declared in a class that has an IRAM_ATTR callback or static void IRAM_ATTR on_* ISR, verify it's either fl::atomic<T> or guarded by a portMUX_TYPE critical section.
  2. Plain volatile on these fields is a violation.

Unsigned Narrowing Cast Must Clamp

Core Principle: When summing channel values, brightness contributions, or any per-element accumulator whose total may exceed the destination type's max, you MUST clamp before the narrowing cast. static_cast<u8>(x) silently wraps at 256, which is defined behavior in C++ but almost always the wrong behavior.

Rules:

  1. NEVER static_cast<u8> a value that can exceed 255.
    • ❌ Bad:
      cpp
      u32 total_mW = red_mW + (white_mW + warm_white_mW) / 3;
      u8 byte = static_cast<u8>(total_mW);  // wraps if total > 255
      
    • ✅ Good:
      cpp
      u32 total_mW = red_mW + (white_mW + warm_white_mW) / 3;
      u8 byte = static_cast<u8>(fl::min<u32>(255u, total_mW));
      
    • ✅ Good (saturating addition helper):
      cpp
      u8 byte = fl::qadd8(red_byte, white_byte);  // saturates at 255
      
  2. The same rule applies to u16 narrowingstatic_cast<u16>(x) where x is u32 and the source can exceed 65535 must clamp first.
  3. Pure narrowing for known-in-range values is fine — e.g. u8 byte = static_cast<u8>(rgb.r); when rgb.r is already a u8 typed via a different value path. The rule fires when there's been arithmetic that can overflow the destination type.

Rationale: Sibling to the existing "Signed Integer Overflow (UB)" rule. Unsigned wrap is defined but produces silently-wrong color / power / brightness values. The signed-UB rule (above) does NOT catch this — u8 + u16 → wrap is fine according to the standard but is the bug pattern flagged twice in #2560 (first review ignored, second review re-flagged).

Test Mirrors Must Call Production Symbols, Not Redeclare Them

Core Principle: When tests/fl/foo/test_bar.cpp mirrors part of src/fl/foo/bar.cpp.hpp (e.g. to verify mathematical identities like Hermite-basis sum = 1), the test MUST call the production symbol directly. Re-declaring the algorithm as a local lambda or helper inside the test file means the test passes regardless of bugs introduced into the production implementation.

Rules:

  1. NEVER redefine a production algorithm inside a test file just to assert properties of it.
    • ❌ Bad:
      cpp
      // tests/fl/gfx/rgbw_colorimetric.cpp
      FL_TEST_CASE("hermite basis sums to 1") {
          auto h0 = [](float t) { return (1 - t)*(1 - t)*(1 + 2*t); };
          auto h1 = [](float t) { return t*t*(3 - 2*t); };
          // …asserts h0(t) + h1(t) == 1. Production hermite_basis is never called.
      }
      
    • ✅ Good:
      cpp
      #include "fl/gfx/rgbw_colorimetric.h"
      FL_TEST_CASE("hermite basis sums to 1") {
          float out[2];
          fl::gfx::hermite_basis(0.5f, out);
          FL_CHECK_CLOSE(out[0] + out[1], 1.0f, 1e-6f);
      }
      
  2. Mirror files are allowed only when test is verifying byte-for-byte text equivalence of two sources (e.g. a port from Python reference). In that case the mirror must be obvious from the file name (e.g. *_mirror_for_test.cpp.hpp) and a static_assert / runtime check must compare the mirror's output against the production output on a sample input.
  3. Header-only inline algorithms can be tested by calling the production header directly — there's no link concern.

Rationale: Caught in #2683, #2707, #2709. The bug pattern: a future regression in the production implementation (e.g. someone swaps the sign of a Hermite coefficient) passes all tests because the tests verify a local copy of the correct algorithm. The production code can silently drift.

ASCII-Only in Source Files (No Emoji, No Unicode Math Glyphs)

Core Principle: Every C++ source file (.h, .cpp, .hpp, .cpp.hpp) and every Python source file in this repo MUST contain only 7-bit ASCII. No emoji in comments, no Unicode math symbols in identifiers or print statements, no curly-quote characters from copy/pasted documentation.

Rules:

  1. NEVER use emoji in C++ comments.
    • ❌ Bad: // ⚠️ This runs in ISR context — keep it short
    • ✅ Good: // WARNING: This runs in ISR context — keep it short
  2. NEVER use Unicode math glyphs in source code.
    • ❌ Bad (Python): print("FastLED − Reference"), ρ = 0.5, Δ = abs(a - b)
    • ✅ Good (Python): print("FastLED - Reference"), rho = 0.5, delta = abs(a - b)
  3. NEVER paste curly quotes / em-dashes from documentation tools. Replace "smart quotes" with "straight quotes", with - or --.
  4. Markdown documentation files (*.md) are exempt — Unicode in human-reading docs is fine. This rule is for code only.

Rationale: Source files are read by lots of tools (compilers, linters, AVR / RP2040 / Teensy toolchains with various Unicode story, terminal log dumps, grep, IDE search). Some toolchains choke; others render the glyphs as garbage in compile errors. Pure ASCII keeps the source legible in every context. Flagged in #2648 (C++ comments with emoji) and #2709 (Python with ×, , ρ, ).

Check Process:

  1. Run grep -rPn '[^\x00-\x7F]' src/ tests/ ci/ (excluding *.md).
  2. Any hits are violations.
  3. Suppression: none — there is always an ASCII equivalent.