Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add OpenTherm component (part 3: rest of the sensors) #7676

Merged
merged 32 commits into from
Nov 12, 2024
Merged

Add OpenTherm component (part 3: rest of the sensors) #7676

merged 32 commits into from
Nov 12, 2024

Conversation

olegtarasov
Copy link
Contributor

@olegtarasov olegtarasov commented Oct 25, 2024 •
edited
Loading

What does this implement/fix?

This is the third (and final) part of OpenTherm component. It contains the code to support all the sensors, inputs and outputs. Sensors configured in YAML are transformed to a set of C++ macros, which in turn generate the final code that handles sensors.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

Pull request in esphome-docs with documentation (if applicable):

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

# An extremely minimal configuration which only enables you to set the boiler's
# water temperature setpoint as a number.

esphome:
  name: thermostat-number-minimal

esp8266:
  board: d1_mini

opentherm:
  in_pin: IN_PIN
  out_pin: OUT_PIN

number:
  - platform: opentherm
    t_set:
      name: "Boiler Control setpoint"

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @olegtarasov,
Thanks for submitting this pull request! Can you add yourself as a codeowner for this integration? This way we can notify you if a bug report for this integration is reported.
In __init__.py of the integration, please add:

CODEOWNERS = ["@olegtarasov"]

And run script/build_codeowners.py

(message by NeedsCodeownersLabel)

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.77%. Comparing base (4d8b5ed) to head (c978a8d).
Report is 1575 commits behind head on dev.

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #7676      +/-   ##
==========================================
+ Coverage   53.70%   53.77%   +0.06%     
==========================================
  Files          50       50              
  Lines        9408     9804     +396     
  Branches     1654     1354     -300     
==========================================
+ Hits         5053     5272     +219     
- Misses       4056     4206     +150     
- Partials      299      326      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olegtarasov
Copy link
Contributor Author

Hi @jesserockz! The final part of OpenTherm integration is ready for review. Thanks!

@olegtarasov olegtarasov mentioned this pull request Oct 27, 2024
Closed
1 task
olegtarasov and others added 10 commits October 27, 2024 18:56
@olegtarasov
Merge branch 'dev' into opentherm-sensors2
dd42a40
@FreeBear-nc
Add new sensor: fan_speed_setpoint - This reads ID35:hb
0943c52 
Modify sensor: fan_speed - This now reads ID35:lb

Both return a value as revs per second. Multiply by 60 to get RPM
@FreeBear-nc
Merge branch 'opentherm-sensors2' into opentherm-fan
13eeb9a
@FreeBear-nc
Whitespace changes to keep clang-format happy.
aa95ce1
@olegtarasov
Merge pull request #2 from FreeBear-nc/opentherm-fan
858764a 
Opentherm fan
@olegtarasov
Merge branch 'dev' into opentherm-sensors2
37652ba
@FreeBear-nc
Extend the parse_u8_* functions to automatically multiply value by 60.
2957e48 
This is only used by ID35{lb,hb} and saves having to use a multiply
filter in the sensor.yaml
@FreeBear-nc
Default icon missing for fan speed. As pointed out by Simone Rossetto
5d01f83 
it should be mdi:fan.
@olegtarasov
Merge pull request #3 from FreeBear-nc/opentherm-fan
1c0bb4a 
Opentherm fan
@olegtarasov
Merge branch 'dev' into opentherm-sensors2
e174332
jesserockz
jesserockz reviewed Nov 3, 2024
View reviewed changes
Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the hub know if a switch (or other control type entity) has been changed by the user? I do not see the add_on_state_callbacks being added.

I find it very difficult to follow and understand this component due to the heavy use of macros.

@FreeBear-nc
Allow message_data to be overriden by a yaml entry.
70569f3 
This allows users to return data in a different format. e.g.:

  fan_speed:
  name: "Boiler fan speed"
  message_data: "u16" # overrides the default u8_lb_60 message.
@olegtarasov
Copy link
Contributor Author

@jesserockz The hub doesn't need to know the exact moment when control entity changes because all sensors, switches and outputs are sent to OpenTherm device constantly in a loop. It's required by OpenTherm standard to communicate with device every 1 second even when there is no change in controls. So the approach when we send all the messages configured in yaml in a loop adds to component's robustness (a message can fail due to protocol error, and we don't need to use complicated retry logic — the same message will be sent again eventually).

I find it very difficult to follow and understand this component due to the heavy use of macros.

I agree, but I see no other way to add class members dynamically, and adding > 100 class members for all the available sensors at once seems very wasteful. What's more, most of them likely won't be supported by end user's boiler.

jesserockz
jesserockz requested changes Nov 6, 2024
View reviewed changes
tests/components/opentherm/common.yaml Outdated Show resolved Hide resolved
tests/components/opentherm/common.yaml Outdated Show resolved Hide resolved
tests/components/opentherm/common.yaml Outdated Show resolved Hide resolved
esphome/components/opentherm/switch/switch.cpp Outdated Show resolved Hide resolved
esphome/components/opentherm/switch/switch.cpp Show resolved Hide resolved
esphome/components/opentherm/switch/switch.cpp Outdated Show resolved Hide resolved
esphome/components/opentherm/switch/switch.cpp Outdated Show resolved Hide resolved
@esphome
Copy link

esphome bot commented Nov 6, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@esphome esphome bot marked this pull request as draft November 6, 2024 01:47
@olegtarasov olegtarasov marked this pull request as ready for review November 6, 2024 17:57
@esphome esphome bot requested a review from jesserockz November 6, 2024 17:58
@jesserockz jesserockz merged commit 58d028a into esphome:dev Nov 12, 2024
25 checks passed
@jesserockz jesserockz mentioned this pull request Nov 13, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jesserockz jesserockz jesserockz approved these changes

Assignees
No one assigned
Labels
by-code-owner has-tests integration: opentherm new-integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olegtarasov @codecov-commenter @jesserockz @FreeBear-nc