Improved Area Support #88
Conversation
WalkthroughThe pull request introduces a new configuration option, Changes
Possibly related PRs
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
custom_components/sat/area.py (1)
141-141: Consider rounding weighted values to prevent floating-point precision issues.The multiplication of values by weights could lead to floating-point precision issues. Consider rounding the results to maintain consistent precision.
- area.heating_curve.update(area.target_temperature * area.weight, current_outside_temperature) + area.heating_curve.update(round(area.target_temperature * area.weight, 2), current_outside_temperature) - area.pid.update(area.error * area.weight, area.heating_curve.value, boiler_temperature) + area.pid.update(round(area.error * area.weight, 2), area.heating_curve.value, boiler_temperature)Also applies to: 150-150
custom_components/sat/climate.py (2)
Line range hint
793-817: Enhance logging for better debugging of PID controller updates.The current logging could be more informative by including additional context about the weight-based calculations and state transitions.
- _LOGGER.info(f"Updating error value to {self.error} (Reset: False)") + _LOGGER.info(f"Updating PID with error: {self.error}, heating curve: {self.heating_curve.value}, boiler temp: {self._coordinator.boiler_temperature_filtered} (Reset: False)") - _LOGGER.info(f"Updating error value to {self.error} (Reset: True)") + _LOGGER.info(f"Resetting PID with error: {self.error}, heating curve: {self.heating_curve.value} (Reset: True)")
519-528: Improve import clarity by explicitly importing constants.The code uses star imports which make it difficult to track where constants are defined. Consider explicitly importing the constants to improve code clarity and maintainability.
-from .const import * +from .const import ( + HEATING_MODE_COMFORT, + MINIMUM_SETPOINT, + DEADBAND, + # ... other constants +)🧰 Tools
🪛 Ruff (0.8.2)
519-519:
HEATING_MODE_COMFORTmay be undefined, or defined from star imports(F405)
525-525:
MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
528-528:
MINIMUM_SETPOINTmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/sat/area.py(6 hunks)custom_components/sat/climate.py(4 hunks)custom_components/sat/const.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/sat/climate.py
519-519: HEATING_MODE_COMFORT may be undefined, or defined from star imports
(F405)
525-525: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
528-528: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
796-796: DEADBAND may be undefined, or defined from star imports
(F405)
796-796: DEADBAND may be undefined, or defined from star imports
(F405)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
custom_components/sat/area.py (1)
151-156: Consider adding bounds checking for the weighted error.The multiplication of error with weight could lead to disproportionate PID responses.
Apply this diff to add bounds checking:
def update(self, boiler_temperature: float) -> None: for area in self.areas: if area.error is not None: - area.pid.update(area.error * area.weight, area.heating_curve.value, boiler_temperature) + weighted_error = min(max(area.error * area.weight, -10), 10) # Limit weighted error to [-10, 10] + area.pid.update(weighted_error, area.heating_curve.value, boiler_temperature) def update_reset(self) -> None: for area in self.areas: if area.error is not None: - area.pid.update_reset(area.error * area.weight, area.heating_curve.value) + weighted_error = min(max(area.error * area.weight, -10), 10) # Limit weighted error to [-10, 10] + area.pid.update_reset(weighted_error, area.heating_curve.value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
custom_components/sat/area.py(6 hunks)custom_components/sat/config_flow.py(2 hunks)custom_components/sat/const.py(2 hunks)custom_components/sat/translations/en.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/sat/const.py
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/sat/config_flow.py
553-553: CONF_ROOMS may be undefined, or defined from star imports
(F405)
680-680: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
681-681: CONF_ROOMS may be undefined, or defined from star imports
(F405)
685-685: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
🔇 Additional comments (5)
custom_components/sat/config_flow.py (1)
551-556: LGTM!The menu options are correctly updated to include the "areas" option when rooms are configured.
🧰 Tools
🪛 Ruff (0.8.2)
553-553:
CONF_ROOMSmay be undefined, or defined from star imports(F405)
custom_components/sat/area.py (3)
37-40: LGTM!The weight property is correctly implemented with proper type hints.
100-104: LGTM!The focus property correctly handles None error values by filtering them out before finding the maximum.
26-26:⚠️ Potential issueImprove error handling in weight initialization.
The current implementation might raise
AttributeErrorifconfig_options.get(CONF_ROOM_WEIGHTS)returnsNone.Apply this diff to fix the error handling:
- self._weight: float = config_options.get(CONF_ROOM_WEIGHTS).get(entity_id, 1.0) + self._weight: float = config_options.get(CONF_ROOM_WEIGHTS, {}).get(entity_id, 1.0)Likely invalid or redundant comment.
custom_components/sat/translations/en.json (1)
217-217: LGTM!The translations are clear, descriptive, and correctly formatted.
Also applies to: 221-224
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
custom_components/sat/config_flow.py (1)
679-703: Improve error handling and define constants.The implementation looks good but could benefit from these improvements:
- Add error handling for state retrieval
- Define the default weight as a constant
Apply this diff to improve the implementation:
+ DEFAULT_ROOM_WEIGHT = 1.0 # Add at the top with other constants + async def async_step_areas(self, _user_input: dict[str, Any] | None = None): room_labels: dict[str, str] = {} room_weights: dict[str, float] = self._options.get(CONF_ROOM_WEIGHTS, {}) for entity_id in self._config_entry.data.get(CONF_ROOMS, []): state = self.hass.states.get(entity_id) - name = state.name if state else entity_id + try: + name = state.name if state and state.name else entity_id + except (AttributeError, KeyError): + name = entity_id room_labels[entity_id] = f"{name} ({entity_id})" if _user_input is not None: return await self.update_options({ CONF_ROOM_WEIGHTS: { entity_id: float(_user_input[friendly_name]) for entity_id, friendly_name in room_labels.items() } }) return self.async_show_form( step_id="areas", data_schema=vol.Schema({ - vol.Required(friendly_name, default=room_weights.get(entity_id, 1.0)): selector.NumberSelector( + vol.Required(friendly_name, default=room_weights.get(entity_id, DEFAULT_ROOM_WEIGHT)): selector.NumberSelector( selector.NumberSelectorConfig(min=0.1, max=3.0, step=0.1) ) for entity_id, friendly_name in room_labels.items() }) )🧰 Tools
🪛 Ruff (0.8.2)
681-681:
CONF_ROOM_WEIGHTSmay be undefined, or defined from star imports(F405)
683-683:
CONF_ROOMSmay be undefined, or defined from star imports(F405)
690-690:
CONF_ROOM_WEIGHTSmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/sat/area.py(5 hunks)custom_components/sat/climate.py(4 hunks)custom_components/sat/config_flow.py(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- custom_components/sat/area.py
- custom_components/sat/climate.py
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/sat/climate.py
520-520: HEATING_MODE_COMFORT may be undefined, or defined from star imports
(F405)
527-527: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
530-530: MINIMUM_SETPOINT may be undefined, or defined from star imports
(F405)
798-798: DEADBAND may be undefined, or defined from star imports
(F405)
798-798: DEADBAND may be undefined, or defined from star imports
(F405)
custom_components/sat/config_flow.py
553-553: CONF_ROOMS may be undefined, or defined from star imports
(F405)
681-681: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
683-683: CONF_ROOMS may be undefined, or defined from star imports
(F405)
690-690: CONF_ROOM_WEIGHTS may be undefined, or defined from star imports
(F405)
🔇 Additional comments (2)
custom_components/sat/config_flow.py (2)
551-556: LGTM!The conditional addition of the "areas" menu option based on room configuration is well-implemented.
🧰 Tools
🪛 Ruff (0.8.2)
553-553:
CONF_ROOMSmay be undefined, or defined from star imports(F405)
553-553: Verify star imports.The static analysis tool flags potential undefined imports from star imports. Please verify that these constants are properly imported:
CONF_ROOMSCONF_ROOM_WEIGHTSRun this script to verify the imports:
Also applies to: 681-681, 683-683, 690-690
✅ Verification successful
Star imports are properly resolved
The constants
CONF_ROOMSandCONF_ROOM_WEIGHTSare defined inconst.pyand are correctly imported intoconfig_flow.pyvia the star importfrom .const import *. While explicit imports would be preferable for maintainability, the current implementation is functionally correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the constants are properly imported # Test: Search for the constant definitions rg -l "^CONF_ROOM_WEIGHTS\s*=|^CONF_ROOMS\s*="Length of output: 79
Script:
#!/bin/bash # Get the constant definitions from const.py echo "=== Checking constant definitions in const.py ===" rg "^CONF_ROOM_WEIGHTS\s*=|^CONF_ROOMS\s*=" custom_components/sat/const.py -A 1 echo -e "\n=== Checking imports in config_flow.py ===" rg "^from.*const.*import|^import.*const" custom_components/sat/config_flow.pyLength of output: 552
🧰 Tools
🪛 Ruff (0.8.2)
553-553:
CONF_ROOMSmay be undefined, or defined from star imports(F405)
|
Hey there @Alexwijn! Very interesting PR, especially since I'm struggling to balance my boiler's output for floor heating w.r.t. radiators in sleeping rooms. |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration
CONF_ROOM_WEIGHTSfor specifying room-specific heating priorities.These updates provide more granular and flexible temperature control for multi-room heating systems.