Skip to content

Account for a microinverter connected at the generator input of the deye hp3 inverter #19256

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

Merged
merged 8 commits into from
Apr 3, 2025

Conversation

Johnny1206
Copy link
Contributor

@Johnny1206 Johnny1206 commented Feb 28, 2025

Added register 667 to total pv power to account for a microinverter connected at the generator input of the deye inverter.

Fix #19171, fix #20235

Added register 667 to total pv power to account for a microinverter connected at the generator input of the deye inverter.
@premultiply
Copy link
Member

Unsure if we should add this by default?

Usage as diesel generator input should not be handled as solar production.

Added an advanced parameter to consider the usage of the generator port. By default the power on the generator port won't be added to the total pv power now.
@Johnny1206
Copy link
Contributor Author

Added an advanced parameter to consider the usage of the generator port. Choices are derived from the generator port usage options in the manual of the inverter.
By default the power on the generator port won't be added to the total pv power now.
Code has not been tested!

@@ -30,6 +30,10 @@ params:
default: 95
advanced: true
- name: maxacpower
- name: GenPortUse
choice: ["Generator", "SmartLoad", "MicroInverter"]
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the options. Wouldn't "inlcude generator port" be better? Why doesn't this affect energy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you could absolutely change this to a Boolean for its current usage.

My approach was to make it expandable for more settings in the future. The three options are taken from the inverter manual. One could say, if the GEN-Port is used in “Generator” mode it may be useful for example to add it to the grid power. Or in “smart load” it is more related to house consumption. And like I already implemented: In “MicroInverter” Mode, it should be added to PV power.

How it can be added to total pv energy however, I do not know. I haven’t found a register related to total generator port energy.

I only really need the Micro-Inverter feature and therefore I would be happy if I had an option for the power on the generator port to be added to the Total PV power in one way or another.

Copy link
Member

Choose a reason for hiding this comment

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

@Johnny1206 will energy then be wrong since its only pv?

@andig andig added the devices Specific device support label Feb 28, 2025
The datatype of the generator port power is signed because the power at this port can flow in both directions.
@andig andig marked this pull request as draft March 2, 2025 13:16
@Johnny1206
Copy link
Contributor Author

I have noticed that there is also a high word for the generator power with register address 671 but as my micro inverter power does not exceed 32 kW, I can not test the implementation.
The correct implementation is probably to also add register 671 and scale it with a factor of 2^16 but i am unsure.

@turbotorsten

This comment was marked as off-topic.

@premultiply
Copy link
Member

@Johnny1206 Could you please attach the current register documentation this is based on? Thanks!

@premultiply premultiply added the waiting for feedback Suspended progress label Mar 10, 2025
@Johnny1206
Copy link
Contributor Author

At the moment i use the Deye.MODBUS.RTU.V104.pdf from dy-support.org (3497-Deye-MODBUS-RTU-V104.pdf). Unfortunately a lot of information in the document is in chinese, so searching the document via STRG+F is not always helpful.

My current understanding is the following:

  • Power from the Generator Port: Low Word in 667 and High Word in 671 as a signed 32 bit integer with a scaling of 1 W
  • Total Energy from the Generator Port: Low Word in 537 and High Word in 538 also as a signed 32 bit integer with a scaling of 0.1 kWh.
  • Register 133 indicates if the Port is configured as generator, smart load or micro inverter. This could potentialy be used instead of the configuration parameter i suggested.

I am unsure on how to add together register 667 and 671. My guess would be to scale 671 by 2^16 and add it to 667 but i do not know if this works for signed values.

@github-actions github-actions bot added the stale Outdated and ready to close label Mar 18, 2025
@Johnny1206
Copy link
Contributor Author

@premultiply
could you please give an update on whether you still plan to implement this?

@github-actions github-actions bot removed the stale Outdated and ready to close label Mar 19, 2025
@andig
Copy link
Member

andig commented Mar 20, 2025

Ping @premultiply wie wäre es den downstream Port optional zu integrieren? Unabhängig von seinem Zweck, das würde ich dem Anwender überlassen?

In jedem Fall sollten wir hie rzu einem Ergebnis kommen.

@Johnny1206
Copy link
Contributor Author

Ja, gerne auch nur eine boolean Option ala "addiere gen port zu pv". Diese Möglichkeit zu haben wäre sehr hilfreich. Am besten natürlich für Leistung und Energie. Registerbeschreibungen dafür stehen weiter oben.

@github-actions github-actions bot added stale Outdated and ready to close and removed stale Outdated and ready to close labels Mar 27, 2025
@andig andig added prio Priority and removed waiting for feedback Suspended progress labels Mar 31, 2025
@andig
Copy link
Member

andig commented Mar 31, 2025

ping @premultiply

@andig
Copy link
Member

andig commented Apr 2, 2025

@Johnny1206 mangels Feedback: willst du PR so umbauen dass wir eine einfache "include generator port" Option bauen?

- changed parameter to boolean "includegenport"
- added energy registers of gen port to the template
@Johnny1206
Copy link
Contributor Author

@andig: habs wie vorgeschlagen abgeändert. allerdings kann ichs nicht testen. Daher bitte mal akribisch reviewen, ob das funktionieren kann, was ich da gebastelt hab.
Außerdem weiß ich nicht, wie ich das High-Word der Leistung am Generatorport da mit rein krieg. Deshalb funktioniert die Implementierung jetzt nur bis 32kW am Gen-Port.

@andig
Copy link
Member

andig commented Apr 2, 2025

allerdings kann ichs nicht teste

Was fehlt Dir?

@andig
Copy link
Member

andig commented Apr 2, 2025

ußerdem weiß ich nicht, wie ich das High-Word der Leistung am Generatorport da mit rein krieg.

32 statt 16 bit auslesen?

@Johnny1206
Copy link
Contributor Author

@andig

Was fehlt Dir?
ich kriegs nicht gebaut bei mir.

32 statt 16 bit auslesen?
so hab ichs bei der energie gemacht. aber bei der leistung sind die addressen nicht aufeinanderfolgenden: 667 und 671. Da weiß ich nicht weiter.

@andig
Copy link
Member

andig commented Apr 2, 2025

ah, lass uns das ignorieren

ich kriegs nicht gebaut bei mir.

Musst du nicht: #19495

@Johnny1206
Copy link
Contributor Author

@andig
danke für den tipp. cooles feature. so kann ichs easy testen.

jetzt muss ich nur noch rausfinden warum der bei "{{- if eq .includegenport true }}" das bool nicht frisst und "incompatible types for comparison" meldet.

put the "true" in the if statement in quotes:
{{- if eq .includegenport "true" }}
@Johnny1206
Copy link
Contributor Author

@andig
So, Problem gefixt -> siehe commit.

Hab das Template jetzt auch durch einbinden der yaml testen können: Das funktioniert soweit. Die Leistung des Balkonkraftwerks am GEN-Port wird bei includegenport=true jetzt zur PV-Leistung addiert und bei includegenport=false weiter wie vorher auch nicht berücksichtigt und somit vom Hausverbrauch abgezogen. Bei der Gesamtenergie der PV addiert auch beide Werte korrekt zusammen.

@andig andig marked this pull request as ready for review April 2, 2025 14:20
@premultiply premultiply requested a review from Copilot April 3, 2025 09:03
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the deye-hybrid-hp3 meter definition to include an option for accounting for a microinverter connected at the generator input by adding an additional Modbus register (667) for input power.

  • Added a new boolean parameter "includegenport" for enabling generator port processing.
  • Conditionally includes Modbus register definitions for generator input power and combines them in energy calculations.
Comments suppressed due to low confidence (2)

templates/definition/meter/deye-hybrid-hp3.yaml:109

  • Since 'includegenport' is defined as a boolean, consider checking its truthiness directly (e.g., 'if .includegenport') instead of comparing it to the string "true" to avoid potential type mismatches.
{{- if eq .includegenport "true" }}

templates/definition/meter/deye-hybrid-hp3.yaml:33

  • [nitpick] Consider renaming 'includegenport' to 'includeGenPort' for improved readability and consistency with common naming conventions.
- name: includegenport

@Johnny1206
Copy link
Contributor Author

@andig Hab Beschreibung hinzugefügt. passt das so?

@premultiply premultiply merged commit c43a92f into evcc-io:master Apr 3, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices Specific device support prio Priority
Projects
None yet
4 participants