espressif / esp-homekit-sdk

550 stars 99 forks source link

Invalid value outside constraints appear causes complete accessory failure #13

Open mbuckaway opened 3 years ago

mbuckaway commented 3 years ago

IDF Version: v4.2-beta1-227-gf0e87c933 HomeKit Version: current (checked out this week from git from master)

I've built a few homekit devices over the last month and had great success. The SDK is one of the easier ones to work with that I have used. So, in creating another device, I ran into an issue where the accessory would refuse to pair IF I added an ambient light sensor. Additional, if I removed the light sensor and got it to pair, enabling the light sensor against with the now pair accessory would cause the entire sensor would be reported as unresponsive in the home app. It took some effort, but I tracked the issue down to the initial value I set on the light sensor being outside the default expected range defined in esp_hap_apple_profiles/src/hap_apple_chars.c. Not sure this is an issue or it is it working as designed, but setting the value outside the expected range causes the entire accessory to be rendered useless. The problem is my light sensor will read 0 when there is no light...and I really don't want it to display 1 or 0.001. Additionally, I would expect that if the value is outside the correct range, it would continued to work or the SDK would possibly reset the value to the minimum value in this case. For testing purposes, I set the initial value to 0 which causes the pairing error. If I set the initial value to 1.0, it worked.

Not sure if this bug in the ESP Homekit SDK or this is the way the Apple Homekit works when the value is outside the range and the accessory won't work.

The quick fix to the issue was either to set the value to a number inside the range or change esp_hap_apple_profiles/src/hap_apple_chars.c to allow a different range.

The code in esp_hap_apple_profiles/src/hap_apple_chars.c around line 502 has:

 hap_char_float_set_constraints(hc, 0.0001, 100000.0, 0.0);

If I changed line to:

 hap_char_float_set_constraints(hc, 0.0, 100000.0, 0.0);

...the sensor worked correctly when I initialized it to 0.

Questions:

Reviewing the other issues, it appears similar to #4.

I attached a log of the failed pairing if that is any help.

error_ls.log

shahpiyushv commented 3 years ago

@mbuckaway this is indeed an expected behaviour and that is how the sdk and iOS side is designed.

If we internally change the value to fit within the constraints, modifying constraints of some standard characteristics becomes tricky. For example, the code below is perfectly valid

hap_char_t *light_level = hap_char_current_ambient_light_level_create(0);
hap_char_float_set_constraints(light_level, 0, 100000.0, 0.0);

In the first call, if we just change the value from 0 to 0.0001 to fit within the constraints, even if you override the constraints using the next call, the value will stay at 0.0001, which is not what you may have wanted. If we decide to adjust the value while actually reporting, it complicates the code and moreover, since that will happen asynchronously, it will not even be reported to your application code and may cause undesired behaviour.

The SDK only checks the values received from the controller, but lets the application code decide the values set through the firmware.

Now, for any characteristic, if iOS finds that the value is outside the constraints, it is considered as an error and so, the controller either discards the pairing or closes the connection (depending on when the error was received). For some characteristics, it may not even allow changing the constraints(Eg. A value of 400 for hue would be wrong even if you modify the constraints. I haven't tested this, but just a possibility).

If the Home app is working fine with the constraints changed to what you want in this particular case (starting with 0), it could be acceptable.

This is not related to #4 , as there, the JSON formatting itself was getting distorted because ESP8266 does not have floating point support enabled by default. You seem to be using ESP32.

mbuckaway commented 3 years ago

Thanks for the quick response. Its unfortunate Apple has choose to cause an entire device to go offline when there is an "error" in a sensor read value. In my case, the light sensor does return 0 when it's night....which Apple believes is not valid.

My work around was to create a custom light sensor based on the ESP HomeKit SDK version, and set to the limit to start from 0 and set int constraints since the sensor returns an int and not a float. This works. The Home app shows 0lux at night and shows the correct value during the day.

This is different for a light or a switch where the values are limited. You can set a switch a value of 2 and it only supports 0 or 1. Sensors on the other hand are read-only devices, and return what they return.

That, I have a working device. So, the case can be closed. The issue should probably appear in a FAQ for the SDK.