indykoning / PyPi_GrowattServer

MIT License
80 stars 34 forks source link

Extended 'mix' type functionality #4

Closed muppet3000 closed 3 years ago

muppet3000 commented 3 years ago

@indykoning - The intention once this is merged is to ask you to do a formal release so that I can then add the Mix functionality to the HomeAssistant integration.

muppet3000 commented 3 years ago

@indykoning any chance we could get this merged in & released so I can integrate my Growatt system into Home Assistant?

muppet3000 commented 3 years ago

This fixes issue: https://github.com/indykoning/PyPi_GrowattServer/issues/5

indykoning commented 3 years ago

Absolutely, once the review has been solved i'll be happy to merge it!

muppet3000 commented 3 years ago

I think you need to approve it? If there's something you need me to do to get it ready please let me know.

muppet3000 commented 3 years ago

@indykoning I've investigated and you're correct it does work when you just pass the serial number in. However, the other functions I've added need the plant ID to be provided.

The latest change I have pushed modifies the ordering of the parameters and makes the plant_id for the mix_info function optional. Therefore, if it becomes a required parameter on the API in the future it should be easy for people to adopt rather than requiring another change to this library.

I have tested that it is backwards compatible and updated the documentation accordingly.

muppet3000 commented 3 years ago

@indykoning are you happy to merge this in now?

However, I've just found another API call that my app makes and I'd like to try adding it in as it may allow additional computations. I'm happy to add that to a future PR though if you're happy for this one to be merged in.

muppet3000 commented 3 years ago

@indykoning I've pushed my final changes now after doing more investigations on the correct values from my end. I've added the best documentation that I can manage for the api calls that I've added by trying to work out on the app where the values are presented. If I find more in the future I'll update them.

Please can you review, merge and release this soon so that I can get on with the home-assistant integration?

muppet3000 commented 3 years ago

@indykoning Please could you review this so that I can move on to the HomeAssistant integration?

muppet3000 commented 3 years ago

@indykoning latest changes from our discussion now pushed, no manual additions to the raw data from the API. I have moved the calculation to the example so that others can use the same method if they wish to.

muppet3000 commented 3 years ago

@indykoning are you happy with all the functionality now?

indykoning commented 3 years ago

At a first glance it looks good, i'll have a last check and will try to release it when i have my computer which may be tomorrow

muppet3000 commented 3 years ago

Thanks, sounds great. If you're able to release it tomorrow that would be amazing I could then start on the additions to HA integration over the weekend.

muppet3000 commented 3 years ago

@indykoning please don't merge it until you're going to release tomorrow, there's a very slim chance I've found the final statistic I've been looking for on the API. I need to verify it when I'm at my desk in about 2 hours time. If it's the stat I think it is then I'll add it in asap and give you an update.

UPDATE: I've found the final statistic that I was looking for, but I need some time today to add the function call properly and ensure it is properly documented. I will push a change later today. Sorry messing you around on this one.

muppet3000 commented 3 years ago

@indykoning I've now added that final api call and fully documented it. It's now down to you for final review, merge & release.

Thanks for waiting.

indykoning commented 3 years ago

It's looking good :tada: I'll merge it and i think i'll release it as a major version just in case. So it'll be 1.0.0 :rocket:

indykoning commented 3 years ago

It is available at: https://pypi.org/project/growattServer/1.0.0/

muppet3000 commented 3 years ago

Awesome, thanks @indykoning it's been fun working with you in this one. I'll probably start on the HomeAssistant integration tomorrow. Fun times!