ksheumaker / homeassistant-apsystems_ecur

Home Assistant custom component for local querying of APSystems ECU-R Solar System
Apache License 2.0
166 stars 42 forks source link

ECU firmware version and timezone #28

Closed HAEdwin closed 2 years ago

HAEdwin commented 2 years ago

The most important update is anticipating the variable length of the firmware and time zone specification. Both are made visible as attributes. In addition, unnecessary code removed and textual adjustments made. CalVer applied (was already needed for HACS and now implemented across the integration on three files).

tv3 commented 2 years ago

Streamline socket code.

Was initially working on kyle's repo and was missing some code after checking your fork. So I see you removed the sync version of the query_ecu function. That was what I actually was playing with :-)

As I haven't HA running (yet. I got me a odriod n2+ which i'm going to setup), I didn't look at the integration part yet.

So I had a look now. The integration uses the async_query_ecu function.

The async stuff and my initial thoughts:

I changed the query_ecu function and abstracted the socket calling stuff in a separate function.

HAEdwin commented 2 years ago

I've removed the sync code that has become redundant function because it was later replaced by the async function and performed better. Sync code caused errors but could be related to hardware, Integration or connection problems, no error handling and not closing the send string with END\n. Kyle converted the code to an integration so that it can be used in HA. When he introduced async he said: " The bigger change is now the querying of the ECU is now done correctly (I think) with the way home assistant does asynchronous I/O. There was a chance in previous versions that querying the ECU could block the main HA thread. This I believe has been fixed - but i’m pretty novice when it comes to writing HA integrations. " Currently I have no issues with the async solution but feel free to reintroduce the sync function (new insights/knowledge may have undone the need for asynchronous io), maybe by introducing a (temporary) configuration parameter with which you can choose the method. I'm happy to test the sync method but we'll have to see what experiences will be with other users (hence the configuration parameter). Feel free to improve everyting, I have little knowledge of Python and could never have converted the initial code to make it an integration. Like Kyle said, code needs cleanup. Some lines are a bit surplus and can be shortened and made easier to interpret. If you want me to test something let me know I'm happy to do so.

HAEdwin commented 2 years ago

Added the sync code again and did a pull request. The way Dennis at https://github.com/HectorMalot/ecur is coding is really inspiring.

HAEdwin commented 2 years ago

@tv3 The sync query contains socket close and can be used to try when you modify init.py Replace data = await self.ecu.async_query_ecu() with data = self.ecu.query_ecu() around line 73.

tv3 commented 2 years ago

Thanks @HAEdwin

Been doing work, so haven't had time to work on it. I'll pull the latest version of your fork and see if I can update with a PR