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.
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
Add thermocouple type, autoconversion, averaging, and has_fault to max31856 #6659
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #6659 +/- ##
==========================================
+ Coverage 53.70% 54.05% +0.34%
==========================================
Files 50 50
Lines 9408 9554 +146
Branches 1654 1687 +33
==========================================
+ Hits 5053 5164 +111
- Misses 4056 4066 +10
- Partials 299 324 +25 ☔ View full report in Codecov by Sentry. |
d1bbca8
to
7487e50
Compare
ddc5d7f
to
58bbdc7
Compare
58bbdc7
to
c3a8ca5
Compare
466231b
to
541f807
Compare
3cf170c
to
e855cd8
Compare
e855cd8
to
6642660
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for the new configurations
void MAX31856Sensor::setup() { | ||
ESP_LOGCONFIG(TAG, "Setting up MAX31856Sensor '%s'...", this->name_.c_str()); | ||
ESP_LOGVV(TAG, "Setting up MAX31856Sensor '%s'...", this->name_.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change this? These should be LOGCONFIG
} | ||
) | ||
.extend(cv.polling_component_schema("60s")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should extend cv.COMPONENT_SCHEMA instead then
cv.Optional(CONF_HAS_FAULT): binary_sensor.binary_sensor_schema( | ||
entity_category=ENTITY_CATEGORY_DIAGNOSTIC, | ||
device_class=DEVICE_CLASS_PROBLEM, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sensors should not have binary sensors as children configuration. They should be in their own binary_sensor.py
file which can link back to the parent.
exclusive_update_interval_with_default = cv.Exclusive( | ||
CONF_UPDATE_INTERVAL, | ||
"mode", | ||
exclusive_err_msg, | ||
"update_interval will enable polling mode", | ||
) | ||
exclusive_update_interval_with_default.default = lambda: "60s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still setting the default update interval to 60s. You can use regular Exclusive and a custom overall validation to add default values for this.
Then, this component should no longer inherit from PollingComponent, but set its own update interval
in setup if the time is set but the pin is not.
There are other components that set their own update interval you can reference (I cannot remember them though)
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I've already spent too much time on this, and I don't have time to go searching the codebase to figure out how to make a bunch of changes here, unfortunately. You can close this, and I'll just use it from my own repo. |
What does this implement/fix?
It adds thermocouple type, autoconversion, averaging, and has_fault to the max31856 sensor while maintaining backwards compatibility with polling mode if the
data_ready_pin
is not specified.Types of changes
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3803
Test Environment
Example entry for
config.yaml
:Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: