Add Modbus service for dynamic parameter reading

[iseeberg79]

Adds HTTP service endpoint /api/modbus/params for reading device parameters based on template definitions without creating a full device instance.

Features (fixes #25857):

• Dynamic register reading using template configurations
• Automatic scaling and type conversion

Enables auto-filling of device parameters in the configuration UI by reading actual values from the device.

Usage examples:

• ModbusTCP

  - name: capacity
    type: int
    unit: kWh
    advanced: true
    usages: ["battery"]
    service: modbus/params?uri={host}:{port}&id={id}&address=1068&type=holding&encoding=float32s&scale=0.001&resulttype=int

• rs485serial

  - name: capacity
    type: float
    unit: kWh
    advanced: true
    usages: ["battery"]
    service: modbus/params?device={device}&baudrate={baudrate}&comset={comset}&id={id}&address=1068&type=holding&encoding=float32s&scale=0.001

[andig]

Das ist grausig viel Logik. Warum muss der modbus Service etwas vin templates wissen? Die sollten ihm egal sein! Spannend ist allerdings die Modbus Config. Dafür hatte ich keine gute Idee :/

[iseeberg79]

Das ist grausig viel Logik. Warum muss der modbus Service etwas vin templates wissen? Die sollten ihm egal sein! Spannend ist allerdings die Modbus Config. Dafür hatte ich keine gute Idee :/

Die zu ermittelnden Register lädt er aus dem Template zur Laufzeit, nachdem die Modbus-Konfiguration verfügbar ist. Dafür der Name des Templates. Man könnte das Laden des Template ggf. überflüssig machen: statt properties, mehr URL Parameter. Auch deinen Hinweis mit dem modbus-Plugin.. - schau' ich mir an.

Die UI Integration ist etwas schwierig, wg. der Einschränkung mit den dataLists und der Positionierung des clear-Feldes bei Feldern mit Einheit. Das ist noch unschön, überlappt aber so aktuell nicht mehr - die Ausrichtung passt aktuell nicht.
Ich stell' das mal hinten an erstmal... - ich hatte zuerst die Werte direkt in die Felder geschrieben, wenn initial - das widerspricht aber der aktuellen Integration/Tests aus demo-service. Die Dropdown's sind schon ganz charmant...

[andig]

Die zu ermittelnden Register lädt er aus dem Template zur Laufzeit,

Die gehören in den Serviceaufruf. HA zeigt wie's geht:

service: homeassistant/entities?uri={uri}&domain=sensor

Die uri wird dynamisch vom UI da rein gebastelt. Ich weiss nur nicht ob/wie wir das für Modbus zum Leben erwecken können.

/cc @naltatis

[iseeberg79]

Ich bin einen Entwurf weiter, klappt soweit auch mit URL Parametern, ist übersichtlicher. Ein Update heute Abend!

Problematisch ist die Parallelität der Browseranfragen und Wiederverwendung des Modbus-Plugins, theoretisch läuft es.

Praktisch blockierte da gerade etwas, was einen weiteren echten Gerätetest braucht.

Der aktuelle Ansatz ist unnötig aufwändig, läuft aber.

[andig]

Checks gerne ein, ich kann auch testen.

[iseeberg79]

Checks gerne ein, ich kann auch testen.

jetzt aber...

[iseeberg79]

@naltatis having values for optional fields with units would really be useful. I've introduced an idea here - could you have a look? logic + UI changes ...

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The modbus service cache key only includes connection string and address; consider including other query attributes that affect the response (e.g. ID, type, encoding, scale, resultType) to avoid returning stale or incorrect cached values for different queries hitting the same address.
• In readRegisterValue, the direct type assertions to plugin.StringGetter and plugin.FloatGetter can panic if the plugin implementation changes; consider using checked assertions (with ok) and returning a clear error if the interface is not supported.
• In getParams, modbus read errors are silently converted to an empty array with HTTP 200; if feasible, surfacing an error (or at least logging it) would make diagnosing connection/encoding issues significantly easier.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The modbus service cache key only includes connection string and address; consider including other query attributes that affect the response (e.g. ID, type, encoding, scale, resultType) to avoid returning stale or incorrect cached values for different queries hitting the same address.
- In `readRegisterValue`, the direct type assertions to `plugin.StringGetter` and `plugin.FloatGetter` can panic if the plugin implementation changes; consider using checked assertions (with `ok`) and returning a clear error if the interface is not supported.
- In `getParams`, modbus read errors are silently converted to an empty array with HTTP 200; if feasible, surfacing an error (or at least logging it) would make diagnosing connection/encoding issues significantly easier.

## Individual Comments

### Comment 1
<location> `assets/js/components/Config/PropertyField.vue:5-14` </location>
<code_context>
+	<div v-if="unitValue" :class="sizeClass">
+		<div class="d-flex">
+			<div class="position-relative flex-grow-1">
+				<input
+					:id="id"
+					v-model="value"
+					:list="datalistId"
+					:type="inputType"
+					:step="step"
+					:placeholder="placeholder"
+					:required="required"
+					:aria-describedby="id + '_unit'"
+					:class="`${datalistId && serviceValues.length > 0 ? 'form-select' : 'form-control'} ${showClearButton ? 'has-clear-button' : ''} ${invalid ? 'is-invalid' : ''}`"
+					class="text-end"
+					style="border-top-right-radius: 0; border-bottom-right-radius: 0"
+					:autocomplete="masked || datalistId ? 'off' : null"
</code_context>

<issue_to_address>
**issue (bug_risk):** The input is now always right-aligned, ignoring the previous `endAlign` prop behavior.

This removes the previous `:class="{ 'text-end': endAlign }"` behavior and forces right alignment in all cases. Callers that relied on left alignment when `endAlign` is false will change. If right alignment should remain optional, move `text-end` back into the dynamic `:class` and keep it controlled by `endAlign`.
</issue_to_address>

### Comment 2
<location> `util/service/modbus.go:71-76` </location>
<code_context>
+		return
+	}
+
+	// Create cache key from connection string and register address
+	connStr := query.URI
+	if connStr == "" {
+		connStr = query.Device
+	}
+	cacheKey := fmt.Sprintf("%s:%d", connStr, query.Address)
+
+	// Check cache first
</code_context>

