Skip to content
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

Add Tesla Solar Energy Monitoring #212

Closed
wants to merge 3 commits into from
Closed

Add Tesla Solar Energy Monitoring #212

wants to merge 3 commits into from

Conversation

bassrock
Copy link
Contributor

This is an initial attempt at adding Tesla Solar energy to the library.

Before I go further, I'd like to get a once over of the PR and the approach.

I have tesla solar working with HA via a Rest Sensor but would like to see it get more integrated with the offical plugin.

@alandtse
Copy link
Collaborator

alandtse commented Sep 3, 2021

Thanks. Approach looks fine. Please remember to add tests. We've been trying to get better test coverage for the library since it's getting more complex.

@alandtse
Copy link
Collaborator

alandtse commented Sep 3, 2021

Actually, one thing to note. One of the issues we have with this library is that we have so much embedded HA stuff down in the API. That makes it so if you want to make a change in HA, you have to make changes in 2 places: the api library and HA. You'll note we have a long open issue #24 to try to break the HA out.

If we're starting from scratch here, I'd suggest you see if you can isolate the raw api calls so they're in this library and then build the entity logic in HA itself. So basically I wouldn't suggest keeping the homeassistant/* files. The downside of course is someone can't access entities in teslajsonpy, but I'm not sure who is actually using the library beyond HA. You may also want to see if we can make ourselves API compatible with the existing powerwall integration. That way you could just contribute to that integration directly too. I believe the main difference between that integration and what is happening here is the powerwall integration relies on local access while this would rely on cloud access. See #49

@bassrock
Copy link
Contributor Author

bassrock commented Sep 3, 2021

@alandtse yea I looked into local access unfortunately it doesn't have any. I have SolarCity panels with a Tesla gateway after the fact which seems to only use cloud.

I'll look at all those prs you mentioned. I also wasn't a big fan of having to do code here and then pr to the HA integration 😞

@alandtse
Copy link
Collaborator

alandtse commented Sep 3, 2021

Yah, so if you're able to just make the necessary API changes here, you can take your HA specific code into HA itself. The only thing we'd need to keep in this component would be the base solar site object so the controller knows it should be fetching and caching information for that device.

@alandtse
Copy link
Collaborator

If you're still working on this, I'd suggest seeing #216 to see if it simplifies your PR. I think that's a more robust way to define/access the underlying API. I'm planning to switch the vehicle related calls to that method and may also let us get the solar stuff for free.

Please also note, that with Tesla being removed from core, any improvements should go to the custom_component I'm hosting now.

@giachello
Copy link
Contributor

giachello commented Oct 2, 2021

Hi, did you end up merging this into the main branch. I have also started writing support for "energy_sites" (i.e. solar panels, not powerwalls) based off this plugin but if you are already working on it, wouldn't continue.

@bassrock
Copy link
Contributor Author

bassrock commented Oct 2, 2021

@giachello i have not had time to continue this. go for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants