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

SolarEdge Local Component #23996

Merged
merged 13 commits into from Jun 5, 2019

Conversation

@drobtravels
Copy link
Contributor

commented May 19, 2019

Description:

Creates a new integration similar to the current SolarEdge component, except this communicates over a local API instead of cloud polling. Note, this is not a replacement for the current SolarEdge component, as many SolarEdge Inverters do not have this API. Details are available in the documentation

Related issue (if applicable): fixes drobtravels/solaredge-local#2

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9490

Example entry for configuration.yaml (if applicable):

sensor:
  platform: solaredge_local
  ip_address: 192.168.1.130
  monitored_conditions:
    - lifetime_energy
    - energy_this_year
    - energy_this_month
    - energy_today
    - current_power

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@homeassistant

This comment has been minimized.

Copy link

commented May 19, 2019

Hi @drobtravels,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@rohankapoorcom

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I don't think we want to have multiple components for this. It seems like the underlying library should accept a local mode or cloud mode flag and abstract away both implementations so that a single component in Home Assistant works.

@drobtravels

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I'm happy to architect this however is recommended, however there are some big differences between the local API and cloud, which may make this component very messy.

To start with I exposed the same parameters to keep it simple, but the local API exposes a lot of details that the cloud version doesn't have such as optimizer level information, production meter information, and real time status. Likewise cloud API has historical data not found in the local api. Some users may want to use both together.

They also use different protocols. The local API uses protocol buffers with no authentication, while the official cloud API supports JSON, XML, and CSV with API key authentication.

Due to these differences, they will likely require 2 independent python packages to communicate and different implementations.

Let me know how you recommend proceeding or if I can provide additional details.

@drobtravels

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@rohankapoorcom Let me know if you have any further guidance based on my comments above. If they were combined into a single component, I can think of two options for user configuration

  1. Let the component decide which API to use for each sensor
sensor:
  - platform: solaredge
    local_api:
      enabled: true
      ip_address: 192.168.1.130
    cloud_api:
      enabled: true
      api_key: API_KEY
      site_id: SITE_ID
    monitored_conditions:
        - lifetime_energy
        - energy_this_year
        - energy_this_month
        - energy_today
        - current_power
        - site_details
        - sensors
        - gateways
        - batteries
  1. Allow the user to specify monitored conditions for each API
sensor:
  - platform: solaredge
    local_api:
      enabled: true
      ip_address: 192.168.1.130
      monitored_conditions:
        - lifetime_energy
        - energy_this_year
        - energy_this_month
        - energy_today
        - current_power
    cloud_api:
      enabled: true
      api_key: API_KEY
      site_id: SITE_ID
      monitored_conditions:
        - site_details
        - sensors
        - gateways
        - batteries

From an end user perspective, I think option 1 might be confusing as different monitored_conditions will be supported based on which APIs they configured. Option 2 seems fine but doesn't seem any better then two separate components IMO. From a code perspective, I think they should remain two different python packages.

@pvizeli

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Based on upcoming ABR003 -> home-assistant/architecture#244

Remove monitor condition. I think the local component is fine because it's a completely different protocol. I agree with @rohankapoorcom but it is how it is with smart homes... So you can choice if you want to merge the APIs together or let it like it is.

@pvizeli pvizeli self-assigned this May 27, 2019

@drobtravels drobtravels force-pushed the drobtravels:solaredge-local branch from 66385ac to e408097 May 28, 2019

@drobtravels

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Removed monitored_conditions configuration options as requested per home-assistant/architecture#244

self.api = api
self.data = {}

def update(self):

This comment has been minimized.

Copy link
@pvizeli

pvizeli May 31, 2019

Member

You need to add a trotthle here

This comment has been minimized.

Copy link
@pvizeli

pvizeli May 31, 2019

Member

See on utils

This comment has been minimized.

Copy link
@drobtravels

drobtravels Jun 5, 2019

Author Contributor

Will do, but out of curiosity, why is it necessary to throttle a local API? I've been running it for several weeks without it.

.coveragerc Outdated
@@ -548,7 +548,7 @@ omit =
homeassistant/components/sochain/sensor.py
homeassistant/components/socialblade/sensor.py
homeassistant/components/solaredge/sensor.py
homeassistant/components/solax/sensor.py

This comment has been minimized.

Copy link
@pvizeli

pvizeli May 31, 2019

Member

why you remove solax?

This comment has been minimized.

Copy link
@drobtravels

drobtravels Jun 5, 2019

Author Contributor

hmm.. Sorry about that. I'm not used to the rebase workflow and must have messed something up.

drobtravels added some commits May 9, 2019

Fix docstyle for init
Of course thats the file I forgot to run tests on
Load all sensors by default
They use the same API endpoint.  This changes was made per home-assistant/architecture#244

@drobtravels drobtravels force-pushed the drobtravels:solaredge-local branch from 840e69e to 6736320 Jun 5, 2019

@pvizeli

pvizeli approved these changes Jun 5, 2019

@pvizeli pvizeli merged commit 4c6ddd4 into home-assistant:dev Jun 5, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing d31140f...3d8009f
Details
codecov/project 94.2% (target 90%)
Details
@Uskompuf

This comment has been minimized.

Copy link

commented Jun 9, 2019

Is this in release yet?

@drobtravels

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

This didn't make this release. It should be part of 0.95 in ~2 weeks.

@balloob balloob removed the new-platform label Jun 25, 2019

@balloob balloob referenced this pull request Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.