manzanotti / geniushub-client

Python library to provide direct connectivity to Genius Hub.
MIT License
9 stars 5 forks source link

Errors logged in HA. #30

Closed GeoffAtHome closed 4 years ago

GeoffAtHome commented 4 years ago

Not really sure why the schedule is required to be processed by HA. HA just needs to reflect the state of each device.

2019-11-13 19:08:06 ERROR (MainThread) [geniushubclient] Failed to convert Zone 23, message: list index out of range.
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/geniushubclient/__init__.py", line 546, in _convert
    result["schedule"]["timer"] = _timer_schedule(raw_json)
  File "/usr/local/lib/python3.7/site-packages/geniushubclient/__init__.py", line 463, in _timer_schedule
    tm_last = setpoints[idx + 1]["iTm"]
IndexError: list index out of range

It would be far easier, for HA, if during initialisation this code was not executed unless I am missing the point as to why it is needed. In HA schedules cannot, currently, be viewed or edited.

I have been away sometime. Finally back home, moved, and get HA up and running in our new house.

zxdavb commented 4 years ago

Hi Geoff,

Welcome back! :)

I am not sure if you're reporting a bug, asking a question, or making a suggestion?

I think this issue is a duplicate of https://github.com/zxdavb/geniushub-client/issues/27, which has been addressed. However, I can't sure unless I know what version of the library (i.e. what version of HA) you're running - it would be helpful to know, but in the meantime, I will close this issue.

I see v0.102.0 of HA has just been released as a beta - I suggest you try that as it uses the latest version of this library, including a fix (#29) for the above (this version of HA has other new features, like turning On/Off zones to Override/On or Timer).

It would be far easier, for HA, if during initialisation this code was not executed unless I am missing the point as to why it is needed. In HA schedules cannot, currently, be viewed or edited.

Correct: HA does not currently use GH schedules (this may change). However, this library was not written solely for HA.

A core design feature of this library is to take v3 JSON, to 'translate' it, and thus present it as v1-compatible JSON. The v1 API presents schedule data, and so this library does too. Since the v3 API that returns the zone data also returns the schedules, the processing is done at the beginning, even of no -one asked for that data.

This take-up of this library has been gradual, and bugs have revealed themselves only when the code has been run outside of my (rather extensive, I thought) test-bed in response to winter.

Nonetheless, I have implemented a suggestion similar to yours, by wrapping the schedule translate code with try ... except clauses so that a failure in this area does not affect the core functionality of the code: https://github.com/zxdavb/geniushub-client/blob/6642551c9c3367dc3e5f9003ade9a0ec8a801b78/geniushubclient/__init__.py#L552-L560

Please do review the algorithms (_timer_schedule(), _footprint_schedule()), as I am always happy to accept any PRs that improve the code

GeoffAtHome commented 4 years ago

@zxdavb I am reporting a bug that maybe a duplicate and making a suggestion.

I am running Home Assistant 0.101.3 so this maybe fixed.

The suggestion is only initialise what is needed for the code in hand. This may be a general purpose library by that is no reason to execute code that is not required by all consumers of the library.

I would be interested in learning more about how you test. I like TDD but haven't got round to running tests in a python environment yet - my bad. More tests means it is easier to change code.

I have seen the discussion about smart-plugs and don't see why these are not switches. These do not need to be assigned to a zone and can be made to work independently - well at least in my code this is how I had them working.

Good work none the less. Thanks for moving this forward.

zxdavb commented 4 years ago

I am running Home Assistant 0.101.3 so this maybe fixed.

Yes, so a HA bug then; but is fixed in 0.102, which will be released in a week - I tried my best to get it in 0.101, but HA are not keen on bumping client libraries, except in dev, or rc/beta branches.

The suggestion is only initialise what is needed for the code in hand. This may be a general purpose library by that is no reason to execute code that is not required by all consumers of the library.

Sorry, wontfix - the overhead is minimal (certainly no more I/O than otherwise), there is additional protection in the latest version of the library, and (see below) it is used for CI testing in any case.

I would be interested in learning more about how you test...

Simple: testing is done by pulling the v1 API via curl, and comparing it (diff) to the output of the library.

That is (BTW, the cURL command includes schedule data):

curl -Ss -X GET https://my.geniushub.co.uk/v1/zones -H "authorization: Bearer ${HUB_TOKEN}"

is used as the 'truth' for the output of both:

python ghclient.py ${HUB_ADDRESS} -u ${USERNAME} -p ${PASSWORD} zones -v
python ghclient.py ${HUB_TOKEN zones -v

If both outputs match the 'truth', then CI passes.

The weakness here is that there is no testing confirming that changes made by the client are actually effected as expected. This needs addressing.

have seen the discussion about smart-plugs and don't see why these are not switches. These do not need to be assigned to a zone and can be made to work independently

Good point (for the sake of anyone else reading: If they are assigned to a zone, then they can't be controlled independently).

I will have a think about how to best do this... It is not as straightforward as you might think (e.g. HA turns these Override/On(GH) for on(HA) and Timer(GH) for off(HA) - this is die to limitations within HA.

[ EDIT: Upon reflection, I think the way forward is to have two different switch types. ]

Certainly worth adding as an issue. Could you remind me of the URI? I guess it's v3 API only?

The current workaround in HA would be to create a GH on/off zone for each GH smartplug.

Good work none the less.

The good news (and bad news) is that the standards at HA are high.

GeoffAtHome commented 4 years ago

For TDD I would be looking at specific unit test. Testing against live data (from your Genius Hub) is good but limited. It works for your data but does not encompass a wide range of data.

I'll take a look at adding some unit tests. Getting the first test in any environment is the hardest. Once does it is normally easy to expand. I have never done this in python before so probably will have some teething issues.

zxdavb commented 4 years ago

It works for your data but does not encompass a wide range of data.

True. I have 2x GH systems, and one of them was pretty wacky at the end, as I miss-configured zones, removed batteries, etc. This wasn't enough, however.

I also have a process where people can send me their v3 JSON and I can emulate a hub with it for test/debug - this has proved useful: https://github.com/zxdavb/geniushub-client/blob/6642551c9c3367dc3e5f9003ade9a0ec8a801b78/ghclient.py#L72

I'll take a look at adding some unit tests.

Great!

GeoffAtHome commented 4 years ago

Thanks - It good to know that you can feed raw json for test purposes without requiring a hub. I'll take a look when I'm ready. First of all I need to understand how to add unit tests to python.

zxdavb commented 4 years ago

Hey Geoff - just thought I'd check you knew I am using Circle CI for testing?

GeoffAtHome commented 4 years ago

Thanks. I didn't look at the .circleci file. Looks like you have the unit tests already covered. I had not come across Circle CI.