Generalize API and remove home-assistant specific architecture #24
Comments
|
@alandtse i'm not sure about moving into HA as an initial step. I would like to suggest next plan:
What do you think? |
|
That seems fine. I would suggest that breaking changes (e.g., #23) all be saved for step 5. I'll get started on it. |
|
Just tossing in a note here that I wholeheartedly agree with this plan. When I went to work on this component a few months back I ran in to the exact same confusion, where there was a bunch of HA-specific logic sitting in this library. |
|
Just to track my thinking, I think I'm going to adopt a lot of structure from simplisafe-python which seems quite mature (e.g., tests, documentation, linting) and also has an async based HA component that seems quite developed per the integration quality scale. For example, they were one of the first to rely on the config_flow mechanism. |
|
Hi folks. I just wanted to chime in and let you know I'm available to help and contribute. I took the standard Tesla component and loaded into my setup as custom with a few modifications to suit my needs, for instance, I implemented several services to use in automations (and disabled "should_poll" to let my car sleep). But as @alandtse mentioned, the HA code in here prevents me to further modify my setup as needed. What's the current status on the generalization? |
|
Not much has happened on generalizing the code. The only major change was the migration to async. The biggest issue with moving any code into HA out of this library is HA has a very long review process. The easiest path is probably to move the HA code into its own subdirectory. We currently use pipenv to set up the dev environment (read the README). To test it with HA you just need to use pip to tell where the library is installed and copy files appropriately. For example, in hassio you can do the following command and then copy the modified files into your config directory. |
As a note, |
|
Thanks for the update @alandtse! I changed the should_poll to easy test the lib. Indeed, if the switch is off, the lib stops polling the vehicle. If the car is asleep it won't be woken. Even when restarting HA (and the switch ON). My ultimate goal is to get rid of TeslaFi, so, my polling interval could be as short as 1min. So, some intelligence would be implemented to dynamically increase interval to 10-15 min to allow the car to sleep. |
|
One thing to note, there is a separate library that is started as a general purpose library. They originally were sync using |
|
Ok, So we're coming up to a year later and still no progress. I'm keen to implement some of the new features in Home Assistant like the Update entity, and I'm really in dev mood, So I'm actually pretty keen to tackle this. The architecture of Home Assistant has changed a lot since this library was created, the the concept of devices and update coordinators. Would anyone be opposed against a |
|
No opposition if you want to do it. |
|
righto, |
|
@Megabytemb could I ask that you include not only vehicles but the other tesla products that are available via teslajsonpy; vehicles and solar are currently supported in this library, powerwall isn't but is available via the same API. Some details are available here: alandtse/tesla#79 |
|
That's a bit hard, cause I don't own a powerwall, so it will be hard to debug. But I'll do my best once I get the Car done. That OK? |
I'm happy to assist with debug. |
|
I've been thinking about the request to properly support Tesla Energy Sites in the component rewrite. I just want to re-affirm, i do believe its worth while, and I think I'm writing it in a way that I could easily also support energy sites. |
|
So I'm by no means feature complete, but I've made a Sizeable dent. I've also added in some new entities like Number and Update entities, and added some extra selects and buttons for config and diagnostics. |
|
Thanks. Please submit a PR so the diff is easy to see and comments can be made about specific portions of the code. You can flag it DRAFT until you're ready. In terms of code style, if it's using |
|
i've got both car and powerwall and am happy to help debug if needed as well. i just found this integration tonight while looking for a way to set operation mode on my powerwalls, will be installing it to check out the car side tonight. |
Done - alandtse/tesla#216 |
|
just going to reference this here as well: as is, powerwall API commands with numeric values don't work with home assistant via teslajsonpy for some reason (and i haven't been able to figure out why). i'm able to set values via string params without an issue, but as soon as i try to use numeric params everything blows up... see here for details: #316 @Megabytemb any ideas what could be causing this or how to fix it? |
Too many moving parts. I tried to set up your link as a custom repo in HACS, but it still downloaded the same version. I then downloaded your fork as a zip file and copied the files to tesla_custom/ but that wouldn't load. I'll try again and forward the error messages. |
|
Yeah unfortunately I don’t think you can setup this repo as a custom repository. I know this is obvious but just to make sure, when you download the repo, make sure you’re going into |
When I copy all your files over into custom_components/tesla_custom I get the following error message after I reboot: homeassistant.log: |
|
It looks like it didn't download the teslajsonpy from my repo, which is required for the changes. If you can get access to where HA is installed, you can do a |
Thanks, that has now loaded and I have additional controls over the car, like charging current numbers and the like:
A lot of sensors have been renamed, breaking change? Number of entities has gone down from 41 to 37.
Nothing seen about the powerwall :-( |
|
Ah I think I know why.. give me ten minutes and I'll push a new update. @purcell-lab I just pushed a new update to the custom integration and teslajsonpy. If you can redownload the Tesla Custom Integration and follow the same steps ( Also to answer your question, there will be breaking changes unfortunately. As a part of the rewrite, I'm trying to conform to HA standards as much as I can. I want to get as much as we can aligned now to prevent future breaking changes. |
Back to skip_add issue. Your updated manifest.json no longer points to your teslajsonpy fork is that deliberate? |
|
Ahhh, it was supposed to be for me only based on how I'm working on it but I forgot to change it back. Sorry about that, here's the line:
|
|
Got it. Still no powerwall. I'm a bit suspicious about this line from my logs above which doesn't have the powerwall product: In contrast to |
|
I'm looking into it now. I'm just filtering by "resource_type" when we get the product list. If it's either "solar" or "battery", it should work. I'll post an update here once I figure it out. The other thing I'm wondering is if it actually downloaded the updated version of teslajsonpy from my repo. Are you able to execute this from the terminal of your HA install:
You should see a "Location:" which should be
Scroll down to line 561 and I just want to make sure it says:
|
All good here, it has that line inside my docker. I'm wondering rather than hijacking this thread further, should we switch to discord or something? |
|
Yep, I'm on the HA server, username just shred. |
|
At a bit of a loss on this one. @alandtse I'm curious if you have any ideas but basically when @purcell-lab is using teslajsonpy to hit the |
|
Interesting my tesljsonpy logs from earlier in the year did show the battery product in the log. |
I have upgraded to tesla_custom 2.4.2 and can confirm I have my battery being reported again via teslajsonpy=2.4.4 PRODUCT_LIST endpoint. @shred86 I'll try your updated custom component again. {
"energy_site_id": XXX,
"resource_type": "battery",
"site_name": "Home Energy Gateway",
"id": "YYYYYYYYYYY-XXXXX",
"gateway_id": "XXX-XX-JX-XXX",
"asset_site_id": "XXX",
"energy_left": 6624.578947368423,
"total_pack_energy": 14033,
"percentage_charged": 47.207147063125646,
"battery_type": "ac_powerwall",
"backup_capable": true,
"battery_power": 520,
"storm_mode_enabled": true,
"powerwall_onboarding_settings_set": true,
"sync_grid_alert_enabled": true,
"breaker_alert_enabled": true,
"components": {
"battery": true,
"battery_type": "ac_powerwall",
"solar": true,
"solar_type": "pv_panel",
"grid": true,
"load_meter": true,
"market_type": "residential"
}
}I end up with KeyError: YYYYYYYYYYY-XXX, where YYYYYYYYYYY-XXX is the "id:" element from the PRODUCTS_LIST endpoint above. |
|
Well that’s good it’s showing up on products list now. So strange why it wasn’t earlier. It looks like you are running my updated version of teslajsonpy with the current version of this integration. You’ll have to do a That would be awesome if you could try the updated version again though as I’m looking for feedback. You will still have to do a Edit: And I already found one error based on your comment. I just made another update to the rewrite version (my forked version) of teslajsonpy. |
|
Success, I have a second device showing up now, which is my battery.
This is what your tesla-rewrite fork is presenting, which has some errors, but it is showing some correct values so it's a great launchpad: Here is what my localAPI is presenting, which I believe are true sensor values: |
|
Nice! Are those two screenshots taking at about the same time? How did they compare with your Tesla app? We're just reporting what's coming from the Tesla API at a 10 second interval, which obviously doesn't account for how often our inverter/gateways report data to the server and how quickly Tesla's pushes it back out. I'm assuming you'll always have some level of difference between the Tesla Powerwall integration (local API) versus this one, but I would also assume it should match the Tesla app. Are you getting any errors in the logs? If so, could you please post them here when you get a chance. Thanks again for testing! Edit: Just pushed a new version of the integration and teslajsonpy. Same thing,
Some questions for you:
|
|
Nice, here is your update. I like that you are reporting W & Wh rather than kW. One error is the Charging status, the battery is charging but the rewrite integration says not charging. Also the battery reserve/ % charge has a 5% variance, but that is expected. When the battery is fully discharged the Tesla app and cloudAPI both report 0%, but the localAPI reports 5% (which I think is a reserve they don't display to users). When the battery is fully charged all APIs report 100% and it is a sliding scale of variance inbetween.
And some comparison screen shots: and the localAPI: |
I'm very much looking forward to the control options you are talking about, as I'm currently setting them via a service call in a teslapy integration. setting backup_reserve_percent determines how much battery to reserve for blackout as is normal operation the battery will not discharge below this level. It is very useful if you want to charge your battery from the grid on demand, as setting backup_reserve to a higher number (upto 100%) will start the battery charging (from the grid). This is shown as a % slider in the app. setting real_mode allows control of the battery. The app has these as radio buttons, but you can only be in one real_mode so a three way option switch is probably the best option.
https://github.com/carboncoop/tesla-gateway-ha-component
In the app these are a series of switches:
Some bonus switches which have been recently added to the cloud API are 'Energy Exports' and 'Grid Charging' switches which limit grid behaviours and would also be welcome additions to the integration. These would be best implemented as switches in the integration.
|
|
Here is a side by side comparision of the tesla-rewrite cloudAPI (top) and localAPI (bottom): |
|
Another issue: Battery % keeps dropping back to 0, maybe some sort of availability check is necessary.
|
|
Ah, good to see it's mostly working. Just pushed an update for both the integration and teslajsonpy.
|
Wow, you are powering through the energy end points, just uploading your changes now. Here are the endpoinrs for exports & charging: |
|
It isn't loving the battery_reserve_percent ;-( Also I wouldn't set the mode and the battery_reserve at the same time, leave them as separate controls. |
|
All, please take the PR specific comments to the PR. This issue should only be for discussions about the issue and not an implementation to resolve that issue. |
Right now, this library creates entities specifically for HA to consume. This has two downsides that I can see.
controller.list_vehicles()returned a list of objects for HA and not just a list of vehicles. It also means people not using HA likely won't use this library which runs against the philosophy from HA requiring the library be put in pypi to allow others to reuse.I think we should move the HA related architecture into the HA component. I'd be happy to take that project on.
From an initial review, controller, connection, and exception make sense to me in this library. Everything based on vehicle probably should go into HA.
@zabuldon, this is your project to start, do you have any objections to this rearchitecture? I'll plan to start on doing that unless you object.
The text was updated successfully, but these errors were encountered: