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] Adjust default ranges for some characteristics #17157

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Jul 26, 2024

Fixes #15818

  • Illuminance can obviously get down to 0, instead of 0.0001lx (especially if your sensor only reports integer values)
  • Raise Illuminance ceiling slightly (from 100k lx)
  • Adjust default range of Color Temperature to the warmest and coolest bulbs I could find on Amazon (for settable characteristics, you don't want to have too large of a range because HomeKit will often use those ranges to determine the scale of the slider). Original is 50-400 mired/2500-20000 K
  • Lower min value of current temperature from 0°C to -100°C (going below freezing is not unusual. lowest recorded temperature on earth is -89.2°C. I just chose a nice round number)

@ccutrer ccutrer requested review from andylintner and yfre as code owners July 26, 2024 16:09
@ccutrer ccutrer force-pushed the homekit-adjust-default-value-ranges branch from f75cfb8 to c4468c6 Compare July 26, 2024 16:12
 * Illuminance can obviously get down to 0, instead of 0.0001lx
   (especially if your sensor only reports integer values)
 * Raise Illuminance ceiling slightly (from 100k lx)
 * Adjust default range of Color Temperature to the warmest and
   coolest bulbs I could find on Amazon (for settable characteristics,
   you don't want to have too large of a range because HomeKit will
   often use those ranges to determine the scale of the slider).
   Original is 50-400 mired/2500-20000 K
 * Lower min value of current temperature from 0°C to -100°C
   (going below freezing is not unusual. lowest recorded temperature
   on earth is -89.2°C. I just chose a nice round number)

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer force-pushed the homekit-adjust-default-value-ranges branch from c4468c6 to df226b1 Compare July 26, 2024 17:16
@lsiepel
Copy link
Contributor

lsiepel commented Jul 26, 2024

Best would be that these values are changed in HAP. Would be nice for @yfre to comment anyway, he’s away for some weeks, so let’s wait a bit before merge this

Code wise I would prefer to have these values in a constant for clarity and maintainability. Otherwise LGTM

@ccutrer
Copy link
Contributor Author

ccutrer commented Jul 26, 2024

The values in HAP-Java match what's in the HAP specification, and should not change (really, we should be able to omit them in the JSON if it matches the default).

@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Aug 1, 2024
@lsiepel lsiepel changed the title [homekit] adjust default ranges for some characteristics [homekit] Adjust default ranges for some characteristics Aug 1, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Aug 13, 2024

#15268 Makes me wonder. If 0.0 snaps to the min value. How would that work with persistence at startup as that could somehow lead to undesired results?!

@lsiepel
Copy link
Contributor

lsiepel commented Aug 24, 2024

Gentle ping @yfre

@yfre
Copy link
Contributor

yfre commented Aug 24, 2024

@lsiepel thanks for the reminder.
in general, Java-HAP has started as re-usable library for different projects.. not sure though if it is used somewhere else besides OpenHAB.

nevertheless, i agree with Cody - Java-HAP should be a reference implementation for the HAP protocol and default values should be according to Apple HAP specification, even if they are not good.

i support this PR and the approach of redefining default values in OH. but i would also prefer them be defined as constant a not as numbers directly inline.

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.

please make default values to constants.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@lsiepel lsiepel merged commit 0252099 into openhab:main Sep 9, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Sep 9, 2024
@ccutrer ccutrer deleted the homekit-adjust-default-value-ranges branch September 9, 2024 21:55
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Sep 24, 2024
* [homekit] adjust default ranges for some characteristics

Signed-off-by: Cody Cutrer <cody@cutrer.us>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* [homekit] adjust default ranges for some characteristics

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* [homekit] adjust default ranges for some characteristics

Signed-off-by: Cody Cutrer <cody@cutrer.us>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* [homekit] adjust default ranges for some characteristics

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.

[Homekit] Detected value out of range for light sensors continue to occur
3 participants