<issue_to_address>
**issue (bug_risk):** Cache key does not include ID, register details, or other connection options, which can lead to incorrect cached values.

The key currently only uses URI/device and `Address`, but the register value also depends on `ID`, `Comset`, `Baudrate`, `Encoding`, `Scale`, register length, and possibly other `Register` fields. Different devices/registers can therefore collide on the same cache entry and return wrong values. Please include all relevant connection and register parameters in the cache key, even if it becomes longer.
</issue_to_address>

### Comment 3
<location> `util/service/modbus.go:87-89` </location>
<code_context>
+	}
+	mu.RUnlock()
+
+	// Read value from modbus using plugin
+	// Use background context so connection isn't tied to HTTP request lifecycle
+	value, err := readRegisterValue(context.TODO(), query)
+	if err != nil {
+		jsonWrite(w, []string{}) // Return empty array on error
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Using `context.TODO()` drops request cancellation and timeouts for the modbus read.

The HTTP handler already exposes a cancellable context via `req.Context()`. Passing `context.TODO()` here means the modbus read will continue even after client disconnects or request timeout. Please propagate `req.Context()` into `readRegisterValue` so long-running modbus operations are properly canceled with the HTTP request.

```suggestion
	// Read value from modbus using plugin
	// Propagate HTTP request context so modbus reads are canceled on client disconnect/timeout
	value, err := readRegisterValue(req.Context(), query)
```
</issue_to_address>

### Comment 4
<location> `util/service/modbus.go:25-28` </location>
<code_context>
+	timestamp time.Time
+}
+
+var (
+	cache    = make(map[string]cacheEntry)
+	mu       sync.RWMutex
+	cacheTTL = 1 * time.Minute // Cache for 1 minute
+)
+
</code_context>

<issue_to_address>
**suggestion (performance):** Global cache map grows without eviction of old keys, which may lead to unbounded memory usage over time.

The TTL check avoids serving stale data but entries are never deleted from `cache`. In high-cardinality environments this can lead to unbounded growth over the lifetime of the process. Please add periodic pruning of expired entries or switch to a bounded cache (e.g., LRU) to cap memory usage.

Suggested implementation:

```golang
var (
	cache    = make(map[string]cacheEntry)
	mu       sync.RWMutex
	// cacheTTL controls how long entries are considered valid.
	cacheTTL = 1 * time.Minute // Cache for 1 minute

	// cachePruneInterval controls how often expired entries are removed to bound memory use.
	cachePruneInterval = 30 * time.Second
)

func init() {
	go func() {
		ticker := time.NewTicker(cachePruneInterval)
		defer ticker.Stop()

		for range ticker.C {
			pruneExpiredCacheEntries()
		}
	}()
}

// pruneExpiredCacheEntries removes expired entries from the cache to prevent unbounded growth.
func pruneExpiredCacheEntries() {
	mu.Lock()
	defer mu.Unlock()

	// Nothing to do if TTL is not positive.
	if cacheTTL <= 0 {
		return
	}

	now := time.Now()
	for key, entry := range cache {
		if now.Sub(entry.timestamp) > cacheTTL {
			delete(cache, key)
		}
	}
}

```

This assumes:
1. `cacheTTL` is the same TTL used when reading from the cache elsewhere in this file.
2. No other logic depends on expired entries remaining in the map (if so, that code should be updated to not assume presence of stale keys).

If there is test coverage around the cache behavior, you may want to:
- Add a unit test that sets a short `cacheTTL`, invokes `pruneExpiredCacheEntries()` directly, and asserts that expired entries are removed while fresh ones remain.
</issue_to_address>

