-
Notifications
You must be signed in to change notification settings - Fork 498
Added support for frient switch devices #2208
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
base: main
Are you sure you want to change the base?
Added support for frient switch devices #2208
Conversation
Duplicate profile check: Passed - no duplicate profiles detected. |
Invitation URL: |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 1259705 |
maximum_interval = 600, | ||
data_type = Alarms.attributes.AlarmCount.base_type, | ||
reportable_change = 1, | ||
|
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.
nit: extra whitespace
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.
we generally use 2-space tab widths
|
||
local function alarm_report_handler(driver, device, zb_rx) | ||
local alarm_status = zb_rx.body.zcl_body | ||
log.error("Received alarm report:"..util.stringify_table(zb_rx.body.zcl_body, nil, true)) |
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 seems like a debug statement, and I expect this message is already logged by the driver framework.
local function device_added(driver, device) | ||
end |
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.
I don't see anything in the base driver's added method that you would need to override.
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 @greens, this override has been added specifically for SPLZB-132 frient switch model. It has an endpoint which is unused and results in creating a non-functional child device because of the main driver. I think we should bring this function back.
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.
@JanJakubiszyn based on my reading of the code, that should only create extra child devices if the profile has more than one component, though. It doesn't seem like any of the fingerprints here do.
local multiplier = device:get_field(VOLTAGE_MEASUREMENT_MULTIPLIER_KEY) or 1 | ||
local divisor = device:get_field(VOLTAGE_MEASUREMENT_DIVISOR_KEY) or 1 | ||
|
||
if divisor == 0 then |
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.
If this is a concern, I would put the check where you write the value to VOLTAGE_MEASUREMENT_DIVISOR_KEY
.
local function current_measurement_divisor_handler(driver, device, divisor, zb_rx) | ||
log.info("current divisor value = ", divisor.value) | ||
local raw_value = divisor.value | ||
device:set_field(CURRENT_MEASUREMENT_DIVISOR_KEY, raw_value, { persist = true }) | ||
end | ||
|
||
local function current_measurement_multiplier_handler(driver, device, multiplier, zb_rx) | ||
log.info("current multipl value = ", multiplier.value) | ||
local raw_value = multiplier.value | ||
device:set_field(CURRENT_MEASUREMENT_MULTIPLIER_KEY, raw_value, { persist = true }) | ||
end | ||
|
||
local function voltage_measurement_divisor_handler(driver, device, divisor, zb_rx) | ||
log.info("V divisor value = ", divisor.value) | ||
|
||
local raw_value = divisor.value | ||
device:set_field(VOLTAGE_MEASUREMENT_DIVISOR_KEY, raw_value, { persist = true }) | ||
end | ||
|
||
local function voltage_measurement_multiplier_handler(driver, device, multiplier, zb_rx) | ||
log.info("Volt Multiplayer value = ") |
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.
several log statements here that ought to be removed. This one doesn't actually log any values.
cluster = ElectricalMeasurement.ID, | ||
attribute = ElectricalMeasurement.attributes.ActivePower.ID, | ||
minimum_interval = 5, | ||
maximum_interval = 600, |
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.
Our default max here is one hour, rather than 5 minutes. Is there a particular reason you've chosen a shorter max interval? Because of the reportable change, any updates to the value itself should still come in (provided they're not more frequent than the minimum interval), so we're wondering if there's a specific reason you want to see the current value reported every five minutes.
If not, we'd appreciate you sticking with our default configurations.
…PR comments. Updated test file
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests