jlusiardi / homekit_python

A python implementation to work as both HomeKit controller and accessory.
Apache License 2.0
221 stars 41 forks source link

Remove padding whitespace to satisfy Tado #182

Closed elmopl closed 4 years ago

elmopl commented 4 years ago

Taking iPhone requests as source of truth about the format of json payloads there should be no whitespace after comma (or anywhere that would be just spacing). Some devices (like Tado Internet Bridge) are really fussy about that, and sending json payload in Python's standard formatting will cause it to return error. I.e. sending perfectly valid json: {"characteristics":[{"iid":15, "aid":2, "ev":true}]} will not work, while: {"characteristics":[{"iid":15,"aid":2,"ev":true}]} is accepted.

Fixes #181

Jc2k commented 4 years ago

Would be good if we could add a test for this as it’s the kind of thing that’s easy to revert by accident

jlusiardi commented 4 years ago

another question: why should we add , separators=(',', ':') only to this occurrence of json.dumps?

or even better: why does tado work on other payloads but not on this one?

Jc2k commented 4 years ago

I don't think we'll ever understand the answer to the 2nd question. I've actually been looking into how a recent firmware update broke Ecobee Switch. It worked if the payload was 1, 0, False but not True, which lead to the bug report "turn off works but turn on doesnt"... All 4 are valid for bool fields according to spec! I think the reality is that a spec is barely any more than an aspiration in this world, and we are not in a position to make the manufacturers comply with it. But they will comply with the format used by iOS in the wild.

But i would say - if iOS never serializes with spaces we should never serialize with spaces. So not just this occurence but all occurences.

jlusiardi commented 4 years ago

I just merged this PR into a branch and added some tests: https://github.com/jlusiardi/homekit_python/tree/fix_181_testing_and_merging

What do you think? @elmopl can you please test with this branch?

jlusiardi commented 4 years ago

Ah I forget /resource. Thanks! Could you add a test for that as I did for the 2 others?

elmopl commented 4 years ago

Run some tests:

pi@raspberrypi:~ $ PYTHONPATH=homekit_python/ python3 -m homekit.get_characteristic -f test.homekit -a tado -c 2.15
{
    "2.15": {
        "value": 25
    }
}
pi@raspberrypi:~ $ PYTHONPATH=homekit_python/ python3 -m homekit.put_characteristic -f test.homekit -a tado -c 2.15 23
pi@raspberrypi:~ $ PYTHONPATH=homekit_python/ python3 -m homekit.get_characteristic -f test.homekit -a tado -c 2.15
{
    "2.15": {
        "value": 23
    }
}

pi@raspberrypi:~ $ PYTHONPATH=homekit_python/ python3 -m homekit.get_events -f test.homekit -a tado -c 2.15 -c 2.16 -c 2.13 -c 2.12 -c 2.14 -c 2.17
event for 2.15: 25
event for 2.12: 0
event for 2.13: 0
event for 2.12: 1
event for 2.13: 1
event for 2.15: 23

All while checking that TRV actually gets updated + last test was with me changing TRV manually.

I have merged fix_181_testing_and_merging and pulled out json.dumps with comment to be in just one place.

Added test for get_resource.

jlusiardi commented 4 years ago

cool thank you!