search

watchforstock / evohome-client

Python client to access the Evohome web service
Apache License 2.0
88 stars 52 forks source link

Multiple Location support #31

Closed freeranger closed 6 years ago

freeranger commented 8 years ago

Hi,

I am using your excellent client in my own app, https://github.com/freeranger/evologger for logging EvoHome data to various places. It is great for me and my single location, but someone who wants to use it has multiple locations and certainly by calling the 'public' methods on the EvohomeClient (e.g. temperatures()), it throws an error.

I see that you have various 'private' methods which I could call to get access to the required location and then the temps etc for that, but calling privates (certainly in languages that support the concept!) is bad practice - do you have any plans to support multiple locations via public methods? Perhaps a get_heating_system that takes a location parameter? Or the public methods - temperatures etc - could take an optional location name (or location object) and use that if supplied rather than calling _get_single_heating_system?

Of course this could potentially throw up other issues, with multiple gateways and multiple control systems (you throw errors for these) but I don't know how these things all hang together - how likely is it there would be multiple of these? Is it just that you get a JSON array back but there should only ever be one, or are there configurations where multiple may indeed be returned?

Thanks in advance.

freeranger commented 8 years ago

Just to add to this, I have extended your client in my own app: https://github.com/freeranger/evologger/blob/master/plugins/evohome/__init__.py and it works well enough for my purposes. Happy to put together a PR for your app if you think it's worth integrating? The approach I went for is that my app only supports one location but you can specify that location in the configuration file, so when I want to get temps etc I can get a heating system for that specific location(name or id) and make my calls on that. It would be trivial to modify your client to support this..I guess the only question really is how - as in what functions do you want to make available or modify? Could be very backwards compatible with the current app by defaulting to the 0th location if none is specified for all the current public methods...

watchforstock commented 8 years ago

Thanks for you messages. The challenge here is partly that as well as an account having multiple locations, each location can have multiple gateways and each gateway can have multiple control systems. At least as far as the API is concerned, whether Honeywell actually support all of these options I'm not sure.

As such, when I originally wrote the library, I took the view that it would support the simple case (one account, one location, one gateway, one control system) easily and then expose the other information to allow the application to deal with any of those being multiple as it saw fit.

The other challenge in this, is for applications to be able to expose the right configuration to their users - ie. the user isn't going to know the ID the API uses for their gateway for example.

I certainly wouldn't be against pulling in changes to the library, but they would need to address the broader problem of selecting each of the bits of the system a user is addressing. That said, the solution you've built for your application sounds like the right approach, especially if you're happy to assume that each location only has a single gateway and single control system.

freeranger commented 8 years ago

Hi,

I don't know under what circumstance one would have multiple gateways etc - and sadly Honeywell don't seem inclined to open out the API and document it :/ As a default position, if there are multiple, one could just select the first and put out a warning about there being more? At least until such time as we have more information about when/if one would actually have multiples.

Ok I'll knock up a PR at some point and you can see if it suits - the aim would be backwards compatibility with the common case you have now. I think I would only need to add get_location and get_heating_system from my class as public methods and change the signatures of your methods temperatures, set_status_heatingoff etc to take in a optional location (same as my get_location does) so we get for example:

    def set_status_heatingoff(self, until=None, locationId=None):
          ... use the supplied location id (or should it take an actual location?) 
              or the default one as it does now?
watchforstock commented 6 years ago

Closing due to lack of activity or a PR - happy to reopen if the issue is still present.