### Comment 5
<location> `util/service/modbus_test.go:35` </location>
<code_context>
+	assert.Equal(t, "[]\n", w.Body.String(), "Expected empty array for failed read")
+}
+
+func TestGetParams_WithResultType(t *testing.T) {
+	// Test with resulttype parameter
+	req := httptest.NewRequest("GET", "/params?uri=192.168.1.1:502&id=1&address=1068&type=holding&encoding=float32s&resulttype=int", nil)
</code_context>

<issue_to_address>
**suggestion (testing):** Add unit tests for applyCast (and optionally callGetter) so type conversion is verified independently of Modbus connectivity.

The `WithResultType` tests currently fail at the Modbus connection step and only assert an empty array, so the `applyCast` logic (including string vs numeric branches) is never exercised. Please add focused unit tests that invoke `applyCast` directly with various inputs and `ResultType` values (`int`, `float`, `string`, and an unknown type) and assert both the resulting types and values. You can also add a small test for `callGetter` using a fake getter that returns a known value and error, to verify both are propagated correctly.

Suggested implementation:

```golang
func TestGetParams_WithResultType(t *testing.T) {
	// Test with resulttype parameter
	req := httptest.NewRequest("GET", "/params?uri=192.168.1.1:502&id=1&address=1068&type=holding&encoding=float32s&resulttype=int", nil)
	w := httptest.NewRecorder()

	getParams(w, req)

	// Connection will fail (no real device), should return empty array
	assert.Equal(t, http.StatusOK, w.Code)
	assert.Equal(t, "[]\n", w.Body.String(), "Expected empty array for failed read")
}

func TestApplyCast_IntResultType(t *testing.T) {
	// numeric input -> int result
	value, err := applyCast(42.9, "int")
	require.NoError(t, err, "applyCast should not error for int result type")

	intVal, ok := value.(int)
	require.True(t, ok, "result should be an int")
	assert.Equal(t, 42, intVal, "numeric value should be truncated/converted to int")
}

func TestApplyCast_FloatResultType(t *testing.T) {
	// integer input -> float result
	value, err := applyCast(42, "float")
	require.NoError(t, err, "applyCast should not error for float result type")

	floatVal, ok := value.(float64)
	require.True(t, ok, "result should be a float64")
	assert.Equal(t, 42.0, floatVal, "integer value should be converted to float")
}

func TestApplyCast_StringResultType(t *testing.T) {
	// numeric input -> string result
	value, err := applyCast(12.34, "string")
	require.NoError(t, err, "applyCast should not error for string result type")

	strVal, ok := value.(string)
	require.True(t, ok, "result should be a string")
	assert.NotEmpty(t, strVal, "string result should not be empty")
	// depending on implementation this may be formatted differently; at minimum it should contain the numeric value
	assert.Contains(t, strVal, "12", "string result should contain the original numeric value")
}

func TestApplyCast_UnknownResultType(t *testing.T) {
	// unknown result type should be handled gracefully (typically by returning an error)
	_, err := applyCast(123, "unknown-type")
	assert.Error(t, err, "applyCast should return an error for unknown result type")
}

func TestCallGetter_PropagatesValueAndError(t *testing.T) {
	expectedValue := []any{float64(1.23)}
	expectedErr := errors.New("test error")

	// fake getter that returns known value and error
	getter := func() ([]any, error) {
		return expectedValue, expectedErr
	}

	result, err := callGetter(getter)

	assert.Equal(t, expectedValue, result, "callGetter should return the value from the getter")
	assert.Equal(t, expectedErr, err, "callGetter should return the error from the getter")
}

func TestGetParams_CompleteRequest(t *testing.T) {

```

1. Ensure the test file imports the additional packages used above:
   - Add `errors` from the standard library.
   - Add `require` from `github.com/stretchr/testify/require`.
   For example, in the import block of `util/service/modbus_test.go`:
   - Add:
     - ` "errors"`
     - ` "github.com/stretchr/testify/require"`
2. Adjust the test calls to match the real signatures of `applyCast` and `callGetter`:
   - If `applyCast` operates on slices or has a different parameter order or return type, update the tests to pass the correct input type (e.g. `[]any` or `[]float32`) and to assert on the correct return value shape.
   - If `callGetter` uses a different functional type (e.g. `func() (any, error)` instead of `func() ([]any, error)`), update the fake `getter` and the corresponding assertions accordingly.
3. If `applyCast` returns both a value and an error in a different order, or wraps errors, adapt the `require.NoError` / `assert.Error` expectations to match the actual behavior (e.g. expecting `nil` error and unchanged value when an unknown type is given, if that’s how it’s implemented).
</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.}

[andig]

Hatte ich hier nicht schonmal

structs.Map(query.Settings)

vorgeschlagen?

[andig]

uint/bool is mentioned here but not part of applyCast?

[iseeberg79]

focussed on mostly used type casts but you are right, added bool and removed uint from comment as it's covered by int cast which should be okay