Skip to content

Add DDF for ZY-M100 human breathing presence sensor#6621

Merged
manup merged 8 commits intodresden-elektronik:masterfrom
BabaIsYou:patch-22
Jan 9, 2023
Merged

Add DDF for ZY-M100 human breathing presence sensor#6621
manup merged 8 commits intodresden-elektronik:masterfrom
BabaIsYou:patch-22

Conversation

@BabaIsYou
Copy link
Contributor

All is described in issue #6517

Suppressing " in default errorcode value
Change default value for errorcode back to "1" as it is a string
@manup manup added this to the v2.20.0-beta milestone Jan 6, 2023
Adding lightlevel as stated in dresden-elektronik#6517
Copy link
Collaborator

@SwoopX SwoopX left a comment

Choose a reason for hiding this comment

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

Thanks for submitting the PR. Apart from the already given comments, I have to say I got mixed feelings on the usage/recycling of quite some resource items. This is pretty much stretched out for this device, e.g. sensitivitymax is not even supposed to be writable.

@manup What's your opinion here?

@BabaIsYou
Copy link
Contributor Author

BabaIsYou commented Jan 6, 2023

, e.g. sensitivitymax is not even supposed to be writable.

I discoverd it later because sensitivitymax is marked as writeable in devices/generic/items/config_sensitivitymax_item.json but write is not permitted in code (!?). We're working on another workaround in the initial issue, then I remove this item. Eventually a new PR will come later.

Copy link
Contributor Author

@BabaIsYou BabaIsYou left a comment

Choose a reason for hiding this comment

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

Modified following remarks on submitted PR

@BabaIsYou BabaIsYou requested a review from SwoopX January 7, 2023 12:37
@manup
Copy link
Member

manup commented Jan 7, 2023

Thanks for submitting the PR. Apart from the already given comments, I have to say I got mixed feelings on the usage/recycling of quite some resource items. This is pretty much stretched out for this device, e.g. sensitivitymax is not even supposed to be writable.

@manup What's your opinion here?

Indeed for sensitivitymax I'd also vote to keep it as a read only item. The actual write counterpart would be sensitivity.

@BabaIsYou
Copy link
Contributor Author

Thanks for submitting the PR. Apart from the already given comments, I have to say I got mixed feelings on the usage/recycling of quite some resource items. This is pretty much stretched out for this device, e.g. sensitivitymax is not even supposed to be writable.
@manup What's your opinion here?

Indeed for sensitivitymax I'd also vote to keep it as a read only item. The actual write counterpart would be sensitivity.

Then the json supporting sensistivitymax need to be updated to R instead of RW ?

@manup
Copy link
Member

manup commented Jan 7, 2023

Then the json supporting sensistivitymax need to be updated to R instead of RW?

git gui blame -- devices/generic/items/config_sensitivitymax_item.json

shows that it's still my initial commit, so should hopefully be safe to set it to R.

@mprajsler
Copy link

Hi, if lux_to_lightlevel.js is used as proposed by SwoopX we need default values here
config/tholddark = 12000
config/tholdoffset = 7000
proposed values found from other light sensors

@Smanar
Copy link
Collaborator

Smanar commented Jan 7, 2023

Ha yes can add in DDF

        {
          "name": "config/tholddark",
          "default": 12000
        },
        {
          "name": "config/tholdoffset",
          "default": 7000
        },

So with that "state/dark" and "name": "state/daylight" will be updated too.

@SwoopX
Copy link
Collaborator

SwoopX commented Jan 7, 2023

@mprajsler Good catch on the threshold values, I apparently missed to copy them. So it would be sufficient to just add them. Default values are already part of the definition file, no need to add it

@BabaIsYou
Copy link
Contributor Author

@mprajsler Good catch on the threshold values, I apparently missed to copy them. So it would be sufficient to just add them. Default values are already part of the definition file, no need to add it

Done ! ;-)

@BabaIsYou
Copy link
Contributor Author

so should hopefully be safe to set it to R.

Done thru PR #6653

@manup
Copy link
Member

manup commented Jan 9, 2023

Is the PR ready to merge? :)

@BabaIsYou
Copy link
Contributor Author

Is the PR ready to merge? :)

Yes it is ;-)
Thx

@manup manup merged commit fc4a9fc into dresden-elektronik:master Jan 9, 2023
@BabaIsYou BabaIsYou deleted the patch-22 branch January 9, 2023 16:00
@BabaIsYou BabaIsYou mentioned this pull request Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants