jrester / tesla_powerwall

Python API for Tesla Powerwall
MIT License
73 stars 24 forks source link

Asyncify the API #55

Closed bubonicbob closed 8 months ago

bubonicbob commented 8 months ago

Change the API to use asyncio to be compatible with Home Assistant. This was discussed here.

bubonicbob commented 8 months ago

Please be kind, this is my first github contribution! :) Let me know how I can make this change better.

I don't have a powerwall myself so I haven't run the integration tests. Is there anybody who would be willing to help me out and try it?

ejbatts commented 8 months ago

Please be kind, this is my first github contribution! :) Let me know how I can make this change better.

I don't have a powerwall myself so I haven't run the integration tests. Is there anybody who would be willing to help me out and try it?

I can offer to assist with testing, but have no idea how. If you explain any procedure I will try.

bubonicbob commented 8 months ago

Please be kind, this is my first github contribution! :) Let me know how I can make this change better. I don't have a powerwall myself so I haven't run the integration tests. Is there anybody who would be willing to help me out and try it?

I can offer to assist with testing, but have no idea how. If you explain any procedure I will try.

Thanks! I'm on a mac, and I would run:

git clone https://github.com/jrester/tesla_powerwall.git
cd tesla_powerwall
gh pr checkout 55
POWERWALL_IP=youraddress POWERWALL_PASSWORD=yourpass tox -e integration

If you're missing any commands like gh and tox, run:

brew install gh tox
bubonicbob commented 8 months ago

Hey @dh99999 would you be able to help test out this pull request for me? Also could I get you to help me get refreshed fixture data. Seems like there has been some big changes in the data structure as compared to version 0.3.19... maybe this corresponds to updated firmware. I wonder why it isn't a breaking change since it deviates a lot from the fixture data in the powerwall HA integration.

ejbatts commented 8 months ago

If you're missing any commands like gh and tox, run:

brew install gh tox

ejbatts commented 8 months ago

I am on a PC, how do I install gh and tox? Brew is not recognised

bubonicbob commented 8 months ago

I am on a PC, how do I install gh and tox? Brew is not recognised

You'll have to find a guide to get python and pip installed. Then install tox.

I've never tried getting gh for windows, but perhaps it is available here? Else it might be easier to get this pull request via the github desktop app.

ejbatts commented 8 months ago

got to the last command, tox has installed correctly but cant be found???

admin@STUDY MINGW64 ~/tesla_powerwall (asyncify) $ powerwall_ip=192.168.1.41 powerwall_password=xxxxxx tox -e integration bash: tox: command not found

bubonicbob commented 8 months ago

got to the last command, tox has installed correctly but cant be found???

admin@STUDY MINGW64 ~/tesla_powerwall (asyncify) $ powerwall_ip=192.168.1.41 powerwall_password=xxxxxx tox -e integration bash: tox: command not found

Hmm, you can try this command and maybe the first line shows you the path to the tox binary?

pip show -f tox
ejbatts commented 8 months ago

Hmm, you can try this command and maybe the first line shows you the path to the tox binary?

pip show -f tox

Okay I can see the path but how do i incorporate that into the cmd

powerwall_ip=192.168.1.41 powerwall_password=xxxxxx tox -e integration

quozl commented 8 months ago

Had a quick look, and saw nothing obviously wrong. Here's what I thought of;

bubonicbob commented 8 months ago

Hmm, you can try this command and maybe the first line shows you the path to the tox binary?

pip show -f tox

Okay I can see the path but how do i incorporate that into the cmd

powerwall_ip=192.168.1.41 powerwall_password=xxxxxx tox -e integration

POWERWALL_IP and POWERWALL_PASSWORD are just two individual environment variables (btw I bet these are case-sensitive so use all caps). I gave you a fancy way that bash can supply the variables temporarily at the same time, but you can follow guides for bash, powershell, or command prompt to just set the variables by themselves so that they take effect when you run tox alone later in your shell.

It is a good habit to not set passwords into your environment so don't go out of your way to make these global settings, or if you really want to make sure, set your POWERWALL_PASSWORD to something bogus when you're done.

