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

support phase currents, and three-phase with enphase envoy #17193

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pdeliot
Copy link

@pdeliot pdeliot commented Nov 11, 2024

Added support phase currents, and three-phase with Enphase envoy #16744

@andig andig requested a review from premultiply November 13, 2024 15:49
@andig andig added the devices Specific device support label Nov 13, 2024
@andig
Copy link
Member

andig commented Nov 18, 2024

Not sure we can do this. This will return empty measurements on devices not supporting this?

@andig andig marked this pull request as draft November 18, 2024 18:17
@pdeliot
Copy link
Author

pdeliot commented Nov 18, 2024

Yes, this was discussed with ivoks on the issue discussion.
Originally I was returning 0 when phase is not available.
ivoks suggested returning empty and tested it on its one phase system successfully.

This is the choosen way to implement 3 or 1 phase support.

@andig
Copy link
Member

andig commented Nov 18, 2024

We cannot simply return „nothing“.

@pdeliot
Copy link
Author

pdeliot commented Nov 18, 2024

It is "empty" because the phase 2 and 3 may not exist :-)
And it is working well.

But if you prefer having "0" and this is not creating issue for systems with only 1 phase.. we can do that.

@andig
Copy link
Member

andig commented Nov 18, 2024

We just cannot have this interface for 1p meters

@pdeliot
Copy link
Author

pdeliot commented Nov 20, 2024

I've pushed version return 0 when value is not available instead on "empty".

@andig
Copy link
Member

andig commented Nov 20, 2024

@premultiply was meinst du? Gute Idee?

@andig andig added the needs decision Unsure if we should really do this label Nov 21, 2024
@pdeliot
Copy link
Author

pdeliot commented Nov 23, 2024

What do you prefer?
-Return "empty"
-Return "0"
-Another way to deal with 1p and 3p

@andig
Copy link
Member

andig commented Nov 23, 2024

Imho we should not have 3p measurements on 1p devices at all, so 3rd option. Would any of the measurements (I guess current?) be missing if this was a 1p device?

@pdeliot
Copy link
Author

pdeliot commented Nov 23, 2024

With 1p, as far as I know "powers", "currents" and "voltages" will not be supported.
But I have no docs about evcc requirement (if you have a pointer on the specifications, feel free to provide :-) )

I'm pushing a new version, with an optional parameter "phases".
Default is "1" then no behavior change for existing installations.

And you can set "3" to gain 3 phases support and linked capabilities.

Adding "phases" optional parameter to switch on 3 phases support.
Added "phases" parameter to activate 3 phase support and attached capacities.
@ivoks
Copy link
Contributor

ivoks commented Nov 28, 2024

I'm worried that this would silently break existing 3p setups.

IMHO templates should collect as much info as possible and core engine should be able to cope with the collected data. If the template returns 'value none none' it should be obvious that it's a single phase system. If it returns 'value 0 0', that's a 3p system. I believe that using 0 in this patch is a wrong approach as you just can't know if the 0 is because there is no current or because there's no phase. Who knows what the future brings; evcc might eventually want to know if it's a 3p or 1p setup.

@ivoks
Copy link
Contributor

ivoks commented Nov 28, 2024

ivoks suggested returning empty and tested it on its one phase system successfully.

I've tested it on a 3p system without CTs ;)

@andig
Copy link
Member

andig commented Dec 1, 2024

To decide @premultiply:

  • remove powers (only needed if currents not signed)
  • remove voltages (only needed for chargers)
  • allow phase measurements for 1p devices (and remove config option)

See evcc-io/docs#650

-removed powers
-removed voltages
-re-added 3 phase measurements for 1 phase devices
-removed 1/3 phase option
@ivoks
Copy link
Contributor

ivoks commented Dec 2, 2024

@pdeliot to work with both single and three phase system, you should really check the length of the array. For example, for phase in production:

curl http://${envoy_ip}/production.json?details=1 | jq 'if (( .production[] | select(.measurementType == "production").activeCount >= 1 ) and ( .production[] | select(.measurementType == "production").lines | length >= 1 )) then .production[] | select(.measurementType == "production").lines[0].rmsCurrent else 0 end'

but then for the second and third line (change the line 1 to 2):

curl http://${envoy_ip}/production.json?details=1 | jq 'if (( .production[] | select(.measurementType == "production").activeCount >= 1 ) and ( .production[] | select(.measurementType == "production").lines | length >= 3 )) then .production[] | select(.measurementType == "production").lines[1].rmsCurrent else 0 end'

And then similar for consumption. What do you think? I think this would cover all requirements; it would work only on CT enabled systems and it would work with both 1 and 3 phase systems without any parameters.

@pdeliot pdeliot marked this pull request as ready for review December 5, 2024 19:08
@andig
Copy link
Member

andig commented Dec 8, 2024

Ping @premultiply

@github-actions github-actions bot added the stale Outdated and ready to close label Dec 15, 2024
@andig andig removed the stale Outdated and ready to close label Dec 16, 2024
@pdeliot
Copy link
Author

pdeliot commented Dec 18, 2024

Ping @premultiply

@github-actions github-actions bot added the stale Outdated and ready to close label Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devices Specific device support needs decision Unsure if we should really do this stale Outdated and ready to close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants