Add a max brightness field for each dimmer in config file #149 #200

AlexPinhasov · 2021-02-12T19:40:58Z

Hi milo I am not sure how can I test it and debug it, I am using VSC, if you could share with me this knowledge I will test and finish this feature fix.

like we discussed I added a new field in config file, first I touch type script language so I hope I added it in the right places

…s" file

This reverts commit 52c2b14.

milo526 · 2021-02-13T13:00:35Z

To test this:

Please make sure you have a local installation of homebridge.
In the directory in which you cloned this git repo, run npm link
Run yarn watch or npm run watch depending on which packaging system you use.

Thank you for your work on this! It is much appreciated.

config.schema.json

package.json

milo526 · 2021-02-13T13:11:55Z

Please also make sure to implement the validateConfigOverwrites method on the DimmerAccessory and LightAccessory classes to validate the provided values.
You can look into the ClimateAccessory for how to do this.

… in package.json

milo526 · 2021-02-14T18:43:22Z

src/accessories/characteristics/brightness.ts

+    } else if (data?.brightness && data?.max_brightness) {
+      stateValue = Math.round(
+        (Number(data.brightness) / data.max_brightness) * 100
+      );


data?.max_brightness will not be defined in case the user does not enter device defaults information.
Please use the old value (255) as the default value In this case.

Suggested change

} else if (data?.brightness && data?.max_brightness) {

stateValue = Math.round(

(Number(data.brightness) / data.max_brightness) * 100

);

} else if (data?.brightness) {

const max_brightness = data?.max_brightness ?? 255

stateValue = Math.round(

(Number(data.brightness) / max_brightness) * 100

);

This will use 255 as fallback incase the max_brightness is not set.

Actually I set the default value for max brightness to be 255 in the config file, then I read that value weather its provided or not, good enough? (check out latests commit)

I believe you only set the "default" value in the ui config. This has nothing to do with the actual config file. Just when users use config-ui-x. Furthermore they can still remove the default value, making it undefined. Or not add the device config at all. Therefor I believe the proposed changes are still necessary.

data?.max_brightness is null for me, before the change I read the value from the config file, where is the connection between data.max_brightness and config.max_brightness should be set?

README.md

config.schema.json

src/api/response.ts

milo526 · 2021-02-14T18:53:11Z

This is a great start! Please see some of my feedback, some of which I didn't catch the first time - sorry for that.

AlexPinhasov · 2021-02-14T19:39:16Z

@milo526 I noticed there is another bug, at some point we stop receiving the real value from the accessory numeric scale (0...255) for example, and moving to (0...100) both for update value in HomeKit and reading values from Tuya app.
Once I close the device and wait a few minutes -> foregrounding the HomeKit app we go back to (0...255) weird.
So while this PR fixes the max brightness thats not the only thing.

milo526 · 2021-02-14T19:44:36Z

@milo526 I noticed there is another bug, at some point we stop receiving the real value from the accessory numeric scale (0...255) for example, and moving to (0...100) both for update value in HomeKit and reading values from Tuya app.
Once I close the device and wait a few minutes -> foregrounding the HomeKit app we go back to (0...255) weird.
So while this PR fixes the max brightness thats not the only thing.

That is due to the caching we provide here: https://github.com/milo526/homebridge-tuya-web/blob/master/src/accessories/characteristics/brightness.ts#L44-L47

Whenever we set the value in HomeKit (through the slider or through fetching data from the API) we cache the last known value. This value is always on the scale from 0 to 100. The characteristic should probably be used to convert the 0...100 scale to 0 to max_brightness

AlexPinhasov · 2021-02-14T19:51:38Z

@milo526 what are those magic numbers ?
const value = ((homekitValue as number) / 10) * 9 + 10;

milo526 · 2021-02-14T19:53:18Z

@milo526 what are those magic numbers ?
const value = ((homekitValue as number) / 10) * 9 + 10;

So this maps the homeKitValue (range 0 - 100) to a range 10 - 100 since Tuya brightness value < 10 will just turn the bulb off in my experience.

AlexPinhasov · 2021-03-07T09:27:20Z

@milo526 Yes why not?, this is a very annoying issue
I need 2 things from you
1)
So this maps the homeKitValue (range 0 - 100) to a range 10 - 100 since Tuya brightness value < 10 will just turn the bulb off in my experience.
((homekitValue as number) / 100) * max_brightness ?

data?.max_brightness is null for me, before the change I read the value from the config file, where is the connection between data.max_brightness and config.max_brightness should be set?

milo526 · 2021-03-07T10:38:06Z

src/accessories/DimmerAccessory.ts

+  validateConfigOverwrites(config: TuyaDeviceDefaults): string[] {
+    const errors = super.validateConfigOverwrites(config);
+    if (config?.max_brightness) {
+      const maxBrigthness = config.max_brightness;


We are missing a cast to number here.
Most config variables are or can be strings, so we need to use
const maxBrightness = Number(config.max_brightness)

That also explains why we write it back.

milo526 · 2021-03-07T10:39:33Z

src/accessories/LightAccessory.ts

+  validateConfigOverwrites(config: TuyaDeviceDefaults): string[] {
+    const errors = super.validateConfigOverwrites(config);
+    if (config?.max_brightness) {
+      const maxBrigthness = config.max_brightness;


Also cast to number (and there is a small typo should be maxBrightness)

milo526 · 2021-03-07T10:40:49Z

src/accessories/characteristics/brightness.ts

@@ -63,7 +63,8 @@ export class BrightnessCharacteristic extends TuyaWebCharacteristic {
    ) {
      stateValue = Number(data.color.brightness);
    } else if (data?.brightness) {
-      stateValue = Math.round((Number(data.brightness) / 255) * 100);
+      const max_brightness = data?.max_brightness ?? 255;


max_brightness should be defined as a getter property on the accessory. It is not part of the data.

Please take a look at the currentTemperatureFactor method in the TemperatureSensorAccessory.

milo526 · 2021-03-07T10:43:42Z

@milo526 Yes why not?, this is a very annoying issue
I need 2 things from you
1)
So this maps the homeKitValue (range 0 - 100) to a range 10 - 100 since Tuya brightness value < 10 will just turn the bulb off in my experience.
((homekitValue as number) / 100) * max_brightness ?

No, please see the what @bamodio said in #149
We need to ensure that the light is not off when you set it to a low percentage (but still > 0).

Your suggestion would still have this problem in place

data?.max_brightness is null for me, before the change I read the value from the config file, where is the connection between data.max_brightness and config.max_brightness should be set?

See my comment on the MR, you should get this value from the accessory device config, not from the data.

AlexPinhasov · 2021-03-10T13:52:07Z

So we can assume we should support 2 types of ranges
TUYA_BRIGHTNESS_RANGE0 = (1, 255)
TUYA_BRIGHTNESS_RANGE1 = (10, 1000)
meaning we can add a tick box for each one, default should be 1-255 for all of them
thats what you to do?

milo526 · 2021-03-10T13:54:35Z

So we can assume we should support 2 types of ranges
TUYA_BRIGHTNESS_RANGE0 = (1, 255)
TUYA_BRIGHTNESS_RANGE1 = (10, 1000)
meaning we can add a tick box for each one, default should be 1-255 for all of them
thats what you to do?

I don't necessarily think so since I seem to vaguely remember that there are also (1,100) ranges.
Please make them user-configurable (both min and max), Make 1 - 255 the default, Document the possible other ranges in at least the readme and possible the config-ui-x

AlexPinhasov · 2021-03-10T13:56:59Z

Ok so final solution:
Add both min + max to settings and use that for the accessory
default should be 1-255
add description for both on readme

thats it?

milo526 · 2021-03-10T13:59:03Z

Ok so final solution:
Add both min + max to settings and use that for the accessory
default should be 1-255
add description for both on readme

thats it?

Sounds good!
Just make sure that the settings can be correctly configured through the config-ui-x plugin as well as through changing the config.json file

AlexPinhasov · 2021-03-11T10:25:36Z

@milo526 Upon working on the minimum value I came a cross an odd behaiover:

the range I set for the device was 10-255
the range I get for the device is 25-255

I printed the 1% and 100% of the device and got those results, it gets weird

if I set the value in Homekit for example 62% and I take the minimum as 10 then Tuya app reads it correctly 62% perfect
If I set the value on Tuya app for example 1% the value I get is 25 which suggests this is the minimum of the device

even weirder is that checking in the automation rules in the Tuya app the range is 25-1000
What is going on

AlexPinhasov · 2021-03-11T12:09:33Z

@bamodio @milo526 Great news !
I think I found the solution, I tested is on my own devices some has 10-255 range and some 10-1000 both seems to work

only 1 issue remains maybe you can assist, the decimal number sometimes can differ in some cases resulting in accuracy of 1-/+ , I noticed that values above 40 % are correct exactly where less then that we might see a 1% different

Still with all of this its already a big leap in solving this once and for all, give it a test and lets roll out?

milo526 · 2021-03-11T12:32:52Z

src/accessories/LightAccessory.ts

@@ -11,6 +11,7 @@ import {
 } from "./characteristics";
 import { ColorAccessory } from "./ColorAccessory";
 import { TuyaDevice } from "../api/response";
+import { TuyaDeviceDefaults } from "../config";


There are some warnings here, could you please take a look at those.

milo526 · 2021-03-11T12:39:08Z

src/accessories/characteristics/brightness.ts

+    const value = Math.round(
+      (100 - correctionFactor) * dimmerPercentage +
+        correctionFactor +
+        Number.EPSILON


Why did you opt to add the Number.EPSILON here (and a few lines later)?

The decimal number is very large, I wanted to be sure as much as possible the round will be correct

milo526 · 2021-03-11T12:45:10Z

Thank you for your work once more.
I'll have some time to check this later this week.
Maybe @bamodio is able to test this in the meantime?

AlexPinhasov · 2021-03-17T08:59:05Z

@milo526 So any news?

bamodio · 2021-03-18T09:41:59Z

I tested this a bit... There's still errors, and I believe they are related to the caching of values, and the mismatch of ranges...

Here's a description and log sample...

My config is set to:
"defaults": [
{
"id": "Gym",
"device_type": "light",
"max_brightness": 1000,
"min_brightness": 10
}
],
In Homebridge UI > Accessories, I choose the Gym light and set the brightness to 82%.
The config works well and I see this line emitted, so far so good!
[3/18/2021, 2:22:49 AM] [Tuya] [Gym] Characteristic.Brightness - [SET] 820
Immediately after that though, there's a set of lines that I think are refreshing the value form the cache...
[3/18/2021, 2:22:49 AM] [Tuya] [Gym] - Requesting device state
[3/18/2021, 2:22:49 AM] [Tuya] [Gym] - Creating new debounced promise
[3/18/2021, 2:22:49 AM] [Tuya] [Gym] - Triggering debouncedDeviceStateRequest
[3/18/2021, 2:22:50 AM] [Tuya] [Gym] - Unsetting debouncedDeviceStateRequestPromise
[3/18/2021, 2:22:50 AM] [Tuya] [Gym] - Renewing cache due to RateLimitError
[3/18/2021, 2:22:50 AM] [Tuya] [Gym] Characteristic.Brightness - [GET] 82
You see that the cache is returning the "Homekit value" (range 1-100) and not the Value I had set above: 820. The expectation here would be that the cache stores this value.
I think there's a problem or confusion with the value being cached... updateValue() gets called multiple time with the value 82, and not 820.

Is this coming from the cache somehow? Is there a disparity of using/caching homekitValue vs the remote value?

This is not working yet, sorry guys.

@milo526 ?

AlexPinhasov · 2021-03-18T11:02:54Z

@bamodio You are correct I have stated before that @milo526 has a problem with caching.
But if you open the Tuya/Smartlife app and change the value then HomeKit will be correct once its not cached, and if you change it via HomeKit the Tuya app is also correct, as far as I am concerned the fix is valid, @milo526 Can you take a look on the caching mechanism ?

milo526 · 2021-03-18T11:04:04Z

Please show me where the caching mechanism is wrong in the current plugin.

AlexPinhasov · 2021-03-18T11:09:37Z

@milo526 The scenario is:

In home kit change the brightness to 67% then go to Tuya/smartlfe app and see 67% so far so good, when you come back to HomeKit instead of showing 67% you see the a cached number which is incorrect, if you wait long enough and reenter the HomeKit app to fetch the value instead of using the cache it goes back to 67% as it should

milo526 · 2021-03-25T09:37:40Z

Hey, I made some time to look at this today but there seems to be some merge conflicts.
I am however not allowed to resolve them for you.

Could you please enable the Allow edits from maintainers option.

AlexPinhasov · 2021-03-25T09:39:50Z

@milo526 Its enabled, didn't touch it

milo526 · 2021-03-25T10:13:14Z

Sadly GitHub will not allow me to push any changes at this time.

AlexPinhasov · 2021-03-25T10:22:50Z

@milo526 I have sent you an invite to be a collaborator in my master branch, accept it and try again

AlexPinhasov · 2021-03-29T09:30:32Z

@milo526 You gave up?

milo526 · 2021-03-29T09:33:17Z

👍 Yeah that is exactly what I did, I did not spend any time on this at all, I have all the time in the world to work on this and have no other obligations.

I'll fix this myself when I have the time.
Thanks for trying.

AlexPinhasov · 2021-03-29T09:40:10Z

@milo526 Lol, did not expect that.
I guess this was fake -> "Thank you for your work on this! It is much appreciated."
I spent days to solve this issue which obviously effects many users. even with the cache issue this PR would have fixed long brightness issues you have.
Thats the thank you I get for spending my time on your repository.

milo526 · 2021-03-29T09:41:38Z

Please feel free to release your own branch as a separate plugin!

Alex Pinhasov added 6 commits January 6, 2021 14:56

Possible fix for issue milo526#149 "Brightness does not match"

08f2236

Fix alignment

2e92194

Fix code styling using prettier

eac49b0

Add max_brightness field in config file and use that in "brightness.t…

9c88c38

…s" file

Change "int" to "number"

0675f75

Run prettier

52c2b14

AlexPinhasov changed the title ~~Add a max brightness field for each dimmer in config file~~ Add a max brightness field for each dimmer in config file #149 Feb 12, 2021

Alex Pinhasov added 2 commits February 12, 2021 22:07

Revert "Run prettier"

58487c3

This reverts commit 52c2b14.

Run prettier and linter, add milo526#149 issue code

dc89a94

milo526 force-pushed the master branch 3 times, most recently from 6663a12 to b402d9c Compare February 13, 2021 12:38

milo526 requested changes Feb 13, 2021

View reviewed changes

config.schema.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Alex Pinhasov added 3 commits February 14, 2021 19:34

Add an explanation for max brightness in read me file, revert changes…

1559b0b

… in package.json

Add validateConfigOverwrites methods for light and dimmer accessories

66c18fe

Run prettier

a271ede

milo526 requested changes Feb 14, 2021

View reviewed changes

Alex Pinhasov added 5 commits February 14, 2021 21:10

Fix typos

1f4b415

Change reading wrong max brightness property

f81a8b4

Change documentation text

cdd44db

Move max brightness to "CustomProperties"

8065695

Run prettier

d729345

Support default max brightness value in brightness.ts

f591f0b

stale bot removed the stale This issue has not received any updates for some time. label Mar 6, 2021

milo526 requested changes Mar 7, 2021

View reviewed changes

Add minimum value for dimmer lights

418c2d1

Fix set remote value and set homekit value correctly

df1f3ba

milo526 requested changes Mar 11, 2021

View reviewed changes

Remove unneeded import at light accessory

e986fef

milo526 closed this Mar 29, 2021

Repository owner locked as too heated and limited conversation to collaborators Mar 29, 2021

Add a max brightness field for each dimmer in config file #149 #200

Add a max brightness field for each dimmer in config file #149 #200

Conversation

AlexPinhasov commented Feb 12, 2021

milo526 commented Feb 13, 2021

milo526 commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milo526 commented Feb 14, 2021

AlexPinhasov commented Feb 14, 2021

milo526 commented Feb 14, 2021

AlexPinhasov commented Feb 14, 2021 • edited Loading

milo526 commented Feb 14, 2021

AlexPinhasov commented Mar 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milo526 commented Mar 7, 2021

AlexPinhasov commented Mar 10, 2021

milo526 commented Mar 10, 2021

AlexPinhasov commented Mar 10, 2021

milo526 commented Mar 10, 2021

AlexPinhasov commented Mar 11, 2021 • edited Loading

AlexPinhasov commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexPinhasov Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

milo526 commented Mar 11, 2021

AlexPinhasov commented Mar 17, 2021

bamodio commented Mar 18, 2021

AlexPinhasov commented Mar 18, 2021

milo526 commented Mar 18, 2021

AlexPinhasov commented Mar 18, 2021

milo526 commented Mar 25, 2021

AlexPinhasov commented Mar 25, 2021

milo526 commented Mar 25, 2021

AlexPinhasov commented Mar 25, 2021

AlexPinhasov commented Mar 29, 2021

milo526 commented Mar 29, 2021

AlexPinhasov commented Mar 29, 2021

milo526 commented Mar 29, 2021

AlexPinhasov commented Feb 14, 2021 •

edited

Loading

AlexPinhasov commented Mar 11, 2021 •

edited

Loading

AlexPinhasov Mar 11, 2021 •

edited

Loading