After setting those two variables for your specific shell, you can just run tox by itself:

tox -e integration
bubonicbob commented 8 months ago

Had a quick look, and saw nothing obviously wrong. Here's what I thought of;

  • if requests is no longer needed, remove from requirements?
  • will merging this break all code that uses this module? i.e. force asyncify of others.

Thank you! Yes merging this will break anything that depends on this library. In my limited experience, it seems typical in python-land that libraries in this situation just hard fork into something with an 'a' in front of the original name and don't bother with maintaining a hybrid api. In the case of this specific library, there's support for taking the plunge and moving the API over.

@jrester since this is a major API change, should the version change to 0.5.0? Not sure the official way to do that.

bubonicbob commented 8 months ago

@bubonicbob Thank you very much for your PR! I think it already looks quite good, just some minor points that I would like to address before merging it. Additionally, I will also do some testing of your implementation soon, to see if everything works as expected.

Nevertheless, I am quite unsure, about the use of aresponses for testing, as the project seems to be inactive for over two years now and aiohttp looks like it already has good support for testing. I would prefer the direct use of aiohttp, but I would see this more as a future improvement, than a requirement for this PR, as it might otherwise blow the scope. If you want to take a look, I would be happy to also merge this, otherwise I will take care of this in the future.

Much appreciated! I'm pretty new to python so I don't know what are the popular choices for testing. I picked aresponses since the tesla-wall-connector similar library used it. I was confused even more to learn that HA wants to stop using unittest.TestCase style tests. It is a jungle out there. 🤷

While you're testing, I'd really appreciate help updating the fixture data for the HA integration tests. I've made a draft pull request to use this library and the fixture data breaks tests over there. I saw these samples from a similar project but still the powerwall firmware is a few years old.

ejbatts commented 8 months ago

I got tox to run by exporting the PATH using: export PATH=$PATH:/c/users/admin/AppData/Roaming/Python/Python312/scripts I note that tox.exe was located in 'scripts' and not 'site-packages' as suggested by previous analysis. Having now successfully run the evaluation I have received the following summary: integration: FAIL code 1 (11.91 seconds) evaluation failed :( (12.05 seconds) It appears that the error lies here: _integration: installdeps> python -I -m pip install aresponses error: subprocess-exited-with-error And here: _Python recognizes 'multidict.multilib' as an importable package[^1], but it is absent from setuptools' packages configuration.

_This leads to an ambiguous overall configuration. If you want to distribute this package, please make sure that 'multidict.multilib' is explicitly added to the packages configuration field.

....so I am looking for more help please.

bubonicbob commented 8 months ago

multidict

Weird. The multidict package should have been installed automatically because there is a reference to aiohttp as a dependency. I wonder if pip is having some issue fetching the dependencies? Try running (from the root tesla_powerwall directory):

tox exec -- pip show aresponses

You can manually run the pip install for it like this:

tox exec -- pip install aresponses
tox exec -- pip install multidict
ejbatts commented 8 months ago

Tried both of the above but failed. Verbose output: error: subprocess-exited-with-error

[removed verbose output SEE BELOW]

ejbatts commented 8 months ago

I am installing Microsoft Visual C++ 14.3 now to see if that is the culprit.

EDIT: now the test runs okay but failed due to login error, need to resolve that now. EDIT: Login error resolved

ejbatts commented 8 months ago

Test still fails :(

ejbatts commented 8 months ago

Attached output from last test:

Verbose Output.txt

bubonicbob commented 8 months ago

Attached output from last test:

Verbose Output.txt

Thank you so much for collecting this! Looks like maybe the tests are overwhelming your gateway, at least after the first test failed with a timeout. I wonder if restarting the tests just once after waiting for some time would change the results or at least reveal the original bug that needs to be fixed.

jrester commented 8 months ago

@jrester since this is a major API change, should the version change to 0.5.0? Not sure the official way to do that.

@bubonicbob Yes, after the PR is merged, I will bump the version and create a new release.

will merging this break all code that uses this module? i.e. force asyncify of others.

@quozl Yes, this will require all clients to also use async. But I believe that this change to async is good, especially since home-assistant is one of the main use cases for this library. Nevertheless, there are other use-cases, especially simple scripts for which using async might be overkill. For those use-cases, I would like to provide a sync API in the future, but I have still to do some research, what implemention is the best practice here. The two ideas I had are:

bubonicbob commented 8 months ago

Attached output from last test:

Verbose Output.txt

@ejbatts Can you try again but this time just run a single test so it doesn't overwhelm your gateway?

Run this first to get most recent code changes:

gh pr checkout 55

This runs an individual test:

tox -e integration -x testenv:integration.commands="python -m  unittest tests/integration/test_powerwall.py -k test_get_charge"
ejbatts commented 8 months ago

@bubonicbob ran test as per above and this seems to be a new error:

tesla_powerwall.error.AccessDeniedError: Access denied for resource https://192.168.1.41/api/system_status/soe: Unable to GET to resource: User does not have adequate access rights

Verbose Output2.txt

The comprehensive test using POWERWALL_IP=youraddress POWERWALL_PASSWORD=yourpass tox -e integration results in the same access error for all tests.

For clarification, i am running these tests within the Git Bash app for Windows, and I have two PW2 batteries connected to a single Gateway.

bubonicbob commented 8 months ago

@bubonicbob ran test as per above and this seems to be a new error:

tesla_powerwall.error.AccessDeniedError: Access denied for resource https://192.168.1.41/api/system_status/soe: Unable to GET to resource: User does not have adequate access rights

Verbose Output2.txt

The comprehensive test using POWERWALL_IP=youraddress POWERWALL_PASSWORD=yourpass tox -e integration results in the same access error for all tests.

For clarification, i am running these tests within the Git Bash app for Windows, and I have two PW2 batteries connected to a single Gateway.

Ok let's try doing the same test without my changes.

First switch to another pull request that doesn't have these changes:

gh pr checkout 56

Then try running the same single test again. Better not to run the full test suite until at least one passes.

ejbatts commented 8 months ago

@bubonicbob If I enter "192.168.1.xx/api/system_status" into my browser whilst logged on as customer (basic login credentials as per these test procedures) I receive the same Access Denied error. However, if I enter the same URL whilst logged on as the installer, it returns the system status information. The problem is that logging on as an installer requires physical toggling of a battery switch, not the simple input of a password.

ejbatts commented 8 months ago

@bubonicbob I think this is a PASS?


integration: recreate env because requirements removed: aresponses integration: remove tox env folder C:\Users\admin\tesla_powerwall.tox\integration integration: install_deps> python -I -m pip install responses .pkg: _optional_hooks> python C:\Users\admin\AppData\Roaming\Python\Python312\site-packages\pyproject_api_backend.py True setuptools.build_meta .pkg: get_requires_for_build_sdist> python C:\Users\admin\AppData\Roaming\Python\Python312\site-packages\pyproject_api_backend.py True setuptools.build_meta .pkg: build_sdist> python C:\Users\admin\AppData\Roaming\Python\Python312\site-packages\pyproject_api_backend.py True setuptools.build_meta integration: install_package_deps> python -I -m pip install requests>=2.22.0 integration: install_package> python -I -m pip install --force-reinstall --no-deps C:\Users\admin\tesla_powerwall.tox.tmp\package\17\tesla_powerwall-0.4.0.tar.gz integration: commands[0]> python -m unittest tests/integration/test_powerwall.py -k test_get_charge

Ran 1 test in 0.902s

OK .pkg: _exit> python C:\Users\admin\AppData\Roaming\Python\Python312\site-packages\pyproject_api_backend.py True setuptools.build_meta integration: OK (17.80=setup[14.80]+cmd[3.00] seconds) congratulations :) (17.95 seconds)

ejbatts commented 8 months ago

Just ran

gh pr checkout 56
POWERWALL_IP=youraddress POWERWALL_PASSWORD=yourpass tox -e integration

and got a PASS on 10 tests. The branch is (fix_integration), not (asyncify).

Switched back to asyncify and received the access errors again.

bubonicbob commented 8 months ago

Just ran gh pr checkout 56 POWERWALL_IP=youraddress POWERWALL_PASSWORD=yourpass tox -e integration and got a PASS on 10 tests. The branch is (fix_integration), not (asyncify).

Switched back to asyncify and received the access errors again.

I just updated the PR with a little extra to verify your authentication cookie in the integration tests. If you pull it again you should get that update. Please post updated test results with that change. If it isn't authenticating and storing the results in a cookie, I'm not sure how to make progress. I'll have to get help from another dev willing to troubleshoot it with a live powerwall.

ejbatts commented 8 months ago

Now returns an "AssertionError" for every test.

Traceback (most recent call last): File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\unittest\async_case.py", line 87, in _callSetUp self._callAsync(self.asyncSetUp) File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\unittest\async_case.py", line 104, in _callAsync return self._asyncioRunner.run( ^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\asyncio\runners.py", line 118, in run return self._loop.run_until_complete(task) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\asyncio\base_events.py", line 684, in run_until_complete return future.result() ^^^^^^^^^^^^^^^ File "C:\Users\admin\tesla_powerwall\tests\integration\test_powerwall.py", line 20, in asyncSetUp assert self.powerwall.is_authenticated() AssertionError

bubonicbob commented 8 months ago

Now returns an "AssertionError" for every test.

Traceback (most recent call last): File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\unittest\async_case.py", line 87, in _callSetUp self._callAsync(self.asyncSetUp) File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\unittest\async_case.py", line 104, in _callAsync return self._asyncioRunner.run( ^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\asyncio\runners.py", line 118, in run return self._loop.run_until_complete(task) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "C:\Users\admin\AppData\Local\Programs\Python\Python312\Lib\asyncio\base_events.py", line 684, in run_until_complete return future.result() ^^^^^^^^^^^^^^^ File "C:\Users\admin\tesla_powerwall\tests\integration\test_powerwall.py", line 20, in asyncSetUp assert self.powerwall.is_authenticated() AssertionError

@jrester Looks like I'm going to need help here. For some reason the aiohttp session isn't storing the auth cookie from the successful login request. @ejbatts testing showed that it works fine in the original requests based login but the same request in the aiohttp POST isn't storing the cookie. I can't see anything in the documentation that suggests the default cookie jar behavior would be different between them and I don't see what would be different between how the code calls into the apis.

Not sure what else to try on my end without having a powerwall available to me.

ejbatts commented 8 months ago

Looks like I'm going to need help here. For some reason the aiohttp session isn't storing the auth cookie from the successful login request. @ejbatts testing showed that it works fine in the original requests based login but the same request in the aiohttp POST isn't storing the cookie.

Just to be sure @bubonicbob , It appears to not be a Access Error anymore, but an Assertion Error. I have attached the output from the single test.

Verbose Output3.txt

bubonicbob commented 8 months ago

Looks like I'm going to need help here. For some reason the aiohttp session isn't storing the auth cookie from the successful login request. @ejbatts testing showed that it works fine in the original requests based login but the same request in the aiohttp POST isn't storing the cookie.

Just to be sure @bubonicbob , It appears to not be a Access Error anymore, but an Assertion Error. I have attached the output from the single test.

Verbose Output3.txt

Yes this new error makes sense since I added a new check to see if the auth cookie had been stored successfully. Seems to not have been stored which would have caused the unauthorized error you had seen before I made this change. Without the cookie it would have been like you had never logged in.

Your test against master proved that your gateway is sending this cookie. I'm at a loss as to what's going on.

If you want to double check what your gateway is doing, you can always use this command... assuming you have curl available in your mingw.

curl -v -X POST -H "Content-Type: application/json" -d "{\"username\":\"customer\",\"password\":\"$POWERWALL_PASSWORD\", \"email\":\"\",\"force_sm_off\":false}" "https://$POWERWALL_IP/api/login/Basic"

Just don't post those results without scrubbing out any secrets.

bubonicbob commented 8 months ago

Looks like I'm going to need help here. For some reason the aiohttp session isn't storing the auth cookie from the successful login request. @ejbatts testing showed that it works fine in the original requests based login but the same request in the aiohttp POST isn't storing the cookie.

Just to be sure @bubonicbob , It appears to not be a Access Error anymore, but an Assertion Error. I have attached the output from the single test.

Verbose Output3.txt

@ejbatts I think I figured it out. The issue is that, by default, aiohttp ignores cookies when your POWERWALL_IP is an IP address instead of a DNS name. If you pull the latest updates, I think it will start working.

https://docs.aiohttp.org/en/v3.7.3/client_advanced.html#cookie-safety

ejbatts commented 8 months ago

Just ran gh pr checkout 55 and the individual test...same result...

File "C:\Users\admin\tesla_powerwall\tests\integration\test_powerwall.py", line 20, in asyncSetUp assert self.powerwall.is_authenticated() AssertionError

bubonicbob commented 8 months ago

Just ran gh pr checkout 55 and the individual test...same result...

File "C:\Users\admin\tesla_powerwall\tests\integration\test_powerwall.py", line 20, in asyncSetUp assert self.powerwall.is_authenticated() AssertionError

My fault, please try one more time.

ejbatts commented 8 months ago

My fault, please try one more time.

SUCCESS!

Ran 10 tests in 27.502s

OK .pkg: _exit> python C:\Users\admin\AppData\Roaming\Python\Python312\site-packages\pyproject_api_backend.py True setuptools.build_meta integration: OK (36.28=setup[8.34]+cmd[27.94] seconds) congratulations :) (36.45 seconds)

bubonicbob commented 8 months ago

@ejbatts

@ejbatts Sweet. I owe you a beer next time you're in Seattle. 🍻

bubonicbob commented 8 months ago

@jrester Sorry for the spammy comments on this. It is ready to land: both integration and unit tests pass. Don't think I have permissions to actually merge it to master.

ejbatts commented 8 months ago

Will the changed integration allow me to access individual battery data? The essential thing I want is BATTERY POWER (charge/discharge rate) as individual entities and not the combined value that Tesla provides as default.

bubonicbob commented 8 months ago

Will the changed integration allow me to access individual battery data? The essential thing I want is BATTERY POWER (charge/discharge rate) as individual entities and not the combined value that Tesla provides as default.

Create an issue on this repo for that and we can all discuss there. Instructions.

ejbatts commented 8 months ago

Oh okay. I actually thought this testing was heading in that direction after I posted this on https://github.com/home-assistant/core/issues/100065#top thread.

I would like a similar feature but the opposite way round. Is it possible to extract data from separate powerwalls attached to a single gateway? I have 2 powerwalls connected to separate phases of a 3 phase installation. Specifically, I would like to know battery load (charge/discharge rate) of each battery so I can monitor house loads on each phase. This separate data is available on //[gateway ip address]/system page, as well as voltage and state of charge, but I don't know how to extract it in HA.

I'll raise a blank issue.

EDIT: DONE!

bdraco commented 8 months ago

Everything seems to be working great after I small fixes I added to https://github.com/home-assistant/core/pull/107164

bdraco commented 8 months ago

Pretty significant drop in cpu usage for the HA integration observed as well

bdraco commented 8 months ago

There's still a lot of JSON being passed around, which is what's mostly left of the CPU runtime, using orjson to parse would probably solve that, but that's for another PR.

bdraco commented 8 months ago

https://github.com/bdraco/nexia/blob/ee3253ab8cf1f128b700e59bb09354e21612851e/nexia/home.py#L244

bubonicbob commented 8 months ago

There's still a lot of JSON being passed around, which is what's mostly left of the CPU runtime, using orjson to parse would probably solve that, but that's for another PR.

Added an issue for it.

bdraco commented 8 months ago

I think this looks ready for a release once the typing comments and the aiohttp session comments are sorted

The HA PR looks like its good to go after this is released and the tests are updated

bubonicbob commented 8 months ago

I think this looks ready for a release once the typing comments and the aiohttp session comments are sorted

The HA PR looks like its good to go after this is released and the tests are updated

I'm ready to get this released. I don't think I'm able to do this myself.

bdraco commented 8 months ago

Retested with latest code. Everything seems to be working as expected