Add PID controller support for any powermeter#315
Conversation
Ports the PID feature from b2500-meter, adapted for AstraMeter's async architecture. Adds PidPowermeter wrapper with built-in anti-windup, bias and replace modes, and global/per-section config via PID_KP, PID_KI, PID_KD, PID_OUTPUT_MAX, and PID_MODE. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a configurable PID controller wrapper for powermeters (configurable globally or per-section), implements anti-windup and output clamping, exposes Changes
Sequence DiagramsequenceDiagram
participant Loader as Configuration Loader
participant Config as Config File
participant Factory as PidPowermeter
participant Wrapped as Wrapped Powermeter
participant Device as Physical Device
Loader->>Config: Read [GENERAL] PID settings
Config-->>Loader: Return global PID params
Loader->>Config: Read per-section PID overrides
Config-->>Loader: Return section-specific values
Loader->>Wrapped: Create base Powermeter instance
Wrapped-->>Loader: Powermeter ready
alt PID_KP > 0
Loader->>Factory: Wrap with PidPowermeter(kp,ki,kd,output_max,mode)
Factory-->>Loader: PidPowermeter wrapper ready
end
Note over Factory,Device: Runtime: get_powermeter_watts()
Factory->>Wrapped: get_powermeter_watts()
Wrapped->>Device: Query phase readings
Device-->>Wrapped: Return [P1, P2, P3]
Wrapped-->>Factory: Return raw readings
Factory->>Factory: Compute error = -(P1+P2+P3)
Factory->>Factory: Calculate P, I (with anti-windup), D
Factory->>Factory: Clamp output to ±output_max
Factory->>Factory: Distribute PID across phases
alt mode == "bias"
Factory->>Factory: adjusted = [P1+pid, P2+pid, P3+pid]
else mode == "replace"
Factory->>Factory: adjusted = [pid, pid, pid]
end
Factory-->>Loader: Return adjusted readings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/astrameter/powermeter/pid_test.py (1)
118-134: Consider adding a regression test for D-term-driven saturation.Current anti-windup coverage is good for integral saturation, but a case where derivative spikes saturate output would protect against integral accumulation while saturated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/powermeter/pid_test.py` around lines 118 - 134, Add a new regression test (e.g., test_derivative_driven_saturation) that uses the same mocking pattern as test_anti_windup_stops_integration but forces a large derivative kick: create a PidPowermeter instance with nonzero ki, a high kp and a tight output_max, call PidPowermeter.get_powermeter_watts once to initialize, then advance time and change mock_powermeter.get_powermeter_watts to produce a sharp step so the D-term saturates the output; assert that after the saturation the integrator does not grow (call get_powermeter_watts again later and verify result is not biased by accumulated integral), referencing PidPowermeter, get_powermeter_watts, kp, ki, and output_max to find and implement the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 365-366: Replace the generic "mode = ..." wording with the actual
configuration key name `PID_MODE` in both bullets so users can copy/paste
correctly; update the two lines to read `PID_MODE = bias` (with the same
explanatory text and recommended Kp guidance) and `PID_MODE = replace` (with the
same explanation about bypassing the device loop) and ensure the backticks use
`PID_MODE` exactly.
In `@src/astrameter/powermeter/pid.py`:
- Around line 125-135: The anti-windup gate currently computes tentative_output
from p_term + self.ki * tentative_integral but clamping later uses P+I+D;
include the derivative term when deciding whether to accept the tentative
integral: compute the d_term (same expression used in output clamping, e.g.,
self.kd * derivative or variable name used in compute) and use tentative_output
= p_term + self.ki * tentative_integral + d_term in the saturation/unwinding
check (and ensure the same d_term variable is used in the final output clamping
logic), so the integral is paused when the full P+I+D output would be saturated.
Ensure you reference and reuse the existing derivative variable name (e.g.,
d_term or self._derivative) and output_max/output_min variables when updating
self._integral.
---
Nitpick comments:
In `@src/astrameter/powermeter/pid_test.py`:
- Around line 118-134: Add a new regression test (e.g.,
test_derivative_driven_saturation) that uses the same mocking pattern as
test_anti_windup_stops_integration but forces a large derivative kick: create a
PidPowermeter instance with nonzero ki, a high kp and a tight output_max, call
PidPowermeter.get_powermeter_watts once to initialize, then advance time and
change mock_powermeter.get_powermeter_watts to produce a sharp step so the
D-term saturates the output; assert that after the saturation the integrator
does not grow (call get_powermeter_watts again later and verify result is not
biased by accumulated integral), referencing PidPowermeter,
get_powermeter_watts, kp, ki, and output_max to find and implement the test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46dd2a49-e2e0-4ad7-9662-b10374dc6e16
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdconfig.ini.examplesrc/astrameter/config/config_loader.pysrc/astrameter/powermeter/__init__.pysrc/astrameter/powermeter/pid.pysrc/astrameter/powermeter/pid_test.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The tentative_output used to decide whether to accept the integral accumulation was missing the derivative term, so the gate checked P+I while the final output clamp used P+I+D. Moving d_term computation before the integral block and adding it to tentative_output means the integral is correctly paused when the full P+I+D output is saturated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
start, stop, get_powermeter_watts in PidPowermeter and the mock_powermeter fixture were undocumented; adding them brings the new files to full docstring coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
tomquist
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the contribution!
Summary
PidPowermeter, an async wrapper that layers a PID (Proportional-Integral-Derivative) controller on top of any powermeter to steer the reported grid power toward zero (net-zero grid exchange)biasmode (adds PID output to raw reading, works alongside the storage device's own loop) andreplacemode (PID output replaces raw reading entirely)PID_OUTPUT_MAXand pauses accumulation while output is saturatedPID_KP,PID_KI,PID_KD,PID_OUTPUT_MAX,PID_MODE) can be set globally in[GENERAL]or overridden per powermeter sectionasyncio.Lockinstead ofthreading.Lock, all methodsasync)Test plan
pid_test.pycover: P/I/D terms individually, output clamping, anti-windup, multi-phase bias and replace modes, zero-gains passthrough, first-call no-derivative-spike,wait_for_messagepass-through, and list immutabilityuv run pytest)ruff format,ruff check, andmypyall clean🤖 Generated with Claude Code
Summary by CodeRabbit