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.

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

[homekit] allow configuring maxValue on VOCDensity #13508

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Oct 7, 2022

the default of 1000 is quite low in reality. tested and confirmed working with iOS 16

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 8, 2022
@lolodomo
Copy link
Contributor

lolodomo commented Oct 9, 2022

@yfre : maybe you can make a first review

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Oct 9, 2022
@yfre
Copy link
Contributor

yfre commented Oct 9, 2022

@ccutrer good idea. but why you have not added the support for step?
not sure whether there is an use case for step, but Homekit and Java-HAP supports custom step and for us is just one line.
so, if it is ok for you, please add support for custom step

VOCDensityCharacteristic.DEFAULT_MIN_VALUE),
taggedItem.getConfigurationAsDouble(HomekitTaggedItem.MAX_VALUE,
VOCDensityCharacteristic.DEFAULT_MAX_VALUE),
1.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

please add support for step

taggedItem.getConfigurationAsDouble(HomekitTaggedItem.STEP,
VOCDensityCharacteristic.DEFAULT_STEP),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. I didn't bother with step, because it's a read only attribute and AFAIK step should only affect UIs for writing things, to make it more helpful. but maybe it does respect it for something else, and I did min -- which one would think should always be zero -- so might as well. Though I could see setting min as something higher than zero if your sensor has a floor of how low it can detect. For example, my CO2 sensors have a floor of 400ppm.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@yfre yfre self-requested a review October 9, 2022 19:19
Copy link
Contributor

@yfre yfre left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo lolodomo merged commit d83c32c into openhab:main Oct 10, 2022
@lolodomo lolodomo added this to the 3.4 milestone Oct 10, 2022
@ccutrer ccutrer deleted the homekit-voc-max-value branch October 11, 2022 22:08
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* [homekit] allow configuring maxValue on VOCDensity

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

* [homekit] allow step value to be configured for VOCDensity

Signed-off-by: Cody Cutrer <cody@cutrer.us>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* [homekit] allow configuring maxValue on VOCDensity

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

* [homekit] allow step value to be configured for VOCDensity

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
* [homekit] allow configuring maxValue on VOCDensity

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

* [homekit] allow step value to be configured for VOCDensity

Signed-off-by: Cody Cutrer <cody@cutrer.us>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* [homekit] allow configuring maxValue on VOCDensity

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

* [homekit] allow step value to be configured for VOCDensity

Signed-off-by: Cody Cutrer <cody@cutrer.us>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
* [homekit] allow configuring maxValue on VOCDensity

the default of 1000 is quite low in reality. tested and confirmed
working with iOS 16

* [homekit] allow step value to be configured for VOCDensity

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants