Revise *.instructions.md: separate AI-relevant special knowledge from background information for contributors (#354)
Revising AI review instructions: * Reduce context window use for AI review tools, by avoiding repeating common knowledge and API information that is (usually) part of the AI training datasets any way. * Introduce a mechanism to maintain both parts in single files, to avoid "silent diversion" over time * Adding a coderabbit path instruction that ensures cross-checking of both parts whenever a PR modifies instruction files Objectives: * Primary Goal: only inject content in AI-visible areas that are WLED-MM–specific or which deviate from general knowledge (the context window "token cost" of true false-positive suppressors is always worth it). * Soft goal: keep each file's AI-facing section lean enough that the signal-to-noise ratio in the attention layer stays high — around 1,500–2,000 words per file type is a reasonable practical ceiling for current models. * Aspirational: 500 words per file if achievable without sacrificing review quality. This is an evolution of #353, based on the discussion in https://github.com/MoonModules/WLED-MM/pull/353#issuecomment-4186989873 --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This commit is contained in:
72
.github/cpp.instructions.md
vendored
72
.github/cpp.instructions.md
vendored
@@ -3,6 +3,11 @@ applyTo: "**/*.cpp,**/*.h,**/*.hpp, **/*.ino"
|
||||
---
|
||||
# C++ Coding Conventions
|
||||
|
||||
> **Note for AI review tools**: sections enclosed in
|
||||
> `<!-- HUMAN_ONLY_START -->` / `<!-- HUMAN_ONLY_END -->` HTML comments contain
|
||||
> contributor reference material. Do **not** use that content as actionable review
|
||||
> criteria — treat it as background context only.
|
||||
|
||||
See also: [CONTRIBUTING.md](../CONTRIBUTING.md) for general style guidelines that apply to all contributors.
|
||||
|
||||
## Formatting
|
||||
@@ -13,6 +18,7 @@ See also: [CONTRIBUTING.md](../CONTRIBUTING.md) for general style guidelines tha
|
||||
- Space between keyword and parenthesis: `if (...)`, `for (...)`. No space between function name and parenthesis: `doStuff(a)`
|
||||
- No enforced line-length limit; wrap when a line exceeds your editor width
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
## Naming
|
||||
|
||||
- **camelCase** for functions and variables: `setValuesFromMainSeg()`, `effectCurrent`
|
||||
@@ -29,18 +35,21 @@ Most headers use `#ifndef` / `#define` guards. Some newer headers add `#pragma o
|
||||
// ...
|
||||
#endif // WLED_EXAMPLE_H
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
## Comments
|
||||
|
||||
- `//` for inline comments, `/* ... */` for block comments. Always put a space after `//`
|
||||
- Mark WLED-MM-specific changes with `// WLEDMM` or `// WLEDMM: description`:
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
```cpp
|
||||
// WLEDMM: increased max bus count for larger installs
|
||||
#ifndef WLED_MAX_BUSSES
|
||||
#define WLED_MAX_BUSSES 20 // WLEDMM default (upstream: 10)
|
||||
#endif
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
- **AI attribution:** When a larger block of code is generated by an AI tool, mark it with an `// AI:` comment so reviewers know to scrutinize it:
|
||||
|
||||
@@ -54,6 +63,7 @@ void calculateCRC(const uint8_t* data, size_t len) {
|
||||
|
||||
Single-line AI-assisted edits do not need the marker — use it when the AI produced a contiguous block that a human did not write line-by-line.
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
- **Function & feature comments:** Every non-trivial function should have a brief comment above it describing what it does. Include a note about each parameter when the names alone are not self-explanatory:
|
||||
|
||||
```cpp
|
||||
@@ -65,6 +75,8 @@ void calculateCRC(const uint8_t* data, size_t len) {
|
||||
***** */
|
||||
uint8_t gammaCorrect(uint8_t value, float gamma);
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
|
||||
Short accessor-style functions (getters/setters, one-liners) may skip this if their purpose is obvious from the name.
|
||||
|
||||
@@ -92,13 +104,18 @@ uint8_t gammaCorrect(uint8_t value, float gamma);
|
||||
## Memory
|
||||
|
||||
- **PSRAM-aware allocation**: use `d_malloc()` (prefer DRAM), `p_malloc()` (prefer PSRAM) from `util.h`
|
||||
- **Avoid Variable Length Arrays (VLAs)**: GCC/Clang support VLAs as an extension (they are not part of the C++ standard), so they look like a legitimate feature — but they are allocated on the stack at runtime. On ESP32/ESP8266, FreeRTOS task stacks are typically only 2–8 KB; a VLA whose size depends on a runtime parameter (segment dimensions, pixel counts, etc.) can silently exhaust the stack and cause the program to behave in unexpected ways or crash. Prefer a fixed-size array with a compile-time bound, or heap allocation (`d_malloc` / `p_malloc`) for dynamically sized buffers. **Any VLA must be explicitly justified in the source code or PR.**
|
||||
- **Avoid Variable Length Arrays (VLAs)**: FreeRTOS task stacks are typically 2–8 KB. A runtime-sized VLA can silently exhaust the stack. Use fixed-size arrays or heap allocation (`d_malloc` / `p_malloc`). Any VLA must be explicitly justified in source or PR.
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
GCC/Clang support VLAs as an extension (they are not part of the C++ standard), so they look like a legitimate feature — but they are allocated on the stack at runtime. On ESP32/ESP8266, a VLA whose size depends on a runtime parameter (segment dimensions, pixel counts, etc.) can silently exhaust the stack and cause the program to behave in unexpected ways or crash.
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
- **Larger buffers** (LED data, JSON documents) should use PSRAM when available and technically feasible
|
||||
- **Hot-path**: some data should stay in DRAM or IRAM for performance reasons
|
||||
- Memory efficiency matters, but is less critical on boards with PSRAM
|
||||
|
||||
## `const` and `constexpr`
|
||||
Add `const` to cached locals in hot-path code (helps the compiler keep values in registers). Pass and store objects by `const&` to avoid copies in loops.
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
`const` is a promise to the compiler that a value (or object) will not change - a function declared with a `const char* message` parameter is not allowed to modify the content of `message`.
|
||||
This pattern enables optimizations and makes intent clear to reviewers.
|
||||
|
||||
@@ -110,11 +127,12 @@ Adding `const` to a local variable that is only assigned once is not necessary
|
||||
const uint_fast16_t cols = virtualWidth();
|
||||
const uint_fast16_t rows = virtualHeight();
|
||||
```
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
### `const` references to avoid copies
|
||||
|
||||
Pass and store objects by `const &` (or `&`) instead of copying them implicitly. This avoids constructing temporary objects on every access — especially important in loops:
|
||||
Pass and store objects by `const &` (or `&`) instead of copying them implicitly. This avoids constructing temporary objects on every access — especially important in loops.
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
```cpp
|
||||
const auto &m = _mappings[i]; // reference, not a copy (bus_manager.cpp)
|
||||
const CRGB& c = ledBuffer[pix]; // alias — avoids creating a temporary CRGB instance
|
||||
@@ -125,8 +143,10 @@ For function parameters that are read-only, prefer `const &`:
|
||||
```cpp
|
||||
BusDigital(BusConfig &bc, uint8_t nr, const ColorOrderMap &com);
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
### `constexpr` over `#define`
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
|
||||
Prefer `constexpr` for compile-time constants. Unlike `#define`, `constexpr` respects scope and type safety, keeping the global namespace clean:
|
||||
|
||||
@@ -155,6 +175,10 @@ static_assert(WLED_MAX_BUSSES <= 32, "WLED_MAX_BUSSES exceeds hard limit");
|
||||
#error "WLED_MAX_BUSSES exceeds hard limit"
|
||||
#endif
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
Prefer `constexpr` over `#define` for typed constants (scope-safe, debuggable). Use `static_assert` instead of `#if … #error` for compile-time validation.
|
||||
Exception: `#define` is required for conditional-compilation guards and build-flag-overridable values.
|
||||
|
||||
### `static` and `const` class methods
|
||||
|
||||
@@ -166,16 +190,18 @@ Marking a member function `const` tells the compiler that it does not modify the
|
||||
uint16_t length() const { return _len; }
|
||||
bool isActive() const { return _active; }
|
||||
```
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
Benefits for GCC/Xtensa/RISC-V:
|
||||
- The compiler knows the method cannot write to `this`, so it is free to **keep member values in registers** across the call and avoid reload barriers.
|
||||
- `const` methods can be called on `const` objects and `const` references — essential when passing large objects as `const &` to avoid copying.
|
||||
- `const` allows the compiler to **eliminate redundant loads**: if a caller already has a member value cached, the compiler can prove the `const` call cannot invalidate it.
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
Declare every getter, query, or inspection method `const`. If you need to mark a member `mutable` to work around this (e.g. for a cache or counter), document the reason.
|
||||
Declare getter, query, or inspection methods `const`. If you need to mark a member `mutable` to work around this (e.g. for a cache or counter), document the reason.
|
||||
|
||||
#### `static` member functions
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
A `static` member function has no implicit `this` pointer. This has two distinct advantages:
|
||||
|
||||
1. **Smaller code, faster calls**: no `this` is passed in a register. On Xtensa and RISC-V, this removes one register argument from every call site and prevents the compiler from emitting `this`-preservation code around inlined blocks.
|
||||
@@ -191,8 +217,9 @@ static BusConfig fromJson(JsonObject obj);
|
||||
static uint8_t gamma8(uint8_t val);
|
||||
static uint32_t colorBalance(uint32_t color, uint8_t r, uint8_t g, uint8_t b);
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
`static` also communicates intent clearly: a reviewer immediately knows the method is stateless and safe to call without a fully constructed object.
|
||||
`static` communicates intent clearly: a reviewer immediately knows the method is stateless and safe to call without a fully constructed object.
|
||||
|
||||
> **Rule of thumb**: if a method does not read or write any member variable, make it `static`. If it only reads member variables, make it `const`. Note: `static` methods cannot also be `const`-qualified because there is no implicit `this` pointer to be const — just use `static`. Both qualifiers reduce coupling and improve generated code on all ESP32 targets.
|
||||
|
||||
@@ -225,6 +252,7 @@ Example signature:
|
||||
void IRAM_ATTR_YN WLED_O2_ATTR __attribute__((hot)) Segment::setPixelColor(int i, uint32_t col)
|
||||
```
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
### Use `uint_fast` Types for Locals
|
||||
|
||||
Use `uint_fast8_t` and `uint_fast16_t` for loop counters, indices, and temporary calculations in hot paths. These let the compiler pick the CPU's native word size (32-bit on ESP32), avoiding unnecessary narrow-type masking:
|
||||
@@ -249,6 +277,7 @@ for (uint_fast8_t i = 0; i < count; i++) {
|
||||
...
|
||||
}
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
### Unsigned Range Check
|
||||
|
||||
@@ -270,8 +299,8 @@ if (unsigned(i) >= virtualLength()) return; // bounds check (catches negative i
|
||||
|
||||
### Avoid Nested Calls — Fast Path / Complex Path
|
||||
|
||||
Avoid calling non-inline functions or making complex decisions inside per-pixel hot-path code. When a function has both a common simple case and a rare complex case, split it into two variants and choose once per frame rather than per pixel:
|
||||
|
||||
Avoid calling non-inline functions or making complex decisions inside per-pixel hot-path code. When a function has both a common simple case and a rare complex case, split it into two variants and choose once per frame rather than per pixel.
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
```cpp
|
||||
// Decision made once per frame in startFrame(), stored in a bool
|
||||
bool simpleSegment = _isSuperSimpleSegment;
|
||||
@@ -288,16 +317,18 @@ The same principle applies to color utilities — `color_add()` accepts a `fast`
|
||||
```cpp
|
||||
uint32_t color_add(uint32_t c1, uint32_t c2, bool fast=false);
|
||||
```
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
|
||||
General rules:
|
||||
- Keep the per-pixel fast path free of non-inline function calls, multi-way branches and complex switch-case decisions.
|
||||
- Hoist the "which path?" decision out of the inner loop (once per frame or per segment)
|
||||
- It is acceptable to duplicate some code between fast and complex variants to keep the fast path lean
|
||||
- Keep fast-path functions free of non-inline calls, multi-way branches, and complex switch-case decisions.
|
||||
- Hoist per-frame decisions (e.g. simple vs. complex segment) out of the per-pixel loop.
|
||||
- Code duplication between fast/slow variants is acceptable to keep the fast path lean.
|
||||
|
||||
### Function Pointers to Eliminate Repeated Decisions
|
||||
|
||||
When the same decision (e.g. "which drawing routine?") would be evaluated for every pixel, assign the chosen variant to a function pointer once and let the inner loop call through the pointer. This removes the branch entirely — the calling code (e.g. the GIF decoder loop) only ever invokes one function per frame, with no per-pixel decision.
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
`image_loader.cpp` demonstrates the pattern: `calculateScaling()` picks the best drawing callback once per frame based on segment dimensions and GIF size, then passes it to the decoder via `setDrawPixelCallback()`:
|
||||
|
||||
```cpp
|
||||
@@ -309,7 +340,8 @@ else
|
||||
```
|
||||
|
||||
Each callback is a small, single-purpose function with no internal branching — the decoder's per-pixel loop never re-evaluates which strategy to use.
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
### Template Specialization (Advanced)
|
||||
|
||||
Templates can eliminate runtime decisions by generating separate code paths at compile time. For example, a pixel setter could be templated on color order or channel count so the compiler removes dead branches and produces tight, specialized machine code:
|
||||
@@ -349,10 +381,11 @@ if (!guard) return; // another task is already sending
|
||||
|
||||
This avoids FreeRTOS semaphore overhead and the risk of forgetting `esp32SemGive`. There are no current examples of this pattern in the codebase — consult with maintainers before introducing it in new code, to ensure it aligns with the project's synchronization conventions.
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
### Pre-Compute Outside Loops
|
||||
|
||||
Move invariant calculations before the loop. Pre-compute reciprocals to replace division with multiplication:
|
||||
|
||||
Move invariant calculations before the loop. Pre-compute reciprocals to replace division with multiplication.
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
```cpp
|
||||
const uint_fast16_t cols = virtualWidth();
|
||||
const uint_fast16_t rows = virtualHeight();
|
||||
@@ -360,6 +393,7 @@ uint_fast8_t fadeRate = (255 - rate) >> 1;
|
||||
float mappedRate_r = 1.0f / (float(fadeRate) + 1.1f); // reciprocal — avoid division inside loop
|
||||
```
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
### Parallel Channel Processing
|
||||
|
||||
Process R+B and W+G channels simultaneously using the two-channel mask pattern:
|
||||
@@ -374,11 +408,10 @@ return rb | wg;
|
||||
### Bit Shifts Over Division (mainly for RISC-V boards)
|
||||
|
||||
ESP32 and ESP32-S3 (Xtensa core) have a fast "integer divide" instruction, so manual shifts rarely help.
|
||||
The compiler already converts power-of-two unsigned divisions to shifts at `-O2`.
|
||||
On RISC-V-based boards (ESP32-C3, ESP32-C6, ESP32-C5) explicit shifts can be beneficial:
|
||||
|
||||
Prefer bit shifts for power-of-two operations:
|
||||
On RISC-V targets (ESP32-C3/C6/P4), prefer explicit bit-shifts for power-of-two arithmetic — the compiler does **not** always convert divisions to shifts on RISC-V at `-O2`. Always use unsigned operands; signed right-shift is implementation-defined.
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
On RISC-V-based boards (ESP32-C3, ESP32-C6, ESP32-C5) explicit shifts can be beneficial.
|
||||
```cpp
|
||||
position >> 3 // instead of position / 8
|
||||
(255U - rate) >> 1 // instead of (255 - rate) / 2
|
||||
@@ -387,6 +420,7 @@ i & 0x0007 // instead of i % 8
|
||||
|
||||
**Important**: The bit-shifted expression should be unsigned. On some MCUs, "signed right-shift" is implemented by an "arithmetic shift right" that duplicates the sign bit: ``0b1010 >> 1 = 0b1101``.
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
### Static Caching for Expensive Computations
|
||||
|
||||
Cache results in static locals when the input rarely changes between calls:
|
||||
@@ -406,6 +440,7 @@ if (lastKelvin != kelvin) {
|
||||
- Use `static inline` for file-local helpers
|
||||
- On ESP32 with `WLEDMM_FASTPATH`, color utilities are inlined from `colorTools.hpp`; on ESP8266 or `WLEDMM_SAVE_FLASH`, they fall back to `colors.cpp`
|
||||
|
||||
<!-- HUMAN_ONLY_START -->
|
||||
### Colors
|
||||
|
||||
- Store and pass colors as `uint32_t` (0xWWRRGGBB)
|
||||
@@ -416,6 +451,7 @@ if (lastKelvin != kelvin) {
|
||||
uint16_t r1 = R(color1); // 16-bit intermediate keeps the multiply result in 32 bits, avoiding 64-bit promotion
|
||||
```
|
||||
|
||||
<!-- HUMAN_ONLY_END -->
|
||||
## Multi-Task Synchronization
|
||||
|
||||
ESP32 runs multiple FreeRTOS tasks concurrently (e.g. network handling, LED output, JSON parsing). Use the WLED-MM mutex macros for synchronization — they expand to FreeRTOS recursive semaphore calls on ESP32 and compile to no-ops on ESP8266:
|
||||
|
||||
Reference in New Issue
Block a user