scottyphillips / pychonet

A simple to use library for interfacing with the ECHONETlite protocol
GNU General Public License v3.0
20 stars 17 forks source link

Make sanity check before accessing data #27

Closed klinkigt closed 2 years ago

klinkigt commented 3 years ago

During my debugging I found that pychonet crashed hard, when providing wrong data. Here the example that might cause the crash of home assistant.

Traceback (most recent call last): File "troubleshoot.py", line 128, in asyncio.run(main()) File "/usr/lib64/python3.8/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) File "/usr/lib64/python3.8/asyncio/base_events.py", line 616, in run_until_complete return future.result() File "troubleshoot.py", line 115, in main device9 = Factory("192.168.1.50", host, 2,163, 1) File "/home/martin/workspace/pychonet/pychonet/init.py", line 14, in Factory instance = EOJX_CLASS[eojgc][eojcc] KeyError: 163

Better to check all parameters before processing them, to avoid such crashes.

scottyphillips commented 3 years ago

Well in that case below it’s a missing value from the lookup table. I would prefer to fix the lookup table rather than ignore the error.

From: klinkigt @.> Reply-To: scottyphillips/pychonet @.> Date: Saturday, 18 September 2021 at 11:07 am To: scottyphillips/pychonet @.> Cc: Subscribed @.> Subject: [scottyphillips/pychonet] Make sanity check before accessing data (#27)

During my debugging I found that pychonet crashed hard, when providing wrong data. Here the example that might cause the crash of home assistant.

Traceback (most recent call last): File "troubleshoot.py", line 128, in asyncio.run(main()) File "/usr/lib64/python3.8/asyncio/runners.py", line 44, in run return loop.run_until_complete(main) File "/usr/lib64/python3.8/asyncio/base_events.py", line 616, in run_until_complete return future.result() File "troubleshoot.py", line 115, in main device9 = Factory("192.168.1.50", host, 2,163, 1) File "/home/martin/workspace/pychonet/pychonet/init.py", line 14, in Factory instance = EOJX_CLASS[eojgc][eojcc] KeyError: 163

Better to check all parameters before processing them, to avoid such crashes.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

scottyphillips commented 3 years ago

I have updated with version 2.0.18. The data wasn't 'wrong' as such it was valid classes missing from the lookup table. I have just updated the lookup tables so it should now work. I will consider what to do about the lookup tables in the factory classes.

klinkigt commented 3 years ago

Sorry, just test and it still gives an error. HA does not crash but the integration does not work. So the hack in the config_flow is still needed:

Traceback (most recent call last): File "/home/martin/.local/lib/python3.9/site-packages/homeassistant/config_entries.py", line 304, in async_setup result = await component.async_setup_entry(hass, self) # type: ignore File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 127, in async_setup_entry await echonetlite.async_update() File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 264, in async_update batch_data = await self._instance.update(flags) File "/home/martin/.homeassistant/deps/lib/python3.9/site-packages/pychonet/EchonetInstance.py", line 123, in update elif epc in list(EPC_CODE[self._eojgc][self._eojcc].keys()): # return hex value if EPC code exists in class but no function found KeyError: 163

scottyphillips commented 3 years ago

Im an idiot, i didnt sanity check the EPC_TABLE when I transcribed the codes for 163 (0xA3) in epc.py

    0xA0: { # Light Controller  # should have been 0xA3.
        0x80: 'Operation status',
        0xB0: 'Illuminance level setting',
        0xC0: 'Scene control setting',
        0xC1: 'Number that can assign scene control setting'
    }

Version 2.0.19 should properly resolve it.

scottyphillips commented 3 years ago

I also need to fix up that function a little better... Getting there slowly. this has been a good stress test.

klinkigt commented 3 years ago

Sorry, to disappoint you, but still the same error:

Traceback (most recent call last): File "/home/martin/.local/lib/python3.9/site-packages/homeassistant/config_entries.py", line 304, in async_setup result = await component.async_setup_entry(hass, self) # type: ignore File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 127, in async_setup_entry await echonetlite.async_update() File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 264, in async_update batch_data = await self._instance.update(flags) File "/home/martin/.homeassistant/deps/lib/python3.9/site-packages/pychonet/EchonetInstance.py", line 123, in update elif epc in list(EPC_CODE[self._eojgc][self._eojcc].keys()): # return hex value if EPC code exists in class but no function found KeyError: 163

scottyphillips commented 3 years ago

Did you update the version in the custom component manifest file to version 2.0.19? If not I will push an update shortly

Scott

Get Outlook for iOShttps://aka.ms/o0ukef


From: klinkigt @.> Sent: Saturday, September 18, 2021 9:19:44 PM To: scottyphillips/pychonet @.> Cc: Scott Phillips @.>; Comment @.> Subject: Re: [scottyphillips/pychonet] Make sanity check before accessing data (#27)

Sorry, to disappoint you, but still the same error:

Traceback (most recent call last): File "/home/martin/.local/lib/python3.9/site-packages/homeassistant/config_entries.py", line 304, in async_setup result = await component.async_setup_entry(hass, self) # type: ignore File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 127, in async_setup_entry await echonetlite.async_update() File "/home/martin/.homeassistant/custom_components/echonetlite/init.py", line 264, in async_update batch_data = await self._instance.update(flags) File "/home/martin/.homeassistant/deps/lib/python3.9/site-packages/pychonet/EchonetInstance.py", line 123, in update elif epc in list(EPC_CODE[self._eojgc][self._eojcc].keys()): # return hex value if EPC code exists in class but no function found KeyError: 163

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/scottyphillips/pychonet/issues/27#issuecomment-922261503, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGHQNDGEQYWAYBHSE5T22XDUCRYVBANCNFSM5EITOYPA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

scottyphillips commented 3 years ago

If you pull the latest troubleshoot.py file:

async def main():

    print(f'pychonet verison is {VERSION}')
    udp = UDPServer()
    loop = asyncio.get_event_loop()
    udp.run("0.0.0.0", 3610, loop=loop)
    server = api(server=udp, loop=loop)

    host = '192.168.1.50'
    await server.discover(host)
    device2 = EchonetInstance("192.168.1.50", 2,163,1, server)
    # value = await device.getMessage(ENL_SETMAP)
    # print(value)
    value = await device2.getMessage(ENL_GETMAP)
    print(f"getmap is {value}")

    value = await device2.getMessage(ENL_SETMAP)
    print(f"setmap is {value}")

    if 0x80 in list(EPC_CODE[2][163].keys()):
        print("EPC TABLE is correct!")

if __name__ == "__main__":
    asyncio.run(main())

The results show that the error message you are seeing should not be there. The only reason that message would still be appearing would be if you are not running the latest version. So please update the custom component and check your manifest file to ensure you are running 2.0.19

pychonet verison is 2.0.19
getmap is False # this is running on my system and I don't have 2,163,1
setmap is False
EPC TABLE is correct!
klinkigt commented 3 years ago

Good morning,

yes the latest version works correctly now. Not sure it there was a problem with my installation but I simple pulled a fresh version and the troubleshoot gives: pychonet verison is 2.0.19 getmap is [128, 192, 240, 129, 193, 241, 130, 242, 244, 245, 134, 246, 151, 247, 136, 152, 137, 249, 138, 154, 250, 139, 140, 141, 157, 142, 158, 159] setmap is [129, 151, 152, 192, 240, 241, 242, 244, 245, 246, 247, 249, 250] EPC TABLE is correct!

also HA does not crash anymore, so the hack is not needed (but I see you already removed it).

Now I also get a model "Lighting system" with the properties: test Number that can assign scene control setting Unknown test Operation status Unknown test Scene control setting Unknown

And you are right, your code is getting more robust now. Not sure yet what this Lighting system should be good for, but it might be the control multiple lights at once (scenes)?

scottyphillips commented 3 years ago

Yeah I think so, but the GET and SET properties that your system is returning don’t match the ECHONET spec for code A3. I can only assume there is possibly some vendor specific stuff going on there.

Also, EPC code 128 should return the operational status on or off as it’s a mandatory code. Not sure why you are seeing “unknown” for operational status. More work is needed I think.

Scott

Get Outlook for iOShttps://aka.ms/o0ukef


From: klinkigt @.> Sent: Sunday, September 19, 2021 10:15:20 AM To: scottyphillips/pychonet @.> Cc: Scott Phillips @.>; Comment @.> Subject: Re: [scottyphillips/pychonet] Make sanity check before accessing data (#27)

Good morning,

yes the latest version works correctly now. Not sure it there was a problem with my installation but I simple pulled a fresh version and the troubleshoot gives: pychonet verison is 2.0.19 getmap is [128, 192, 240, 129, 193, 241, 130, 242, 244, 245, 134, 246, 151, 247, 136, 152, 137, 249, 138, 154, 250, 139, 140, 141, 157, 142, 158, 159] setmap is [129, 151, 152, 192, 240, 241, 242, 244, 245, 246, 247, 249, 250] EPC TABLE is correct!

also HA does not crash anymore, so the hack is not needed (but I see you already removed it).

Now I also get a model "Lighting system" with the properties: test Number that can assign scene control setting Unknown test Operation status Unknown test Scene control setting Unknown

And you are right, your code is getting more robust now. Not sure yet what this Lighting system should be good for, but it might be the control multiple lights at once (scenes)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/scottyphillips/pychonet/issues/27#issuecomment-922392672, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGHQNDCYVUH5GELEAYLDO7TUCUTRRANCNFSM5EITOYPA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

scottyphillips commented 3 years ago

image

Looks like you support the mandatory 0x80, 0xC1, and 0xC0. 0xB0 is not mandatory and you dont seem to have it in your tables. 0xC0 is only a SETMAP value.

All the other values appear to be proprietary.

Have you seen any data values return yet for those 4 sensors???

scottyphillips commented 3 years ago

You can try running this updated troubleshoot.py file and we can see what sort of data your lighting system is returning.

import asyncio
from pprint import pprint

from aioudp import UDPServer
from pychonet import ECHONETAPIClient as api
from pychonet import Factory, EchonetInstance
from pychonet.lib.const import VERSION, ENL_SETMAP, ENL_GETMAP, ENL_UID, ENL_MANUFACTURER
from pychonet.lib.epc import EPC_CODE

# This example will list the properties for all discovered instances on a given host

async def main():

    print(f'pychonet verison is {VERSION}')
    udp = UDPServer()
    loop = asyncio.get_event_loop()
    udp.run("0.0.0.0", 3610, loop=loop)
    server = api(server=udp, loop=loop)

    host = '192.168.1.50'
    await server.discover(host)
    device = EchonetInstance("192.168.1.50", 2,163,1, server)
    # value = await device.getMessage(ENL_SETMAP)
    # print(value)
    getmap = await device.getMessage(ENL_GETMAP)
    print(f"getmap is {getmap}")

    setmap = await device.getMessage(ENL_SETMAP)
    print(f"setmap is {setmap}")

    for epc in getmap:
        value = await device.getMessage(epc)
        print(f"getmap value EPC code {epc} is {value}")

    for epc in setmap:
        value = await device.getMessage(epc)
        print(f"setmap value EPC code {epc} is {value}")

if __name__ == "__main__":
    asyncio.run(main())
klinkigt commented 3 years ago

Yes, those koizumi device is a bit tricky. But I can also understand somehow, as this echonetlite is very limited and it seems koizumi was not in the group from the start, so they were not able to push their ideas: here is the data you requested:

pychonet verison is 2.0.19 getmap is [128, 192, 240, 129, 193, 241, 130, 242, 244, 245, 134, 246, 151, 247, 136, 152, 137, 249, 138, 154, 250, 139, 140, 141, 157, 142, 158, 159] setmap is [129, 151, 152, 192, 240, 241, 242, 244, 245, 246, 247, 249, 250] getmap value EPC code 128 is b'0' getmap value EPC code 192 is b'\x00' getmap value EPC code 240 is b'\x00\x000\xd60\xea0\xc30\xb8\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' getmap value EPC code 129 is b'\x00' getmap value EPC code 193 is b' ' getmap value EPC code 241 is b'\x01\x02\x00\x00' getmap value EPC code 130 is b'\x00\x00K\x00' getmap value EPC code 242 is b'\x01' getmap value EPC code 244 is b'\x01\x00\x00\x00\x00\x00\x00\x00' getmap value EPC code 245 is b'\x01\x02\x01\x00\x01\x02\x03' getmap value EPC code 134 is b'\x02\x00\x00\xe8\x00\x00' getmap value EPC code 246 is b'' getmap value EPC code 151 is b'\n\x01' getmap value EPC code 247 is b'1' getmap value EPC code 136 is b'B' getmap value EPC code 152 is b'\x07\xe5\t\x13' getmap value EPC code 137 is b'\x00\x00' getmap value EPC code 249 is b'\x00' getmap value EPC code 138 is Koizumi Lighting Technology getmap value EPC code 154 is b'C\x00\x00(\xee' getmap value EPC code 250 is False getmap value EPC code 139 is b'\x00\x00\x00' getmap value EPC code 140 is b'AE50264E ' getmap value EPC code 141 is b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' getmap value EPC code 157 is b'\x03\x80\x81\x88' getmap value EPC code 142 is b'\x07\xe4\x03\x1a' getmap value EPC code 158 is [129, 151, 152, 192, 240, 241, 242, 244, 245, 246, 247, 249, 250] getmap value EPC code 159 is [128, 192, 240, 129, 193, 241, 130, 242, 244, 245, 134, 246, 151, 247, 136, 152, 137, 249, 138, 154, 250, 139, 140, 141, 157, 142, 158, 159] setmap value EPC code 129 is b'\x00' setmap value EPC code 151 is b'\n\x01' setmap value EPC code 152 is b'\x07\xe5\t\x13' setmap value EPC code 192 is b'\x00' setmap value EPC code 240 is b'\x00\x000\xd60\xea0\xc30\xb8\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' setmap value EPC code 241 is b'\x01\x02\x00\x00' setmap value EPC code 242 is b'\x01' setmap value EPC code 244 is b'\x01\x00\x00\x00\x00\x00\x00\x00' setmap value EPC code 245 is b'\x01\x02\x01\x00\x01\x02\x03' setmap value EPC code 246 is b'' setmap value EPC code 247 is b'1' setmap value EPC code 249 is b'\x00' setmap value EPC code 250 is False