Homewizard improvements #26938
Closed
Homewizard improvements #26938
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add phase parameter (1-3) to specify installation phase for HWE-KWH1 and SDM230-wifi - Single-phase meters now report voltage and current on configured phase - Three-phase meters (HWE-KWH3, SDM630-wifi) continue to report all phases - Default phase is L1 (phase=1)
evcc-bot
added
devices
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The single‑phase handling in
CurrentsandVoltagesis hard‑coded againstProductTypestring literals; consider centralizing these model identifiers as constants and defining a clear default path for unknown/emptyProductTypevalues to avoid fragile string matching. - The phase-to-L1/L2/L3 mapping logic is duplicated in both
CurrentsandVoltages; extracting a small helper (e.g. that returns three floats given a single value andphase) would reduce repetition and the risk of inconsistencies. - For the charger (
charger/homewizard.go), the phase is currently hard‑coded to1when constructing the connection; if smart sockets can be installed on other phases, it may be worth threading a configurable phase parameter through the charger config as well for consistency with the meter template.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The single‑phase handling in `Currents` and `Voltages` is hard‑coded against `ProductType` string literals; consider centralizing these model identifiers as constants and defining a clear default path for unknown/empty `ProductType` values to avoid fragile string matching.
- The phase-to-L1/L2/L3 mapping logic is duplicated in both `Currents` and `Voltages`; extracting a small helper (e.g. that returns three floats given a single value and `phase`) would reduce repetition and the risk of inconsistencies.
- For the charger (`charger/homewizard.go`), the phase is currently hard‑coded to `1` when constructing the connection; if smart sockets can be installed on other phases, it may be worth threading a configurable phase parameter through the charger config as well for consistency with the meter template.
## Individual Comments
### Comment 1
<location> `charger/homewizard.go:46` </location>
<code_context>
// NewHomeWizard creates HomeWizard charger
func NewHomeWizard(embed embed, uri string, usage string, standbypower float64, cache time.Duration) (*HomeWizard, error) {
- conn, err := homewizard.NewConnection(uri, usage, cache)
+ conn, err := homewizard.NewConnection(uri, usage, 1, cache)
if err != nil {
return nil, err
</code_context>
<issue_to_address>
**question:** Hard-coding phase=1 for chargers may misrepresent installations on L2/L3.
`NewConnection` now hard-codes `phase` to 1. If a single-phase charger is actually on L2/L3, any phase-specific values (e.g., Currents/Voltages) will be reported as L1 and may conflict with other phase-aware meters. If non-L1 installations are possible, consider passing `phase` as a configuration value or clearly documenting that chargers are assumed to be on L1 only.
</issue_to_address>
### Comment 2
<location> `meter/homewizard/types_test.go:41` </location>
<code_context>
+ jsonstr := `{"wifi_ssid": "My Wi-Fi","wifi_strength": 100,"total_power_import_kwh": 30.511,"total_power_export_kwh": 85.951,"total_power_import_t1_kwh": 30.511,"total_power_export_t1_kwh": 85.951,"active_power_w": 543,"active_power_l1_w": 28,"active_power_l2_w": 0,"active_power_l3_w": -181,"active_voltage_l1_v": 235.4,"active_voltage_l2_v": 235.8,"active_voltage_l3_v": 236.1,"active_current_l1_a": 1.19,"active_current_l2_a": 0.37,"active_current_l3_a": -0.93}`
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit test coverage for single‑phase meters using `active_voltage_v` and `active_current_a` only
Please add a subtest using a real single‑phase kWh meter payload that exposes only `active_voltage_v` and `active_current_a` (no `*_l1_*` fields). This will verify that `DataResponse` and downstream logic correctly support the new single‑phase schema without relying on phase‑specific fields.
Suggested implementation:
```golang
{
// https://www.homewizard.com/shop/wi-fi-kwh-meter-1-phase/
t.Run("single_phase_meter_active_voltage_and_current_only", func(t *testing.T) {
var res DataResponse
// Single-phase payload exposing only active_voltage_v and active_current_a (no *_l1_* fields)
jsonstr := `{"wifi_ssid":"My Wi-Fi","wifi_strength":100,"total_power_import_kwh":12.345,"total_power_export_kwh":0.0,"active_power_w":123,"active_voltage_v":230.5,"active_current_a":0.53}`
require.NoError(t, json.Unmarshal([]byte(jsonstr), &res))
assert.Equal(t, float64(12.345), res.TotalPowerImportkWh)
assert.Equal(t, float64(0.0), res.TotalPowerExportkWh)
assert.Equal(t, float64(123), res.ActivePowerW)
assert.Equal(t, float64(230.5), res.ActiveVoltageV)
assert.Equal(t, float64(0.53), res.ActiveCurrentA)
})
// Existing payload remains as regression test for phase-specific fields.
var res DataResponse
jsonstr := `{"wifi_ssid": "My Wi-Fi","wifi_strength": 100,"total_power_import_kwh": 30.511,"total_power_export_kwh": 85.951,"total_power_import_t1_kwh": 30.511,"total_power_export_t1_kwh": 85.951,"active_power_w": 543,"active_power_l1_w": 28,"active_power_l2_w": 0,"active_power_l3_w": -181,"active_voltage_l1_v": 235.4,"active_voltage_l2_v": 235.8,"active_voltage_l3_v": 236.1,"active_current_l1_a": 1.19,"active_current_l2_a": 0.37,"active_current_l3_a": -0.93}`
require.NoError(t, json.Unmarshal([]byte(jsonstr), &res))
assert.Equal(t, float64(30.511), res.TotalPowerImportkWh)
assert.Equal(t, float64(85.951), res.TotalPowerExportkWh)
assert.Equal(t, float64(543), res.ActivePowerW)
assert.Equal(t, float64(235.4), res.ActiveVoltageL1V)
assert.Equal(t, float64(235.8), res.ActiveVoltageL2V)
```
1. This assumes `DataResponse` exposes single-phase fields as `ActiveVoltageV` and `ActiveCurrentA`. If the actual field names differ, adjust the assertions accordingly (e.g. `res.ActiveVoltage_v`).
2. Ensure `testing.T` is in scope in this test function (it already is, since the existing code uses `t` with `require`/`assert`).
</issue_to_address>
### Comment 3
<location> `meter/homewizard/types_test.go:44-45` </location>
<code_context>
- assert.Equal(t, float64(30.511), res.TotalPowerImportT1kWh+res.TotalPowerImportT2kWh+res.TotalPowerImportT3kWh+res.TotalPowerImportT4kWh)
- assert.Equal(t, float64(85.951), res.TotalPowerExportT1kWh+res.TotalPowerExportT2kWh+res.TotalPowerExportT3kWh+res.TotalPowerExportT4kWh)
+ assert.Equal(t, float64(30.511), res.TotalPowerImportkWh)
+ assert.Equal(t, float64(85.951), res.TotalPowerExportkWh)
assert.Equal(t, float64(543), res.ActivePowerW)
assert.Equal(t, float64(235.4), res.ActiveVoltageL1V)
</code_context>
<issue_to_address>
**suggestion (testing):** Assert new current/voltage aggregate fields in kWh data response
Please also extend this test to assert the `ActiveCurrentA` and `ActiveVoltageV` values when present in the JSON, so changes to their JSON tags or types are caught by tests.
Suggested implementation:
```golang
jsonstr := `{"wifi_ssid":"redacted","wifi_strength":78,"smr_version":50,"meter_model":"Landis + Gyr","unique_id":"redacted","active_tariff":2,"total_power_import_kwh":18664.997,"total_power_import_t1_kwh":10909.724,"total_power_import_t2_kwh":7755.273,"total_power_export_kwh":13823.608,"total_power_export_t1_kwh":4243.981,"total_power_export_t2_kwh":9579.627,"active_power_w":203.000,"active_power_l1_w":-21.000,"active_power_l2_w":57.000,"active_power_l3_w":168.000,"active_voltage_v":226.000,"active_voltage_l1_v":228.000,"active_voltage_l2_v":226.000,"active_voltage_l3_v":225.000,"active_current_a":1.091,"active_current_l1_a":-0.092,"active_current_l2_a":0.252,"active_current_l3_a":0.747,"voltage_sag_l1_count":12.000,"voltage_sag_l2_count":12.000,"voltage_sag_l3_count":19.000,"voltage_swell_l1_count":5055.000,"voltage_swell_l2_count":1950.000,"voltage_swell_l3_count":0.000,"any_power_fail_count":12.000,"long_power_fail_count":2.000,"total_gas_m3":5175.363,"gas_timestamp":241106093006,"gas_unique_id":"redacted","external":[{"unique_id":"redacted","type":"gas_meter","timestamp":241106093006,"value":5175.363,"unit":"m3"}]}`
```
```golang
assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh)
assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh)
assert.Equal(t, float64(203), res.ActivePowerW)
assert.Equal(t, float64(226), res.ActiveVoltageV)
assert.Equal(t, float64(1.091), res.ActiveCurrentA)
assert.Equal(t, float64(228), res.ActiveVoltageL1V)
```
</issue_to_address>
### Comment 4
<location> `meter/homewizard/types_test.go:64-65` </location>
<code_context>
- assert.Equal(t, float64(18664.997), res.TotalPowerImportT1kWh+res.TotalPowerImportT2kWh+res.TotalPowerImportT3kWh+res.TotalPowerImportT4kWh)
- assert.Equal(t, float64(13823.608), res.TotalPowerExportT1kWh+res.TotalPowerExportT2kWh+res.TotalPowerExportT3kWh+res.TotalPowerExportT4kWh)
+ assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh)
+ assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh)
assert.Equal(t, float64(203), res.ActivePowerW)
assert.Equal(t, float64(228), res.ActiveVoltageL1V)
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertion for `ActiveCurrentA` (and optionally `ActiveVoltageV`) in P1 data response
The P1 sample JSON already includes `"active_current_a": 1.091`, and the struct has an `ActiveCurrentA` field, but this test doesn’t assert it. Please add an assertion like `assert.Equal(t, 1.091, res.ActiveCurrentA)` (and optionally one for `ActiveVoltageV`) so the unmarshalling of these aggregate fields is covered.
Suggested implementation:
```golang
assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh)
assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh)
assert.Equal(t, float64(203), res.ActivePowerW)
assert.Equal(t, float64(228), res.ActiveVoltageL1V)
assert.Equal(t, float64(226), res.ActiveVoltageL2V)
assert.Equal(t, float64(1.091), res.ActiveCurrentA)
```
If the struct also exposes an aggregate `ActiveVoltageV` field that is expected to be populated from the P1 JSON (for example, derived from one of the per-phase voltages), you can add a similar assertion beside these ones, e.g.:
`assert.Equal(t, float64(228), res.ActiveVoltageV)`
Adjust the expected value to match how `ActiveVoltageV` is defined in your implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+44
to
+45
| assert.Equal(t, float64(30.511), res.TotalPowerImportkWh) | ||
| assert.Equal(t, float64(85.951), res.TotalPowerExportkWh) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Assert new current/voltage aggregate fields in kWh data response
Please also extend this test to assert the ActiveCurrentA and ActiveVoltageV values when present in the JSON, so changes to their JSON tags or types are caught by tests.
Suggested implementation:
jsonstr := `{"wifi_ssid":"redacted","wifi_strength":78,"smr_version":50,"meter_model":"Landis + Gyr","unique_id":"redacted","active_tariff":2,"total_power_import_kwh":18664.997,"total_power_import_t1_kwh":10909.724,"total_power_import_t2_kwh":7755.273,"total_power_export_kwh":13823.608,"total_power_export_t1_kwh":4243.981,"total_power_export_t2_kwh":9579.627,"active_power_w":203.000,"active_power_l1_w":-21.000,"active_power_l2_w":57.000,"active_power_l3_w":168.000,"active_voltage_v":226.000,"active_voltage_l1_v":228.000,"active_voltage_l2_v":226.000,"active_voltage_l3_v":225.000,"active_current_a":1.091,"active_current_l1_a":-0.092,"active_current_l2_a":0.252,"active_current_l3_a":0.747,"voltage_sag_l1_count":12.000,"voltage_sag_l2_count":12.000,"voltage_sag_l3_count":19.000,"voltage_swell_l1_count":5055.000,"voltage_swell_l2_count":1950.000,"voltage_swell_l3_count":0.000,"any_power_fail_count":12.000,"long_power_fail_count":2.000,"total_gas_m3":5175.363,"gas_timestamp":241106093006,"gas_unique_id":"redacted","external":[{"unique_id":"redacted","type":"gas_meter","timestamp":241106093006,"value":5175.363,"unit":"m3"}]}` assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh)
assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh)
assert.Equal(t, float64(203), res.ActivePowerW)
assert.Equal(t, float64(226), res.ActiveVoltageV)
assert.Equal(t, float64(1.091), res.ActiveCurrentA)
assert.Equal(t, float64(228), res.ActiveVoltageL1V)
- Add configurable phase parameter to charger template (default: 1) - Ensures consistency with meter configuration - Enables correct phase reporting for socket power monitoring
Contributor
Author
Updates based on code reviewI've made the following improvements to address code quality and maintainability: Refactoring changes
These changes improve code quality without affecting functionality - all existing configurations remain backwards compatible. |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
TotalEnergyyou now rely solely onTotalPowerImportkWh/TotalPowerExportkWh; consider keeping a fallback to the legacy T1–T4 fields for devices/firmware that might not expose the aggregated totals to avoid breaking older meters. - The
isSinglePhasehelper depends on an explicit whitelist ofProductTypevalues; you might want to either normalize/relax the comparison or base the decision on the presence ofActiveCurrentA/ActiveVoltageVto be more robust to new or slightly different product type strings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TotalEnergy` you now rely solely on `TotalPowerImportkWh`/`TotalPowerExportkWh`; consider keeping a fallback to the legacy T1–T4 fields for devices/firmware that might not expose the aggregated totals to avoid breaking older meters.
- The `isSinglePhase` helper depends on an explicit whitelist of `ProductType` values; you might want to either normalize/relax the comparison or base the decision on the presence of `ActiveCurrentA`/`ActiveVoltageV` to be more robust to new or slightly different product type strings.
## Individual Comments
### Comment 1
<location> `meter/homewizard/connection.go:164-170` </location>
<code_context>
- if c.usage == "pv" {
+
+ // Single-phase meters only have one current reading
+ if isSinglePhase(c.ProductType) {
+ current := res.ActiveCurrentA
+ if c.usage == "pv" || c.usage == "battery" {
+ current = -current
+ }
+
+ l1, l2, l3 := mapValueToPhase(current, c.phase)
+ return l1, l2, l3, err
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify behavior for single-phase devices where ActiveCurrentL1A is also provided.
`DataResponse` for single-phase meters also exposes `ActiveCurrentL1A`. If a device reports both `ActiveCurrentA` and `ActiveCurrentL1A` with different semantics (e.g. summed/signed vs. phase-specific), this mapping could diverge from how three-phase devices use `ActiveCurrentLxA`. Consider deriving single-phase currents from the L1/L2/L3 fields for consistency, or explicitly assert/validate that `ActiveCurrentA` matches the L1 reading for these product types.
</issue_to_address>
### Comment 2
<location> `meter/homewizard/types.go:20-29` </location>
<code_context>
- ActiveVoltageL2V float64 `json:"active_voltage_l2_v"`
- ActiveVoltageL3V float64 `json:"active_voltage_l3_v"`
+ ActivePowerW float64 `json:"active_power_w"`
+ TotalPowerImportkWh float64 `json:"total_power_import_kwh"`
+ TotalPowerExportkWh float64 `json:"total_power_export_kwh"`
+ ActiveCurrentA float64 `json:"active_current_a"`
</code_context>
<issue_to_address>
**question (bug_risk):** Switching from tariff-based totals to aggregate import/export fields may affect compatibility with existing devices/firmware.
This change assumes `total_power_import_kwh` and `total_power_export_kwh` exist on all supported HomeWizard devices/firmware. Please verify that older KWH/P1 versions don’t expose only `TotalPowerImportT{1..4}kWh` / `TotalPowerExportT{1..4}kWh`. If some do, consider preferring the aggregate fields when available and falling back to summing the tariff fields when aggregates are missing or zero.
</issue_to_address>
### Comment 3
<location> `meter/homewizard/types_test.go:66` </location>
<code_context>
- assert.Equal(t, float64(13823.608), res.TotalPowerExportT1kWh+res.TotalPowerExportT2kWh+res.TotalPowerExportT3kWh+res.TotalPowerExportT4kWh)
+ assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh)
+ assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh)
assert.Equal(t, float64(203), res.ActivePowerW)
assert.Equal(t, float64(228), res.ActiveVoltageL1V)
assert.Equal(t, float64(226), res.ActiveVoltageL2V)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `NewConnection` phase validation and default phase behavior
Since `NewConnection` now validates that `phase` is in [1,3], add tests that:
- Call `NewConnection` with invalid phases (e.g. 0, 4, and a negative) and assert it returns an error mentioning the valid range.
- Use the default `Phase: 1` in charger and meter configs, ensure the resulting `Connection` is valid, and confirm the internal `phase` is 1.
This will guard against regressions in phase validation and defaulting during future refactors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+164
to
+170
| if isSinglePhase(c.ProductType) { | ||
| current := res.ActiveCurrentA | ||
| if c.usage == "pv" || c.usage == "battery" { | ||
| current = -current | ||
| } | ||
|
|
||
| l1, l2, l3 := mapValueToPhase(current, c.phase) |
Contributor
There was a problem hiding this comment.
question (bug_risk): Clarify behavior for single-phase devices where ActiveCurrentL1A is also provided.
DataResponse for single-phase meters also exposes ActiveCurrentL1A. If a device reports both ActiveCurrentA and ActiveCurrentL1A with different semantics (e.g. summed/signed vs. phase-specific), this mapping could diverge from how three-phase devices use ActiveCurrentLxA. Consider deriving single-phase currents from the L1/L2/L3 fields for consistency, or explicitly assert/validate that ActiveCurrentA matches the L1 reading for these product types.
Comment on lines
+20
to
+29
| TotalPowerImportkWh float64 `json:"total_power_import_kwh"` | ||
| TotalPowerExportkWh float64 `json:"total_power_export_kwh"` | ||
| ActiveCurrentA float64 `json:"active_current_a"` | ||
| ActiveCurrentL1A float64 `json:"active_current_l1_a"` | ||
| ActiveCurrentL2A float64 `json:"active_current_l2_a"` | ||
| ActiveCurrentL3A float64 `json:"active_current_l3_a"` | ||
| ActiveVoltageV float64 `json:"active_voltage_v"` | ||
| ActiveVoltageL1V float64 `json:"active_voltage_l1_v"` | ||
| ActiveVoltageL2V float64 `json:"active_voltage_l2_v"` | ||
| ActiveVoltageL3V float64 `json:"active_voltage_l3_v"` |
Contributor
There was a problem hiding this comment.
question (bug_risk): Switching from tariff-based totals to aggregate import/export fields may affect compatibility with existing devices/firmware.
This change assumes total_power_import_kwh and total_power_export_kwh exist on all supported HomeWizard devices/firmware. Please verify that older KWH/P1 versions don’t expose only TotalPowerImportT{1..4}kWh / TotalPowerExportT{1..4}kWh. If some do, consider preferring the aggregate fields when available and falling back to summing the tariff fields when aggregates are missing or zero.
| assert.Equal(t, float64(13823.608), res.TotalPowerExportT1kWh+res.TotalPowerExportT2kWh+res.TotalPowerExportT3kWh+res.TotalPowerExportT4kWh) | ||
| assert.Equal(t, float64(18664.997), res.TotalPowerImportkWh) | ||
| assert.Equal(t, float64(13823.608), res.TotalPowerExportkWh) | ||
| assert.Equal(t, float64(203), res.ActivePowerW) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for NewConnection phase validation and default phase behavior
Since NewConnection now validates that phase is in [1,3], add tests that:
- Call
NewConnectionwith invalid phases (e.g. 0, 4, and a negative) and assert it returns an error mentioning the valid range. - Use the default
Phase: 1in charger and meter configs, ensure the resultingConnectionis valid, and confirm the internalphaseis 1.
This will guard against regressions in phase validation and defaulting during future refactors.
Contributor
Author
Test Setup ReferenceFor reference, here's my actual test setup showing the improvements in action: Hardware configuration:
This setup demonstrates the new single-phase support (HWE-KWH1) and battery meter functionality (HWE-KWH3) working together in a real-world installation. 
|
andig
reviewed
Jan 28, 2026
| *request.Helper | ||
| uri string | ||
| usage string | ||
| phase int |
Member
There was a problem hiding this comment.
The entire single phase mapping is a bad idea, please remove.
Contributor
Author
There was a problem hiding this comment.
But why is it a bad idea? Load limits apply to each phase and so the measurements should be counted to the right phase.
Contributor
Author
There was a problem hiding this comment.
But why is it a bad idea? Load limits apply to each phase and so the measurements should be counted to the right phase.
Member
There was a problem hiding this comment.
We‘re not doing this for any device
Member
There was a problem hiding this comment.
Please remove or open fresh pr without
Member
|
Sorry, I don‘t have the capacity to review ai-generated prs. |
Contributor
Author
Do I understand it correctly that you are not willing to review the pull request because the pr description is ai generated? |
Member
|
It doesn‘t help, instead just creates mental load and false claims. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves HomeWizard kWh meter support with better single-phase meter handling and adds battery meter functionality.
Changes
Single-phase meter improvements
ActiveVoltageVandActiveCurrentAfields instead of phase-specific fields for single-phase metersphaseparameter (1-3) to specify which phase a single-phase meter is installed onBattery meter support
batteryoption to theusageparameter (alongside existingpvandcharge)TotalPowerExportkWhfor battery energy trackingConfiguration
phaseparameter to meter template (default: 1, options: 1-3, advanced setting)Testing
Tested with:
Breaking Changes
None. All changes are backwards compatible with default values.