feat: modernize non-linear power scaling#2362
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded configurable non-linear brightness→power scaling via a global exponent plus forward/reverse 0–255 lookup tables; power estimation and brightness-clamping now map brightness through these tables. Public APIs to set/get the exponent were added and tests updated to validate linear vs non-linear behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CFastLED
participant PowerModel
participant PowerScalingState
User->>CFastLED: setPowerScalingExponent(exponent)
CFastLED->>PowerScalingState: set_power_scaling_exponent(exponent)
Note over PowerScalingState: rebuild forward/reverse 0–255 tables if available (exponent != 1.0)
User->>CFastLED: getEstimatedPowerInMilliWatts(apply_limiter)
CFastLED->>PowerModel: calculate_unscaled_power_mW(LEDs)
PowerModel-->>CFastLED: total_unscaled_mW
CFastLED->>PowerScalingState: scale_power_for_brightness(total_unscaled_mW, brightness)
PowerScalingState-->>CFastLED: adjusted_mW
CFastLED-->>User: estimated_power_mW
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
Maintainer summary: This is not a literal replay of the 2020 patch. It is the same feature direction reimplemented on top of the current power-management code. Key review points:
So the tradeoff here is: smaller, reviewable modernization of #1048 now, instead of trying to solve the whole controller-aware power architecture in one PR. |
Contributor
📊 Header Compilation PerformanceTotal Time: 952.01ms Top 5 Slowest Headers
Warnings
Full Report |
Contributor
📊 Header Compilation PerformanceTotal Time: 1246.89ms Top 5 Slowest Headers
Warnings
Full Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fl/power_estimation.cpp (1)
26-31:⚠️ Potential issue | 🟡 MinorReset the exponent after the fixture test as well.
Line 200 leaves the FastLED singleton with a non-linear exponent after this test finishes. The constructor reset protects tests before they run, but not later tests in the same process.
Proposed fix
struct PowerEstimationFixture { PowerEstimationFixture() { // Reset power model to WS2812 @ 5V default (80, 55, 75, 5) set_power_model(PowerModelRGB()); set_power_scaling_exponent(1.0f); } + + ~PowerEstimationFixture() { + set_power_scaling_exponent(1.0f); + } };Also applies to: 200-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fl/power_estimation.cpp` around lines 26 - 31, The fixture resets the power model/exponent in the constructor but never restores the exponent after tests, leaving FastLED with a non-linear exponent; add a destructor for PowerEstimationFixture that restores the global state by calling set_power_scaling_exponent(1.0f) (and optionally set_power_model(PowerModelRGB()) if you want to fully revert) so the exponent is reset when the fixture goes out of scope; implement ~PowerEstimationFixture() { set_power_scaling_exponent(1.0f); } in the same struct to ensure later tests run with the default exponent.
🧹 Nitpick comments (1)
tests/power_mgt.cpp (1)
12-20: Restore the previous exponent in the scoped helper.
ScopedPowerScalingExponentcurrently resets to1.0funconditionally. Capturing the previous value keeps the helper safe for nested scopes or future tests with non-linear baselines.Proposed refactor
struct ScopedPowerScalingExponent { - explicit ScopedPowerScalingExponent(float exponent) { + explicit ScopedPowerScalingExponent(float exponent) + : previous_exponent(get_power_scaling_exponent()) { set_power_scaling_exponent(exponent); } ~ScopedPowerScalingExponent() { - set_power_scaling_exponent(1.0f); + set_power_scaling_exponent(previous_exponent); } + + float previous_exponent; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/power_mgt.cpp` around lines 12 - 20, ScopedPowerScalingExponent resets the exponent unconditionally to 1.0f in its destructor; change it to capture the current exponent on construction and restore that stored value on destruction. Add a private float member (e.g., previous_exponent_) in struct ScopedPowerScalingExponent, set previous_exponent_ = get_power_scaling_exponent() in the constructor before calling set_power_scaling_exponent(exponent), and in ~ScopedPowerScalingExponent() call set_power_scaling_exponent(previous_exponent_); if get_power_scaling_exponent() doesn't exist, add or use the appropriate accessor to read the current exponent so nested scopes restore correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/power_mgt.cpp.hpp`:
- Around line 250-256: The reverse LUT is built too optimistically by choosing
the first source with state.forward[source] >= scaled; instead build a
conservative floor inverse so reverse[scaled] maps to the largest source whose
forward value is <= scaled (never exceed the scaled target). Change the loop
that sets state.reverse to advance source while source+1 < 256 and
state.forward[source+1] <= scaled, then assign state.reverse[scaled] =
static_cast<fl::u8>(source); this uses state.forward and state.reverse to
produce a safe floor inverse (affecting the limiter and unmap_power_value
paths).
---
Outside diff comments:
In `@tests/fl/power_estimation.cpp`:
- Around line 26-31: The fixture resets the power model/exponent in the
constructor but never restores the exponent after tests, leaving FastLED with a
non-linear exponent; add a destructor for PowerEstimationFixture that restores
the global state by calling set_power_scaling_exponent(1.0f) (and optionally
set_power_model(PowerModelRGB()) if you want to fully revert) so the exponent is
reset when the fixture goes out of scope; implement ~PowerEstimationFixture() {
set_power_scaling_exponent(1.0f); } in the same struct to ensure later tests run
with the default exponent.
---
Nitpick comments:
In `@tests/power_mgt.cpp`:
- Around line 12-20: ScopedPowerScalingExponent resets the exponent
unconditionally to 1.0f in its destructor; change it to capture the current
exponent on construction and restore that stored value on destruction. Add a
private float member (e.g., previous_exponent_) in struct
ScopedPowerScalingExponent, set previous_exponent_ =
get_power_scaling_exponent() in the constructor before calling
set_power_scaling_exponent(exponent), and in ~ScopedPowerScalingExponent() call
set_power_scaling_exponent(previous_exponent_); if get_power_scaling_exponent()
doesn't exist, add or use the appropriate accessor to read the current exponent
so nested scopes restore correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a2bb39d-4d4a-4f83-bbdb-b3f810f4f066
📒 Files selected for processing (6)
src/FastLED.cpp.hppsrc/FastLED.hsrc/power_mgt.cpp.hppsrc/power_mgt.htests/fl/power_estimation.cpptests/power_mgt.cpp
Member
Author
|
Follow-up cleanup pushed:
This matches the repo's existing memory-tier convention more closely than enabling the new path everywhere. |
Contributor
📊 Header Compilation PerformanceTotal Time: 982.05ms Top 5 Slowest Headers
Warnings
Full Report |
zackees
force-pushed
the
powertweaks-modern
branch
from
c686306 to
82111e8
Compare
Contributor
📊 Header Compilation PerformanceTotal Time: 1186.59ms Top 5 Slowest Headers
Warnings
Full Report |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/FastLED.h (1)
1362-1375: Considerconst-qualifyinggetPowerScalingExponent().
getEstimatedPowerInMilliWatts(...)at line 1435 isconst, and if it ever needs to consult the configured exponent via this accessor it won't be able to. The setter/getter pair is stateless here (delegates to free functions), so marking the getterconstis free and future-proof.♻️ Proposed change
- inline float getPowerScalingExponent() { + inline float getPowerScalingExponent() const { return get_power_scaling_exponent(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FastLED.h` around lines 1362 - 1375, The getter getPowerScalingExponent() should be const-qualified so const methods (e.g., getEstimatedPowerInMilliWatts) can call it; update the member function signature of getPowerScalingExponent() to be inline float getPowerScalingExponent() const and ensure it still returns get_power_scaling_exponent(); leave setPowerScalingExponent(float) unchanged and keep delegating to set_power_scaling_exponent(float).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/FastLED.h`:
- Around line 1362-1368: Update the documentation for setPowerScalingExponent
(and its wrapper set_power_scaling_exponent) to state that the exponent must be
> 0.0 and that values ≤ 0.0 are treated as invalid and will cause the
implementation to fall back to linear (1.0) behavior with identity tables; also
note that values very close to 1.0 are treated as linear for stability and will
likewise fallback to the identity mapping. Ensure the new note clarifies this
behavior applies only to power estimation/limiting and not rendered brightness,
matching the implementation's defensive checks.
---
Nitpick comments:
In `@src/FastLED.h`:
- Around line 1362-1375: The getter getPowerScalingExponent() should be
const-qualified so const methods (e.g., getEstimatedPowerInMilliWatts) can call
it; update the member function signature of getPowerScalingExponent() to be
inline float getPowerScalingExponent() const and ensure it still returns
get_power_scaling_exponent(); leave setPowerScalingExponent(float) unchanged and
keep delegating to set_power_scaling_exponent(float).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c72b0540-f8da-47b5-a923-97bb3f16da34
📒 Files selected for processing (6)
src/FastLED.cpp.hppsrc/FastLED.hsrc/power_mgt.cpp.hppsrc/power_mgt.htests/fl/power_estimation.cpptests/power_mgt.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/power_mgt.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/FastLED.cpp.hpp
- tests/fl/power_estimation.cpp
- src/power_mgt.h
- src/power_mgt.cpp.hpp
zackees
force-pushed
the
powertweaks-modern
branch
from
82111e8 to
34820bb
Compare
Contributor
📊 Header Compilation PerformanceTotal Time: 1183.30ms Top 5 Slowest Headers
Warnings
Full Report |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/power_mgt.cpp.hpp (1)
271-292:set_power_scaling_exponentcorrectly round-trips throughset_power_model, but note the read-modify-write on the singleton.
set_power_scaling_exponentfetches the current model by value, mutatesexponent, and callsset_power_model, which on small-memory targets stompsexponentback to1.0f. That matches the documented behavior (no-op on small memory,get_power_scaling_exponent()returns 1.0).Minor: since this is a read-modify-write on
gPowerModel(), any future concurrent caller (e.g., a UI task setting the exponent whileshow()runs) would race. Not an issue today given FastLED is single-threaded on MCU, but if you ever expose this from a UI widget running on a separate core (ESP32), consider guarding the singleton or documenting single-thread-only usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/power_mgt.cpp.hpp` around lines 271 - 292, set_power_scaling_exponent performs a read-modify-write on the global singleton gPowerModel() which can race if accessed from multiple threads/cores; to fix, protect the operation (and any other writers) with a lightweight lock or critical section around the read-modify-write and the setter path (e.g., add a mutex/critical section used by set_power_scaling_exponent and set_power_model), or alternatively add a brief comment documenting that gPowerModel()/set_power_scaling_exponent are single-thread-only; update references to gPowerModel(), set_power_model(), and get_power_scaling_exponent() accordingly so all writers/readers use the same protection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/power_mgt.cpp.hpp`:
- Around line 271-292: set_power_scaling_exponent performs a read-modify-write
on the global singleton gPowerModel() which can race if accessed from multiple
threads/cores; to fix, protect the operation (and any other writers) with a
lightweight lock or critical section around the read-modify-write and the setter
path (e.g., add a mutex/critical section used by set_power_scaling_exponent and
set_power_model), or alternatively add a brief comment documenting that
gPowerModel()/set_power_scaling_exponent are single-thread-only; update
references to gPowerModel(), set_power_model(), and get_power_scaling_exponent()
accordingly so all writers/readers use the same protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51cd47c5-3714-4a43-bbd0-da6c07129327
📒 Files selected for processing (6)
src/FastLED.cpp.hppsrc/FastLED.hsrc/power_mgt.cpp.hppsrc/power_mgt.htests/fl/power_estimation.cpptests/power_mgt.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/power_mgt.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/FastLED.cpp.hpp
- tests/fl/power_estimation.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a refreshed replacement for #1048, rebuilt against current
master.Problem being addressed:
What this PR changes:
1.0fso existing users retain today's linear behavior unless they opt inFastLED.getEstimatedPowerInMilliWatts()so its estimate matches the limiter's modelSKETCH_HAS_LARGE_MEMORYWhy this is different from the original 2020 patch:
masteralready has a power-model API, so this version keeps the change scoped to today's subsystem instead of replaying the old file-level diffDeliberate non-goals in this PR:
Focused verification:
uv run test.py power_mgtuv run test.py power_estimationIf maintainers want to carry the original SK6805-1515 calibration forward, that can now be expressed as a user-configured exponent instead of a library-wide baked-in constant.
Summary by CodeRabbit
New Features
Documentation
Tests