Skip to content

copilot-instructions rework, new C++ coding guide for AI reviews#5480

Merged
softhack007 merged 48 commits intomainfrom
copilot_restructure
Apr 8, 2026
Merged

copilot-instructions rework, new C++ coding guide for AI reviews#5480
softhack007 merged 48 commits intomainfrom
copilot_restructure

Conversation

@softhack007
Copy link
Copy Markdown
Member

@softhack007 softhack007 commented Apr 5, 2026

Reworks AI instructions, to separate "coding agent" from "code review" instructions.
Adds C++ coding guidelines that were extracted from the WLED source code (mainly by AI). The new guides can be used both for contributor information, and as a "best practice" database in automated code reviews by the rabbit.

Update: I've moved the coding style guides into their own directory /docs, and "out of the way" for non-review agent tasks. It seems that AI agent tasks ("I need a new bus driver for my Daimler") work faster without the "code review" add-on guides - the agent will still find docs when it needs help. Code reviews (coderabbitAI) with detailed guides are still much better.

Details

  • shortens previous instruction file, by removing duplicated information and shortening sentences
  • moves agent instructions into a separate file
  • adds coding conventions for C++, webUI, CI/CD (based on WLED-MM, needs adjustment for WLED)
  • adds .coderabbit.yaml so the rabbit uses the same instructions for reviews that copilot sees.
  • Introduces a HUMAN_ONLY mechanism that hides generic information from the AI, to increase "attention layer excitement"

Example for a coderabbit review with the new instructions: MoonModules#324 (comment)
(notice that recommendations refer to the new guidelines, and there is a "things done well" section, too)

This is a request for discussions

The new instructions already work well in WLED-MM, but we need to adjust them for WLED. Most importantly, optimization guides for ESP8266 still need to be added.

more information:


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
  • Primary Goal: only inject content in AI-visible areas that are WLED–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.

Guiding Principles for Efficient AI Instruction Files

  1. Only document what can't be inferred from the code or from general training.
    If the AI would get it right without instruction 95% of the time, the instruction probably isn't worth the extra space in context memory.

  2. Prefer short, actionable rules over explanations.
    "Do not flag uint8_t x = int(f) as a bug when the comment says // float→int wrapping" is more useful than a paragraph explaining why. The paragraph is for humans; the rule is for AI.

  3. Keep the total injected instruction size per file type low (<1000 lines, best under ~500 words).
    With lots of instructions, the value of additional instructions drops rapidly and the distraction cost rises.

  4. Encode "false positive suppressors" first, feature guidance second.
    "Don't flag pattern X as a bug" is more impactful than "always do Y" because it directly prevents noise.

  5. Treat instruction files like tests — they need maintenance when the code changes.
    If you wouldn't update the instruction file when refactoring the relevant code, don't write the instruction.

Large language models already have extensive general knowledge about C++, ESP-IDF, GitHub Actions, etc. The genuine value of instruction files is narrow but real:

What AI genuinely doesn't know without you telling it What AI already knows well enough
Your project's intentional deviations from general best practice General C++ / ESP-IDF / GitHub Actions patterns
Decisions that look like bugs but aren't (e.g. a float→int→uint8_t wrapping pattern) Standard style rules
Which files/patterns to ignore or treat as deprecated Common security practices
Your PR process expectations How delay() works in FreeRTOS
Project-specific macro semantics (WLED_DISABLE_2D, d_malloc) Memory allocation tradeoffs
Which reviewer comments are out of scope for this PR Naming conventions

The key insight: instruction files are most valuable when they prevent false positives (AI flagging intentional patterns as bugs) and encode decisions that can't be inferred from the code alone.


Summary by CodeRabbit

  • Documentation

    • Added comprehensive C++, Web UI, and CI/CD coding conventions; condensed setup/build guide; new agent-mode build/test workflow with ordered commands, timeouts, required validation gates, manual web validation guidance, and troubleshooting.
  • Chores

    • Introduced repository-level AI review/configuration rules, workflow best-practices, safeguards to detect/flag edits to generated web assets, and alignment checks between AI-facing rules and human-only reference sections.

Initial commit based on WLED-MM version
* shorten previous instructions, by removing duplicated information and shortening wording
* move agent instructions into a separate file
* added coding conventions for C++, webUI, CI/CD (based on WLED-MM, needs adjustment for WLED)
* added .coderabbit.yaml so the rabbits picks the same instructions for reviews.

more information:
* MoonModules#353
* MoonModules#354
* MoonModules#356
@softhack007 softhack007 requested a review from netmindz April 5, 2026 22:34
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 5, 2026

Walkthrough

Adds a repository-root CodeRabbit config plus multiple new or updated docs and instruction files (C++ conventions, web UI rules, CI/CD guidance, agent-build workflow, condensed setup), updates CONTRIBUTING links, and adds rules preventing edits to generated web headers. No source or public API changes.

Changes

Cohort / File(s) Summary
CodeRabbit configuration
/.coderabbit.yaml
New repo-level config with path-scoped review instructions for C++ (**/*.{cpp,h,hpp,ino}), web UI assets (wled00/data/**), usermods (usermods/**), GitHub workflows (.github/workflows/*.{yml,yaml}), and alignment checks for **/*.instructions.md human-only blocks.
Agent build & setup docs
.github/agent-build.instructions.md, .github/copilot-instructions.md
Adds agent-mode build/test workflow and timeouts, mandatory gates (npm test, pio run -e esp32dev), reworks setup/build flow, moves troubleshooting to the agent doc, and adds usermod guidelines.
Contributor guidance
CONTRIBUTING.md
Updated internal links to new local instruction docs and added pointers to docs/* guidance and coderabbit review behavior.
C++ coding conventions
docs/cpp.instructions.md
New comprehensive C++ style/performance rules: formatting, naming, include/guard expectations, memory/PSRAM allocators, prohibition of exceptions, hot-path optimization patterns, and AI-attribution markers.
Web UI conventions & generated assets
docs/web.instructions.md, wled00/data/**
New web UI conventions (tabs for HTML/JS/CSS, JS naming, reuse of common.js), build integration via tools/cdata.js, instruction to run npm run build, and explicit prohibition on editing generated wled00/html_*.h/wled00/js_*.h.
CI/CD workflow guidance
docs/cicd.instructions.md, .github/workflows/*.{yml,yaml}
New GitHub Actions guidance: triggers, job/step naming, pinned third‑party actions, least‑privilege permissions, safe run: interpolation, matrix settings, caching/artifact naming, and PR/fork security notes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • DedeHai
  • Aircoookie
  • willmmiles
  • netmindz
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly reflects the main changes: reworking copilot-instructions documentation and adding new C++ coding guidelines for AI-assisted code reviews.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch copilot_restructure

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

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

coderabbitai[bot]

This comment was marked as resolved.

softhack007 and others added 3 commits April 6, 2026 00:39
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
coderabbitai[bot]

This comment was marked as resolved.

@softhack007 softhack007 added enhancement discussion awesome AI Partly generated by an AI. Make sure that the contributor fully understands the code! labels Apr 5, 2026
@softhack007 softhack007 added this to the 16.1 milestone Apr 5, 2026
coderabbitai[bot]

This comment was marked as resolved.

Clarify float-to-unsigned conversion behavior and update semaphore return note.
coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

softhack007 and others added 2 commits April 6, 2026 13:43
Emphasize the importance of avoiding heap fragmentation and suggest strategies for memory allocation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@softhack007 softhack007 marked this pull request as draft April 6, 2026 12:13
Clarify heap fragmentation concerns and provide guidelines for memory allocation.
Comment thread docs/cpp.instructions.md
... and making some "actionable decision rules" visible to AI for review
coderabbitai[bot]

This comment was marked as resolved.

softhack007 and others added 2 commits April 8, 2026 16:01
absolute links can send contributors on non-main branches to mismatched guidance. Prefer relative links so docs stay consistent with the checked-out branch.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as low quality.

* moved "general" section to the front, renamed last section
* minor styling
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/cpp.instructions.md`:
- Around line 267-491: Add a short subsection after "Inlining Strategy" that
instructs developers to use WLED's optimized trig functions from wled_math.cpp
instead of FastLED aliases in hot-path code (e.g. FX_fcn.cpp, FX_2Dfcn.cpp,
bus_manager.cpp, colors.cpp); specify the exact replacements: sin8()→sin8_t(),
cos8()→cos8_t(), sin16()→sin16_t(), cos16()→cos16_t(), and float
sinf()/cosf()→sin_approx()/cos_approx(), and note that reviewers should flag any
remaining FastLED trig usages and switch them to these WLED implementations.
- Around line 106-142: The docs lack guidance about LittleFS filename length
limits causing silent failures; add a short bullet under Memory (near where
file/heap guidance is given) stating that constructed LittleFS file paths must
not exceed the default LFS_NAME_MAX (255 bytes) and that callers should validate
lengths before calling file.open() or building paths; reference
LittleFS/LFS_NAME_MAX and file.open() in the sentence so reviewers can spot
places (path construction, file.open() calls) to validate and truncate or error
early.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f3d65356-6d4b-4a53-81b6-2d7f6300b074

📥 Commits

Reviewing files that changed from the base of the PR and between 409cc81 and c9854b5.

📒 Files selected for processing (1)
  • docs/cpp.instructions.md

Comment thread docs/cpp.instructions.md
Comment thread docs/cpp.instructions.md
Comment on lines +267 to +491
## Hot-Path Optimization

The hot path is the per-frame pixel pipeline: **Segment → Strip → BusManager → Bus(Digital,HUB75,Network) or PolyBus → LED driver, plus ``WS2812FX::show()`` and below**.
Speed is the priority here. The patterns below are taken from existing hot-path code (`FX_fcn.cpp`, `FX_2Dfcn.cpp`, `bus_manager.cpp`, `colors.cpp`) and should be followed when modifying these files.

Note: `FX.cpp` (effect functions) is written by many contributors and has diverse styles — that is acceptable.

### Function Attributes

Stack the appropriate attributes on hot-path functions. Defined in `const.h`:

| Attribute | Meaning | When to use |
|---|---|---|
| `__attribute__((hot))` | Branch-prediction hint | hot-path functions with complex logic |
| `IRAM_ATTR` | Place in fast IRAM (ESP32) | Critical per-pixel functions (e.g. `BusDigital::setPixelColor`) |
| `IRAM_ATTR_YN` | IRAM on ESP32, no-op on ESP8266 | Hot functions that ESP8266 can't fit in IRAM |
| `WLED_O2_ATTR` | Force `-O2` optimization | Most hot-path functions |
| `WLED_O3_ATTR` | Force `-O3,fast-math` | Innermost color math (e.g. `color_blend`) |
| `[[gnu::hot]] inline` | Modern C++ attribute + inline | Header-defined accessors (e.g. `progress()`, `currentBri()`) |

Note: `WLED_O3_ATTR` sometimes causes performance loss compared to `WLED_O2_ATTR`. Choose optimization levels based on test results.

Example signature:

```cpp
void IRAM_ATTR_YN WLED_O2_ATTR __attribute__((hot)) Segment::setPixelColor(unsigned i, uint32_t c)
```

<!-- HUMAN_ONLY_START -->
### Cache Members to Locals Before Loops

Copy class members and virtual-call results to local variables before entering a loop:

```cpp
uint_fast8_t count = numBusses; // avoid repeated member access
for (uint_fast8_t i = 0; i < count; i++) {
Bus* const b = busses[i]; // const pointer hints to compiler
uint_fast16_t bstart = b->getStart();
uint_fast16_t blen = b->getLength();
...
}
```
<!-- HUMAN_ONLY_END -->

### Unsigned Range Check

Replace two-comparison range tests with a single unsigned subtraction:

```cpp
// Instead of: if (pix >= bstart && pix < bstart + blen)
if ((uint_fast16_t)(pix - bstart) < blen) // also catches negative pix via unsigned underflow
```

### Early Returns

Guard every hot-path function with the cheapest necessary checks first:

```cpp
if (!isActive()) return; // inactive segment
if (unsigned(i) >= vLength()) return; // bounds check (catches negative i too)
```

### 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.

General rules:
- 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
// calculateScaling() — called once per frame
if ((perPixelX < 2) && (perPixelY < 2))
decoder.setDrawPixelCallback(drawPixelCallbackDownScale2D); // downscale-only variant
else
decoder.setDrawPixelCallback(drawPixelCallback2D); // full-scaling variant
```

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:

```cpp
template<bool hasWhite>
void setChannel(uint8_t* out, uint32_t col) {
out[0] = R(col); out[1] = G(col); out[2] = B(col);
if constexpr (hasWhite) out[3] = W(col); // compiled out when hasWhite==false
}
```

Use sparingly — each instantiation duplicates code in flash. On ESP8266 and small-flash ESP32 boards this can exhaust IRAM/flash. Prefer templates only when the hot path is measurably faster and the number of instantiations is small (2–4).

### RAII Lock-Free Synchronization (Advanced)

Where contention is rare and the critical section is short, consider replacing mutex-based locking with lock-free techniques using `std::atomic` and RAII scoped guards. A scoped guard sets a flag on construction and clears it on destruction, guaranteeing cleanup even on early return:

```cpp
struct ScopedBusyFlag {
std::atomic<bool>& flag;
bool acquired;
ScopedBusyFlag(std::atomic<bool>& f) : flag(f), acquired(false) {
bool expected = false;
acquired = flag.compare_exchange_strong(expected, true);
}
~ScopedBusyFlag() { if (acquired) flag.store(false); }
explicit operator bool() const { return acquired; }
};

// Usage
static std::atomic<bool> busySending{false};
ScopedBusyFlag guard(busySending);
if (!guard) return; // another task is already sending
// ... do work — flag auto-clears when guard goes out of scope
```

This avoids FreeRTOS semaphore overhead and the risk of forgetting to return a semaphore. 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.
<!-- HUMAN_ONLY_START -->
```cpp
const uint_fast16_t cols = virtualWidth();
const uint_fast16_t rows = virtualHeight();
uint_fast8_t fadeRate = (255U - 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:

```cpp
constexpr uint32_t TWO_CHANNEL_MASK = 0x00FF00FF;
uint32_t rb = (((c1 & TWO_CHANNEL_MASK) * amount) >> 8) & TWO_CHANNEL_MASK;
uint32_t wg = (((c1 >> 8) & TWO_CHANNEL_MASK) * amount) & ~TWO_CHANNEL_MASK;
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.
On RISC-V targets (ESP32-C3/C6/P4) and ESP8266, prefer explicit bit-shifts for power-of-two arithmetic — the compiler does **not** always convert divisions to shifts.
Always use unsigned operands for right shifts; 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
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:

```cpp
static uint16_t lastKelvin = 0;
static byte correctionRGB[4] = {255,255,255,0};
if (lastKelvin != kelvin) {
colorKtoRGB(kelvin, correctionRGB); // expensive — only recalculate when input changes
lastKelvin = kelvin;
}
```

### Inlining Strategy

- Move frequently-called small functions to headers for inlining (e.g. `Segment::setPixelColorRaw` is in `FX.h`)
- Use `static inline` for file-local helpers

### `delay()` vs `yield()` in ESP32 Tasks
<!-- HUMAN_ONLY_START -->
* On ESP32, `delay(ms)` calls `vTaskDelay(ms / portTICK_PERIOD_MS)`, which **suspends only the calling task**. The FreeRTOS scheduler immediately runs all other ready tasks.
* The Arduino `loop()` function runs inside `loopTask`. Calling `delay()` there does *not* block the network stack, audio FFT, LED DMA, nor any other FreeRTOS task.
* This differs from ESP8266, where `delay()` stalls the entire system unless `yield()` was called inside.
<!-- HUMAN_ONLY_END -->

- On ESP32, `delay()` is generally allowed, as it helps to efficiently manage CPU usage of all tasks.
- On ESP8266, only use `delay()` and `yield()` in the main `loop()` context. If not sure, protect with `if (can_yield()) ...`.
- Do *not* use `delay()` in effects (FX.cpp) or in the hot pixel path.
- `delay()` on the bus-level is allowed, it might be needed to achieve exact timing in LED drivers.

#### IDLE Watchdog and Custom Tasks on ESP32

- In arduino-esp32, `yield()` calls `vTaskDelay(0)`, which only switches to tasks at equal or higher priority — the IDLE task (priority 0) is never reached.
- **Do not use `yield()` to pace ESP32 tasks or assume it feeds any watchdog**.
- **Custom `xTaskCreate()` tasks must call `delay(1)` in their loop, not `yield()`.** Without a real blocking call, the IDLE task is starved. The IDLE watchdog panic is the first visible symptom — but the damage starts earlier: deleted task memory leaks, software timers stop firing, light sleep is disabled, and Wi-Fi/BT idle hooks don't run. Structure custom tasks like this:
```cpp
// WRONG — IDLE task is never scheduled; yield() does not feed the idle task watchdog.
void myTask(void*) {
for (;;) {
doWork();
yield();
}
}

// CORRECT — delay(1) suspends the task for ≥1 ms, IDLE task runs, IDLE watchdog is fed
void myTask(void*) {
for (;;) {
doWork();
delay(1); // DO NOT REMOVE — lets IDLE(0) run and feeds its watchdog
}
}
```

- Prefer blocking FreeRTOS primitives (`xQueueReceive`, `ulTaskNotifyTake`, `vTaskDelayUntil`) over `delay(1)` polling where precise timing or event-driven behaviour is needed.
- **Watchdog note.** WLED disables the Task Watchdog by default (`WLED_WATCHDOG_TIMEOUT 0` in `wled.h`). When enabled, `esp_task_wdt_reset()` is called at the end of each `loop()` iteration. Long blocking operations inside `loop()` — such as OTA downloads or slow file I/O — must call `esp_task_wdt_reset()` periodically, or be restructured so the main loop is not blocked for longer than the configured timeout.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add FastLED trigonometric function replacement guidance.

The Hot-Path Optimization section should include guidance on FastLED trigonometric function aliases. WLED has its own optimized implementations in wled_math.cpp that should be used instead of FastLED's versions.

Based on learnings: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace sin8() and cos8() with sin8_t() and cos8_t(). Replace sin16() and cos16() with sin16_t() and cos16_t(). For float versions, replace sinf() and cosf() with sin_approx() and cos_approx(). See wled_math.cpp for the proper WLED implementations.

📝 Suggested addition

Add a new subsection after line 453 (after "Inlining Strategy"):

 - Use `static inline` for file-local helpers
 
+### Trigonometric Functions
+
+Use WLED's optimized implementations from `wled_math.cpp` instead of FastLED aliases:
+
+| Avoid (FastLED) | Use (WLED) | Notes |
+|---|---|---|
+| `sin8()`, `cos8()` | `sin8_t()`, `cos8_t()` | 8-bit sine/cosine |
+| `sin16()`, `cos16()` | `sin16_t()`, `cos16_t()` | 16-bit sine/cosine |
+| `sinf()`, `cosf()` | `sin_approx()`, `cos_approx()` | Fast float approximations |
+
 ### `delay()` vs `yield()` in ESP32 Tasks
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/cpp.instructions.md` around lines 267 - 491, Add a short subsection
after "Inlining Strategy" that instructs developers to use WLED's optimized trig
functions from wled_math.cpp instead of FastLED aliases in hot-path code (e.g.
FX_fcn.cpp, FX_2Dfcn.cpp, bus_manager.cpp, colors.cpp); specify the exact
replacements: sin8()→sin8_t(), cos8()→cos8_t(), sin16()→sin16_t(),
cos16()→cos16_t(), and float sinf()/cosf()→sin_approx()/cos_approx(), and note
that reviewers should flag any remaining FastLED trig usages and switch them to
these WLED implementations.

✅ Addressed in commit 2f109f8

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai We now have our own fastled-slim as a full replacement, see wled00/src/dependancies/fastled-slim/

@DedeHai i think the part about sin/cos/sqrt is still correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Let me check the fastled-slim dependency and wled_math.cpp to verify the current state of trig functions.


🏁 Script executed:

#!/bin/bash
# Check the fastled-slim directory structure
find . -path "*/fastled-slim*" -name "*.h" | head -30

# Check wled_math.cpp/h for sin/cos/sqrt functions
fd "wled_math" --exec grep -n "sin\|cos\|sqrt" {}

# Check if sin8_t, cos8_t etc. are defined in wled_math
rg -n "sin8_t|cos8_t|sin16_t|cos16_t|sin_approx|cos_approx" --type=cpp --type=h -l

Length of output: 2476


🏁 Script executed:

#!/bin/bash
# Check if fastled-slim directory exists
find . -path "*fastled-slim*" -name "*.h" 2>/dev/null | head -20
find . -path "*fastled*" -type d 2>/dev/null | head -10

# Check if sin8, cos8 (original FastLED aliases) are still present anywhere
rg -n "^[[:space:]]*#define sin8\b|^[[:space:]]*#define cos8\b|^[[:space:]]*#define sin16\b|^[[:space:]]*#define cos16\b" --type=h

# Check what fastled-slim provides for trig (if it exists)
rg -rn "sin8|cos8|sin16|cos16" --include="*.h" --include="*.cpp" -l | grep -i fastled | head -10

Length of output: 480


🏁 Script executed:

#!/bin/bash
# Check fastled_slim directory contents
find ./wled00/src/dependencies/fastled_slim -name "*.h" | head -30

# Check if sin8, cos8 aliases are defined in fastled_slim
rg -n "sin8|cos8|sin16|cos16" ./wled00/src/dependencies/fastled_slim/ 2>/dev/null | head -30

# Check if there are `#define` aliases like sin8 -> sin8_t
rg -n "sin8|cos8|sin16|cos16" wled00/fcn_declare.h | head -20

# Check lib_deps or platformio.ini for fastled-slim reference
rg -n "fastled" platformio.ini | head -10

Length of output: 643


🏁 Script executed:

#!/bin/bash
# Check if fastled_slim.h has any compatibility shims for old FastLED names
grep -n "sin8\b\|cos8\b\|sin16\b\|cos16\b\|sinf\|cosf\|inoise\|perlin" ./wled00/src/dependencies/fastled_slim/fastled_slim.h | head -40

# Check if old FastLED sin8/cos8 (without _t) are still callable (backward compat aliases)
grep -n "#define sin8\|#define cos8\|#define sin16\|#define cos16\|using sin8\|using cos8" ./wled00/src/dependencies/fastled_slim/fastled_slim.h

# See if the FX.cpp still uses the old names
rg -n "\bsin8\b|\bcos8\b|\bsin16\b|\bcos16\b" wled00/FX.cpp | grep -v "_t(" | grep -v "sin16_t\|cos16_t\|sin8_t\|cos8_t" | head -20

Length of output: 0


@softhack007 — confirmed, the _t guidance is still correct and is actually stronger now:

From inspecting fastled_slim.h, the custom fastled_slim library already natively declares sin8_t, cos8_t, sin16_t, cos16_t (lines 562–565 in the header) with no backward-compat shims for the old bare names. This means the old sin8() / cos8() aliases won't compile at all with fastled_slim — so the guidance is not just a preference, it's a hard requirement.

The framing in the doc suggestion could be updated accordingly — rather than "replace FastLED aliases with WLED alternatives," it's more accurately "use the _t variants — they are the only available names now that fastled_slim is the full replacement." The sin_approx() / cos_approx() float wrappers from wled_math.cpp similarly remain the right choice over raw sinf() / cosf().

I'll update the learning to capture the fastled_slim context:


✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:267-491
Timestamp: 2026-04-08T15:03:02.041Z
Learning: In WLED (wled00/src/dependencies/fastled_slim/fastled_slim.h and wled_math.cpp): Since the original FastLED library has been replaced by the custom `fastled_slim` fork, the old FastLED trigonometric function aliases (`sin8()`, `cos8()`, `sin16()`, `cos16()`) no longer exist — there are no backward-compat shims. The `_t` variants are the only valid names: use `sin8_t()`, `cos8_t()`, `sin16_t()`, `cos16_t()` (declared in fastled_slim.h, implemented in wled_math.cpp), and `sin_approx()` / `cos_approx()` for float approximations. Calling the old names without `_t` will cause a compile error.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:43-43
Timestamp: 2026-03-22T00:49:53.727Z
Learning: In WLED (wled00/file.cpp and related), all callers of writeObjectToFile(), writeObjectToFileUsingId(), readObjectFromFile(), and readObjectFromFileUsingId() execute exclusively on the main Arduino loop task. No async webserver callback (which runs on a separate FreeRTOS task on ESP32) directly calls any of these file functions — async handlers either set flags or delegate to the main loop. This means the static File f in file.cpp is never concurrently accessed from multiple tasks, and the window between doCloseFile=false and f.close() in closeFile() cannot be exploited by a parallel open() call.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/FX_fcn.cpp:1484-1498
Timestamp: 2026-03-21T22:29:23.438Z
Learning: In WLED (wled00/FX_fcn.cpp), `WS2812FX::waitForLEDs(unsigned maxWaitMS)` is intentionally a const wait-only helper — it does NOT call `strip.suspend()` or `strip.resume()`. When a caller needs to protect a critical OS section (e.g. `file.close()`, `ESP.getFreeHeap()`) from concurrent LED DMA/RMT output, the caller must own the full `strip.suspend()` → `strip.waitForLEDs()` → critical section → `strip.resume()` sequence. Putting `resume()` inside `waitForLEDs()` would release the lock before the critical call and defeat the purpose.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:283-292
Timestamp: 2026-03-31T17:33:09.706Z
Learning: In the WLED Zigbee RGB Light usermod (`usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h`), mutex timeouts used with `xSemaphoreTake(zbStateMutex, ...)` should be at least 100–200 ms (not 10 ms), because the Zigbee FreeRTOS task can be busy for longer than 10 ms during ZCL frame construction, NWK encryption, and ZBOSS scheduler operations on ESP32-C6.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.

Learnt from: willmmiles
Repo: wled/WLED PR: 4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.h:336-339
Timestamp: 2026-03-28T01:36:54.706Z
Learning: In WLED (wled00/wled.h), the RX/TX hardware pin guard `#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX)))` is intentional and correct. For all non-classic ESP32 targets (C3, C5, C6, S2, S3, P4, C61), the arduino-esp32 framework always defines RX and TX according to the target's native UART pins, so the unconditional entry into the hardwareRX/hardwareTX branch for `!defined(CONFIG_IDF_TARGET_ESP32)` is safe. The `|| (defined(RX) && defined(TX))` arm handles classic ESP32 boards that provide custom framework-defined UART pins. Do not flag this condition as potentially undefined-macro unsafe.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.

Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.

Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.

Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/util.cpp:400-422
Timestamp: 2026-03-21T11:37:39.788Z
Learning: In WLED (wled00/util.cpp), `beatsin88_t` is a 1:1 replacement of FastLED's `beatsin88` and expects Q8.8 fixed-point BPM values (e.g., 120 BPM = 30720). Callers in FX.cpp already pass Q8.8-encoded values. Do not flag these callers as passing plain BPM — there is no regression from the FastLED replacement.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.

Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: DedeHai
Repo: wled/WLED PR: 4791
File: wled00/util.cpp:737-743
Timestamp: 2025-09-15T19:13:56.469Z
Learning: In WLED's util.cpp, the *_realloc_malloc functions (p_realloc_malloc and d_realloc_malloc) are intentionally designed to free the original buffer on realloc failure and allocate a new buffer, implementing a "replace buffer" semantic rather than traditional realloc behavior. This is documented in the function comments and is the intended design by the author DedeHai.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-05T01:11:24.101Z
Learning: Applies to wled00/**/*.{cpp,h,hpp} : Use spaces (2 per level) for C++ indentation in firmware files

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).

Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:85-87
Timestamp: 2026-04-07T22:49:49.796Z
Learning: In WLED C++ code reviews and documentation (docs/cpp.instructions.md), the F() macro and PROGMEM/FlashStringHelper behavior is platform-specific:
- On ESP8266: F() / PROGMEM explicitly stores string literals in flash, saving RAM. This is important because ESP8266 has very limited RAM and string literals default to RAM. Always recommend F() for print-only string literals on ESP8266.
- On ESP32: PROGMEM is defined as a no-op, PSTR(s) is just (s), and string literals already reside in flash/rodata. F() yields little to no RAM savings on ESP32 but is harmless and may be needed to satisfy __FlashStringHelper* overloads. Do not expect RAM savings from F() on ESP32.

Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-05T01:11:24.101Z
Learning: Ensure AI-generated code is well documented with explanatory comments

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-15T13:21:32.008Z
Learning: In WLED code reviews (and generally), when a PR discussion involves "is this worth doing?" uncertainty about a proposed reliability, safety, or data-integrity mechanism (e.g. CRC checks, backups, power-loss protection), suggest performing a software FMEA (Failure Mode and Effects Analysis). The FMEA should: enumerate failure modes for the feared event, assess each mitigation's effectiveness per failure mode, note common-cause/common-mode failures, and rate credibility for the typical WLED use case. This was successfully used in PR `#5421` to evaluate CRC-based config file protection and was well-received by the WLED maintainers (softhack007 explicitly requested this be remembered for future reviews).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@softhack007, thanks for the clarification! This is already captured in my long-term memory (learning b66905e4): since fastled_slim is a full replacement (not just a slim subset), the old FastLED trigonometric aliases (sin8(), cos8(), sin16(), cos16()) don't exist at all — calling them without the _t suffix causes a compile error, not just a performance issue.

This actually makes the documentation addition even more important: the wording in docs/cpp.instructions.md should reflect that these old names are outright invalid (compile error), not merely discouraged. I'll update the suggested table note accordingly:

### Trigonometric Functions

WLED uses a custom `fastled_slim` fork (not the original FastLED library). The old FastLED trig aliases **do not exist** — using them causes a compile error:

| ❌ Do not use (compile error) | ✅ Use instead | Notes |
|---|---|---|
| `sin8()`, `cos8()` | `sin8_t()`, `cos8_t()` | 8-bit sine/cosine (fastled_slim.h) |
| `sin16()`, `cos16()` | `sin16_t()`, `cos16_t()` | 16-bit sine/cosine (fastled_slim.h) |
| `sinf()`, `cosf()` | `sin_approx()`, `cos_approx()` | Fast float approximations (wled_math.cpp) |

Regarding sqrt — yes, it would be worth @DedeHai confirming: does WLED have a preferred fast-sqrt replacement (e.g. sqrt16() or an integer approximation from wled_math.cpp), or is sqrtf() / sqrt() acceptable directly on hot paths?


🏁 Script executed:

#!/bin/bash
# Check what sqrt-related functions exist in wled_math.cpp or fastled_slim
rg -n "sqrt" --type=cpp -C2

