-
Notifications
You must be signed in to change notification settings - Fork 508
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
DDF add support for Neo TS0601 #7832
base: master
Are you sure you want to change the base?
Conversation
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 and thanks for the PR. Besides a couple of minors, there's 1-2 changes to be made to stay consistent with other sirens.
I'm not too sure if the would allow us to remove quite some lines of the current source code presumably dedicated only to this device, but those can also be deleted later on with a seperate PR.
"status": "Gold", | ||
"subdevices": [ | ||
{ | ||
"type": "$TYPE_ON_OFF_LIGHT", |
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.
The type warning device would be more appropriate here
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.
The c++ code block tuya siren (they are not using the WD IAS cluster), not possible to use warning device type.
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'm afraid I have to disagree here, at least partially. With a DDF supported device, the DDF defines what type of device it is, not the device itself anymore (meaning it's changable to your liking). So, if you make a warning device out of it, this generally allows following the expected code paths. Tuya is not blocked here per se.
In case the siren is still not able to sound up, it is more a matter of what you pass over for processing.
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 not able to make it work as a warning device. Changing it to a light was a fix for me. More info at #6112
{ | ||
"name": "state/on", | ||
"write": { | ||
"dpid": 13, | ||
"dt": "0x10", | ||
"eval": "Item.val == 1 ? 1 : 0;", | ||
"fn": "tuya" | ||
}, | ||
"parse": { | ||
"dpid": 13, | ||
"eval": "Item.val == 1 ? 0 : 1;", | ||
"fn": "tuya" | ||
}, | ||
"default": false | ||
}, |
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.
None of the sirens/warning devices has this attribute and we should stay consistent as much as possible. I'd rather see the functions move up to state/alert
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.
Same comment, not possible to use state/alert for tuya siren.
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.
Resource item usage is not (or should not be) limited to a vendor/manufacturer, but generally usable. Therefore, the 3 possible functions define what needs to be done with sent/received data. If that cannot be ensured in this case, we might want to change the (legacy) code to allow this in future. This shouldn't be anything heretic 😂
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.
No It's the same problem for tuya covering.
The DDF core can handle state/on for exemple but not state/open or state/alert.
The problem is not the vendor/manufacturer, but the DDF code. We have already speak about that.
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 is the workround. I was not able to get it work as a warning device.
Took me a while to have a closer look. Thanks for the recent changes, the majority seems to look fine. I'm afraid this PR doesn't get my vote to get merged for a couple of reasons. Let me try to explain:
Having said that, @ThiemeNL could you please let us know with which value you test |
You don't remember ? with DDF to send request (all is working for incoming request), you can use only state/on state/bri (with this PR #5868 ), not possible to use state/lift for exemple, make a try on your side on a covering, you can reverse a return but not reverse the request. And it's not locked for tuya, it's locked for ALL brands. You need to use defaut request or give up. |
No, I indeed do not remember that one, thanks for sharing. That also finally helped me explaining why it is working at all with However, all this doesn't change anything on my overall position. Everything is possible with DDFs, so I'll neither go with the default requests nor will I give up. In essence, adding a single line of code would be sufficient but practically, it will be just a few more. The current state changes are created, but not set up correctly. Will raise some PRs for making sirens and covers work shortly. |
Hey @ThiemeNL, thanks for your pull request! Tip Modified bundles can be downloaded here. DDB changesModified
Validation🕜 Updated for commit 84492a2 |
Not sure what is the problem reported by the validator because the file exist. Maybe an update of the base branch could help |
Product name: Neo siren alarm with battery
Manufacturer: _TZE200_t1blo2bj
Model identifier: TS0601
Device type to add: Siren, but recognized as light
#6112 with help of @Smanar