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 8 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 8 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    421 suites   0s ⏱️
2 172 tests 2 172 ✅ 0 💤 0 ❌
3 696 runs  3 696 ✅ 0 💤 0 ❌

Results for commit a54d9c4.

♻️ 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 91% ✅
/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/aqara/init.lua 92% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua 86% ❌

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against a54d9c4

@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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

greens
greens reviewed Mar 28, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua Outdated
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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

greens
greens reviewed Mar 28, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua Outdated
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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

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.

@GAFfrient
Copy link
Author

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

Unit tests added in the latest commit.

@GAFfrient GAFfrient requested a review from greens April 11, 2025 06:08
@KKlimczukS KKlimczukS requested a review from wkhenon April 11, 2025 07:15
greens
greens reviewed Apr 14, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
@@ -1,4 +1,4 @@
-- Copyright 2022 SmartThings
-- Copyright 2025 SmartThings
Copy link
Contributor

Choose a reason for hiding this comment

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

please leave the copyright date as-is

greens
greens reviewed Apr 14, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
local frient_smoke_detector = {
NAME = "Freint Smoke Detector",
local function generate_event_from_zone_status(driver, device, zone_status, zigbee_message)
print("Received ZoneStatus:", zone_status.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this print statement, or change it to a log message

greens
greens reviewed Apr 14, 2025
View reviewed changes
drivers/SmartThings/zigbee-smoke-detector/src/frient/init.lua
device:emit_event(smokeDetector.smoke.detected())
else
device.thread:call_with_delay(6, function ()
print("Smoke cleared!")
Copy link
Contributor

Choose a reason for hiding this comment

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

same with these

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

@cbaumler cbaumler Awaiting requested review from cbaumler

@wkhenon wkhenon Awaiting requested review from wkhenon

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