Add Shelly EM1 - #31992
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
usageparameter is defined but never referenced in the template render; consider either wiring it into the configuration or removing it to avoid confusion. - For
currentsandvoltages, you currently hardcode two additional phases as constant zero values; if this is meant to be strictly single-phase, consider modeling it explicitly as single-phase or making the number of phases configurable instead of padding with zeros.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `usage` parameter is defined but never referenced in the template render; consider either wiring it into the configuration or removing it to avoid confusion.
- For `currents` and `voltages`, you currently hardcode two additional phases as constant zero values; if this is meant to be strictly single-phase, consider modeling it explicitly as single-phase or making the number of phases configurable instead of padding with zeros.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I also have a template prepared for 3-phase EM-based Shelly devices, but as I do not have one for testing yet, I've not pushed it. In case anybody is available for testing it, let me know. |
Code review
The template declares This is inconsistent with the established convention across other modbus meter templates in this repo: templates offering a evcc/templates/definition/meter/eastron-sdm120.yaml Lines 17 to 23 in 8487a31 address: 0x0c # Active power
type: input
decode: float32
{{- if eq .usage "pv" }}
scale: -1
{{- end }}The same pattern appears in (Note: this overlaps with Sourcery's first comment above.) |
|
We don't have a consistent model for phase values on 1p devices- please remove phase readings for time being. May revisit later. /cc @premultiply |
|
/cc @thierolm |
|
@FrankvdAa , what's the bennefit of your new modbus EM1 Shelly connection compared to the existing Shelly API support of evcc? |
|
Yikes. Please let me know if this needs be reverted and/or the model moved to an existing template. Thank you! |
For HTTP API I have to configure admin password in EVCC for each Shelly, while modbus allows me to have local readonly access to valyes without authentication, while webinterface can still require password for access. |
|
I've reverted- lets agree on a strategy first. We surely don't want to duplicate implementations. |
|
@andig, I think Modbus TCP is the more widely used protocol for energy systems. The HTTP API is currently what's implemented for Shelly within EVCC and does not need to be removed in my opinion, but not adding the Modbus integration seems strange to me. Modbus TCP is enabled by default on the newer Shelly devices: https://support.shelly.cloud/en/support/solutions/articles/103000316046-which-devices-supports-modbus- |
|
@premultiply @thierolm whyt? |
|
I would prefer not to add a dedicated Modbus implementation. => Unless Modbus provides functionality that is unavailable through the existing HTTP API, the additional protocol mainly adds complexity without delivering additional user value. |
I think this isn't a valid argument, as that is handled by the templates and isn't directly visible to the normal users. At the moment you already have three Shelly templates of which two are I do not see an issue with adding two modbus variants, one being 1p and the other 3p, that support all Shelly devices with modbus support.
These device templates (also non-Shelly) are not directly supported by EVCC crew and rely on community to support them, right? |
|
Not on me to decide :-) |
Me neither, unfortunatly ;-) |
This PR adds modbus support for Shelly EM1-based devices.
The EM1 are 1-phase meters, so I set phase2/3 to 0 with const for currents and voltages, as I couldn't find how to only provide single phase. I reckon it's enough to always provide L1 data, right? Otherwise I can add a parameter with which you could set the phase it's measuring.