-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add support for new ubisys S1-R (Series 2). #7915
Conversation
commandsColorCtrl({endpointNames: ['2', '3']}), | ||
], | ||
configure: async (device, coordinatorEndpoint) => { | ||
const endpoint = device.getEndpoint(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.
Everything inside here is not needed because of electricityMeter
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 think its needed because configureReporting
is set to false
for electricityMeter
. The reason is that the currentSummDelivered
is not bindable for this device (as far i can tell for the most ubisys devices). As workaround only the instantaneousDemand
attribute is binded manually and currentSummDelivered
read in onEvent
.
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 see now, I think we can improve the ubisysOnEventReadCurrentSummDelivered
a bit, I see 2 issues with it:
- In case the Ubisys devices is sending a lot of messages, a lot of reads are done
- In case no messages are send, the value becomes stale
We can solve this by polling at a certain interval, you can use the Tuya one as an example
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.
Sounds good. I will take a closer look at the weekend when I have the device for testing at hand.
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 was a bit uncertain to change this for devices i do not own myself, so i added a separate method that can be used for polling.
src/devices/ubisys.ts
Outdated
await reporting.readMeteringMultiplierDivisor(endpoint); | ||
await reporting.instantaneousDemand(endpoint); | ||
}, | ||
meta: {multiEndpointSkip: ['state', 'power', 'energy']}, |
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.
Use multiEndpointSkip
of deviceEndpoints
instead
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.
done
src/devices/ubisys.ts
Outdated
endpoints: [{ID: 1, profileID: 260, deviceID: 266, inputClusters: [0, 3, 4, 5, 6, 1794, 2820], outputClusters: []}], | ||
}, | ||
], | ||
model: 'S1-R (Series 2)', |
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.
model: 'S1-R (Series 2)', | |
model: 'S1-R-2', |
Can we use this here? It's a bit more friendly without the spaces
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.
done
…ndent of the actual amount of device messages.
e8b23c7
to
8c4345f
Compare
Co-authored-by: Koen Kanters <[email protected]>
Thanks! |
Adds support for the new ubisys S1-R (Series 2). Currently a technical reference document for the device is not (yet) available. The main difference seems to be that there is no endpoint
4
in the new device and the energy related clusters are located in endpoint1
(instead of4
like the regular S1-R).The device seems to be working fine:
One thing i am unsure about is if the fingerprint is working correctly. When i test this
Definition
as external converter with only thefingerprint
defined (nozigbeeModel
attribute), the normal S1-RDefinition
is used. As external converter the newDefinition
is only picked up when i trump with thezigbeeModel
attribute (S1-R (5601)
). Not sure if this is normal behaviour for external converters or if there something wrong about the fingerprinting?db entry (paired with the Definition of this PR)