Length of output: 10338


@softhack007, confirmed — the trig function guidance is still correct. Since fastled_slim is a full replacement with no backward-compat shims, using the old names (sin8(), cos8(), sin16(), cos16()) causes a compile error now, which makes documentation even more critical.

Regarding sqrt, the codebase search reveals a clear split:

  • Integer sqrt → use sqrt32_bw() from wled_math.cpp / fcn_declare.h (used in hot paths in FX_fcn.cpp, FX.cpp, audio_reactive.cpp)
  • Float sqrtsqrtf() is used directly throughout — no WLED-specific float replacement exists

I also spotted atan2_t() in use in FX.cpp. Here's the updated suggested section for docs/cpp.instructions.md:

### Math / Trigonometric Functions

WLED uses a custom `fastled_slim` fork — the old FastLED trig aliases **do not exist** and
cause a **compile error** if used. Use the `_t` variants and WLED math helpers instead:

| ❌ Do not use (compile error or suboptimal) | ✅ Use instead | Source |
|---|---|---|
| `sin8()`, `cos8()` | `sin8_t()`, `cos8_t()` | `fastled_slim.h` |
| `sin16()`, `cos16()` | `sin16_t()`, `cos16_t()` | `fastled_slim.h` |
| `sinf()`, `cosf()` | `sin_approx()`, `cos_approx()` | `wled_math.cpp` |
| `atan2f()`, `atan2()` | `atan2_t()` | `wled_math.cpp` |
| `sqrt()` on integers | `sqrt32_bw()` | `wled_math.cpp` / `fcn_declare.h` |
| `sqrtf()` on floats | `sqrtf()` (acceptable) | — no WLED replacement |

The learning b66905e4 I already have covers the fastled_slim compile-error behavior, so no new learning entry is needed unless DedeHai has corrections on the sqrt side.


🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:43-43
Timestamp: 2026-03-22T00:49:53.727Z
Learning: In WLED (wled00/file.cpp and related), all callers of writeObjectToFile(), writeObjectToFileUsingId(), readObjectFromFile(), and readObjectFromFileUsingId() execute exclusively on the main Arduino loop task. No async webserver callback (which runs on a separate FreeRTOS task on ESP32) directly calls any of these file functions — async handlers either set flags or delegate to the main loop. This means the static File f in file.cpp is never concurrently accessed from multiple tasks, and the window between doCloseFile=false and f.close() in closeFile() cannot be exploited by a parallel open() call.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/FX_fcn.cpp:1484-1498
Timestamp: 2026-03-21T22:29:23.438Z
Learning: In WLED (wled00/FX_fcn.cpp), `WS2812FX::waitForLEDs(unsigned maxWaitMS)` is intentionally a const wait-only helper — it does NOT call `strip.suspend()` or `strip.resume()`. When a caller needs to protect a critical OS section (e.g. `file.close()`, `ESP.getFreeHeap()`) from concurrent LED DMA/RMT output, the caller must own the full `strip.suspend()` → `strip.waitForLEDs()` → critical section → `strip.resume()` sequence. Putting `resume()` inside `waitForLEDs()` would release the lock before the critical call and defeat the purpose.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:283-292
Timestamp: 2026-03-31T17:33:09.706Z
Learning: In the WLED Zigbee RGB Light usermod (`usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h`), mutex timeouts used with `xSemaphoreTake(zbStateMutex, ...)` should be at least 100–200 ms (not 10 ms), because the Zigbee FreeRTOS task can be busy for longer than 10 ms during ZCL frame construction, NWK encryption, and ZBOSS scheduler operations on ESP32-C6.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.

Learnt from: willmmiles
Repo: wled/WLED PR: 4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.h:336-339
Timestamp: 2026-03-28T01:36:54.706Z
Learning: In WLED (wled00/wled.h), the RX/TX hardware pin guard `#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX)))` is intentional and correct. For all non-classic ESP32 targets (C3, C5, C6, S2, S3, P4, C61), the arduino-esp32 framework always defines RX and TX according to the target's native UART pins, so the unconditional entry into the hardwareRX/hardwareTX branch for `!defined(CONFIG_IDF_TARGET_ESP32)` is safe. The `|| (defined(RX) && defined(TX))` arm handles classic ESP32 boards that provide custom framework-defined UART pins. Do not flag this condition as potentially undefined-macro unsafe.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.

Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:267-491
Timestamp: 2026-04-08T15:03:02.041Z
Learning: In WLED (wled00/src/dependencies/fastled_slim/fastled_slim.h and wled_math.cpp): Since the original FastLED library has been replaced by the custom `fastled_slim` fork, the old FastLED trigonometric function aliases (`sin8()`, `cos8()`, `sin16()`, `cos16()`) no longer exist — there are no backward-compat shims. The `_t` variants are the only valid names: use `sin8_t()`, `cos8_t()`, `sin16_t()`, `cos16_t()` (declared in fastled_slim.h, implemented in wled_math.cpp), and `sin_approx()` / `cos_approx()` for float approximations. Calling the old names without `_t` will cause a compile error.

Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED noise function aliases (`inoise8`, `inoise16`, etc.) are not used. These should be replaced with the proper WLED functions (`perlin8`, `perlin16`, etc.) as the `inoise*` functions are only aliases kept for compatibility reasons.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.

Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.

Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/util.cpp:400-422
Timestamp: 2026-03-21T11:37:39.788Z
Learning: In WLED (wled00/util.cpp), `beatsin88_t` is a 1:1 replacement of FastLED's `beatsin88` and expects Q8.8 fixed-point BPM values (e.g., 120 BPM = 30720). Callers in FX.cpp already pass Q8.8-encoded values. Do not flag these callers as passing plain BPM — there is no regression from the FastLED replacement.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.

Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: DedeHai
Repo: wled/WLED PR: 4791
File: wled00/util.cpp:737-743
Timestamp: 2025-09-15T19:13:56.469Z
Learning: In WLED's util.cpp, the *_realloc_malloc functions (p_realloc_malloc and d_realloc_malloc) are intentionally designed to free the original buffer on realloc failure and allocate a new buffer, implementing a "replace buffer" semantic rather than traditional realloc behavior. This is documented in the function comments and is the intended design by the author DedeHai.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-05T01:11:24.101Z
Learning: Applies to wled00/**/*.{cpp,h,hpp} : Use spaces (2 per level) for C++ indentation in firmware files

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.

Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:85-87
Timestamp: 2026-04-07T22:49:49.796Z
Learning: In WLED C++ code reviews and documentation (docs/cpp.instructions.md), the F() macro and PROGMEM/FlashStringHelper behavior is platform-specific:
- On ESP8266: F() / PROGMEM explicitly stores string literals in flash, saving RAM. This is important because ESP8266 has very limited RAM and string literals default to RAM. Always recommend F() for print-only string literals on ESP8266.
- On ESP32: PROGMEM is defined as a no-op, PSTR(s) is just (s), and string literals already reside in flash/rodata. F() yields little to no RAM savings on ESP32 but is harmless and may be needed to satisfy __FlashStringHelper* overloads. Do not expect RAM savings from F() on ESP32.

Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-05T01:11:24.101Z
Learning: Ensure AI-generated code is well documented with explanatory comments

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-15T13:21:32.008Z
Learning: In WLED code reviews (and generally), when a PR discussion involves "is this worth doing?" uncertainty about a proposed reliability, safety, or data-integrity mechanism (e.g. CRC checks, backups, power-loss protection), suggest performing a software FMEA (Failure Mode and Effects Analysis). The FMEA should: enumerate failure modes for the feared event, assess each mitigation's effectiveness per failure mode, note common-cause/common-mode failures, and rate credibility for the typical WLED use case. This was successfully used in PR `#5421` to evaluate CRC-based config file protection and was well-received by the WLED maintainers (softhack007 explicitly requested this be remembered for future reviews).

Added guidelines for supporting reviews and discussions in PRs.
Add notes on  LittleFS filenames and float-to-unsigned conversion.
Updated the LittleFS filename path in instructions to reflect the correct directory structure.
example file path: don't use directories
coderabbitai[bot]

This comment was marked as resolved.

Corrected a typo in the LittleFS filenames caveat.
Avoid that AI reviews over-apply the F() rule.
@softhack007

This comment was marked as duplicate.

@coderabbitai

This comment was marked as duplicate.

1 similar comment
@coderabbitai

This comment was marked as duplicate.

coderabbitai[bot]

This comment was marked as resolved.

* Math and trigonometric function usage guidelines for WLED, replacing deprecated FastLED aliases with new function names and providing a comparison table.
* Minor re-styling
coderabbitai[bot]

This comment was marked as outdated.

was jumping from H2 -> H4
@DedeHai
Copy link
Copy Markdown
Collaborator

DedeHai commented Apr 8, 2026

@softhack can you give a summary on what it does? I can not judge if this is good or not but it probably is a lot better than what we had.

coderabbitai[bot]

This comment was marked as resolved.

@softhack007
Copy link
Copy Markdown
Member Author

softhack007 commented Apr 8, 2026

@softhack can you give a summary on what it does? I can not judge if this is good or not but it probably is a lot better than what we had.

@DedeHai Yes, sure.

  • Its a replacement for the old copilot-instructions.md, with "agent mode" text moved into its own file.
  • The new files in docs/ are AI-generated coding rules, extracted from the source code directly (actually from WLED-MM). With a lot of editing and additions from myself.

I've tried to structure everything so that copilot "agent" tasks will not be forced pre-load the coding guidelines (would make agent mode requests very slow), but just use very generic statements to guide work.

The rabbit has a new .coderabbit.yaml that makes the generic copilot-instructions.md plus coding style in "docs/" applicable = adds our rules for each PR code review. The rabbit will still do all other stuff it did before.

