-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Haier component update to support more protocol variations #7040
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #7040 +/- ##
==========================================
+ Coverage 53.70% 53.78% +0.07%
==========================================
Files 50 50
Lines 9408 9660 +252
Branches 1654 1704 +50
==========================================
+ Hits 5053 5196 +143
- Misses 4056 4140 +84
- Partials 299 324 +25 ☔ View full report in Codecov by Sentry. |
@@ -39,7 +39,7 @@ lib_deps = | |||
bblanchon/ArduinoJson@6.18.5 ; json | |||
wjtje/qr-code-generator-library@1.7.0 ; qr_code | |||
functionpointer/arduino-MLX90393@1.0.0 ; mlx90393 | |||
pavlodn/HaierProtocol@0.9.28 ; haier | |||
pavlodn/HaierProtocol@0.9.31 ; haier |
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 links to the releases so we can take a quick look over what is being changed. This is not for reviewing the library itself, but for making sure that there is no hidden malicious code in them.
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.
Hi @jesserockz,
My library code is here: https://github.com/paveldn/HaierProtocol
Here you can see the difference between 28 and 31: https://github.com/paveldn/HaierProtocol/compare/7bab6a487cc0c77f7b0d5aba831de7580ca3bcbe..a356ba37cec091ad9c963e40f93f8f4125e8bb3c?diff=unified&w=
Changes are not that big. Most of the changes are related to tests and tools. The biggest change is I changed some headers to be able to build it for the host platform together with the UART component for the host. You can ignore everything that is not in src or include folders. As you can see in the library.json only those folders are included in the library itself.
Let me know if I can help or if you have questions.
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.
Thank for that. Your diff url is backwards btw =)
Not required, but would be nice if you tagged the repo at the point of the package release too so it is clear to anyone else who wants to inspect. Otherwise they will not know what commit to look at.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
What does this implement/fix?
Update for haier climate component. What is new:
Types of changes
Related issue or feature (if applicable): fixes
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#4020
Test Environment
Example entry for
config.yaml
:Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: