gpulido / python-airzone

MIT License
10 stars 3 forks source link

Forked, local API addon #6

Closed Mike-de-bike closed 3 years ago

Mike-de-bike commented 3 years ago

Hi Gabriel,

actually working on a fork of it. Request PUSH section is in first state. have a look.

Kind regards Stefan

gpulido commented 3 years ago

Hello @Mike-de-bike Have you made the pull request? I have created a new issue for tracking the development of the Local API integration here: https://github.com/gpulido/python-airzone/issues/7

Mike-de-bike commented 3 years ago

Hello Gabriel,

created a pull request, had not it on the mind to dl it. 😀

⁣kind regards Stefan

Am 29. Juni 2021, 00:32, um 00:32, Gabriel Pulido @.***> schrieb:

Hello @Mike-de-bike Have you made the pull request? I have created a new issue for tracking the development of the Local API integration here: https://github.com/gpulido/python-airzone/issues/7

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/gpulido/python-airzone/issues/6#issuecomment-870090305

gpulido commented 3 years ago

@Mike-de-bike I have just created a localapi branch so we can work on it before moving to master. Would you mind create the pr against such branch? Thank you!!

Mike-de-bike commented 3 years ago

No problem, done!

⁣Grüße Stefan  Senftleben ​

Am 29. Juni 2021, 11:09, um 11:09, Gabriel Pulido @.***> schrieb:

@Mike-de-bike I have just created a localapi branch so we can work on it before moving to master. Would you mind create the pr against such branch? Thank you!!

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/gpulido/python-airzone/issues/6#issuecomment-870420521

gpulido commented 3 years ago

Hello! Sorry for disturb you again, I have just made a commit to localapi with your code refactored a little. Please take a look and check that it still works. Again, if you can get a "json" and just write as a file and attach so I can use as a base for testing and mocking it would be great!

Mike-de-bike commented 3 years ago

Hello,

no problem, you do not disturb me.

I will check it today and, of course, I will create a appropriate file for your testing purposes.

⁣ Stefan

Am 29. Juni 2021, 13:17, um 13:17, Gabriel Pulido @.***> schrieb:

Hello! Sorry for disturb you again, I have just made a commit to localapi with your code refactored a little. Please take a look and check that it still works. Again, if you can get a "json" and just write as a file and attach so I can use as a base for testing and mocking it would be great!

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/gpulido/python-airzone/issues/6#issuecomment-870506424

Mike-de-bike commented 3 years ago

What is the best practice to test your code adaption on local api. Should I refork the branch localapi?

Am 29. Juni 2021, 13:17, um 13:17, Gabriel Pulido @.***> schrieb:

Hello! Sorry for disturb you again, I have just made a commit to localapi with your code refactored a little. Please take a look and check that it still works. Again, if you can get a "json" and just write as a file and attach so I can use as a base for testing and mocking it would be great!

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/gpulido/python-airzone/issues/6#issuecomment-870506424

gpulido commented 3 years ago

Hello again @Mike-de-bike, Just added another commit to the localapi branch. We are getting closer to a working beta. Please take a look

Mike-de-bike commented 3 years ago

Good morning @gpulido ! Pulled it, it is working fine and looking good!

gpulido commented 3 years ago

Hello @Mike-de-bike I think you need to integrate the main repo localapi changes into your branch before making a new pr as your localapi branch still have commits already integrated. BEst aproach is to fork again the localapi branch and apply your changes on top if needed.

Mike-de-bike commented 3 years ago

@gpulido Did the new forking of it. quite problematic to develop on a fork and another person on the source.

gpulido commented 3 years ago

@Mike-de-bike I apologize for that, however once you generate a PR and that PR is integrated, you should ditch the local branch and fork again, because if you keep working on the old forked and integrated PR, the commits from those branch are detected by git as new, even if the changes of those commits are already integrated (because of the pr). I already resolved the conflicts and integrated the previous PR, but when you did the new PR the old commits where there again :)

I hope I explained it right. Btw, I will try to not made a lot of changes :) I'm starting to plan the ha part.

Mike-de-bike commented 3 years ago

@gpulido Hello, no need for apologizing. :-) I did remove and repull the fork. My test to use localapi.py is not successfull, the same error is displayed. Waiting curiously for your checkup and your plans of the integration in HA.

gpulido commented 3 years ago

@Mike-de-bike You were right I have just pushed some changes that fixes it. Also I added a first working pytest. Please take a look

Mike-de-bike commented 3 years ago

@gpulido Thanks, that was a quick fix. What for is the test_localapi.py?

gpulido commented 3 years ago

@Mike-de-bike I just created the file and add a basic test, more can (and should) be added. If all is working for you, I will integrate the localapi branch into the main branch and generate a new version. Then I'll move to the homeassistant-airzone plugin so it can use the new "integration"

Mike-de-bike commented 3 years ago

@gpulido Some questions before, please answer it to me. Question 1: Why multiple decorators called property exists? No such named method exists. Is it for HA? 2nd question: setter-decorator exist, what for are they?

my test was done by this: print(m) m.set_zone_parameter_value(1, 'mode', 2) print(m.operation_mode) m.set_zone_parameter_value(1, 'on', 0) m.set_zone_parameter_value(1, 'setpoint', 22) print(m.machine_state) m.set_zone_parameter_value(2, 'setpoint', 23) print(m.machine_state the output of it is: OperationMode.COOLING [{'systemID': 1, 'zoneID': 1, 'on': 0, 'setpoint': 22, 'mode': 2}] [{'systemID': 1, 'zoneID': 2, 'on': 0, 'setpoint': 23, 'mode': 2}]

=>

gpulido commented 3 years ago

@Mike-de-bike 1) I don't unterstand what do you mean.

2) The setter / getter decorator is the "pythonic" way of creating "properties" like other languages (for example the get{} set{} of c#) You don't need to use them, but is more correct if you do. Take a look here for example: https://www.programiz.com/python-programming/property I'm aware that I haven't been using it consistently on the other integrations, but I'm trying to raise the code quality for the plugin.

Regarding your test. The localapi is a "rare" integration as it abstracting "wrongly" the concept pair of "machine" and "zone". There are some properties that are global like the operation_mode, so it belongs to the "machine" where all zones are linked. And some properties are by zone: localtemperature for example.

The problem is that on the localapi api, both properties are linked to the "zone", but them they propagate if needed. If you set the operation_mode on one zone it updates on all zones, instead of having the "machine" concept on the api.

However that is not the proper way to abstract both concepts, no matter the system you use to control it (for example HA). That is why I have splitted them properly, so they can be use properly on HA (or whatever other IoT system that wants to use it).

I have one concern, as I the documentation doesn't explain the put response so we were using what you put there. Are you sure that no matter what zone / property you set with the put, the api is returning a JSON with all the system properties like if you make a post to the zone 0 ?

For testing also, try to use the "properties" instead of the internal methods. For example instead of doing m.set_zone_parameter_value(1, 'mode', 2)

Do this:

m.operation_mode = OperationMode.COOLING

And for the zone, I just realized that we need to add a "get_zone(self, zone_id)" that returns the Zone object, but you can use for now this:

m._zones[1].signal_temperature_value = 22

m._zones[2].signal_temperature_value = 23

You can just do a print(str(m)) after each operation to check.

It should work, if not, we have a bug that needs to be fixed.

Mike-de-bike commented 3 years ago

Hi @gpulido , thank you, the programiz explaination of the property decorator helped me to understand your code much more. The private variables with the prefix "_" were mentioned in the article, that point I tried to use to get in touch with the property decorators, building in the private variables. And the str() method I get used the decorator style. I hope the new pull request is okay for you.

Your code example for switching the operation mode works.

gpulido commented 3 years ago

Hello @Mike-de-bike Sorry for the late answer, I have been sick for the last days and I haven't been able to keep working on the integration. But now I have just commited a new push to the localapi branch, I expect to be almost the last one before mergin into master and realisng the new version of the python-airzone with localapi integrated. Please take a look before I made the merge.

In order to try it, you can use the cli or using the factory for example (on the init.py)

gpulido commented 3 years ago

@Mike-de-bike I've just released the 0.8.0 that includes the localapi implementation.