Boiler and PWM improvements #97
Conversation
Refactor Manufacturer Identification
…or reports it doesn't support it
Add service constants, rename boiler state, update PWM
WalkthroughThe changes introduce numerous enhancements to the sat integration. They add new funding details and service endpoints while refactoring many components. Major modifications include updating boiler state management (introducing a new BoilerStatus enum and related properties), revising PWM control (introducing a new Cycles class and updating function signatures), and streamlining manufacturer resolution by renaming methods from “name” to “friendly_name” and adjusting class structures. Additionally, properties formerly named “supports_relative_modulation_management” have been renamed, and the config flow now supports a simulator mode with altered logic. Various constants and test assertions were also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CF as SatFlowHandler
participant FS as async_step_finish
U->>CF: Initiate config flow (simulator mode)
CF->>CF: Check if CONF_MODE == MODE_SIMULATOR
CF->>FS: Bypass steps, call async_step_finish
FS-->>CF: Return finish result
sequenceDiagram
participant U as User
participant HA as Home Assistant
participant S as Service Handler
participant C as Climate Entity
U->>HA: Trigger service call (reset_integral or PWM)
HA->>S: Forward service request to integration
S->>C: Execute reset_integral / pulse_width_modulation update
C-->>S: Acknowledge action
S-->>HA: Return service completion
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
coderabbitai
bot
changed the title
Alexwijn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
custom_components/sat/pwm.py (1)
66-66: Potential type mismatch in_setpoint_offset.The annotation declares
_setpoint_offset: int, but it is sometimes assigned a float value (0.5).
Consider updating the type annotation to float or using an integer to avoid confusion and improve type consistency.- self._setpoint_offset: int = 0.5 if supports_relative_modulation_management else 1 + self._setpoint_offset: float = 0.5 if supports_relative_modulation_management else 1custom_components/sat/coordinator.py (1)
334-337: Simplify return logic.These blocks return False if
isinstance(...), otherwise True. Inline the negation to eliminate the explicit if-else.
Although minor, this makes the code shorter and more readable:-if isinstance(self.manufacturer, (Ideal, Intergas, Geminox, Nefit)): - return False -return True +return not isinstance(self.manufacturer, (Ideal, Intergas, Geminox, Nefit))Also applies to: 346-349
🧰 Tools
🪛 Ruff (0.8.2)
334-337: Return the negated condition directly
Inline condition
(SIM103)
custom_components/sat/services.py (2)
14-29: Add error handling for the reset operation.Consider adding try-catch blocks to handle potential errors during the reset operation and log any failures.
Apply this diff to add error handling:
async def reset_integral(call: ServiceCall): """Service to reset the integral part of the PID controller.""" target_entities = call.data.get("entity_id", []) for climate in get_climate_entities(hass, target_entities): _LOGGER.info("Reset Integral action called for %s", climate.entity_id) + try: climate.pid.reset() climate.areas.pids.reset() + except Exception as e: + _LOGGER.error("Failed to reset integral for %s: %s", climate.entity_id, str(e))
31-49: Add error handling and state validation.Consider adding:
- Try-catch blocks to handle potential errors during enable/disable operations
- State validation to prevent redundant operations
Apply this diff to add error handling and state validation:
async def pulse_width_modulation(call: ServiceCall): """Service to enable or disable Pulse Width Modulation.""" enabled = call.data.get("enabled") target_entities = call.data.get("entity_id", []) for climate in get_climate_entities(hass, target_entities): _LOGGER.info("Pulse Width Modulation action called for %s with enabled=%s", climate.entity_id, enabled) + try: + # Skip if already in desired state + if enabled == climate.pwm.is_enabled(): + _LOGGER.debug("PWM already in desired state for %s", climate.entity_id) + continue + if enabled: climate.pwm.enable() else: climate.pwm.disable() + except Exception as e: + _LOGGER.error("Failed to set PWM state for %s: %s", climate.entity_id, str(e))custom_components/sat/fake/__init__.py (1)
93-95: Consider removing this pass-through property in a future update.The property
supports_relative_modulationsimply returnssupports_relative_modulation_management. While this is likely part of a transition to rename the property across the codebase, consider removing this pass-through property in a future update once all references have been updated to usesupports_relative_modulationdirectly.custom_components/sat/util.py (2)
71-83: Consider replacing star imports with explicit imports.The function changes look good, but there are several constants imported with star imports that could be made explicit for better maintainability.
Replace star imports with explicit imports:
-from .const import * +from .const import ( + CONF_CYCLES_PER_HOUR, + CONF_AUTOMATIC_DUTY_CYCLE, + CONF_DUTY_CYCLE, + CONF_MODE, + MODE_SWITCH, + CONF_FORCE_PULSE_WIDTH_MODULATION, +)🧰 Tools
🪛 Ruff (0.8.2)
74-74:
CONF_CYCLES_PER_HOURmay be undefined, or defined from star imports(F405)
75-75:
CONF_AUTOMATIC_DUTY_CYCLEmay be undefined, or defined from star imports(F405)
76-76:
CONF_DUTY_CYCLEmay be undefined, or defined from star imports(F405)
77-77:
CONF_MODEmay be undefined, or defined from star imports(F405)
77-77:
MODE_SWITCHmay be undefined, or defined from star imports(F405)
77-77:
CONF_FORCE_PULSE_WIDTH_MODULATIONmay be undefined, or defined from star imports(F405)
86-103: Consider replacing star imports with explicit imports.The function implementation looks good, but there are constants imported with star imports that could be made explicit for better maintainability.
Replace star imports with explicit imports:
-from .const import * +from .const import DOMAIN, CLIMATE🧰 Tools
🪛 Ruff (0.8.2)
95-95:
DOMAINmay be undefined, or defined from star imports(F405)
98-98:
CLIMATEmay be undefined, or defined from star imports(F405)
custom_components/sat/services.yaml (1)
1-6: Service 'reset_integral' DefinitionThe new
reset_integralservice is added with a target specifying that only climate entities integrated with "sat" will be affected. While the target configuration is correctly defined, consider whether adding an inline description here would improve clarity and consistency with the translations file.custom_components/sat/translations/en.json (1)
259-268: Translation Entry for 'pulse_width_modulation' ServiceThe translation entry for
pulse_width_modulationis functional; however, the description could be rephrased for improved clarity. For example, consider updating it to:
"Force enable or disable Pulse Width Modulation; note that the PWM state may automatically revert shortly after being set."
This adjustment would help avoid potential confusion about the transient nature of the setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.github/FUNDING.yml(1 hunks)custom_components/sat/__init__.py(2 hunks)custom_components/sat/area.py(0 hunks)custom_components/sat/binary_sensor.py(1 hunks)custom_components/sat/boiler.py(3 hunks)custom_components/sat/climate.py(8 hunks)custom_components/sat/config_flow.py(4 hunks)custom_components/sat/const.py(2 hunks)custom_components/sat/coordinator.py(7 hunks)custom_components/sat/entity.py(1 hunks)custom_components/sat/esphome/__init__.py(1 hunks)custom_components/sat/fake/__init__.py(1 hunks)custom_components/sat/manufacturer.py(1 hunks)custom_components/sat/manufacturers/atag.py(1 hunks)custom_components/sat/manufacturers/baxi.py(1 hunks)custom_components/sat/manufacturers/brotge.py(1 hunks)custom_components/sat/manufacturers/de_dietrich.py(1 hunks)custom_components/sat/manufacturers/ferroli.py(1 hunks)custom_components/sat/manufacturers/geminox.py(1 hunks)custom_components/sat/manufacturers/ideal.py(1 hunks)custom_components/sat/manufacturers/immergas.py(1 hunks)custom_components/sat/manufacturers/intergas.py(1 hunks)custom_components/sat/manufacturers/itho.py(1 hunks)custom_components/sat/manufacturers/nefit.py(1 hunks)custom_components/sat/manufacturers/other.py(1 hunks)custom_components/sat/manufacturers/radiant.py(1 hunks)custom_components/sat/manufacturers/remeha.py(1 hunks)custom_components/sat/manufacturers/sime.py(1 hunks)custom_components/sat/manufacturers/simulator.py(0 hunks)custom_components/sat/manufacturers/vaillant.py(1 hunks)custom_components/sat/manufacturers/viessmann.py(1 hunks)custom_components/sat/manufacturers/worcester.py(1 hunks)custom_components/sat/mqtt/ems.py(1 hunks)custom_components/sat/mqtt/opentherm.py(1 hunks)custom_components/sat/overshoot_protection.py(2 hunks)custom_components/sat/pwm.py(11 hunks)custom_components/sat/relative_modulation.py(1 hunks)custom_components/sat/sensor.py(1 hunks)custom_components/sat/serial/__init__.py(1 hunks)custom_components/sat/services.py(1 hunks)custom_components/sat/services.yaml(1 hunks)custom_components/sat/simulator/__init__.py(1 hunks)custom_components/sat/translations/en.json(1 hunks)custom_components/sat/util.py(2 hunks)tests/test_manufacturer.py(2 hunks)
💤 Files with no reviewable changes (2)
- custom_components/sat/manufacturers/simulator.py
- custom_components/sat/area.py
✅ Files skipped from review due to trivial changes (14)
- .github/FUNDING.yml
- custom_components/sat/manufacturers/sime.py
- custom_components/sat/manufacturers/brotge.py
- custom_components/sat/manufacturers/nefit.py
- custom_components/sat/manufacturers/intergas.py
- custom_components/sat/manufacturers/de_dietrich.py
- custom_components/sat/manufacturers/viessmann.py
- custom_components/sat/manufacturers/atag.py
- custom_components/sat/mqtt/ems.py
- custom_components/sat/serial/init.py
- custom_components/sat/mqtt/opentherm.py
- custom_components/sat/overshoot_protection.py
- custom_components/sat/esphome/init.py
- custom_components/sat/simulator/init.py
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/sat/__init__.py
24-24: .const.SERVICE_RESET_INTEGRAL imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
25-25: .const.SERVICE_PULSE_WIDTH_MODULATION imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
29-29: .util.get_climate_entities imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
custom_components/sat/util.py
10-10: from .const import * used; unable to detect undefined names
(F403)
74-74: CONF_CYCLES_PER_HOUR may be undefined, or defined from star imports
(F405)
75-75: CONF_AUTOMATIC_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
76-76: CONF_DUTY_CYCLE may be undefined, or defined from star imports
(F405)
77-77: CONF_MODE may be undefined, or defined from star imports
(F405)
77-77: MODE_SWITCH may be undefined, or defined from star imports
(F405)
77-77: CONF_FORCE_PULSE_WIDTH_MODULATION may be undefined, or defined from star imports
(F405)
95-95: DOMAIN may be undefined, or defined from star imports
(F405)
98-98: CLIMATE may be undefined, or defined from star imports
(F405)
custom_components/sat/config_flow.py
357-357: CONF_MODE may be undefined, or defined from star imports
(F405)
357-357: MODE_SIMULATOR may be undefined, or defined from star imports
(F405)
446-446: CONF_MODE may be undefined, or defined from star imports
(F405)
446-446: MODE_SIMULATOR may be undefined, or defined from star imports
(F405)
467-467: CONF_MODE may be undefined, or defined from star imports
(F405)
467-467: MODE_SIMULATOR may be undefined, or defined from star imports
(F405)
custom_components/sat/coordinator.py
14-14: from .const import * used; unable to detect undefined names
(F403)
108-108: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
111-112: Use a single if statement instead of nested if statements
(SIM102)
128-128: DEADBAND may be undefined, or defined from star imports
(F405)
301-301: CONF_MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
334-337: Return the negated condition directly
Inline condition
(SIM103)
346-349: Return the negated condition directly
Inline condition
(SIM103)
custom_components/sat/manufacturer.py
46-46: Local variable member_id is assigned to but never used
(F841)
custom_components/sat/climate.py
42-42: from .const import * used; unable to detect undefined names
(F403)
564-564: MINIMUM_RELATIVE_MODULATION may be undefined, or defined from star imports
(F405)
🔇 Additional comments (38)
custom_components/sat/pwm.py (3)
103-103: Verify the parameter order inupdatemethod.The method signature has changed to
async def update(self, boiler: BoilerState, requested_setpoint: float).
If existing call sites rely on the previous parameter order, this will cause breakage.
Please confirm or update all call sites accordingly.
137-143: Double-check logic for adjusted setpoint transitions.In these lines,
_setpointis either adjusted downward (- _setpoint_offset), set to_setpoint_adjuster.current, or forced to boiler temperature plus 10.
Ensure each transition is thoroughly tested to avoid abrupt or unintended temperature jumps.
269-272: Return fallback setpoint.Returning
self._setpoint or MINIMUM_SETPOINTensures a valid setpoint even if_setpointisNone.
This is a reasonable fallback, but confirm any edge cases where an uninitialized_setpointmight mask a bug.
Overall, the logic appears appropriate.custom_components/sat/coordinator.py (2)
103-143: Comprehensive expansion ofBoilerStatus.The newly introduced and refined statuses (
INITIALIZING,HOT_WATER,IDLE, etc.) add clarity.
Ensure that any custom logic (e.g.,COOLING_DOWN,OVERSHOOT_HANDLING) is covered by appropriate tests and that downstream components handle the new states correctly.🧰 Tools
🪛 Ruff (0.8.2)
108-108:
MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
111-112: Use a single
ifstatement instead of nestedifstatements(SIM102)
128-128:
DEADBANDmay be undefined, or defined from star imports(F405)
298-302: Potential undefined import forCONF_MINIMUM_SETPOINT.Referencing
self._data.get(CONF_MINIMUM_SETPOINT)relies on the star import from.const.
Ensure thatCONF_MINIMUM_SETPOINTis always in scope or consider importing explicitly to avoid potential reference issues highlighted by static analysis.🧰 Tools
🪛 Ruff (0.8.2)
301-301:
CONF_MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
custom_components/sat/climate.py (3)
563-565: Check definition ofMINIMUM_RELATIVE_MODULATION.Static analysis indicates that
MINIMUM_RELATIVE_MODULATIONmay come from a star import.
Verify that it is indeed defined in.constor elsewhere to prevent runtime NameError.🧰 Tools
🪛 Ruff (0.8.2)
564-564:
MINIMUM_RELATIVE_MODULATIONmay be undefined, or defined from star imports(F405)
928-930: Confirm logic for disabling PWM during cooling phase.Disabling PWM when the system transitions to
COOLING_DOWNand the calculated setpoint is higher thanpwm.setpointhelps avoid overshoot.
Ensure that any partial or mid-cycle transitions won’t leave the system in an unintended state.
950-959: Sequential checks for dynamic minimum setpoint.When
self._minimum_setpoint_version == 1and the device is actively heating, the code updates the dynamic minimum setpoint and then toggles heater state depending on_setpoint.
Confirm that timing edge cases (e.g., partial flame cycles or quick toggles) don’t lead to frequent on/off cycling.🧰 Tools
🪛 Ruff (0.8.2)
959-959:
MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
custom_components/sat/manufacturers/baxi.py (1)
6-7: LGTM! Consistent with the codebase-wide property renaming.The renaming from
nametofriendly_nameis well-implemented and maintains the original functionality.custom_components/sat/manufacturers/ideal.py (1)
6-7: LGTM! Maintains consistency with other manufacturer implementations.The property renaming is implemented correctly and follows the same pattern as other manufacturer classes.
custom_components/sat/manufacturers/geminox.py (1)
6-7: LGTM! Follows the established pattern for manufacturer classes.The property renaming is implemented consistently with other manufacturer classes.
custom_components/sat/manufacturers/ferroli.py (1)
6-7:✅ Verification successful
LGTM! Consistent implementation across all manufacturer classes.
The property renaming is implemented correctly. Let's verify the impact across the codebase.
🏁 Script executed:
#!/bin/bash # Description: Verify that all manufacturer classes use the new property name # and there are no remaining references to the old property name # Check for any remaining uses of the old property name echo "Checking for remaining uses of 'def name' in manufacturer classes..." rg "def name" "custom_components/sat/manufacturers/" # Check for consistent use of the new property name echo "Verifying consistent use of 'friendly_name' in manufacturer classes..." rg "def friendly_name" "custom_components/sat/manufacturers/" # Check for any remaining references to '.name' in the codebase echo "Checking for remaining references to '.name' property..." rg "\.name" "custom_components/sat/"Length of output: 2149
Approved: Manufacturer Implementation Consistency Confirmed
The update to use
friendly_namein manufacturer classes is correctly applied across the board. Our checks confirm that no manufacturer file still declares anameproperty, and all manufacturer classes (includingferroli.py) consistently implementfriendly_name.
- All manufacturer files now define and use
friendly_name(as seen in multiple files includingcustom_components/sat/manufacturers/ferroli.py).- The only remaining reference to
.nameis incustom_components/sat/switch/__init__.py. If that reference is meant for a different type of entity or is intentional, no further action is needed.LGTM!
custom_components/sat/manufacturers/itho.py (1)
4-7: LGTM!The implementation follows the established pattern for manufacturer classes, with proper inheritance, property decoration, and type annotation.
custom_components/sat/manufacturers/radiant.py (1)
6-6: LGTM!The renaming from
nametofriendly_namealigns with the broader changes across manufacturer classes while maintaining the same functionality.custom_components/sat/manufacturers/other.py (1)
4-7: LGTM!The implementation follows the established pattern for manufacturer classes, with proper inheritance, property decoration, and type annotation.
custom_components/sat/manufacturers/vaillant.py (1)
6-6: LGTM!The renaming from
nametofriendly_namealigns with the broader changes across manufacturer classes while maintaining the same functionality.custom_components/sat/manufacturers/immergas.py (1)
6-7: LGTM!The renaming of the
nameproperty tofriendly_nameis consistent with the broader refactoring effort to standardize manufacturer naming across the integration.custom_components/sat/manufacturers/remeha.py (1)
4-7: LGTM!The new
Remehamanufacturer class follows the established pattern and correctly implements thefriendly_nameproperty.custom_components/sat/manufacturers/worcester.py (1)
4-7: LGTM!The new
Worcestermanufacturer class follows the established pattern and correctly implements thefriendly_nameproperty.custom_components/sat/entity.py (1)
30-30: LGTM!The update to use
friendly_nameinstead ofnameis consistent with the manufacturer class changes while maintaining proper null checks and default values.tests/test_manufacturer.py (2)
10-10: LGTM!The assertion correctly verifies that the resolved manufacturer's class name matches the name from MANUFACTURERS.
19-21: LGTM!The changes improve test coverage by:
- Correctly building the member_id_to_names dictionary using the new MANUFACTURERS structure
- Adding an additional assertion to verify member_id
- Using friendly_name in the assertion message for better clarity
Also applies to: 28-29
custom_components/sat/manufacturer.py (2)
6-25: LGTM!The simplified MANUFACTURERS dictionary structure improves maintainability by directly mapping manufacturer names to their IDs.
28-39: LGTM!The changes improve the Manufacturer class by:
- Making it an abstract base class
- Adding member_id property with proper initialization
- Renaming name to friendly_name for better clarity
custom_components/sat/relative_modulation.py (1)
37-43: LGTM!The changes improve the code by:
- Simplifying the control flow
- Reducing nested conditions
- Making the order of checks more logical and easier to follow
custom_components/sat/util.py (1)
79-80: LGTM! Good use of the Cycles class.The introduction of the
Cyclesclass to encapsulate cycle-related parameters improves code organization and maintainability.custom_components/sat/const.py (2)
22-23: LGTM! Good improvement in constant naming.The renaming from
*_MODto*_MODULATIONimproves code clarity by using the full word.
169-169: LGTM! Service constant follows naming convention.The new service constant follows the existing naming pattern and is properly grouped with other service-related constants.
custom_components/sat/boiler.py (4)
10-10: Verify the impact of increased setpoint margin.The
EXCEED_SETPOINT_MARGINhas been increased from0.1to1.0, which could significantly impact temperature tracking behavior. Please ensure this change aligns with the expected boiler behavior.
13-28: LGTM! Well-structured enum for boiler states.The
BoilerStatusenum provides a comprehensive and well-named set of boiler states. Good use ofstrinheritance for string representation.
35-44: LGTM! Good enhancement of boiler state tracking.The
BoilerStateclass has been enhanced with proper typing and additional state tracking capabilities through thedevice_statusandflame_on_sinceparameters.
135-138: LGTM! Improved temperature tracking logic.The temperature tracking logic has been improved with:
- Simplified condition for exceeding setpoint
- Added margin to stabilization check
- More precise temperature comparisons
custom_components/sat/__init__.py (1)
65-66: LGTM! Service registration added.The addition of service registration enhances the functionality of the integration.
custom_components/sat/sensor.py (1)
241-241: LGTM! Property renamed for consistency.The change from
nametofriendly_nameis consistent with the broader property renaming effort across the codebase.custom_components/sat/config_flow.py (2)
357-359: LGTM! Simulator mode flow optimization.The changes optimize the configuration flow by bypassing unnecessary steps when in simulator mode, providing a more streamlined user experience.
Also applies to: 446-448, 467-469
🧰 Tools
🪛 Ruff (0.8.2)
357-357:
CONF_MODEmay be undefined, or defined from star imports(F405)
357-357:
MODE_SIMULATORmay be undefined, or defined from star imports(F405)
492-492: LGTM! Manufacturer resolution improved.The changes improve manufacturer resolution by:
- Using a default value of -1 when no manufacturers are found
- Consistently using friendly_name for manufacturer display
Also applies to: 497-499
custom_components/sat/services.yaml (1)
7-15: Service 'pulse_width_modulation' DefinitionThe
pulse_width_modulationservice is defined with the necessary target details and includes a required fieldenabledwith an example value. The YAML structure appears correct and follows standard configuration practices. Optionally, you might want to add an inline description if it isn’t solely provided via translations for clarity.custom_components/sat/translations/en.json (1)
254-258: Translation Entry for 'reset_integral' ServiceThe JSON entry for
reset_integralincludes a clear name and description that appropriately explains its functionality in resetting the integral part of the PID controller. The content is both descriptive and user-friendly.
Summary by CodeRabbit
New Features
Improvements