Add service constants, rename boiler state, update PWM #96
Conversation
|
Warning Rate limit exceeded@Alexwijn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request introduces modifications across the Changes
Sequence Diagram(s)sequenceDiagram
participant HA as Home Assistant
participant SAT as SAT Integration
participant Entities as Climate Entities
HA->>SAT: Call reset_integral service
SAT->>SAT: Retrieve entity_id from service data
SAT->>Entities: get_climate_entities(entity_id)
Entities-->>SAT: Return matching entities
loop For each entity
SAT->>Entity: Invoke PID.reset() and Area.PID.reset()
end
SAT->>HA: Acknowledge service completion
sequenceDiagram
participant HA as Home Assistant
participant SAT as SAT Integration
participant Entities as Climate Entities
HA->>SAT: Call pulse_width_modulation service with enabled flag
SAT->>SAT: Extract entity_id and enabled flag
SAT->>Entities: get_climate_entities(entity_id)
Entities-->>SAT: Return matching entities
loop For each entity
alt enabled is true
SAT->>Entity: Enable PWM Controller
else
SAT->>Entity: Disable PWM Controller
end
end
SAT->>HA: Acknowledge service completion
Possibly related PRs
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 (
|
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: 1
🧹 Nitpick comments (10)
custom_components/sat/fake/__init__.py (1)
93-95: Consider completing the renaming refactoring.The property
supports_relative_modulationsimply delegates tosupports_relative_modulation_management. This suggests an incomplete renaming refactoring. Consider:
- Renaming
supports_relative_modulation_managementtosupports_relative_modulation.- Updating all references to use the new name.
- Removing this delegation property.
This would improve code maintainability by eliminating unnecessary property delegation.
custom_components/sat/pwm.py (2)
23-37: Consider using a dataclass for clarity
The newly introducedCyclesclass is straightforward, storing two integer properties. A dataclass might further enhance maintainability by providing default values, type-checking, and auto-generated methods such as__repr__.
63-67: Initialization logic looks consistent
The_setpoint_adjustermight benefit from more descriptive docstrings, clarifying how the offset is determined (0.5 vs 1). Otherwise, these lines look fine.custom_components/sat/coordinator.py (2)
327-338: Conditional logic by manufacturer
This approach is straightforward, though consider offloading logic to each manufacturer class for maintainability. If new manufacturers are added or existing ones change capabilities, you can maintain it in those classes.🧰 Tools
🪛 Ruff (0.8.2)
334-337: Return the negated condition directly
Inline condition
(SIM103)
346-349: Device manufacturer check
Mirrors the same pattern assupports_relative_modulation. This repeated logic might be consolidated.🧰 Tools
🪛 Ruff (0.8.2)
346-349: Return the negated condition directly
Inline condition
(SIM103)
custom_components/sat/util.py (3)
10-10: Consider using explicit imports instead of star imports.Replace
from .const import *with explicit imports of the constants you need. This makes the code more maintainable by clearly showing which constants are being used.🧰 Tools
🪛 Ruff (0.8.2)
10-10:
from .const import *used; unable to detect undefined names(F403)
10-10: Consider using explicit imports instead of star imports.Replace
from .const import *with explicit imports to improve code maintainability and make dependencies clearer.🧰 Tools
🪛 Ruff (0.8.2)
10-10:
from .const import *used; unable to detect undefined names(F403)
10-10: Replace star import with explicit imports.Using star imports makes it difficult to track which constants are being used and may lead to naming conflicts.
Replace the star import with explicit imports of the constants used in this file:
-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, + DOMAIN, + CLIMATE +)🧰 Tools
🪛 Ruff (0.8.2)
10-10:
from .const import *used; unable to detect undefined names(F403)
custom_components/sat/__init__.py (1)
29-29: Remove unused import.The
get_climate_entitiesfunction is imported but not used in this file.-from .util import get_climate_entities🧰 Tools
🪛 Ruff (0.8.2)
29-29:
.util.get_climate_entitiesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
custom_components/sat/services.yaml (1)
9-19: Add newline at end of file and consider enhancing the description.The
pulse_width_modulationservice is well-structured, but has two minor issues:
- Missing newline at end of file (YAML best practice)
- The description could be clearer about when automatic changes might occur
Apply this diff:
required: true - example: true + example: true +Consider updating the description to be more specific about automatic changes:
- description: "Force enable or disable the Pulse Width Modulation, do note that it may be turned on or off automatically right after again." + description: "Force enable or disable the Pulse Width Modulation. Note that the state may change automatically based on the system's control logic and current conditions."🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 19-19: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
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(7 hunks)custom_components/sat/const.py(2 hunks)custom_components/sat/coordinator.py(7 hunks)custom_components/sat/esphome/__init__.py(1 hunks)custom_components/sat/fake/__init__.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/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/util.py(2 hunks)
💤 Files with no reviewable changes (1)
- custom_components/sat/area.py
✅ Files skipped from review due to trivial changes (6)
- custom_components/sat/overshoot_protection.py
- custom_components/sat/mqtt/opentherm.py
- custom_components/sat/simulator/init.py
- custom_components/sat/serial/init.py
- custom_components/sat/mqtt/ems.py
- custom_components/sat/esphome/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/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)
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)
🪛 YAMLlint (1.35.1)
custom_components/sat/services.yaml
[error] 19-19: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (50)
custom_components/sat/pwm.py (14)
8-11: Imports appear consistent with usage
It seems these imports align with new code references in the file.
42-42: Constructor signature updated as expected
Switching to use aCyclesobject is a good move for readability and modular code. Ensure that all instantiations pass the new parameter.
48-48: Storing theCyclesinstance
Persisting thecyclesparameter in_cyclesensures consistent usage. Implementation looks fine.
69-70: Improved logging
Including the offset in the debug message provides more clarity. Good update.
78-78: Reset_current_cycleinresetmethod
Resets are logically consistent. Ensure any external references account for the cycle count resetting.
103-103: Parameter order changed
Switching the order of parameters ensures consistency with their usage. Confirm that all callers have updated references accordingly.
119-119: Cycle count reset
Resets_current_cycleafter one hour window. This logic is consistent with the new cycle approach.
136-143: Ensure consistent setpoint transitions
The logic handles scenarios selectively—when flame is active vs. inactive. Validate that the+10°Cforced offset is correct for all non-flame scenarios and consider a default if none of the conditions match.
146-154: Cycle management
This logic effectively prevents exceeding the allowed maximum cycles per hour, then increments_current_cycleif a new ON cycle starts. Logging the current cycle count is valuable for troubleshooting.
184-185: Manual duty cycle logic
Calculating on/off times directly from_last_duty_cycle_percentageis a clear approach. Confirm that_last_duty_cycle_percentageis properly maintained.
217-217: Expanded debug logging
Logging cycle count, on_time, and off_time clarifies the system’s behavior at low duty cycle.
228-228: Useful debug details
Including_current_cyclein the log simplifies tracking mid-range duty cycles.
239-239: Enhanced logging
Consistent usage of logs helps diagnose high duty cycle scenarios.
268-272: Returning a fallback setpoint
ReturningMINIMUM_SETPOINTif_setpointisNoneensures a safe default. Confirm that the rest of the code can handle scenarios when_setpointis unset.custom_components/sat/coordinator.py (6)
13-20: New manufacturer and boiler references
These updated imports align with usingBoilerTemperatureTracker,BoilerState, andBoilerStatus. The manufacturer imports (Geminox, Ideal, Intergas, Nefit) are consistent with references below.🧰 Tools
🪛 Ruff (0.8.2)
14-14:
from .const import *used; unable to detect undefined names(F403)
103-143: Refactored device status checks
Switching fromDeviceStatustoBoilerStatusclarifies enumerations. Ensure all transitions cover valid states, especially as the code references temperature derivatives and setpoints. This approach is consistent with the new boiler management logic.🧰 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)
145-156: Newstateproperty
Encapsulating multiple boiler attributes into aBoilerStateobject centralizes the state. This approach is neat for typed usage. Confirm that external code references thisstateproperty correctly.
298-302: Handle missingCONF_MINIMUM_SETPOINT
Consider providing a default or error handling ifCONF_MINIMUM_SETPOINTis not in_data, to avoid potentialTypeErrorfrom castingNonetofloat.🧰 Tools
🪛 Ruff (0.8.2)
301-301:
CONF_MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
396-396: Skipping updates in HOT_WATER state
ExcludingBoilerStatus.HOT_WATERfrom temperature derivative updates can prevent interference with normal heating loops. Ensure that separate hot-water logic is handled properly elsewhere.
419-422: Cold temperature logic
Using the walrus operator streamlines the check. The fallbackmin(self.boiler_temperature, self._boiler_temperature_cold)helps keep track of a proper lower bound.custom_components/sat/climate.py (2)
34-45: Refined imports
ImportingHomeAssistant,Event,CoreState,BoilerStatus,SatDataUpdateCoordinator, andDeviceStateindicates usage of new or reorganized references. No immediate issues found.🧰 Tools
🪛 Ruff (0.8.2)
42-42:
from .const import *used; unable to detect undefined names(F403)
564-566: Fallback for relative modulation
Defaulting toMINIMUM_RELATIVE_MODULATIONwhenever_relative_modulationis disabled but management is supported ensures a safe lower bound. Confirm thatMINIMUM_RELATIVE_MODULATIONis defined or imported.🧰 Tools
🪛 Ruff (0.8.2)
564-564:
MINIMUM_RELATIVE_MODULATIONmay be undefined, or defined from star imports(F405)
custom_components/sat/services.py (6)
14-29: LGTM! The reset_integral service is well-implemented.The service correctly handles resetting both the climate's PID and areas' PIDs, with proper logging.
31-49: LGTM! The pulse_width_modulation service is well-implemented.The service correctly handles both enabling and disabling PWM, with proper logging.
14-29: LGTM! The reset_integral service is well-implemented.The service correctly handles resetting both the main PID and area PIDs for the specified climate entities, with proper logging and schema validation.
31-49: LGTM! The pulse_width_modulation service is well-implemented.The service correctly handles enabling/disabling PWM for the specified climate entities, with proper logging and schema validation.
14-29: LGTM! Well-structured service implementation.The reset_integral service is well-implemented with proper error handling, logging, and schema validation.
31-49: LGTM! Robust service implementation.The pulse_width_modulation service is well-implemented with proper parameter validation, error handling, and logging.
custom_components/sat/util.py (6)
71-83: LGTM! The create_pwm_controller function is well-implemented.The function correctly handles the creation of PWM controller with the new
supports_relative_modulation_managementparameter.🧰 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: LGTM! The get_climate_entities function is well-implemented.The function correctly handles entity retrieval with proper validation checks.
🧰 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)
71-83: LGTM! The create_pwm_controller function is well-updated.The function correctly handles the new parameter and Cycles class, maintaining proper configuration extraction and PWM controller creation.
🧰 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: LGTM! The get_climate_entities function is well-implemented.The function safely retrieves and validates climate entities, with proper error handling for missing entries.
🧰 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)
71-83: LGTM! Well-structured PWM controller creation.The function is well-implemented with proper type hints and clear parameter usage.
🧰 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: LGTM! Robust climate entities retrieval.The function implements proper error handling and entity validation.
🧰 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/const.py (3)
22-23: LGTM! The constant changes improve clarity.
- Renaming
MINIMUM_RELATIVE_MODandMAXIMUM_RELATIVE_MODto include "MODULATION" improves readability.- Adding
SERVICE_PULSE_WIDTH_MODULATIONsupports the new PWM service.Also applies to: 169-169
22-23: LGTM! Constants are well-named and properly defined.The renaming improves clarity by using more explicit terms, and the new service constant is properly defined.
Also applies to: 169-169
22-23: LGTM! Clear and consistent constant naming.The changes improve clarity by:
- Renaming modulation constants for better understanding
- Adding a well-named service constant for PWM functionality
Also applies to: 169-169
custom_components/sat/boiler.py (10)
13-28: LGTM! The BoilerStatus enum is well-designed.The enum provides a comprehensive and clear set of states for tracking boiler status.
35-43: LGTM! The BoilerState class changes enhance state tracking.The addition of
device_statusandflame_on_sinceproperties improves the ability to track boiler state and flame ignition time.Also applies to: 51-54, 61-64
10-10: Verify the impact of the increased setpoint margin.The EXCEED_SETPOINT_MARGIN has been increased from 0.1 to 1.0, which affects:
- When the boiler is considered to exceed the setpoint
- The stabilization logic below setpoint
Please verify that this change doesn't negatively impact temperature control precision.
Also applies to: 135-138
13-28: LGTM! The BoilerStatus enum is well-designed.The enum provides a comprehensive and clear representation of possible boiler states.
10-10: Verify the impact of increased EXCEED_SETPOINT_MARGIN.The margin has been increased from 0.1 to 1.0, which could significantly affect temperature control behavior. Please ensure this change aligns with the desired temperature control characteristics.
35-44: LGTM! The BoilerState initialization is well-structured.The class properly handles the new status and timing properties with appropriate type hints.
138-138: Verify the impact of the modified temperature comparison.The addition of
+ 1in the temperature comparison could affect the stabilization behavior. Please ensure this change aligns with the desired temperature control characteristics.
13-28: LGTM! Well-structured boiler status enumeration.The BoilerStatus enum provides a clear and comprehensive set of states for tracking boiler operation.
35-44: LGTM! Clear state management implementation.The BoilerState class is well-structured with proper type hints and clear initialization.
135-138: Verify the impact of modified temperature margins.The changes to EXCEED_SETPOINT_MARGIN and stabilization logic could affect boiler behavior:
- EXCEED_SETPOINT_MARGIN increased from 0.1 to 1.0
- Added +1 to stabilization comparison
Please confirm that these changes:
- Don't cause excessive temperature fluctuations
- Maintain efficient boiler operation
- Don't impact user comfort
custom_components/sat/binary_sensor.py (1)
37-37: LGTM!The property rename from
supports_relative_modulation_managementtosupports_relative_modulationimproves clarity while maintaining functionality.custom_components/sat/__init__.py (1)
65-67: LGTM!The addition of service registration during setup is a good practice, ensuring that the new
reset_integralandpulse_width_modulationservices are properly initialized.custom_components/sat/services.yaml (1)
1-8: LGTM!The
reset_integralservice is well-defined with:
- Clear description of its purpose and use case
- Proper targeting to climate entities within the SAT integration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
custom_components/sat/services.yaml (1)
1-15: Service Definitions are Accurate but YAML File is Missing a Newline
The newreset_integralandpulse_width_modulationservices are well defined with the correct targets and field specifications as per requirements. However, YAMLlint has flagged that there is no new line at the end of the file. Please add a newline at the end to adhere to YAML formatting standards.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
custom_components/sat/translations/en.json (1)
253-269: Validate Service Descriptions and Improve Grammar
The added definitions for thereset_integralandpulse_width_modulationservices are clear and comprehensive. As a minor improvement, consider updating the grammar in thereset_integraldescription. For example, change:-"description": "This service reset the integrating part of the PID controller for the specified climate entity. This may be useful if the integral value has become too large or if the PID controller's performance has degraded." +"description": "This service resets the integrating part of the PID controller for the specified climate entity. This may be useful if the integral value has become too large or if the PID controller's performance has degraded."This tweak will improve clarity and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/sat/services.yaml(1 hunks)custom_components/sat/translations/en.json(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
custom_components/sat/services.yaml
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
…or reports it doesn't support it
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/sat/climate.py (1)
42-42: Replace star import with explicit imports.Using star imports can lead to namespace pollution and make it harder to track where symbols are coming from. Consider explicitly importing the required constants.
Replace the star import with explicit imports:
-from .const import * +from .const import ( + DOMAIN, + COORDINATOR, + CLIMATE, + CONF_THERMOSTAT, + # ... add other required constants +)🧰 Tools
🪛 Ruff (0.8.2)
42-42:
from .const import *used; unable to detect undefined names(F403)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/sat/climate.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
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 (2)
custom_components/sat/climate.py (2)
923-924: LGTM! Boiler status handling has been improved.The migration from
DeviceStatustoBoilerStatusimproves code clarity and the PWM update logic has been simplified.Also applies to: 928-929, 934-934
950-952: LGTM! Boiler status check is consistent.The boiler status check for hot water and flame activity is correctly implemented.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor