Improve/fix support for Bosch Junkers boilers through EMS-ESP#154
Open
enricoschroeder wants to merge 7 commits intoAlexwijn:developfrom
Open
Improve/fix support for Bosch Junkers boilers through EMS-ESP#154enricoschroeder wants to merge 7 commits intoAlexwijn:developfrom
enricoschroeder wants to merge 7 commits intoAlexwijn:developfrom
Conversation
Fixes toggling between pre-selected heating temp and flow temp calculated by SAT. Use 'selburnpow' instead of 'burnmaxpower' to set maximum boiler power. Adjust minimum possible flow temp to 12° (this seems to be the lowest value that is accepted by this boiler).
Only accept selected flow temperatures >= 12 (otherwise I get random toggling of flow temperatures on my boiler) -> needs to be investigated.
Contributor
WalkthroughUpdates to the EMS MQTT handler to clamp control setpoint values and switch the max modulation command; inline comments added explaining min values and rationale. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@custom_components/sat/mqtt/ems.py`:
- Around line 136-142: The method async_set_control_max_relative_modulation
clamps the published value but still passes the original unclamped value to the
superclass, causing a desync; change it to compute a single clamped variable
(e.g., clamped = max(value, 20)), use that clamped value in the JSON sent to
selburnpow, and call super().async_set_control_max_relative_modulation(clamped)
so both the device and the superclass state stay consistent.
- Around line 111-116: Compute a single clamped value once (e.g. clamped =
max(value, 12)) inside async_set_control_setpoint and use that clamped variable
for both the _publish_command payload and the call to
super().async_set_control_setpoint so the internal state/UI stays in sync with
the actual command sent (update references in async_set_control_setpoint to use
the clamped value for both the publish and the super call).
Comment on lines
111
to
116
| async def async_set_control_setpoint(self, value: float) -> None: | ||
| await self._publish_command(f'{{"cmd": "selflowtemp", "value": {0 if value == 10 else value}}}') | ||
| # Minimum valid setting for Bosch/Junkers boiler seems to be 12°. | ||
| # Lower values set the boiler to 12° except 0° which sets the boiler to 5°. | ||
| await self._publish_command(f'{{"cmd": "selflowtemp", "value": {max(value, 12)}}}') | ||
|
|
||
| await super().async_set_control_setpoint(value) |
Contributor
There was a problem hiding this comment.
Clamp once and pass the clamped value to super() for state consistency.
You publish a clamped value but forward the original value to the superclass, which can leave the internal state/UI out of sync with the actual boiler command. Consider clamping once and using that value for both calls.
🔧 Proposed fix
- await self._publish_command(f'{{"cmd": "selflowtemp", "value": {max(value, 12)}}}')
+ clamped_value = max(value, 12)
+ await self._publish_command(f'{{"cmd": "selflowtemp", "value": {clamped_value}}}')
- await super().async_set_control_setpoint(value)
+ await super().async_set_control_setpoint(clamped_value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def async_set_control_setpoint(self, value: float) -> None: | |
| await self._publish_command(f'{{"cmd": "selflowtemp", "value": {0 if value == 10 else value}}}') | |
| # Minimum valid setting for Bosch/Junkers boiler seems to be 12°. | |
| # Lower values set the boiler to 12° except 0° which sets the boiler to 5°. | |
| await self._publish_command(f'{{"cmd": "selflowtemp", "value": {max(value, 12)}}}') | |
| await super().async_set_control_setpoint(value) | |
| async def async_set_control_setpoint(self, value: float) -> None: | |
| # Minimum valid setting for Bosch/Junkers boiler seems to be 12°. | |
| # Lower values set the boiler to 12° except 0° which sets the boiler to 5°. | |
| clamped_value = max(value, 12) | |
| await self._publish_command(f'{{"cmd": "selflowtemp", "value": {clamped_value}}}') | |
| await super().async_set_control_setpoint(clamped_value) |
🤖 Prompt for AI Agents
In `@custom_components/sat/mqtt/ems.py` around lines 111 - 116, Compute a single
clamped value once (e.g. clamped = max(value, 12)) inside
async_set_control_setpoint and use that clamped variable for both the
_publish_command payload and the call to super().async_set_control_setpoint so
the internal state/UI stays in sync with the actual command sent (update
references in async_set_control_setpoint to use the clamped value for both the
publish and the super call).
Comment on lines
136
to
142
| async def async_set_control_max_relative_modulation(self, value: int) -> None: | ||
| await self._publish_command(f'{{"cmd": "burnmaxpower", "value": {max(value, 20)}}}') | ||
| # Do not set 'burnmaxpower' as this is an EEPROM-stored value and will wear out the EEPROM. | ||
| # Use 'selburnpow' instead. | ||
| # (see https://github.com/emsesp/EMS-ESP32/discussions/2641#discussioncomment-14611481) | ||
| await self._publish_command(f'{{"cmd": "selburnpow", "value": {max(value, 20)}}}') | ||
|
|
||
| await super().async_set_control_max_relative_modulation(value) |
Contributor
There was a problem hiding this comment.
Keep super() in sync with the clamped modulation value.
The published value is clamped, but the superclass receives the unclamped input, which can desync reported vs. applied max modulation.
🔧 Proposed fix
- await self._publish_command(f'{{"cmd": "selburnpow", "value": {max(value, 20)}}}')
+ clamped_value = max(value, 20)
+ await self._publish_command(f'{{"cmd": "selburnpow", "value": {clamped_value}}}')
- await super().async_set_control_max_relative_modulation(value)
+ await super().async_set_control_max_relative_modulation(clamped_value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def async_set_control_max_relative_modulation(self, value: int) -> None: | |
| await self._publish_command(f'{{"cmd": "burnmaxpower", "value": {max(value, 20)}}}') | |
| # Do not set 'burnmaxpower' as this is an EEPROM-stored value and will wear out the EEPROM. | |
| # Use 'selburnpow' instead. | |
| # (see https://github.com/emsesp/EMS-ESP32/discussions/2641#discussioncomment-14611481) | |
| await self._publish_command(f'{{"cmd": "selburnpow", "value": {max(value, 20)}}}') | |
| await super().async_set_control_max_relative_modulation(value) | |
| async def async_set_control_max_relative_modulation(self, value: int) -> None: | |
| # Do not set 'burnmaxpower' as this is an EEPROM-stored value and will wear out the EEPROM. | |
| # Use 'selburnpow' instead. | |
| # (see https://github.com/emsesp/EMS-ESP32/discussions/2641#discussioncomment-14611481) | |
| clamped_value = max(value, 20) | |
| await self._publish_command(f'{{"cmd": "selburnpow", "value": {clamped_value}}}') | |
| await super().async_set_control_max_relative_modulation(clamped_value) |
🤖 Prompt for AI Agents
In `@custom_components/sat/mqtt/ems.py` around lines 136 - 142, The method
async_set_control_max_relative_modulation clamps the published value but still
passes the original unclamped value to the superclass, causing a desync; change
it to compute a single clamped variable (e.g., clamped = max(value, 20)), use
that clamped value in the JSON sent to selburnpow, and call
super().async_set_control_max_relative_modulation(clamped) so both the device
and the superclass state stay consistent.
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.
Improve/fix support for Bosch Junkers boilers through EMS-ESP.
See #109
Fixes flow temperature behaving erratically because of wrong commands send to EMS-ESP. Changed commands according to feedback from EMS-ESP developers (emsesp/EMS-ESP32#2641 (reply in thread)).
Changes running reliably with my Bosch Junkers Condens GC7000i for a couple of weeks.
Pull Request Template
Summary
Checklist
Screenshots / Visuals
(Add screenshots if the PR introduces UI changes.)
Additional Notes
(Any extra information for reviewers.)
Summary by CodeRabbit