Templates: render template-specific modbus defaults in instance mode

[premultiply]

Symptom

After upgrading to 0.306.3, existing Wallbe installations stop working with modbus i/o timeouts:

[lp-1 ] ERROR ... charger enabled: read tcp 192.168.13.3:51042->192.168.13.5:502: i/o timeout
[lp-1 ] ERROR ... charger status:  read tcp 192.168.13.3:51048->192.168.13.5:502: i/o timeout

The TCP connection itself succeeds, the device just doesn't answer.

Root cause

#29647 removed the deprecated wallbe/wallbe-meter/wallbe-pre2019/wallbe-pre2019-meter templates and the wallbe.go charger. Existing configs that still reference template: wallbe are now routed via the covers: directive in phoenix-ev-eth.yaml to the phoenix-ev-eth template (the underlying Phoenix controller is the same silicon as the Wallbe one, just with slightly different firmware).

phoenix-ev-eth declares the Wallbe/Phoenix Modbus slave id on its modbus param:

params:
  - name: modbus
    choice: ["tcpip"]
    id: 255

For a config such as

chargers:
  - name: Wallbee
    type: template
    template: wallbe
    host: 192.168.13.5
    port: 502

rendering in RenderModeInstance produced:

type: phoenix-ev-eth
id: 1                       # ← wrong, should be 255
# Modbus TCP
uri: 192.168.13.5:502
rtu: false

Modbus requests at slave id 1 reach a non-existing unit on the Wallbe controller, hence the timeouts. The legacy wallbe.yaml got away without this because it just rendered type: wallbe and the Go implementation hardcoded const wbSlaveID = 255, bypassing the modbus rendering path entirely.

The bug sits in util/templates/template_modbus.go ModbusValues:

for _, p := range typeParams {
    if values[p.Name] != nil { continue }      // (1) protect user-supplied values

    values[p.Name] = p.DefaultValue(renderMode) // (2) generic default → id = 1

    var defaultValue string
    switch p.Name {
    case ModbusParamId:
        if modbusParam.ID != 0 {
            defaultValue = strconv.Itoa(modbusParam.ID) // (3) "255" from template
        }
    ...
    }

    if defaultValue != "" {
        if renderMode == RenderModeInstance {
            t.SetParamDefault(p.Name, defaultValue) // (4) updates t.Params only
        } else {
            values[p.Name] = defaultValue            // (5) docs/tests: correct
        }
    }
}

In RenderModeInstance, step (4) writes the template-specific value only into the param struct's Default field. The values map - from which RenderResult builds the actual YAML - keeps the generic 1 set in step (2). So the per-template id: 255 directive is silently dropped at runtime. In RenderModeDocs/RenderModeUnitTest, step (5) writes back into values, which is why docs and tests rendered correctly and the regression went unnoticed.

The RenderModeInstance carve-out was introduced in #25029 to keep the Config UI in sync with the param struct without overwriting user-edited values. The user-protection part is, however, already handled by guard (1), so the special-case is unnecessary.

This regression affects any template that declares modbus defaults on its modbus param (id, port, baudrate, comset); the Wallbe → phoenix-ev-eth migration just made it visible because the gap between the generic default (id: 1) and the template-level default (id: 255) is large enough to break communication.

Fix

Always write template-specific modbus defaults into the render values map. For RenderModeInstance, also keep updating the param's Default to preserve the UI behavior from #25029.

 if defaultValue != "" {
-    if renderMode == RenderModeInstance {
-        t.SetParamDefault(p.Name, defaultValue)
-    } else {
-        values[p.Name] = defaultValue
-    }
+    values[p.Name] = defaultValue
+    if renderMode == RenderModeInstance {
+        t.SetParamDefault(p.Name, defaultValue)
+    }
 }

Guard (1) still ensures that user-supplied values are not overwritten.

Tests

Adds util/templates/template_modbus_test.go with regression coverage for:

• phoenix-ev-eth rendering produces id: 255 in all three render modes when the user does not supply an explicit id;
• user-supplied id still wins over the template default;
• every covered legacy template name (wallbe, wallbe-meter, wallbe-pre2019, wallbe-pre2019-meter) routes to phoenix-ev-eth and renders id: 255.

Fixes #29804

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In the TestModbusTemplateDefaultID loop, consider defining the render mode name mapping once (e.g., as a global or local var) instead of recreating the map[int]string on every iteration to keep the test code simpler and avoid per-iteration allocations.
• The three tests in template_modbus_test.go share repeated setup (host/port maps, template lookup by name); you could factor this into small helper functions to reduce duplication and make the intent of each test clearer.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In the `TestModbusTemplateDefaultID` loop, consider defining the render mode name mapping once (e.g., as a global or local var) instead of recreating the `map[int]string` on every iteration to keep the test code simpler and avoid per-iteration allocations.
- The three tests in `template_modbus_test.go` share repeated setup (`host`/`port` maps, template lookup by name); you could factor this into small helper functions to reduce duplication and make the intent of each test clearer.

## Individual Comments

### Comment 1
<location path="util/templates/template_modbus_test.go" line_range="33-35" />
<code_context>
+	}
+}
+
+// TestModbusTemplateUserIDOverridesTemplate ensures a user-supplied id wins
+// over the template default in all render modes.
+func TestModbusTemplateUserIDOverridesTemplate(t *testing.T) {
+	tmpl, err := ByName(Charger, "phoenix-ev-eth")
+	require.NoError(t, err)
</code_context>
<issue_to_address>
**issue (testing):** The test docstring says "in all render modes" but the test only covers RenderModeInstance.

The implementation only exercises RenderModeInstance, so this test doesn’t actually validate behavior across all modes. To align the test with the contract, please either (a) make it table-driven over all render modes (similar to TestModbusTemplateDefaultID), or (b) narrow the docstring to instance mode only. Given the bug context, (a) is preferable for preventing regressions in user override handling across render modes.
</issue_to_address>

### Comment 2
<location path="util/templates/template_modbus_test.go" line_range="28" />
<code_context>
+		_, values, err := tmpl.RenderResult(mode, map[string]any{
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that RenderModeInstance also updates the parameter default, not just the rendered values.

Right now this only checks that the rendered values include the template-specific ID. To fully cover the intended behavior, please also assert (for the RenderModeInstance path) that the corresponding param in tmpl.Params has its default updated after RenderResult. That way, future changes can’t drop the default from the Config UI while still passing this test.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (testing): The test docstring says "in all render modes" but the test only covers RenderModeInstance.

The implementation only exercises RenderModeInstance, so this test doesn’t actually validate behavior across all modes. To align the test with the contract, please either (a) make it table-driven over all render modes (similar to TestModbusTemplateDefaultID), or (b) narrow the docstring to instance mode only. Given the bug context, (a) is preferable for preventing regressions in user override handling across render modes.

[sourcery-ai]

suggestion (testing): Add an assertion that RenderModeInstance also updates the parameter default, not just the rendered values.

Right now this only checks that the rendered values include the template-specific ID. To fully cover the intended behavior, please also assert (for the RenderModeInstance path) that the corresponding param in tmpl.Params has its default updated after RenderResult. That way, future changes can’t drop the default from the Config UI while still passing this test.

[Copilot]

Pull request overview

Fixes a regression in template rendering where template-specific Modbus defaults (notably id: 255 for Wallbe/Phoenix controllers) were not applied to instance-mode YAML output, causing timeouts for migrated legacy wallbe* configurations.

Changes:

• Apply template-specific Modbus defaults to the render values map for all render modes (while still updating param defaults in RenderModeInstance for UI behavior).
• Add regression tests covering phoenix-ev-eth Modbus default id rendering across render modes and verifying legacy wallbe* names route via covers:.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
util/templates/template_modbus.go	Ensures template-specific Modbus defaults are reflected in rendered instance YAML while preserving UI default behavior.
util/templates/template_modbus_test.go	Adds regression coverage for phoenix-ev-eth Modbus default id rendering and legacy wallbe* cover routing.