Skip to content

Modbus: unify delay and timeout settings (BC) - #31996

Merged
andig merged 9 commits into
masterfrom
feat/modbus-cache
Jul 23, 2026
Merged

Modbus: unify delay and timeout settings (BC)#31996
andig merged 9 commits into
masterfrom
feat/modbus-cache

Conversation

@andig

@andig andig commented Jul 20, 2026

Copy link
Copy Markdown
Member

Adds delay and timeout as first-class modbus settings, available consistently everywhere modbus is configured — template, yaml and Go device level. Fixes #31972.

util/modbus

  • Settings and TcpSettings gain Delay and Timeout fields
  • New (Tcp)Settings.Connection(ctx) method creates the connection and applies both (optional protocol override for forced-proto devices, e.g. ABL/ASCII)

Templates

  • delay/timeout added to the modbus include: advanced params on all modbus connection types, rendered by modbus.tpl when set — available in all 188 modbus templates
  • Device-specific defaults can be set as attributes on the modbus param (like id/port/baudrate/comset); carried over:
    • luxtronik: timeout: 10s
    • huawei-sun2000-inverter/-hybrid: timeout: 15s
    • wattsonic: delay: 100ms
  • Hand-crafted timeout/delay params in abl, pracht-alpha, luxtronik, wattsonic, huawei-sun2000-*, solaredge-inverter marked deprecated (kept, not removed) and their per-source render lines dropped — the config keys stay valid and values continue to apply via the modbus include
  • The shared param injection skips names a template already defines, so deprecated stubs stay authoritative

Devices

  • All native modbus chargers/meters (~55 files) migrated to settings.Connection(ctx)-based constructors taking modbus.(Tcp)Settings — delay/timeout now work uniformly
  • Hand-crafted delay defaults carried over as user-overridable settings: Heidelberg EC delay: 100ms, EVSE-DIN delay: 200ms, Em2Go delay: 60ms, Alphatec delay: 20ms
  • plugin/modbus, plugin/sunspec, meter/mbmd and the modbus proxy use the same mechanism (connectdelay unchanged)
  • Device-level cache handling (Bender, Hoymiles DTU, Ambibox) is unchanged

🤖 Generated with Claude Code

@andig andig added the devices Specific device support label Jul 20, 2026
@github-actions github-actions Bot added the enhancement New feature or request label Jul 20, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 2 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="util/templates/template_modbus.go" line_range="93-95" />
<code_context>

 		for _, p := range typeParams {
 			// don't overwrite custom values
-			if values[p.Name] != nil {
+			if v, ok := values[p.Name]; ok && v != nil && v != "" {
 				continue
 			}
</code_context>
<issue_to_address>
**issue (bug_risk):** Comparing interface values to empty string can panic for non-comparable underlying types.

`v != ""` on an `any` can panic when `v` holds a non-comparable type (e.g. map, slice), since interface equality requires a comparable underlying type. Please avoid direct comparison and instead type-assert to `string` before comparing, or use a helper that inspects the concrete type to decide whether it is "empty."
</issue_to_address>

### Comment 2
<location path="meter/cfos.go" line_range="40" />
<code_context>
 	}

-	return NewCfosPowerBrain(ctx, cc.URI, cc.ID)
+	return NewCfosPowerBrain(ctx, cc)
 }

</code_context>
<issue_to_address>
**issue (bug_risk):** Same type mismatch when passing config to NewCfosPowerBrain.

`cc` is an anonymous struct that embeds `modbus.TcpSettings`, but `NewCfosPowerBrain` now expects a `modbus.TcpSettings`. You should pass `cc.TcpSettings` instead of `cc`; otherwise this will not compile.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread util/templates/template_modbus.go Outdated
Comment thread meter/cfos.go
Comment thread util/templates/defaults.yaml
@andig andig changed the title Modbus: unified cache, delay and timeout settings Modbus: unified delay and timeout settings Jul 20, 2026
@andig andig changed the title Modbus: unified delay and timeout settings Modbus: unify delay and timeout settings Jul 20, 2026
@andig
andig requested a review from premultiply July 20, 2026 19:01
@andig andig added infrastructure Basic functionality and removed enhancement New feature or request devices Specific device support labels Jul 20, 2026
@andig andig changed the title Modbus: unify delay and timeout settings Modbus: unify delay and timeout settings (BC) Jul 20, 2026
@andig

andig commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

@premultiply had wanted to do this for a very long time...

@andig

andig commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

Why are we getting spammed here?

@andig

andig commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

# Conflicts:
#	templates/definition/meter/wattsonic.yaml

Copilot AI commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merged origin/master into the branch. There was one conflict in templates/definition/meter/wattsonic.yaml — master had removed the wrong grid currents section (#32049) while this branch still had it. Resolved by accepting master's removal (keeping only the powers section). Go code compiles cleanly after the merge.

Copilot finished work on behalf of andig July 23, 2026 10:09
@andig
andig merged commit 02a6625 into master Jul 23, 2026
9 of 10 checks passed
@andig
andig deleted the feat/modbus-cache branch July 23, 2026 15:03
@CiNcH83

CiNcH83 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

How about connectdelay? It is kind of weird that it is added to the first property of the grid/pv/battery blocks...

@andig

andig commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

It is kind of weird that it is added to the first property of the grid/pv/battery blocks...

wdym?

@CiNcH83

CiNcH83 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Inside the Huawei template, the connectdelay parameter is always part of the first property (in fact the power property) in each of the three blocks (grid/pv/battery).

Search for connectdelay in the template. You'll find it 3 times, one time in each block.

@andig

andig commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

It's the odd one out. With only a single device ever needing this I'd much rather remove it entirely...

@CiNcH83

CiNcH83 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Didn't know it was only für Huawei. Will try to reach out to them.

@CiNcH83

CiNcH83 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude says that swallowed requests after a connect is a common problem with many Modbus devices and that it is advisable to have a connect delay of 0,5–2s.

@andig

andig commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Claude says many things during a long day...

@CiNcH83

CiNcH83 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

mbproxy and modbus-proxy both have a connect delay parameter.

@mptr8956

mptr8956 commented Aug 9, 2026

Copy link
Copy Markdown

mbproxy and modbus-proxy both have a connect delay parameter.

exactly the reason i need to use external modbus proxy - with the huawei inverter i need a timeout of 15s and connection_time of 2s to work properly with tiagocouthinos modbus-proxy. when i configure it with evcc, it does not work at all. maybe worth implementing it here as well?

@andig

andig commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Would it help if we honored delay after connect to get rid of the connect delay? It‘s really only Huawei that needs this, very annoying…

@CiNcH83

CiNcH83 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Huawei requires a 1-2s delay after connect, also depending on how old the FW is. Applying a multi-second delay to requests as well is not a good idea IMHO.

@mptr8956 The 1s delay works for most setups. Maybe you want to open a discussion where we can discuss your setup.

@mptr8956

mptr8956 commented Aug 9, 2026

Copy link
Copy Markdown

Would it help if we honored delay after connect to get rid of the connect delay? It‘s really only Huawei that needs this, very annoying…

I think so :)

@CiNcH83 : opened the discussion here. #32674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Basic functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Charger i/o timeout

4 participants