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

Adding support for frient Intelligent Smoke Alarm (SMSZB-120) #2024

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Adding support for frient Intelligent Smoke Alarm (SMSZB-120) #2024

wants to merge 5 commits into from

Conversation

GAFfrient
Copy link

@GAFfrient GAFfrient commented Mar 26, 2025 •
edited
Loading

Check all that apply

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • I have signed the CLA
    • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
  • Bug fix
  • New feature
  • Refactor

Checklist

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have verified my changes by testing with a device or have communicated a plan for testing
  • I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

Summary of Completed Tests

@CLAassistant
Copy link

CLAassistant commented Mar 26, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@lelandblue
Copy link
Contributor

Hey @GAFfrient. We will begin a review of this PR.

When you have a free moment, please sign the required Contributors License Agreement #2024 (comment)

@github-actions GitHub Actions
Copy link

Duplicate profile check: Passed - no duplicate profiles detected.

@github-actions GitHub Actions
Copy link

Invitation URL:
https://bestow-regional.api.smartthings.com/invite/RBlEL7REpN2E

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 27, 2025 •
edited
Loading

Test Results

   66 files  ±0    420 suites  ±0   0s ⏱️ ±0s
2 152 tests ±0  2 148 ✅  - 4  0 💤 ±0  4 ❌ +4 
3 673 runs  ±0  3 669 ✅  - 4  0 💤 ±0  4 ❌ +4 

For more details on these failures, see this check.

Results for commit 443a6b3. ± Comparison against base commit 0db6c68.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 27, 2025 •
edited
Loading

File Coverage
All files 80% ❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-smoke-detector/src/aqara/init.lua 92% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-smoke-detector/src/aqara-gas/init.lua 94% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua 51% ❌

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 443a6b3

@GAFfrient
Copy link
Author

Hey @GAFfrient. We will begin a review of this PR.

When you have a free moment, please sign the required Contributors License Agreement #2024 (comment)

Hello.

Now I did :)

greens
greens reviewed Mar 28, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.

Copy link
Contributor

Choose a reason for hiding this comment

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

leave this in place, thanks

greens
greens reviewed Mar 28, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
NAME = "Freint Smoke Detector",
local function do_configure(self, device)
device:configure()
device:send(TemperatureMeasurement.server.attributes.MeasuredValue:configure_reporting(device, 60, 600, 100):to_endpoint(0x26))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use add_configured_attribute in the init handler. (example here: https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/blob/main/drivers/SmartThings/zigbee-dimmer-remote/src/init.lua#L30)

this will ensure the configure_reporting message is sent by device:configure as well as creates a binding to the TemperatureMeasurement cluster

greens
greens reviewed Mar 28, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
Comment on lines +150 to +151
supported_capabilities = {
alarm
Copy link
Contributor

Choose a reason for hiding this comment

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

this field doesn't do anything for sub-drivers

greens
greens reviewed Mar 28, 2025
View reviewed changes
Copy link
Contributor

@greens greens left a comment

Choose a reason for hiding this comment

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

We'd like to see some tests covering the new behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greens greens greens left review comments

@wkhenon wkhenon wkhenon approved these changes

@cbaumler cbaumler Awaiting requested review from cbaumler

Assignees
No one assigned
Labels
do not merge WWST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GAFfrient @CLAassistant @lelandblue @greens @wkhenon