0.8.0 Refactoring code #77
Conversation
* Refactor code * Clean up * Make code more pythonish Co-authored-by: Igor Cherkaev <igor.cherkaev@copart.com>
|
Oh, oops, didn't know |
|
🤯 !! You did this today?! Amazing @emptywee ! A lot to review but I like the style consistency and the So far, I like what I see. As you mention, we should look at the breaking changes and call them out. Also, proxy is not running. I will probably start with that and back in to what is breaking (first pass looks like it is related to missing |
Yeah :) It's not much, tbh, just moved some code around, so that we can work on it further later.
Oopsie, I am not super familiar with the project and all moving parts :( I'll look into the proxy piece, too. |
align env vars of the example scripts. minor bugfixes, linting, etc
|
Fixed the proxy piece to be compatible with the new structure of the library. Tested on my local - worked well with both local powerwall and cloud access. |
|
Also, I saw some kind of an effort to support py2? I think we can remove it unless it's required. We don't mention it on pypi, we don't run any tests with it either. If we have to support py2, I'll have to drop all the type hints I've added with this PR. @jasonacox what do you think? |
Yes, we can remove py2 support. That's just legacy code. We also assert Python 3 in our setup.py Line 24 in 7b1fc45 |
|
Great, I'll remove it then as part of this code refresh. Thanks. |
* remove python2 legacy compatibility code from the scanner (`scan.py`) * make `tessolarcharge.py` look more pythonic
|
Removed py2 stuff, looks like it was only one script there. |
* getting rid of hundreds of if-elif-elif-elif in the cloud poll function
|
@jasonacox what do you think about 1ccfbc3 ? If we get rid of tens of if-elif-elif-elif in the poll function there? Not very cryptic, I hope :) |
| "/api/devices/vitals": self.get_api_devices_vitals, | ||
| "/api/meters/aggregates": self.get_api_meters_aggregates, | ||
| "/api/operation": self.get_api_operation, | ||
| "/api/site_info": self.get_api_site_info, | ||
| "/api/site_info/site_name": self.get_api_site_info_site_name, | ||
| "/api/status": self.get_api_status, | ||
| "/api/system_status": self.get_api_system_status, | ||
| "/api/system_status/grid_status": self.get_api_system_status_grid_status, | ||
| "/api/system_status/soe": self.get_api_system_status_soe, | ||
| "/vitals": self.get_vitals, | ||
| # Possible Actions | ||
| "/api/login/Basic": self.api_login_basic, | ||
| "/api/logout": self.api_logout, | ||
| # Mock Actions | ||
| "/api/auth/toggle/supported": self.get_api_auth_toggle_supported, | ||
| "/api/customer": self.get_api_customer, | ||
| "/api/customer/registration": self.get_api_customer_registration, | ||
| "/api/installer": self.get_api_installer, | ||
| "/api/meters": self.get_api_meters, | ||
| "/api/meters/readings": self.get_api_unimplemented_timeout, | ||
| "/api/meters/site": self.get_api_meters_site, | ||
| "/api/meters/solar": self.get_unimplemented_api, | ||
| "/api/networks": self.get_api_unimplemented_timeout, | ||
| "/api/powerwalls": self.get_api_powerwalls, | ||
| "/api/site_info/grid_codes": self.get_api_unimplemented_timeout, | ||
| "/api/sitemaster": self.get_api_sitemaster, | ||
| "/api/solar_powerwall": self.get_api_solar_powerwall, | ||
| "/api/solars": self.get_api_solars, | ||
| "/api/solars/brands": self.get_api_solars_brands, | ||
| "/api/synchrometer/ct_voltage_references": self.get_api_synchrometer_ct_voltage_references, | ||
| "/api/system/update/status": self.get_api_system_update_status, | ||
| "/api/system_status/grid_faults": self.get_api_system_status_grid_faults, | ||
| "/api/troubleshooting/problems": self.get_api_troubleshooting_problems, |
There was a problem hiding this comment.
Instead of if/elif/elif/elif/.../else we define a map with api -> function and then use it in the self.poll() function, making it compact and easy to extend.
There was a problem hiding this comment.
I have mixed feeling about that. I have started using that method for many of my smaller sets and find it cluttered and harder to read, especially for longer lists. I notice CoPilot prefers this.
A better option would be to move to routers (ala flask) and perhaps consider moving to asyncio / uvicorn. That would be a slightly bigger lift. :)
There was a problem hiding this comment.
Continued... To be fair... willing to entertain it. I've been moving to this in other projects. Do we have any performance data or other indicators that this is a preferred path?
There was a problem hiding this comment.
Likely, it'll be faster to get a pointer to the required function with a key in the dictionary versus traversing all if and elif, but to me it also looks compact and more readable. As you may have noticed already I like to have small and short functions, I prefer those that fit on one screen :)
There was a problem hiding this comment.
A better option would be to move to routers (ala flask) and perhaps consider moving to asyncio / uvicorn. That would be a slightly bigger lift. :)
Definitely, that's why we should take this big first step to decouple big functions into smaller class methods first, then we'll be able to transition into something else.
Also, once this is all reviewed and ironed out, we'll be able to plug in other tesla libraries if anything better comes into play. All we'll need to do is implement a few required methods. And then let users choose which way to interact with their powerwall/tesla gateway. But first, need to get rid of hard-coded logic here and there and make the whole thing a little bit more flexible, in my opinion.
There was a problem hiding this comment.
Fair point. Adding support for the official Tesla FleetAPI is a perfect example. Sold. ;)
| data = API_METERS_AGGREGATES_STUB | ||
| data['site'].update({ | ||
| "last_communication_time": timestamp, | ||
| "instant_power": grid_power, | ||
| }) | ||
| data['battery'].update({ | ||
| "last_communication_time": timestamp, | ||
| "instant_power": battery_power, | ||
| "num_meters_aggregated": battery_count, | ||
| }) | ||
| data['load'].update({ | ||
| "last_communication_time": timestamp, | ||
| "instant_power": load_power, | ||
|
|
||
| }) | ||
| data['solar'].update({ | ||
| "last_communication_time": timestamp, | ||
| "instant_power": solar_power, | ||
| "num_meters_aggregated": solar_inverters, | ||
| }) |
There was a problem hiding this comment.
Moved large dictionaries to a separate file with stubs and just update the fields we have information for.
|
|
||
| return data | ||
|
|
||
| @not_implemented_mock_data |
There was a problem hiding this comment.
Added a decorator here that warns users that fake/mock data is returned
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| # Global Variables | |||
| AUTHFILE = ".pypowerwall.auth" | |||
| authpath = os.getenv("PW_AUTHPATH", "") | |||
| authpath = os.getenv("PW_AUTH_PATH", "") | |||
There was a problem hiding this comment.
That was it! The setup works now.
There was a problem hiding this comment.
Using: jasonacox/pypowerwall:0.8.0t51-beta2
There was a problem hiding this comment.
Yeah, sorry, my bad, I mechanically replaced it as if that was added by me for local testing =(
|
Doing my final testing. @mcbirse if you have time, check to see if |
|
Do we really need this or perhaps move this to DEBUG? This is going to be too noisy for cloud mode users of the proxy. |
Hmm, what if we print it once and suppress further? |
|
Why do we feed proxy users in cloud mode with mock data, btw? |
Actually, I think it would be better if it was included in the DEBUG line with the API information. Here is a snapshot of logs with a basic dashboard running (power flow animation): We could just decorate the DEBUG line with "[mock data]" qualifier and turn into
The intent of cloud mode is to simulate local mode as a drop-in replacement. Tools built for local mode access to powerwall can use the same API even if their system doesn't have a local API (e.g. Powerwall 3). |
|
@jasonacox |
|
That works! Proxy logs now using Tested on: |
|
New proxy container built with latest commit: jasonacox/pypowerwall:0.8.0t51-beta4 Functions tested ( import pypowerwall
pw = pypowerwall.Powerwall(host,password,email,timezone)
pw.poll()
pw.level()
pw.power()
pw.site()
pw.solar()
pw.battery()
pw.load()
pw.grid()
pw.home()
pw.vitals()
pw.strings()
pw.din()
pw.uptime()
pw.version()
pw.status()
pw.site_name()
pw.temps()
pw.alerts()
pw.system_status()
pw.battery_blocks()
pw.grid_status()
pw.is_connected()
pw.get_reserve()
pw.get_time_remaining() |
|
Kind like how it's working out :) |
Happy to help! Do you want to keep it as something that only polls data? I've already extended the 0.7.x version of your library with a post method to change battery reserve settings, if you are cool with it, I'll open a PR when I adapt it for 0.8.0. Unfortunately, local mode doesn't allow you to control battery reserve threshold anymore (starting from some firmware), but it is what it is. |
Yes! ❤️ We have the external tools (scripts for reserve and mode) but there is no reason we couldn't include it in pypowerwall. It makes more sense with the cloud support now. Also, we can make it work via command line, expanding on your argparse work. ;) |
Sweet! But for now let's focus on merging this in and let folks test it in the wild :) |
@emptywee and @jasonacox - great work on this! I have re-tested and no longer have an exception raised for strings or logging spam from that. Functionally all appears to be working well, in both local and cloud mode. I did note the following though, and am wondering if it should be handled differently? On my Powerwall, requests to Perhaps it should be treated like the "vitals" call and give an error once, then get disabled? Not sure. I did also test the api call directly to the TEG installer WiFi, to see if I get any data, which you can see in the logs below (I have bridged 192.168.91.1 onto my local LAN, no headache of multi-homed host setup 😉 ). Not a big issue, but the calls telegraf makes to /strings and /alerts/pw means it will show in the logs all the time if DEBUG is enabled, example below. Kind of seems like an error/notification you would want to see only once like with vitals? Thoughts? BTW because of the lack of the solar_powerwalls api, I do not get any alerts in local mode (not even mock alerts like I do in cloud mode such as the SystemConnectedToGrid). All good to merge either way - seems to be working well. 👍 |
|
@mcbirse your setup seems a lot like mine. I've got a tesla gateway, enphase micro inverters, powerwall2 batteries. I'm getting same empty responses here and there with 200 OK status codes. I think we could discuss how the library should behave and treat them in the next iteration of changes. Maybe even introduce a hybrid mode? Where it goes for data to cloud if not available in local, all being automatically and transparent for users? Just a thought. |
|
Thanks @mcbirse ! We could definitely add some Alerts. I started an issue jasonacox/Powerwall-Dashboard#457 Also, in the past, I had a few versions with memory leaks. This release looks good. 
Past 24 hours. Here we go... 0.8.0 🚀 |
|
Hooray! We finally did it! I'll drink to that! |
|
Thanks again @emptywee !! Yes, Cheers 🍻 Thanks for the great work. 🙏
|
@jasonacox I know it's a little too much to review, but what do you think if we go this way as the first round of refactoring?
It's a little hard to sync it with upstream changes, but if we transition to this structure, it'll be easy to both continue maintaining the current code and refactoring it further.
I noticed that meaning of the
jsonformatparam was inconsistent across the board, so I fixed that as well, but it might break things for any users of the library if they used the methods directly. Mainly it was thepoll()method, which treated the param the other way around.I also asked about code style guide or anything along those lines, you didn't see it I guess :) So I made it look a bit more Python-ish, if you don't mind.
Please, let me know what you think.