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 growatt battery devices #34773

Merged
merged 6 commits into from
Jun 16, 2020
Merged

Add growatt battery devices #34773

merged 6 commits into from
Jun 16, 2020

Conversation

indykoning
Copy link
Contributor

@indykoning indykoning commented Apr 27, 2020

Proposed change

This PR aims to add battery sensor information to the Growatt server integration. This has been tested on my Home assistant instance with two accounts:

  • One with my own "Plant" with just two inverters
  • One with the "Plant" of someone nice enough to hand me their login with just one battery

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
sensor:
  - platform: growatt_server
    username: GROWATT_SERVER_USERNAME
    password: GROWATT_SERVER_PASSWORD

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@MartinHjelmare MartinHjelmare changed the title Added support for storage type devices Add support for storage type devices Apr 28, 2020
@MartinHjelmare MartinHjelmare changed the title Add support for storage type devices Add growatt storage type devices Apr 28, 2020
@Hedda
Copy link
Contributor

Hedda commented Apr 28, 2020

FYI, you should not use the term "Storage devices" for batteries because "storage device" most commonly refers to a "data storage device":

https://en.wikipedia.org/w/index.php?title=Data_storage_device

I believe that the correct terminology is "energy storage" or "home energy storage" as a general term for any type of energy storing devices:

https://en.wikipedia.org/wiki/Energy_storage

If always specifically only batteries then it technically is "Electrochemical energy storage" where products are today commercially commonly referred to as "Battery Energy Storage System" (or "BESS" for short).

https://en.wikipedia.org/wiki/Home_energy_storage

Other very common "energy storage" devices for private homes are accumulator which usually store energy as heat (normally hot water), like a hot water storage tank:

https://en.wikipedia.org/wiki/Accumulator_(energy)

https://en.wikipedia.org/wiki/Hot_water_storage_tank

Commercial buildings take accumulators to the next-level, storing both heat and cold, few private homes do that but see: https://www.youtube.com/watch?v=N3Em64OBGqI

@indykoning
Copy link
Contributor Author

indykoning commented Apr 28, 2020

That is fair enough, it can be confusing if context is not clear. I will change the title and description of this PR once i have the time.

in code and in context of Growatt and their API it may be more clear to use storage since that is what their actual api endpoints are called: https://github.com/indykoning/PyPi_GrowattServer/blob/1fa78f2b332f41ecaa9f66065f7617e6b3cb8a2f/growattServer/__init__.py#L119 see newStorageApi, getStorageParams and storageId.

@indykoning indykoning changed the title Add growatt storage type devices Add growatt battery devices Apr 30, 2020
@stale
Copy link

stale bot commented Jun 3, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jun 3, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good!

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please solve the merge conflict.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@indykoning
Copy link
Contributor Author

There were some issues caused by the merge, it should be fine now :)
I have tested the latest changes on my own HA instance on 0.111

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes to mark the correct status. See above.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MartinHjelmare MartinHjelmare merged commit d278dd9 into home-assistant:dev Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants