Prevent device value less than 1% showing as 100% in HomeKit #674
Conversation
Update `transformValueFromMqtt` to return minimum at 1 percent value.
|
Kudos, SonarCloud Quality Gate passed! |
|
🆗 Published SonarCloud and code coverage data. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #674 +/- ##
==========================================
+ Coverage 52.86% 52.88% +0.01%
==========================================
Files 40 40
Lines 2457 2460 +3
Branches 616 617 +1
==========================================
+ Hits 1299 1301 +2
- Misses 618 619 +1
Partials 540 540
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
When I have time, I'll see if I can also come up with an automated test that verifies this fix. |
| return out_minimum + percentage * (out_maximum - out_minimum); | ||
| const result = out_minimum + percentage * (out_maximum - out_minimum); | ||
| if (result < 1 && result > 0) { | ||
| return Math.ceil(result); |
There was a problem hiding this comment.
Shouldn't we simply just return 1 here, as we already know the outcome of Math.ceil in this case because of the if condition.
| return Math.ceil(result); | |
| return 1; |
There was a problem hiding this comment.
Oh sorry, I didn't check properly. This shouldn't be a fixed value. I think we definitely need a test to figure this one out.
I would expect the condition to some how relate to the out_minimum boundary (and perhaps we need the same for the out_maximum).
There was a problem hiding this comment.
This shouldn't be a fixed value.
Your initial thought was correct, returning a fixed value of 1 probably is the right approach here. 😉
Otherwise, a Zigbee brightness level of 1 (out of 254) would result in about 0.39%, which HomeKit subsequently rounds down to 0%, an implied invalid value when a light is still powered on.
|
@jtheller Having the same problem #673 and can confirm that the mapping/translation of brightness ranges is definitely not ideal in its current state. As I also mentioned in my mail to @itavero, the official Philips Hue bridge has the exact same issue. According to the HAP specification,
|
| @@ -145,6 +145,10 @@ export class NumericCharacteristicMonitor extends BaseCharacteristicMonitor { | |||
| } | |||
| const percentage = (input - this.input_min) / (this.input_max - this.input_min); | |||
|
|
|||
| return out_minimum + percentage * (out_maximum - out_minimum); | |||
| const result = out_minimum + percentage * (out_maximum - out_minimum); | |||
| if (result < 1 && result > 0) { | |||
There was a problem hiding this comment.
| if (result < 1 && result > 0) { | |
| if ((result < 1 && result > 0) && (actualCharacteristic.UUID === '00000008-0000-1000-8000-0026BB765291')) { |
Checking the UUID of the actual characteristic we're dealing with, should prevent us from accidentally breaking other numeric characteristics like, e.g. TargetPosition, and thus would only treat Brightness differently.
There was a problem hiding this comment.
Can confirm this works quite well, albeit looking not very pretty. 😅
All characteristics, except Brightness, are unaffected by this change.
The actual implementation still needs a bit of work, but it's a good starting point.
|
It appears this PR hasn't had any activity for a long time. Please check if it is still applicable and if it can be merged without any issues. If there isn't any activity in the next three weeks, this PR will be closed automatically. |
|
Replaced by #863 |
Add suggested implementation for