gazoodle / geckolib

Library to interface with Gecko Alliance spa pack systems via in.touch2 module
GNU General Public License v3.0
62 stars 24 forks source link

STATP handling #20

Closed maegibbons closed 3 years ago

maegibbons commented 3 years ago

Hi.

I possibly have a faulty tub. Occasionly the temperature reads to low and overheats the water and I then get an OH error on reboot as the temperature is above 40.

I mention the above in relation to the behaviour with geckolib that I have been experiecing.

Basically i have been having high and low spikes in temperature readings which is shown in the HA graphs below.

On a seperate machine i installed geckolib so i could explore using the shell. I was seeing a great deal of STATP packets all with very variable water temperature over a few seconds.

Mainly RhWaterTemp was very variable but also DisplayedTemp

I then disabled the STATP handler so the packets were not processed by accesor and no changes were seen by observables.

This has smoothed out the graphs. Now I never saw the variability in temps in the Gecko App and I am wondering if the app ignores STATP messages. STATP seem to be very variable. Either that or the lib is not processing them correctly.

Anybody else seen huge variability in temperature in the STATP messages?

Krs

Mark

Screenshot_20210522-110948_Home Assistant

rct commented 3 years ago

@maegibbons - How are you getting data from the Gecko into Home Assistant?

My tub has been empty for the winter. I'm late in starting up for the season. I haven't looked at Geckolib in 6 months or so, I need to set something up to log the data.

maegibbons commented 3 years ago

Gazoodle has written an integration which can be installed through HACS.

Works well apart from the anomolies with STATP processing that I have noted.

Very easy to install so give it a go!

Mark

maegibbons commented 3 years ago

Hi

I think the STATP handler is reprocessing older packets.

The clock "Hours" move forward then back.

My coding skills are no where near good enough to either understand or fix the problem but pretty sure there is a problem in STATP handling.

Gazoodle, when you are ready to engage, I can send you logs.

Krs

Mark

maegibbons commented 3 years ago

Hi.

Ok I think I have found and fixed the problem. However my code understanding is limited and my solution may cause other problems!!

Currently the driver is reprocessing EVERY STATP "change" it has received since startup when it receives another STATP.

I have fixed this by modifying:

    def _on_partial_status_update(self, handler, socket, sender):
        for change in handler.changes:
            self.struct.replace_status_block_segment(change[0], change[1])

by adding an else for when the current changes have been processed to clear the list as follows:

    def _on_partial_status_update(self, handler, socket, sender):
        for change in handler.changes:
            self.struct.replace_status_block_segment(change[0], change[1])
        else:
            handler.changes.clear()

Krs

Mark

rct commented 3 years ago

I'm looking at the code above. Is that the complete fix?

I get the gist that the previous partial updates aren't getting cleared out. But I'm confused by seeing an else with no conditionals.

I'm surprised that is valid Python but I guess I learned something new. else is valid after for but it seems to execute unconditionally, so why the else?

I think this is the equivalent:

    def _on_partial_status_update(self, handler, socket, sender):
        for change in handler.changes:
            self.struct.replace_status_block_segment(change[0], change[1])

        handler.changes.clear()
maegibbons commented 3 years ago

The else makes sure that what follows is executed outside the for loop. For me it is more explicit code.

Code it how you wish. The important point is that the handlers list is cleared AFTER FOR LOOP COMPLETION and is not reprocessed when another STATP packet is received.

Krs

Mark

rct commented 3 years ago

The else makes sure that what follows is executed outside the for loop. For me it is more explicit code.

Python's expression of structure through indentation can be really counter-intuitive and jarring until you get used to it. I've never seen an else used for that before. There is a keyword pass that is sometimes used to help with structural issues. As long as the indentation for the next statement matches the indentation of the for you'll get the behavior you wanted.

Code it how you wish. The important point is that the handlers list is cleared AFTER FOR LOOP COMPLETION and is not reprocessed when another STATP packet is received.

In any case, do you intend to submit a PR for this fix?

maegibbons commented 3 years ago

I have not forked it and no real intention to.

Assuming gazoodle returns he can just incorporate it.

But if you have a repo drawn down already be my guest.

There are a couple of issues with watercare that I am currently looking at.

maegibbons commented 3 years ago

Hi Everybody

In a standard Hass OS Home Assistant install it is not trivial to get shell access to the home assistant docker container to make patch changes.

It IS achievable by using a script run from HA to overwrite the driver files. I have created such a script.

  1. To get this working intall 'pyscript' integration using HACS
  2. Enable the pyscript integration by adding it in Configuration>Integrations and tick all boxes on configuration.
  3. Create a diretory in /config directory called 'geckolib.changes' by executing mkdir geckolib.changes if you are in the std ssh shell
  4. Download the file http://downloads.oem.org.uk/geckolib/patch_geckolib.py and place it in /config/pyscript. If you are in the std ssh shell you can use wget.
  5. Any/all updated files that you place in 'geckolib.changes' will be copied to the /usr/local/lib/python3.8/site-packages/geckolib/ directory and subdirectories
  6. The script will run everytime HA is restarted. When HA is upgraded the patch will be installed again. Restart HA Again so the patched library is used by HA

Optional

  1. I have already a patched copy of spa.py which you can download here http://downloads.oem.org.uk/geckolib/spa.py or use wget to place it directly in to 'geckolib.changes' directory.
  2. Restart HA and the patched spa.py will be copied in to the docker container /usr/local/lib/python3.8/site-packages/geckolib/ directory
  3. Restart HA again and the HA geckolib integration will use the patched driver

Krs

Mark

rct commented 3 years ago

To use a local copy of Geckolib that will survive container updates.

  1. Put a copy of src/geckolib from the Geckolib repo under /config/geckolib,
  2. Remove the requirements line that mentions geckolib from custom_components/gecko/manifest.json.

Note to remember it is there when Geckolib gets updated (or the custom component gets integrated)


use a module from GitHub

Alternate that I haven't tested from Hass dev docs, Change manifest.json to point to a specific GitHub repo/branch:

See https://developers.home-assistant.io/docs/creating_integration_manifest#custom-requirements-during-development--testing

https://developers.home-assistant.io/docs/creating_integration_manifest#custom-requirements-during-development--testing

This would probably be more useful to anyone that isn't doing any hacking on Geckolib, but does introduce a dependency on GitHub.

gazoodle commented 3 years ago

Great work on this @maegibbons. I've added your fix into the library v0.3.19