Asyncify the API #55

bubonicbob · 2024-01-03T18:35:35Z

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

bubonicbob · 2024-01-03T18:37:06Z

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?

…y, I shouldn't have removed the logic to automatically create an http client session and the close method. Adds this functionality back.

ejbatts · 2024-01-03T19:52:12Z

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 · 2024-01-03T20:08:24Z

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 · 2024-01-04T00:13:41Z

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 · 2024-01-04T04:57:10Z


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

brew install gh tox

ejbatts · 2024-01-04T04:58:01Z

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

bubonicbob · 2024-01-04T05:08:46Z

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 · 2024-01-04T06:08:56Z

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 · 2024-01-04T06:31:33Z

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

jrester

@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.

tesla_powerwall/powerwall.py

pyproject.toml

tests/unit/test_powerwall.py

ejbatts · 2024-01-04T07:13:54Z

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 · 2024-01-04T07:47:21Z

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.

bubonicbob · 2024-01-04T15:02:33Z

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

…ew feedback.

bubonicbob · 2024-01-04T15:23:29Z

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 · 2024-01-04T15:37:27Z

@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 · 2024-01-04T20:58:16Z

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: install_deps> 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 · 2024-01-04T21:16:31Z

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 · 2024-01-04T22:07:38Z

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

[removed verbose output SEE BELOW]

ejbatts · 2024-01-04T22:15:04Z

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 · 2024-01-04T22:57:12Z

Test still fails :(

ejbatts · 2024-01-04T23:44:21Z

Attached output from last test:

Verbose Output.txt

bubonicbob · 2024-01-06T00:10:43Z

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 · 2024-01-06T00:46:46Z

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 · 2024-01-06T00:56:58Z

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

bubonicbob · 2024-01-06T01:02:46Z

@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 · 2024-01-06T01:15:29Z

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 · 2024-01-06T01:21:17Z

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 · 2024-01-06T02:08:40Z

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!

tesla_powerwall/api.py

tesla_powerwall/powerwall.py

tesla_powerwall/api.py

bdraco · 2024-01-07T20:33:22Z

Everything seems to be working great after I small fixes I added to home-assistant/core#107164

bdraco · 2024-01-07T20:54:53Z

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

bdraco · 2024-01-07T21:27:54Z

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 · 2024-01-07T21:30:50Z

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

bubonicbob · 2024-01-08T00:33:09Z

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 · 2024-01-08T00:38:01Z

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

…ated with "unsafe" cookies

bubonicbob · 2024-01-08T01:46:38Z

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.

tox.ini

bdraco · 2024-01-10T01:06:02Z

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

jrester · 2024-01-10T10:18:09Z

Thank you very much @bubonicbob for implementing this. Also thank you very much @bdraco, @ejbatts and @quozl for testing and reviewing!

jrester · 2024-01-10T10:31:24Z

Version 0.5.0 is now live on PyPi

quozl · 2024-01-10T12:45:58Z

No worries. Well done everyone.

Josh added 2 commits January 3, 2024 10:17

Change API to be async

a0f6036

Modify docs

3fb09f0

bubonicbob mentioned this pull request Jan 3, 2024

PowerWall integration does not support more than one gateway connected to the same set of power walls home-assistant/core#100065

Closed

After looking at the Home Assistant integration that uses this librar…

d987e9c

…y, I shouldn't have removed the logic to automatically create an http client session and the close method. Adds this functionality back.

Fix integration test

a74dc7b

jrester requested changes Jan 4, 2024

View reviewed changes

tesla_powerwall/powerwall.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

tests/unit/test_powerwall.py Outdated Show resolved Hide resolved

tests/unit/test_powerwall.py Outdated Show resolved Hide resolved

Based on review feedback, remove dependency on requests and misc revi…

bd07ea7

…ew feedback.

bubonicbob requested a review from jrester January 4, 2024 18:13

bubonicbob closed this Jan 4, 2024

bubonicbob reopened this Jan 4, 2024

Move cookie fix to the integration test

4331e9f

bdraco reviewed Jan 7, 2024

View reviewed changes

tesla_powerwall/api.py Outdated Show resolved Hide resolved

bdraco reviewed Jan 7, 2024

View reviewed changes

tesla_powerwall/powerwall.py Outdated Show resolved Hide resolved

bdraco reviewed Jan 7, 2024

View reviewed changes

tesla_powerwall/powerwall.py Outdated Show resolved Hide resolved

bdraco reviewed Jan 7, 2024

View reviewed changes

tesla_powerwall/api.py Outdated Show resolved Hide resolved

Address review feedback with typing and make default http session cre…

f6a8392

…ated with "unsafe" cookies

jrester requested changes Jan 8, 2024

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

Remove tox linting and type checking

59cbffc

bubonicbob requested a review from jrester January 8, 2024 18:23

jrester approved these changes Jan 10, 2024

View reviewed changes

jrester merged commit aa75e6f into jrester:master Jan 10, 2024

jrester mentioned this pull request Jan 10, 2024

Fix error running integration tests #56

Closed

Asyncify the API #55

Asyncify the API #55

Conversation

bubonicbob commented Jan 3, 2024

bubonicbob commented Jan 3, 2024

ejbatts commented Jan 3, 2024

bubonicbob commented Jan 3, 2024 • edited Loading

bubonicbob commented Jan 4, 2024

ejbatts commented Jan 4, 2024 • edited Loading

ejbatts commented Jan 4, 2024

bubonicbob commented Jan 4, 2024

ejbatts commented Jan 4, 2024

bubonicbob commented Jan 4, 2024

jrester left a comment • edited Loading

Choose a reason for hiding this comment

ejbatts commented Jan 4, 2024

quozl commented Jan 4, 2024

bubonicbob commented Jan 4, 2024

bubonicbob commented Jan 4, 2024

bubonicbob commented Jan 4, 2024 • edited Loading

ejbatts commented Jan 4, 2024 • edited Loading

bubonicbob commented Jan 4, 2024

ejbatts commented Jan 4, 2024 • edited Loading

ejbatts commented Jan 4, 2024 • edited Loading

ejbatts commented Jan 4, 2024

ejbatts commented Jan 4, 2024

bubonicbob commented Jan 6, 2024

ejbatts commented Jan 6, 2024

bubonicbob commented Jan 6, 2024

bubonicbob commented Jan 6, 2024

ejbatts commented Jan 6, 2024

bubonicbob commented Jan 6, 2024

ejbatts commented Jan 6, 2024 • edited Loading

bdraco commented Jan 7, 2024

bdraco commented Jan 7, 2024

bdraco commented Jan 7, 2024

bdraco commented Jan 7, 2024

bubonicbob commented Jan 8, 2024

bdraco commented Jan 8, 2024

bubonicbob commented Jan 8, 2024

bdraco commented Jan 10, 2024

jrester commented Jan 10, 2024

jrester commented Jan 10, 2024

quozl commented Jan 10, 2024 via email

bubonicbob commented Jan 3, 2024 •

edited

Loading

ejbatts commented Jan 4, 2024 •

edited

Loading

jrester left a comment •

edited

Loading

bubonicbob commented Jan 4, 2024 •

edited

Loading

ejbatts commented Jan 4, 2024 •

edited

Loading

ejbatts commented Jan 4, 2024 •

edited

Loading

ejbatts commented Jan 4, 2024 •

edited

Loading

ejbatts commented Jan 6, 2024 •

edited

Loading