Modbus: fix missing template delay and timeout defaults (#32694)

Author: andig

util/modbus/connection.go

@@ -11,9 +11,9 @@ import (
 type Connection struct {
 	*logger
 	meters.Connection
-	slaveID uint8 // duplicated from meters.Connection
-	logical meters.Logger
-	delay   time.Duration
+	physical *meterConnection
+	slaveID  uint8 // duplicated from meters.Connection
+	logical  meters.Logger
 }
 
 func (c *Connection) Addr() string {
@@ -24,35 +24,33 @@ func (c *Connection) Logger(logger meters.Logger) {
 	c.logical = logger
 }
 
+// Delay applies the delay to the shared physical connection
 func (c *Connection) Delay(delay time.Duration) {
-	c.delay = delay
+	c.physical.setDelay(delay)
 }
 
 func (c *Connection) Clone(slaveID uint8) *Connection {
 	return &Connection{
 		slaveID:    slaveID,
 		Connection: c.Connection.Clone(slaveID),
 		logger:     c.logger,
+		physical:   c.physical,
 	}
 }
 
-// TODO resolve conflicts
+// ConnectDelay applies the connect delay to the shared physical connection
 func (c *Connection) ConnectDelay(delay time.Duration) {
-	if delay > 0 {
-		c.Connection.ConnectDelay(delay)
-	}
+	c.physical.setConnectDelay(delay)
 }
 
-// TODO resolve conflicts
+// Timeout applies the timeout to the shared physical connection
 func (c *Connection) Timeout(timeout time.Duration) {
-	if timeout > 0 {
-		_ = c.Connection.Timeout(timeout)
-	}
+	c.physical.setTimeout(timeout)
 }
 
 func (c *Connection) exec(fun func() ([]byte, error)) ([]byte, error) {
 	return c.WithLogger(c.logical, func() ([]byte, error) {
-		time.Sleep(c.delay)
+		time.Sleep(c.physical.getDelay())
 
 		b, err := fun()
 		if err != nil {

util/modbus/modbus.go

@@ -103,9 +103,52 @@ type meterConnection struct {
 	meters.Connection
 	proto Protocol
 	refs  int // count of references; first connection has ref count 0
+
+	// largest value requested by any of the sharing logical connections
+	delay        time.Duration
+	connectDelay time.Duration
+	timeout      time.Duration
+
 	*logger
 }
 
+// setDelay applies the delay if larger than the current value
+func (c *meterConnection) setDelay(delay time.Duration) {
+	mu.Lock()
+	defer mu.Unlock()
+
+	c.delay = max(c.delay, delay)
+}
+
+func (c *meterConnection) getDelay() time.Duration {
+	mu.Lock()
+	defer mu.Unlock()
+
+	return c.delay
+}
+
+// setConnectDelay applies the connect delay if larger than the current value
+func (c *meterConnection) setConnectDelay(delay time.Duration) {
+	mu.Lock()
+	defer mu.Unlock()
+
+	if delay > c.connectDelay {
+		c.connectDelay = delay
+		c.Connection.ConnectDelay(delay)
+	}
+}
+
+// setTimeout applies the timeout if larger than the current value
+func (c *meterConnection) setTimeout(timeout time.Duration) {
+	mu.Lock()
+	defer mu.Unlock()
+
+	if timeout > c.timeout {
+		c.timeout = timeout
+		_ = c.Connection.Timeout(timeout)
+	}
+}
+
 var (
 	connections = make(map[string]*meterConnection)
 	mu          sync.Mutex
@@ -176,6 +219,7 @@ func NewConnection(ctx context.Context, uri, device, comset string, baudrate int
 		slaveID:    slaveID,
 		Connection: conn.Clone(slaveID),
 		logger:     conn.logger,
+		physical:   conn,
 	}
 
 	return res, nil

util/modbus/modbus_test.go

@@ -2,10 +2,40 @@ package modbus
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 )
 
+// TestSharedSettings ensures the largest delay and timeout wins for all
+// connections sharing the same physical connection
+func TestSharedSettings(t *testing.T) {
+	ctx := t.Context()
+	uri := "localhost:15020"
+
+	c1, err := Settings{URI: uri, ID: 1, Delay: 2 * time.Second, Timeout: time.Second}.Connection(ctx)
+	require.NoError(t, err)
+
+	c2, err := Settings{URI: uri, ID: 2, Delay: time.Second, Timeout: 3 * time.Second}.Connection(ctx)
+	require.NoError(t, err)
+
+	// unset settings don't reset the shared values
+	c3, err := Settings{URI: uri, ID: 3}.Connection(ctx)
+	require.NoError(t, err)
+
+	require.Same(t, c1.physical, c2.physical)
+	require.Same(t, c1.physical, c3.physical)
+	require.Same(t, c1.physical, c1.Clone(4).physical)
+
+	for _, c := range []*Connection{c1, c2, c3} {
+		require.Equal(t, 2*time.Second, c.physical.getDelay())
+		require.Equal(t, 3*time.Second, c.physical.timeout)
+	}
+
+	// timeout has been applied to the physical connection
+	require.Equal(t, 3*time.Second, c1.physical.Connection.Timeout(3*time.Second))
+}
+
 func TestParsePoint(t *testing.T) {
 	tc := []struct {
 		in  string

util/templates/template_modbus.go

@@ -69,9 +69,12 @@ func (t *Template) ModbusValues(renderMode int, values map[string]any) {
 		typeParams := modbusConfig.Types[iface].Params
 
 		for _, p := range typeParams {
-			// don't overwrite custom values
-			if values[p.Name] != nil {
-				continue
+			// don't overwrite custom values. Params the template deprecated in favour
+			// of the modbus definition are pre-populated with an empty string default.
+			if v := values[p.Name]; v != nil {
+				if s, ok := v.(string); !ok || s != "" {
+					continue
+				}
 			}
 
 			values[p.Name] = p.DefaultValue(renderMode)