-
Notifications
You must be signed in to change notification settings - Fork 464
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
adding SAT402ZB Thermostat #1506
base: main
Are you sure you want to change the base?
adding SAT402ZB Thermostat #1506
Conversation
Invitation URL: |
Test Results 63 files 397 suites 0s ⏱️ Results for commit 4ec57d2. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 4ec57d2 |
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 review my comments when your schedule allows, then this can proceed. Thank you.
manufacturer: Stelpro | ||
model: MaestroStat | ||
model: SMT402AD | ||
deviceProfileName: thermostat-stelpro-profile |
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.
Hello @gcavalcantistelpro Thank you for your submission. I would encourage that we focus this change only on adding one new device, the SAT402ZB device. Can you please update the PR to no longer make changes to the "Selpro/MaestroStat"device?
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.
Hello @lelandblue , I've just added a commit for the SAT402ZB thermostat, i left the maestro untouched and added new files for this specific thermostat
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.
Hello @lelandblue , i could validate locally that the device is working after my last commit, please let me know if any further modifications are required on the PR side. Thanks
drivers/SmartThings/zigbee-thermostat/src/stelpro/stelpro_maestrostat.lua
Show resolved
Hide resolved
@@ -0,0 +1,81 @@ | |||
-- Copyright 2022 SmartThings |
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.
date
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 PR needs to be only additive.
Just adding this device's fingerprints to the existing stelpro_maestrostat
sub-driver should be enough. It seems you've attempted to do that, but inadvertently made a second subdriver with identical code.
@@ -0,0 +1,479 @@ | |||
-- Copyright 2022 SmartThings |
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.
date
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.
Date still needs to be updated.
Duplicate profile check: Passed - no duplicate profiles detected. |
@greens Could you re-review this please? Thank you! |
No description provided.