search

jesserizzo / envoy_reader

MIT License
37 stars 26 forks source link

Update envoy_reader.py getData() #68

Closed stuartwishart closed 3 years ago

stuartwishart commented 3 years ago

feature/optional-fetch-inverters Update getData() to add optional input parameter to not read inverters. Inverters only update every 5 or 15 mins, depending on Envoy-S configuration, whereas production data updates every minute. Also, requesting inverter data at too short an interval can cause the Envoy to start lagging and eventually timeout. See https://thecomputerperson.wordpress.com/2016/08/03/enphase-envoy-s-data-scraping/#comment-1565 for more details.

gtdiehl commented 3 years ago

@stuartwishart I've been away for a while and catching up. I noticed that the Envoy sensor integration on the Home Assistant side has changed a little as well. To move to the new config_flow process the sensor code has been updated.

The retrieval of data has been moved into the __init__.py file of the Envoy sensor. I feel with these changes no changes will need to be done on the API side. Only changes on the Home Assistant side would need to be made.

Take a look at the latest code on the Home Assistant side and see if you can make the changes there without any needed API side changes. If API side changes are needed lets see what they would be based on the newer Home Assistant sensor code.

stuartwishart commented 3 years ago

@gtdiehl I had a look at the updated Home Assistant Envoy sensor code and from what I can see the processing of the envoy_reader.getData() is still the same.

One change is that if during initialisation of the envoy_reader class it throws an ConfigEntryAuthFailed exception then the sensor disables reading the inverters through directly changing the config setting in the class by envoy_reader.get_inverters = False.

I could use the same direct access methodology to implement my proposed change all on the Home Assistant side.

gtdiehl commented 3 years ago

@stuartwishart On the Home Assistant side if you modify the envoy_reader.get_inverters variable directly do you still need any change on the API side here? If not can we close this PR?

stuartwishart commented 3 years ago

@gtdiehl I must admit it goes against my normal programming practices to directly manipulate internal configuration variables of an external class. Ideally from a good programming practice I would prefer to temporarily disable downloading inverter data via the getData() function call, however, if you would prefer not to change the API then it should be possible to implement the functionality by directly changing the envoy_reader.get_inverters variable.

I haven't prototype this solution yet. Is it possible to leave the PR open until I can test this?

gtdiehl commented 3 years ago

@stuartwishart I get what you're saying. I'm looking through the code and trying to see why (probably me) the self.get_inverters exists. For now I guess this change is minimal and does have a default value of retrieving inverter data, so I'll merge the change.

I'm thinking the reason why it exists was because the EnvoyReader object would be created each time Home Assistant polled for the data. Now it appears the object is re-used and probably makes sense to move the self.get_inverters to getData(). But for now we could leave in both and see if the original is really needed now.