For optimization, i've added this HUMAN_ONLY_START / HUMAN_ONLY_END mechanism.

  • HUMAN_ONLY_ = examples, basic facts, detailed tables.
  • AI facing = doc without HUMAN_ONLY_ => rules, project specific "way to do", only the core info.

I know the C++ file is quite big - just read over it and say if there is stuff you don't like (will remove), or important stuff missing (will add).

Overall idea: an "automated" database of coding rules and best practices. The rabbit will automatically use parts of them for reviews, but contributors don't need to read through anything.

There are a few caveats and quick "recovery steps" in case that too much low-prio BS pop up during reviews, or in case that copiliot tasks are producing worse results. We needs to have an eye on it initially. I'll talk to netmindz to share my "brain dump" just in case.

coderabbitai[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/cpp.instructions.md`:
- Line 511: The docs currently hardcode WLED_MAX_SEGNAME_LEN = 64 in the
LittleFS filename-length guidance; update the wording around the LFS_NAME_MAX /
file.open() guidance to avoid a single platform-specific value by referring to
the platform default (e.g., "platform default WLED_MAX_SEGNAME_LEN") or
explicitly noting the common targets (ESP8266 vs ESP32, e.g., 32/64) and ensure
the example for constructed paths (like "/ledmap_" + segment name + ".json")
states to validate against LFS_NAME_MAX rather than a hardcoded 64; keep
references to LFS_NAME_MAX and file.open() so readers can locate the guidance.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1368e382-1410-444b-822c-e2d3c93ff05f

📥 Commits

Reviewing files that changed from the base of the PR and between 0784334 and 2910f72.

📒 Files selected for processing (1)
  • docs/cpp.instructions.md

Comment thread docs/cpp.instructions.md

## Caveats and Pitfalls

- **LittleFS filenames**: File paths passed to `file.open()` must not exceed 255 bytes (`LFS_NAME_MAX`). Validate constructed paths (e.g., `/ledmap_` + segment name + `.json`) stay within this limit (assume standard configurations, like WLED_MAX_SEGNAME_LEN = 64).
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid hardcoding WLED_MAX_SEGNAME_LEN = 64 in cross-platform guidance.

Line 511 is otherwise correct, but pinning the example to 64 can mislead ESP8266 reviewers (where the practical segment-name limit differs). Prefer neutral wording like “platform default WLED_MAX_SEGNAME_LEN” or explicitly mention 32/64 by target.

Suggested doc tweak
-- **LittleFS filenames**: File paths passed to `file.open()` must not exceed 255 bytes (`LFS_NAME_MAX`). Validate constructed paths (e.g., `/ledmap_` + segment name + `.json`) stay within this limit (assume standard configurations, like WLED_MAX_SEGNAME_LEN = 64).
+- **LittleFS filenames**: File paths passed to `file.open()` must not exceed 255 bytes (`LFS_NAME_MAX`). Validate constructed paths (e.g., `/ledmap_` + segment name + `.json`) stay within this limit (assume standard platform defaults for `WLED_MAX_SEGNAME_LEN`).

Based on learnings: In WLED, segment name limits are platform-dependent (ESP8266 vs ESP32), and filename-length checks should assume standard default configurations rather than a single hardcoded value.

📝 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
- **LittleFS filenames**: File paths passed to `file.open()` must not exceed 255 bytes (`LFS_NAME_MAX`). Validate constructed paths (e.g., `/ledmap_` + segment name + `.json`) stay within this limit (assume standard configurations, like WLED_MAX_SEGNAME_LEN = 64).
- **LittleFS filenames**: File paths passed to `file.open()` must not exceed 255 bytes (`LFS_NAME_MAX`). Validate constructed paths (e.g., `/ledmap_` + segment name + `.json`) stay within this limit (assume standard platform defaults for `WLED_MAX_SEGNAME_LEN`).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/cpp.instructions.md` at line 511, The docs currently hardcode
WLED_MAX_SEGNAME_LEN = 64 in the LittleFS filename-length guidance; update the
wording around the LFS_NAME_MAX / file.open() guidance to avoid a single
platform-specific value by referring to the platform default (e.g., "platform
default WLED_MAX_SEGNAME_LEN") or explicitly noting the common targets (ESP8266
vs ESP32, e.g., 32/64) and ensure the example for constructed paths (like
"/ledmap_" + segment name + ".json") states to validate against LFS_NAME_MAX
rather than a hardcoded 64; keep references to LFS_NAME_MAX and file.open() so
readers can locate the guidance.

Copy link
Copy Markdown
Member Author

@softhack007 softhack007 Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional - the rule is about "max filename limits" and "assume default configurations". In this sense, its safe to check with WLED_MAX_SEGNAME_LEN=64 even when esp8266 only has 32 chars for segments. We don't have esp8266-only features in the WLED core, so running on esp32 with 64 chars segment must be safe, too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softhack007, that's clear — thank you for the explanation. Using 64 (ESP32's default) as the example is the correct conservative choice: any path that fits with a 64-character segment name is trivially safe on ESP8266 where segment names are limited to 32 characters. My concern was misplaced.


✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:511-511
Timestamp: 2026-04-08T19:10:48.754Z
Learning: In WLED documentation and review guidance (`docs/cpp.instructions.md`), using `WLED_MAX_SEGNAME_LEN = 64` as the example value in LittleFS filename-length checks is intentional and correct. The reasoning: ESP32 uses 64-char segment names (the larger default), and if a constructed path (e.g. `/ledmap_` + segment name + `.json`) stays within 255 bytes when the name is 64 chars, it is trivially safe on ESP8266 where segment names are limited to 32 chars. WLED core has no ESP8266-only features, so validating against the larger ESP32 default is the right cross-platform conservative choice. Do NOT flag `WLED_MAX_SEGNAME_LEN = 64` in this guidance as misleading or platform-specific.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/FX_fcn.cpp:1484-1498
Timestamp: 2026-03-21T22:29:23.438Z
Learning: In WLED (wled00/FX_fcn.cpp), `WS2812FX::waitForLEDs(unsigned maxWaitMS)` is intentionally a const wait-only helper — it does NOT call `strip.suspend()` or `strip.resume()`. When a caller needs to protect a critical OS section (e.g. `file.close()`, `ESP.getFreeHeap()`) from concurrent LED DMA/RMT output, the caller must own the full `strip.suspend()` → `strip.waitForLEDs()` → critical section → `strip.resume()` sequence. Putting `resume()` inside `waitForLEDs()` would release the lock before the critical call and defeat the purpose.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:43-43
Timestamp: 2026-03-22T00:49:53.727Z
Learning: In WLED (wled00/file.cpp and related), all callers of writeObjectToFile(), writeObjectToFileUsingId(), readObjectFromFile(), and readObjectFromFileUsingId() execute exclusively on the main Arduino loop task. No async webserver callback (which runs on a separate FreeRTOS task on ESP32) directly calls any of these file functions — async handlers either set flags or delegate to the main loop. This means the static File f in file.cpp is never concurrently accessed from multiple tasks, and the window between doCloseFile=false and f.close() in closeFile() cannot be exploited by a parallel open() call.

Learnt from: softhack007
Repo: wled/WLED PR: 5435
File: wled00/file.cpp:42-55
Timestamp: 2026-03-21T22:55:37.522Z
Learning: In WLED (wled00/file.cpp), `closeFile()` intentionally clears `doCloseFile = false` *before* the `strip.suspend()` / `strip.waitForLEDs()` / `f.close()` / `strip.resume()` sequence. This is the correct design: it prevents a concurrent or re-entrant call (via `if (doCloseFile) closeFile()` at lines 287 and 353) from attempting a double-close of the shared static `f` during the wait window. The inverse risk — another task reopening `f` via `WLED_FS.open()` while the wait is in progress — is not realistic because: (1) on ESP8266 (single-core), `delay(1)` only yields to lwIP/TCP callbacks which never call `writeObjectToFile`/`readObjectFromFile`; (2) on standard ESP32 without shared RMT, the wait block is not compiled so `f.close()` is immediate; (3) in WLED's architecture, file operations are dispatched from the main loop task only. Do not recommend moving the `doCloseFile = false` assignment to after `f.close()`.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:283-292
Timestamp: 2026-03-31T17:33:09.706Z
Learning: In the WLED Zigbee RGB Light usermod (`usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h`), mutex timeouts used with `xSemaphoreTake(zbStateMutex, ...)` should be at least 100–200 ms (not 10 ms), because the Zigbee FreeRTOS task can be busy for longer than 10 ms during ZCL frame construction, NWK encryption, and ZBOSS scheduler operations on ESP32-C6.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.h:336-339
Timestamp: 2026-03-28T01:36:54.706Z
Learning: In WLED (wled00/wled.h), the RX/TX hardware pin guard `#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX)))` is intentional and correct. For all non-classic ESP32 targets (C3, C5, C6, S2, S3, P4, C61), the arduino-esp32 framework always defines RX and TX according to the target's native UART pins, so the unconditional entry into the hardwareRX/hardwareTX branch for `!defined(CONFIG_IDF_TARGET_ESP32)` is safe. The `|| (defined(RX) && defined(TX))` arm handles classic ESP32 boards that provide custom framework-defined UART pins. Do not flag this condition as potentially undefined-macro unsafe.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048` (wled00/cfg.cpp), the NTP crash on ESP32-C5 (pioarduino + IDF 5.5.x) is caused by both CONFIG_LWIP_TCPIP_CORE_LOCKING=y AND CONFIG_LWIP_CHECK_THREAD_SAFETY=y being enabled by default in the pioarduino/arduino-esp32 IDF 5.x sdkconfig. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6. Since pioarduino already has TCPIP_CORE_LOCKING enabled, wrapping ntpUdp.begin() (and any other raw lwIP API call) with LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() from lwip/tcpip.h is directly applicable without sdkconfig changes in platformio.ini.

Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.

Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.

Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED, code paths that appear unreachable may actually be safety fallbacks for low-heap conditions. These fallbacks are intentional defensive programming measures to ensure UI functionality is preserved when system resources are constrained.

Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.

Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.

Learnt from: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.

Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-29T21:50:15.920Z
Learning: In WLED 0.15.x (0_15_x branch, e.g. commit 4fb5fbeafbc6d2593d8428d6bfd554894a3ca921), `MAX_LEDS` in `wled00/const.h` is defined as: ESP8266=1664, ESP32-S2=2048, all other ESP32 variants (classic, S3, C3, etc., with or without PSRAM)=8192. The PSRAM-based MAX_LEDS split (PSRAM=2048, no PSRAM=1664 for ESP32) exists only in the 0.16/main branch, NOT in 0.15.x. Do not apply 0.16 MAX_LEDS values when analyzing 0.15.x code.

Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-08T19:07:13.970Z
Learning: Applies to **/*.cpp,**/*.h : Mark larger blocks of AI-generated code with an `// AI: below section was generated by an AI` comment

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-08T19:07:13.970Z
Learning: Applies to **/*.cpp,**/*.h : Every non-trivial AI-generated function should have a brief comment describing what it does and explaining parameters when their names alone are not self-explanatory

Learnt from: softhack007
Repo: wled/WLED PR: 5480
File: docs/cpp.instructions.md:85-87
Timestamp: 2026-04-07T22:49:49.796Z
Learning: In WLED C++ code reviews and documentation (docs/cpp.instructions.md), the F() macro and PROGMEM/FlashStringHelper behavior is platform-specific:
- On ESP8266: F() / PROGMEM explicitly stores string literals in flash, saving RAM. This is important because ESP8266 has very limited RAM and string literals default to RAM. Always recommend F() for print-only string literals on ESP8266.
- On ESP32: PROGMEM is defined as a no-op, PSTR(s) is just (s), and string literals already reside in flash/rodata. F() yields little to no RAM savings on ESP32 but is harmless and may be needed to satisfy __FlashStringHelper* overloads. Do not expect RAM savings from F() on ESP32.

Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-08T19:07:13.970Z
Learning: Applies to wled00/html_*.h,wled00/js_*.h : Never edit or commit auto-generated headers `wled00/html_*.h` and `wled00/js_*.h`

Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-15T13:21:32.008Z
Learning: In WLED code reviews (and generally), when a PR discussion involves "is this worth doing?" uncertainty about a proposed reliability, safety, or data-integrity mechanism (e.g. CRC checks, backups, power-loss protection), suggest performing a software FMEA (Failure Mode and Effects Analysis). The FMEA should: enumerate failure modes for the feared event, assess each mitigation's effectiveness per failure mode, note common-cause/common-mode failures, and rate credibility for the typical WLED use case. This was successfully used in PR `#5421` to evaluate CRC-based config file protection and was well-received by the WLED maintainers (softhack007 explicitly requested this be remembered for future reviews).

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.

@softhack007 softhack007 merged commit 2302863 into main Apr 8, 2026
53 checks passed
@softhack007 softhack007 deleted the copilot_restructure branch April 8, 2026 19:06
pcmihnea pushed a commit to pcmihnea/WLED that referenced this pull request Apr 13, 2026
…d#5480)

* comprehensive C++, Web UI, and CI/CD conventions, a condensed setup/build guide, and a new agent-mode build/test workflow with ordered commands, timeouts, validation gates, manual web validation, and troubleshooting steps.

* repository-level AI review/configuration rules, workflow best-practices, safeguards to detect and flag edits to generated web assets, and alignment checks linking AI-facing rules with human-only reference sections.

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Partly generated by an AI. Make sure that the contributor fully understands the code! awesome discussion enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants