-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v0.8.1 - Set battery reserve, operation mode #78
Conversation
* Added `get_mode()` function. * Added `set_battery_op_reserve()` function to set battery operation mode and/or reserve level. Likely won't work in the local mode. * Added basic validation for main class `__init__()` parameters (a.k.a. user input).
if r.status_code == 404: | ||
log.debug('404 Powerwall API not found at %s' % url) | ||
return None | ||
if 400 <= r.status_code < 500: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In local mode any attempt to send payload to /api/operation
results in 403. Should we treat it here as Unauthorized instead of re-attempting to re-login?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good idea.
pypowerwall/__init__.py
Outdated
self._check_if_dir_is_writable(dirname) | ||
|
||
@staticmethod | ||
def _check_if_dir_is_writable(dirpath): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be tested on Windows, if it causes issues we can fix it or remove the whole thing or the particular piece of it which causes issues.
pypowerwall/__init__.py
Outdated
return data['real_mode'] | ||
return None | ||
|
||
def set_battery_op_reserve(self, level: Optional[float] = None, mode: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions if the name of the function is too long/odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine, but we could abbreviate as set_operation()
since that is the Tesla API and it describes what it does, but I would like to see two interfaces to this that align to their "get" equivalents:
set_reserve(level)
set_mode(mode)
try: | ||
return json.dumps(response) | ||
except JSONDecodeError: | ||
log.error(f"Unable to dump response '{response}' as JSON. I know you asked for it, sorry.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're funny. 😂
This is great @emptywee !!!!
This still allows us to run this as a standalone script (in cloud mode) to change these settings (e.g. cron jobs or embedded in other scrips). Technically (read: Jason hasn't tried) a user of the library can instantiate two instances, one for local, one for cloud and use that one for control commands. Will a local mode user get enough of an error to indicate they need to swtich to cloud mode? We could add that as a log error. |
With some modifications to error handling, it can produce:
|
* Added `set_mode()` function. * Added `set_reserve()` function. * Handle 401/403 errors from Powerwall separately in local mode. * Handle 50x errors from Powerwall in local mode.
@jasonacox ok see 8d100fa |
|
OK, now I am not sure, does PW respond with 403 in local mode when session has expired? I'd respond with 401, but who knows? Do you know for sure @jasonacox ? Because your simulator uses 403 to force the example script to "re-login". If PW uses 403 for real to indicate an expired session, I'll have to adjust error handling in this case. |
Ugh, you are right. That's been the problem. It bounced between 401 and 403 with previous Firmware versions which is why we eventually went with the range. I haven't check to see what the current version is doing, but I suggest we keep the range or figure out some other logic. |
# Rate limited - Switch to cooldown mode for 5 minutes | ||
self.pwcooldown = time.perf_counter() + 300 | ||
log.error('429 Rate limited by Powerwall API at %s - Activating 5 minute cooldown' % url) | ||
# Serve up cached data if it exists | ||
# Serve up cached data if it exists (@emptywee: this doesn't look like we serve up cached data here?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment needs to be removed. If we hit this, we are at the TTL for the cached objects which is what the user expects us to do. After that TTL we should send fresh data or return None indicating something bad happened or no connection.
I'll test with my pw today, see how it behaves. |
Well, in case of token expiration, they respond with 401:
Should we adjust the simulation server to mirror this behavior if we want to test how the library re-authenticates? |
Yes, I'm good with that. |
Would you be able to build a new docker image for the pwsim? It is used in PR checks as P.S. I updated the pwsim |
Or maybe the workflow could build a new image and push it Docker Hub automatically each time or if pwsim code was changed, or something along these lines. |
I pushed the update for pwsimulator and it passes now. I've always wanted to investigate how to do the automated docker push, but haven't. I have a build script that does it with little effort and typically would only run this when we cut a release. docker buildx build --no-cache --platform linux/amd64,linux/arm64,linux/arm/v7 --push -t jasonacox/pwsimulator:${VER} . |
I'll test with my pw today, see how it behaves.
https://docs.github.com/en/actions/publishing-packages/publishing-docker-images this? |
Nice. I can make some little change to pwsim to test the new gh workflow :) |
..and test the new gh workflow
@jasonacox hmm, do you need to enable the new workflow somewhere? I didn't see it kicked off on my push to pwsimulator... or it has to be in the main branch? |
Oh, it ran in my/forked repo =) Weird. |
Ah, glad that you find it helpful and not irritating :) was a bit afraid of the latter! |
@jasonacox How's testing going? Ready to merge or not yet? |
I haven't had time to troubleshoot why the
|
Oh, absolutely. Thought you figured it out already and I was sitting duck waiting for you to merge it :P |
@jasonacox so, I looked at it and it's coming from the part which I didn't really touch, if you check the original code from 0.7.x, e.g.: pypowerwall/pypowerwall/cloud.py Lines 202 to 248 in 7b1fc45
|
You're amazing! Great find. Yes, the main cloud use up to this point was "read only" to get the metric data which is delayed in the cloud anyway, so it hasn't surfaced as an issue. However, with your great work here, we now have "write" functions that could change state that would not show as changed until the cache TTL expires. That doesn't seem ideal. In fact, I wonder if I am still good with merging this but will wait if you want to try to add the force feature first. |
alerts.append(alert) | ||
if grid_status.get('grid_services_active'): | ||
alerts.append('GridServicesActive') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emptywee - I'm slipped this in for 0.8.1 instead of cutting a new PR. 😁
@mcbirse - The data from grid_status represents some of what we lost but also adds other alerts showing power transition and grid services state. There are a few other booleans that indicate alert states that we may want to use (e.g. updating
which could be inverse as old FWUpdateSucceeded).
@@ -148,7 +152,8 @@ def get_value(a, key): | |||
try: | |||
pw = pypowerwall.Powerwall(host, password, email, timezone, cache_expire, | |||
timeout, pool_maxsize, siteid=siteid, | |||
authpath=authpath, authmode=authmode) | |||
authpath=authpath, authmode=authmode, | |||
cachefile=cachefile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the issue. The proxy was using the cachefile default (local directory .cachefile) which would not be writable. This has been an issue for a long time! It wouldn't surface because the service would just use the password to log in at restart. However, that creates more load on the Powerall.
@emptywee your init check code caught this! Love that! ❤️
I added some additional details to the init messages to help troubleshoot and identify. I kept hunting the authpath one but it was really the cachefile one.
Sure, adding the force flag should be trivial at this point. We can clear certain cache entries once "write" operation is complete. |
…le, invalidate respective cache data on write operations
@jasonacox I was thinking something like this c46b479 |
Nice @emptywee ! I'm testing... |
Ok, this is odd. When you set_reserve(), and then try get_reserve() it will return None from cache. Something is wrong with the cache logic. But, if you force=True, it does work.
|
Weird, I'll double check the caching condition there. Must be something odd there. |
@jasonacox yeah it was a silly thing. Can you check now? Sorry it took that long, I was taking a shower :) |
# Conflicts: # pypowerwall/local/pypowerwall_local.py
Calling it a night now, but it should work now as intended. |
It works!! Thanks @emptywee ! I'm testing |
✅ Tests look great. Thanks @emptywee !!! Great enhancements. |
Library: https://pypi.org/project/pypowerwall/0.8.1/ Proxy Container: jasonacox/pypowerwall:0.8.1t52 |
Woot woot! |
Brilliant work guys! Looks like you've both got all the testing covered... I'll test the new container at some point. Sorry I haven't had a chance to be involved and help with this - got a work project on that is currently sapping all my time and energy. |
No worries @mcbirse ! I understand that. :-) I'm going to cut a PR for Powerwall-Dashboard that includes our latest pypowerwall container. It adds a handful of new alerts and seems to be performing well. A few in the community have already started running the beta. A surprise to me was how much GridServicesActive flapps. I don't know what that may indicate but find it interesting. |
My stackstorm actions now got simplified :) woohoo! |
I'm not 100% certain, however I was under the impression that GridServicesActive is supposed to show when your Powerwall is participating in a VPP event... For it to be toggling so frequently however, could it be used to indicate something else as well, perhaps? |
get_mode()
function.set_battery_op_reserve()
function to set battery operation mode and/or reserve level. Likely won't work in the local mode.__init__()
parameters (a.k.a. user input).