aa55 udp: support block reads

[toeklk]

Fix #29094

This pull request is an for feature request #29094 in order to improve the stability of aa55udp for ET devices.

• Implement block read and caching to limit the number of UDP messages sent to the inverter
• Adds trace statements to make further debugging easier
• Refactor the code that became a lot more complex and split up in several file, with block read code as much as possible separated from register read code.

Currently still missing (and could potentially further improve stability): impose a delay between sending messages.

[sourcery-ai]

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

• When constructing an AA55UDP in block-read mode, consider validating that Offset is non-negative (and perhaps within a reasonable upper bound) early in NewAA55UDPFromConfig so misconfigurations fail fast instead of only surfacing later in decodeAt.
• The global responseCache in aa55udp_block.go currently grows without bound as new (raddr, pdu) combinations are used; consider adding a simple eviction strategy or maximum size to avoid unbounded memory usage over long runtimes or in dynamic environments.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- When constructing an `AA55UDP` in block-read mode, consider validating that `Offset` is non-negative (and perhaps within a reasonable upper bound) early in `NewAA55UDPFromConfig` so misconfigurations fail fast instead of only surfacing later in `decodeAt`.
- The global `responseCache` in `aa55udp_block.go` currently grows without bound as new (raddr, pdu) combinations are used; consider adding a simple eviction strategy or maximum size to avoid unbounded memory usage over long runtimes or in dynamic environments.

## Individual Comments

### Comment 1
<location path="plugin/aa55udp_block.go" line_range="28" />
<code_context>
+// responseCacheTTL must be long enough to cover all sequential source reads
+// within one evcc poll cycle (typically < 1 s), but short enough that the
+// next cycle fetches fresh data.
+const responseCacheTTL = 5 * time.Second
+
+// fetchBlock returns the response payload for p.pdu, serving from the cache
</code_context>
<issue_to_address>
**issue (bug_risk):** Cache TTL and comment seem inconsistent and may serve stale data for multiple poll cycles.

With a 5s TTL and sub‑second poll cycles, several cycles will reuse the same cached payload instead of fetching fresh data each time. Please either align the TTL with the stated intention (e.g. ~1s or tied to the poll interval) or update the comment to accurately describe the behavior.
</issue_to_address>

### Comment 2
<location path="plugin/aa55udp.go" line_range="108" />
<code_context>
-	}, nil
+	}
+
+	if cc.PDU != "" {
+		// Block read mode: caller supplies the full PDU hex and the byte
+		// offset of the desired value within the response payload.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Configuration silently prefers block mode when both PDU and register/count are set, which may be surprising.

When both `pdu` and `register`/`count`/`id` are set, the code silently prefers `pdu` and ignores the others, which is hard to spot. Consider rejecting such mixed configurations (e.g., if `PDU != ""` and any of `Register`, `Count`, or `Id` differ from their defaults), and return an explicit error instead.

Suggested implementation:

```golang
	if cc.PDU != "" {
		// Reject mixed configuration where both block-mode PDU and
		// register-based parameters are specified.
		if cc.Register != 0 || cc.Count != 0 || cc.Id != "" {
			return nil, fmt.Errorf("invalid configuration: PDU cannot be combined with register/count/id settings")
		}

		// Block read mode: caller supplies the full PDU hex and the byte
		// offset of the desired value within the response payload.
		pdu, err := buildPDUFromHex(cc.PDU)
		if err != nil {
			return nil, err
		}
		p.pdu = pdu
		p.offset = cc.Offset
		p.fetch = p.fetchBlock
	} else {
		// Register read mode: build a targeted single-register PDU; value is
		// always at offset 0 of the (2- or 4-byte) response payload.

```

1. Ensure `fmt` is imported at the top of `plugin/aa55udp.go`:
   - Add `fmt` to the existing import block, e.g.:
     `import ("fmt" /* other imports... */)`.
2. Confirm that the defaults used in the guard (`Register != 0`, `Count != 0`, `Id != ""`) match the actual zero/default values in your `cc` configuration type. If different defaults are used (e.g., `Count` defaults to 1), adjust the comparisons accordingly.
</issue_to_address>

### Comment 3
<location path="plugin/aa55udp.go" line_range="36" />
<code_context>
+	offset int    // byte offset into the response payload (0 for register reads)
+	decode string // int32be | uint32be | uint32nan | int16be | uint16be | float32be
 	scale  float64
+	fetch  func() ([]byte, error) // set to fetchRegister or fetchBlock at construction
 }

</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the fetch function field with an explicit mode enum, deriving the remote address from the UDP connection instead of storing it, and moving mode-specific setup into helpers to simplify AA55UDP’s control flow and construction.

You can keep all functionality while simplifying the control flow and constructor responsibilities:

### 1. Replace `fetch func()` with an explicit mode

Since you only have two modes, an enum + `switch` in `query` avoids the indirection of a function field and makes control flow clearer:

```go
type aa55Mode int

const (
	modeRegister aa55Mode = iota
	modeBlock
)

type AA55UDP struct {
	log    *util.Logger
	conn   *net.UDPConn
	pdu    []byte
	offset int
	decode string
	scale  float64
	mode   aa55Mode
	// remove: fetch func() ([]byte, error)
}
```

Then `query` becomes explicit:

```go
func (p *AA55UDP) query() (float64, error) {
	var (
		payload []byte
		err     error
	)

	switch p.mode {
	case modeRegister:
		payload, err = p.fetchRegister()
	case modeBlock:
		payload, err = p.fetchBlock()
	default:
		err = fmt.Errorf("aa55udp: unknown mode %d", p.mode)
	}

	if err != nil {
		return 0, err
	}

	v, err := decodeAt(payload, p.offset, p.decode)
	if err != nil {
		return 0, fmt.Errorf("aa55udp: %w", err)
	}

	return v * p.scale, nil
}
```

### 2. Avoid storing `raddr` on the struct

If `raddr` is only used for logging and cache keys in `fetchBlock`, you can derive it from the connected `UDPConn` when needed:

```go
func (p *AA55UDP) remoteKey() string {
	if addr := p.conn.RemoteAddr(); addr != nil {
		return addr.String()
	}
	return "" // or some fallback
}
```

Then, in `fetchBlock`:

```go
func (p *AA55UDP) fetchBlock() ([]byte, error) {
	key := p.remoteKey() + "|" + string(p.pdu) // or whatever existing key logic
	// use key for cache and logging
}
```

This lets you remove `raddr *net.UDPAddr` from `AA55UDP`.

### 3. Extract mode-specific setup into helpers

You can keep `NewAA55UDPFromConfig` readable by pushing mode-specific logic into small helpers rather than doing everything inline:

```go
func NewAA55UDPFromConfig(_ context.Context, other map[string]interface{}) (Plugin, error) {
	cc := /* existing config decode */

	if err := validateDecode(cc.Decode); err != nil {
		return nil, err
	}

	raddr, err := net.ResolveUDPAddr("udp4", net.JoinHostPort(cc.Host, "8899"))
	if err != nil {
		return nil, fmt.Errorf("aa55udp: resolve %s: %w", cc.Host, err)
	}

	conn, err := net.DialUDP("udp4", nil, raddr)
	if err != nil {
		return nil, fmt.Errorf("aa55udp: dial %s: %w", cc.Host, err)
	}

	p := &AA55UDP{
		log:    util.NewLogger("aa55udp"),
		conn:   conn,
		decode: cc.Decode,
		scale:  cc.Scale,
	}

	if cc.PDU != "" {
		if err := initBlockMode(p, cc); err != nil {
			return nil, err
		}
	} else {
		if err := initRegisterMode(p, cc); err != nil {
			return nil, err
		}
	}

	return p, nil
}
```

With small, focused helpers:

```go
func initBlockMode(p *AA55UDP, cc yourConfigType) error {
	pdu, err := buildPDUFromHex(cc.PDU)
	if err != nil {
		return err
	}
	p.pdu = pdu
	p.offset = cc.Offset
	p.mode = modeBlock
	return nil
}

func initRegisterMode(p *AA55UDP, cc yourConfigType) error {
	if cc.Count == 0 {
		return errors.New("aa55udp: count must be ≥ 1")
	}
	if cc.Id < 0 || cc.Id > 255 {
		return fmt.Errorf("aa55udp: id must be 0-255, got %d", cc.Id)
	}
	p.pdu = buildPDU(byte(cc.Id), cc.Register, cc.Count)
	p.offset = 0
	p.mode = modeRegister
	return nil
}
```

These changes keep your new block/offset/decode functionality intact while making the object’s behavior easier to reason about: construction clearly sets a mode, and `query` clearly selects the appropriate fetch path without a function pointer.
</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]

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

• The response cache size limit is only soft — when len(responseCache) >= responseCacheMaxSize and no entries are expired, a new entry is still added, so if you intend to bound memory usage you may want to enforce eviction (or skip insert) when the map remains at or above the maximum after the cleanup pass.
• In initBlockMode, rejecting any non-default register/count/id when pdu is set is quite strict and may surprise users migrating from register mode; consider either ignoring these fields with a warning or relaxing the constraints so configuration is less brittle while still preventing obviously conflicting settings.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The response cache size limit is only soft — when `len(responseCache) >= responseCacheMaxSize` and no entries are expired, a new entry is still added, so if you intend to bound memory usage you may want to enforce eviction (or skip insert) when the map remains at or above the maximum after the cleanup pass.
- In `initBlockMode`, rejecting any non-default `register/count/id` when `pdu` is set is quite strict and may surprise users migrating from register mode; consider either ignoring these fields with a warning or relaxing the constraints so configuration is less brittle while still preventing obviously conflicting settings.

## Individual Comments

### Comment 1
<location path="plugin/aa55udp_block.go" line_range="62-71" />
<code_context>
+	}
+
+	responseCacheMu.Lock()
+	// Evict expired entries before inserting to keep the map bounded.
+	if len(responseCache) >= responseCacheMaxSize {
+		now := time.Now()
+		for k, v := range responseCache {
+			if now.After(v.expiresAt) {
+				delete(responseCache, k)
+			}
+		}
+	}
+	responseCache[key] = cacheEntry{payload: payload, expiresAt: time.Now().Add(responseCacheTTL)}
+	responseCacheMu.Unlock()
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The cache size is not actually bounded by responseCacheMaxSize and can grow beyond it.

Eviction only runs when `len(responseCache) >= responseCacheMaxSize`, and you always insert afterward. If no entries are expired, the insert will still happen and `len(responseCache)` will grow beyond `responseCacheMaxSize`. To enforce a hard cap, either evict at least one entry when full and nothing has expired, or skip insertion in that case. If this soft-limit behavior is intentional, consider updating the comment/constant name to reflect that it’s not a strict bound.
</issue_to_address>

### Comment 2
<location path="plugin/aa55udp.go" line_range="36" />
<code_context>
+//	Block read:    whole register block, value extracted at a byte offset.
+//	               Multiple source blocks sharing the same (host, pdu) pair
+//	               share one UDP exchange per poll cycle via a response cache.
 type AA55UDP struct {
 	log    *util.Logger
 	conn   *net.UDPConn
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying configuration and query handling by replacing the explicit mode enum and init helpers with a single constructor that injects a fetch function strategy into AA55UDP.

You can keep all features (block mode, offset, caching, `uint32nan`) while simplifying the config/setup path and `query` by:

1. Dropping the `aa55Mode` enum and `switch` in `query`.
2. Removing `initBlockMode` / `initRegisterMode` and wiring the instance in one place.
3. Using a function field to select the fetch strategy at construction time.

That keeps the behavior (including any cache logic in `fetchBlock`) but removes a whole mode layer and extra helpers.

### Sketch of the simplification

```go
type AA55UDP struct {
	log    *util.Logger
	conn   *net.UDPConn
	pdu    []byte
	offset int
	decode string
	scale  float64

	fetch func() ([]byte, error) // inject strategy at construction
}
```

Constructor with a single validation/wiring path:

```go
func NewAA55UDPFromConfig(_ context.Context, other map[string]interface{}) (Plugin, error) {
	cc := struct {
		Host     string  `mapstructure:"host"`
		Id       int     `mapstructure:"id"`
		PDU      string  `mapstructure:"pdu"`
		Register uint16  `mapstructure:"register"`
		Count    uint16  `mapstructure:"count"`
		Offset   int     `mapstructure:"offset"`
		Decode   string  `mapstructure:"decode"`
		Scale    float64 `mapstructure:"scale"`
	}{
		Id:       int(aa55InverterAddr),
		Register: 0,
		Count:    2,
		Scale:    1.0,
	}
	if err := util.DecodeOther(other, &cc); err != nil {
		return nil, err
	}
	if err := validateDecode(cc.Decode); err != nil {
		return nil, err
	}

	raddr, err := net.ResolveUDPAddr("udp4", net.JoinHostPort(cc.Host, "8899"))
	if err != nil {
		return nil, fmt.Errorf("aa55udp: resolve %s: %w", cc.Host, err)
	}
	conn, err := net.DialUDP("udp4", nil, raddr)
	if err != nil {
		return nil, fmt.Errorf("aa55udp: dial %s: %w", cc.Host, err)
	}

	p := &AA55UDP{
		log:    util.NewLogger("aa55udp"),
		conn:   conn,
		decode: cc.Decode,
		scale:  cc.Scale,
	}

	if cc.PDU != "" {
		// Block mode
		if cc.Offset < 0 {
			return nil, fmt.Errorf("aa55udp: offset must be non-negative, got %d", cc.Offset)
		}
		pdu, err := buildPDUFromHex(cc.PDU)
		if err != nil {
			return nil, err
		}
		p.pdu = pdu
		p.offset = cc.Offset
		p.fetch = p.fetchBlock
	} else {
		// Register mode
		if cc.Count == 0 {
			return nil, errors.New("aa55udp: count must be ≥ 1")
		}
		if cc.Id < 0 || cc.Id > 255 {
			return nil, fmt.Errorf("aa55udp: id must be 0-255, got %d", cc.Id)
		}
		p.pdu = buildPDU(byte(cc.Id), cc.Register, cc.Count)
		p.offset = 0
		p.fetch = p.fetchRegister
	}

	return p, nil
}
```

`query` becomes a straight call to the injected strategy:

```go
func (p *AA55UDP) query() (float64, error) {
	payload, err := p.fetch()
	if err != nil {
		return 0, err
	}

	v, err := decodeAt(payload, p.offset, p.decode)
	if err != nil {
		return 0, fmt.Errorf("aa55udp: %w", err)
	}
	return v * p.scale, nil
}
```

This removes:

- The `aa55Mode` type and `mode` field.
- The `switch` in `query`.
- The separate `initBlockMode` / `initRegisterMode` helpers (their logic is now clearly localized in the constructor).

If you still want a guard against mixed block/register config, you can keep a simple, explicit condition in the constructor instead of encoding defaults into it, e.g.:

```go
if cc.PDU != "" && (cc.Register != 0 || cc.Count != 2 || cc.Id != int(aa55InverterAddr)) {
	return nil, errors.New("aa55udp: pdu cannot be combined with register/count/id settings")
}
```

That preserves your current semantics while avoiding an extra helper and mode enum.
</issue_to_address>

### Comment 3
<location path="plugin/aa55udp_block.go" line_range="29" />
<code_context>
+	responseCacheMaxSize = 64 // max number of (host, pdu) pairs to cache
+)
+
+var (
+	responseCache   = make(map[string]cacheEntry, responseCacheMaxSize)
+	responseCacheMu sync.Mutex
</code_context>
<issue_to_address>
**issue (complexity):** Consider encapsulating the response cache and key generation so that `fetchBlock` delegates caching concerns and becomes a simpler orchestration function.

You can keep the behavior but reduce complexity by:

1. Encapsulating the cache (hide global map/mutex behind a small type).
2. Extracting key/PDU-hex construction.
3. Simplifying `fetchBlock` by delegating cache concerns.

### 1. Encapsulate cache in a small type

This keeps the package-level singleton but removes raw global state from call sites:

```go
type responseCacheT struct {
	mu   sync.Mutex
	data map[string]cacheEntry
}

func newResponseCacheT() *responseCacheT {
	return &responseCacheT{
		data: make(map[string]cacheEntry, responseCacheMaxSize),
	}
}

func (c *responseCacheT) get(key string) ([]byte, bool) {
	c.mu.Lock()
	defer c.mu.Unlock()

	if entry, ok := c.data[key]; ok && time.Now().Before(entry.expiresAt) {
		return entry.payload, true
	}
	return nil, false
}

func (c *responseCacheT) put(key string, payload []byte) {
	c.mu.Lock()
	defer c.mu.Unlock()

	if len(c.data) >= responseCacheMaxSize {
		now := time.Now()
		for k, v := range c.data {
			if now.After(v.expiresAt) {
				delete(c.data, k)
			}
		}
	}

	c.data[key] = cacheEntry{
		payload:   payload,
		expiresAt: time.Now().Add(responseCacheTTL),
	}
}
```

Singleton (same behavior as your globals, but better localized):

```go
var responseCache = newResponseCacheT()
```

### 2. Extract key + PDU hex helper

This removes repetition and makes logging/readability better:

```go
func (p *AA55UDP) cacheKeyAndPDUHex() (string, string) {
	pduHex := hex.EncodeToString(p.pdu)
	key := p.conn.RemoteAddr().String() + "/" + pduHex
	return key, pduHex
}
```

### 3. Simplify `fetchBlock`

Use the helper + cache methods to flatten the logic:

```go
func (p *AA55UDP) fetchBlock() ([]byte, error) {
	key, pduHex := p.cacheKeyAndPDUHex()

	if payload, ok := responseCache.get(key); ok {
		p.log.TRACE.Printf("cache hit for %s pdu=%s", p.conn.RemoteAddr(), pduHex)
		return payload, nil
	}

	packet := append(p.pdu, modbusCRC16(p.pdu)...)

	raw, err := p.sendRecv(packet)
	if err != nil {
		return nil, err
	}

	payload, err := stripAA55Header(raw)
	if err != nil {
		return nil, fmt.Errorf("aa55udp: %w", err)
	}

	responseCache.put(key, payload)
	return payload, nil
}
```

This keeps all existing behavior (cache TTL, bounded size, logging, key structure) but:

- Removes raw global map/mutex usage from `fetchBlock`.
- Avoids repeated `hex.EncodeToString`.
- Separates responsibilities: `fetchBlock` orchestrates; cache type handles locking and eviction; helper builds keys.
</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]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="plugin/aa55udp.go" line_range="37" />
<code_context>
+//	Block read:    whole register block, value extracted at a byte offset.
+//	               Multiple source blocks sharing the same (host, pdu) pair
+//	               share one UDP exchange per poll cycle via a response cache.
 type AA55UDP struct {
-	log    *util.Logger
-	conn   *net.UDPConn
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying AA55UDP by removing explicit mode/minPayloadLen state, folding constructor and mode selection logic into NewAA55UDPFromConfig, and inlining the trivial cache key helper to make behavior easier to reason about.

You can keep all new features while simplifying the lifecycle and state a fair bit.

### 1. Drop `mode` and `minPayloadLen` from persistent state

`mode` and `minPayloadLen` don’t seem to be needed as struct invariants:

- `mode` only affects which fetch function is called.
- `minPayloadLen` is never used in the snippet and is always `offset + decodeSize(decode)`, which is derivable.

You can replace both with a single `useCache` flag and compute `minPayloadLen` locally where needed:

```go
type AA55UDP struct {
	log    *util.Logger
	conn   *net.UDPConn
	pdu    []byte
	offset int
	decode string
	scale  float64

	useCache bool // true for block-read/cached mode, false for simple register read
}
```

Then in `query` you don’t need `mode`:

```go
func (p *AA55UDP) query() (float64, error) {
	payload, err := p.fetch()
	if err != nil {
		return 0, err
	}

	v, err := decodeAt(payload, p.offset, p.decode)
	if err != nil {
		return 0, fmt.Errorf("aa55udp: %w", err)
	}
	return v * p.scale, nil
}

func (p *AA55UDP) fetch() ([]byte, error) {
	if p.useCache {
		return p.fetchWithCache()
	}
	return p.fetchDirect()
}
```

Where `fetchDirect` is the old “build packet / send / recv / strip header” logic and `fetchWithCache` wraps it with the shared-response cache. This keeps the two behaviors but avoids a stateful mode-machine.

`minPayloadLen` can be checked on demand right before decode if you still want an explicit length check:

```go
func (p *AA55UDP) query() (float64, error) {
	payload, err := p.fetch()
	if err != nil {
		return 0, err
	}

	if len(payload) < p.offset+decodeSize(p.decode) {
		return 0, fmt.Errorf("aa55udp: payload too short (len=%d, need=%d)", len(payload), p.offset+decodeSize(p.decode))
	}

	v, err := decodeAt(payload, p.offset, p.decode)
	// ...
}
```

### 2. Collapse constructor logic into one place

You can keep `validateDecode` but inline the “mode selection” and cross-field checks into `NewAA55UDPFromConfig` rather than via `initBlockMode`/`initRegisterMode`. That way all invariants are visible in one function:

```go
func NewAA55UDPFromConfig(_ context.Context, other map[string]interface{}) (Plugin, error) {
	cc := struct {
		Host     string  `mapstructure:"host"`
		Id       int     `mapstructure:"id"`
		PDU      string  `mapstructure:"pdu"`
		Register uint16  `mapstructure:"register"`
		Count    uint16  `mapstructure:"count"`
		Offset   int     `mapstructure:"offset"`
		Decode   string  `mapstructure:"decode"`
		Scale    float64 `mapstructure:"scale"`
	}{
		Id:       int(aa55InverterAddr),
		Count:    2,
		Scale:    1.0,
	}

	if err := util.DecodeOther(other, &cc); err != nil {
		return nil, err
	}
	if err := validateDecode(cc.Decode); err != nil {
		return nil, err
	}

	raddr, err := net.ResolveUDPAddr("udp4", net.JoinHostPort(cc.Host, "8899"))
	if err != nil {
		return nil, fmt.Errorf("aa55udp: resolve %s: %w", cc.Host, err)
	}
	conn, err := net.DialUDP("udp4", nil, raddr)
	if err != nil {
		return nil, fmt.Errorf("aa55udp: dial %s: %w", cc.Host, err)
	}

	p := &AA55UDP{
		log:    util.NewLogger("aa55udp"),
		conn:   conn,
		decode: cc.Decode,
		scale:  cc.Scale,
	}

	if cc.PDU != "" {
		// block mode
		if cc.Register != 0 || cc.Count != 2 || cc.Id != int(aa55InverterAddr) {
			return nil, errors.New("aa55udp: pdu cannot be combined with register/count/id settings")
		}
		if cc.Offset < 0 {
			return nil, fmt.Errorf("aa55udp: offset must be non-negative, got %d", cc.Offset)
		}
		pdu, err := buildPDUFromHex(cc.PDU)
		if err != nil {
			return nil, err
		}
		p.pdu = pdu
		p.offset = cc.Offset
		p.useCache = true
	} else {
		// register mode
		if cc.Count == 0 {
			return nil, errors.New("aa55udp: count must be ≥ 1")
		}
		if cc.Id < 0 || cc.Id > 255 {
			return nil, fmt.Errorf("aa55udp: id must be 0-255, got %d", cc.Id)
		}
		p.pdu = buildPDU(byte(cc.Id), cc.Register, cc.Count)
		p.offset = 0
		p.useCache = false
	}

	return p, nil
}
```

This preserves:

- Block-mode PDU hex and offset semantics.
- Register-mode `id/register/count` semantics.
- All cross-field validation.
- The caching distinction (via `useCache`).

But it removes `aa55Mode`, `initBlockMode`, `initRegisterMode`, and persistent `minPayloadLen`, making `AA55UDP`’s invariants easier to see.

### 3. Inline trivial helper `cacheKeyAndPDUHex`

`cacheKeyAndPDUHex` only concatenates `RemoteAddr` and `hex(PDU)` and doesn’t encode a non-trivial invariant. Inlining it where used keeps the cache logic self-evident:

```go
key := p.conn.RemoteAddr().String() + "/" + hex.EncodeToString(p.pdu)
```

If you still want a helper, consider a pure function instead of a method, to make it clear it doesn’t depend on hidden state:

```go
func cacheKey(addr net.Addr, pdu []byte) string {
	return addr.String() + "/" + hex.EncodeToString(pdu)
}
```

These changes keep all added capabilities (block reads, offsets, caching, new `uint32nan` decode) while reducing the amount of mode/state machinery you have to reason about.
</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]

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

• In NewAA55UDPFromConfig, the block/PDU mode guard (if cc.Register != 0 || cc.Count != 2 || cc.Id != int(aa55InverterAddr)) effectively rejects configurations where users still specify an inverter id (e.g. 247 for ET) even though the PDU already encodes it; consider instead ignoring register/count/id when pdu is set, or only checking that they are unset, to avoid surprising validation errors on existing configs.
• The response cache decodeSize/validateDecode split assumes decode was always prevalidated; since decodeSize returns 0 on unknown types, it might be safer to either panic/log or return an error when an unknown type is encountered to catch future misuses early instead of silently accepting an invalid configuration.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `NewAA55UDPFromConfig`, the block/PDU mode guard (`if cc.Register != 0 || cc.Count != 2 || cc.Id != int(aa55InverterAddr)`) effectively rejects configurations where users still specify an inverter id (e.g. 247 for ET) even though the PDU already encodes it; consider instead ignoring `register/count/id` when `pdu` is set, or only checking that they are unset, to avoid surprising validation errors on existing configs.
- The response cache `decodeSize`/`validateDecode` split assumes `decode` was always prevalidated; since `decodeSize` returns 0 on unknown types, it might be safer to either panic/log or return an error when an unknown type is encountered to catch future misuses early instead of silently accepting an invalid configuration.

## Individual Comments

### Comment 1
<location path="plugin/aa55udp.go" line_range="37" />
<code_context>
+	offset   int    // byte offset into the response payload (0 for register reads)
+	decode   string // int32be | uint32be | uint32nan | int16be | uint16be | float32be
+	scale    float64
+	useCache bool // true for block-read/cached mode, false for simple register read
 }

</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the fetch logic, config-mode resolution, and decode helpers so the new caching and block-read capabilities are driven by a single, simpler control flow.

You can keep all new capabilities while simplifying the control flow a bit.

### 1. Unify `fetch`, `fetchDirect`, and `fetchWithCache`

You don’t really need three methods and a `useCache` flag. The cache decision can be local to `fetch`, with a single `sendRecv` path.

Example:

```go
// fetch returns the response payload, using caching when enabled.
func (p *AA55UDP) fetch() ([]byte, error) {
	packet := append(p.pdu, modbusCRC16(p.pdu)...)

	// no caching: direct send/recv
	if !p.useCache {
		raw, err := p.sendRecv(packet)
		if err != nil {
			return nil, err
		}
		return stripAA55Payload(raw)
	}

	// caching: shared reads by (addr, pdu)
	key := p.conn.RemoteAddr().String() + "/" + hex.EncodeToString(p.pdu)

	if payload, ok := responseCache.get(key); ok {
		p.log.TRACE.Printf("cache hit for %s pdu=%s", p.conn.RemoteAddr(), hex.EncodeToString(p.pdu))
		return payload, nil
	}

	raw, err := p.sendRecv(packet)
	if err != nil {
		return nil, err
	}

	payload, err := stripAA55Payload(raw)
	if err != nil {
		return nil, err
	}

	responseCache.put(key, payload)
	return payload, nil
}

func stripAA55Payload(raw []byte) ([]byte, error) {
	payload, err := stripAA55Header(raw)
	if err != nil {
		return nil, fmt.Errorf("aa55udp: %w", err)
	}
	return payload, nil
}
```

This keeps `useCache`, preserves the cache behavior, and removes `fetchDirect`/`fetchWithCache` as separate concepts.

### 2. Normalize config mode selection

`NewAA55UDPFromConfig` currently has two branches with overlapping validation. You can centralize the “mode resolution” into one helper that returns a normalized config.

Example:

```go
type readConfig struct {
	pdu    []byte
	offset int
	useCache bool
}

func buildReadConfig(cc *struct {
	Host     string
	Id       int
	PDU      string
	Register uint16
	Count    uint16
	Offset   int
	Decode   string
	Scale    float64
}) (readConfig, error) {
	if err := validateDecode(cc.Decode); err != nil {
		return readConfig{}, err
	}

	// PDU/block mode
	if cc.PDU != "" {
		if cc.Register != 0 || cc.Count != 2 || cc.Id != int(aa55InverterAddr) {
			return readConfig{}, errors.New("aa55udp: pdu cannot be combined with register/count/id settings")
		}
		if cc.Offset < 0 {
			return readConfig{}, fmt.Errorf("aa55udp: offset must be non-negative, got %d", cc.Offset)
		}
		pdu, err := buildPDUFromHex(cc.PDU)
		if err != nil {
			return readConfig{}, err
		}
		return readConfig{pdu: pdu, offset: cc.Offset, useCache: true}, nil
	}

	// register mode
	if cc.Count == 0 {
		return readConfig{}, errors.New("aa55udp: count must be ≥ 1")
	}
	if cc.Id < 0 || cc.Id > 255 {
		return readConfig{}, fmt.Errorf("aa55udp: id must be 0-255, got %d", cc.Id)
	}
	pdu := buildPDU(byte(cc.Id), cc.Register, cc.Count)
	return readConfig{pdu: pdu, offset: 0, useCache: false}, nil
}
```

Then `NewAA55UDPFromConfig` becomes:

```go
cfg, err := buildReadConfig(&cc)
if err != nil {
	return nil, err
}

p := &AA55UDP{
	log:      util.NewLogger("aa55udp"),
	conn:     conn,
	pdu:      cfg.pdu,
	offset:   cfg.offset,
	decode:   cc.Decode,
	scale:    cc.Scale,
	useCache: cfg.useCache,
}
return p, nil
```

This removes duplicated validation and keeps all invariants in one place.

### 3. Centralize decode metadata

`validateDecode`, `decodeSize`, and `decodeAt` share the same decode universe. You can avoid divergence by keeping the metadata in one switch and deriving the helpers from it.

Example:

```go
type decodeInfo struct {
	size int
}

func decodeMeta(name string) (decodeInfo, bool) {
	switch name {
	case "float32be", "int32be", "uint32be", "uint32nan":
		return decodeInfo{size: 4}, true
	case "int16be", "uint16be":
		return decodeInfo{size: 2}, true
	default:
		return decodeInfo{}, false
	}
}

func validateDecode(decode string) error {
	if _, ok := decodeMeta(decode); !ok {
		return fmt.Errorf("aa55udp: unsupported decode %q (want int32be|uint32be|uint32nan|int16be|uint16be|float32be)", decode)
	}
	return nil
}

func decodeSize(decode string) int {
	if info, ok := decodeMeta(decode); ok {
		return info.size
	}
	return 0
}
```

`decodeAt` keeps the actual decoding logic but relies on the same set of names, reducing the chance of adding a new type in one place and forgetting another.
</issue_to_address>

### Comment 2
<location path="plugin/aa55udp_block.go" line_range="11" />
<code_context>
+	"time"
+)
+
+// cacheEntry holds a cached response payload with its expiration time.
+type cacheEntry struct {
+	payload   []byte
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the cache implementation, making the cache dependency explicit, and deduplicating PDU hex parsing to keep the feature with less code and mental overhead.

You can keep the feature while trimming quite a bit of complexity. Two concrete areas:

---

### 1. Simplify the cache: drop max size + eviction on `put`

Given the 2s TTL and small keyspace, the manual size limit and eviction on `put` adds more bookkeeping than benefit.

You can:

- Remove `responseCacheMaxSize`
- Drop `expiresAt` from the entry and store `createdAt` (or reuse `expiresAt` only for checks)
- In `get`, treat expired entries as a miss and delete them
- In `put`, just overwrite

That keeps all semantics (“no reuse across poll cycles”) with less mental overhead and less code in the hot path.

Example:

```go
type cacheEntry struct {
    payload   []byte
    expiresAt time.Time
}

const responseCacheTTL = 2 * time.Second

type responseCacheT struct {
    mu   sync.Mutex
    data map[string]cacheEntry
}

func newResponseCacheT() *responseCacheT {
    return &responseCacheT{
        data: make(map[string]cacheEntry),
    }
}

func (c *responseCacheT) get(key string) ([]byte, bool) {
    c.mu.Lock()
    defer c.mu.Unlock()

    entry, ok := c.data[key]
    if !ok {
        return nil, false
    }
    if time.Now().After(entry.expiresAt) {
        delete(c.data, key)
        return nil, false
    }
    return entry.payload, true
}

func (c *responseCacheT) put(key string, payload []byte) {
    c.mu.Lock()
    defer c.mu.Unlock()

    c.data[key] = cacheEntry{
        payload:   payload,
        expiresAt: time.Now().Add(responseCacheTTL),
    }
}
```

This preserves the TTL behavior and shared-block semantics but removes the size/eviction logic and its edge cases.

If you later discover unbounded growth is a real issue, you can add a periodic cleaner (ticker in the caller) instead of coupling eviction to the hot path.

---

### 2. Make the cache dependency explicit (optional but helpful)

If the rest of the code currently uses a package-global `responseCache`, you can clarify the behavior and improve testability by wiring it through your main type (e.g. `AA55UDP`) instead of relying on hidden global state.

For example:

```go
type AA55UDP struct {
    // ...
    cache *responseCacheT
}

func NewAA55UDP(/* existing args */) *AA55UDP {
    return &AA55UDP{
        // ...
        cache: newResponseCacheT(),
    }
}
```

Then use `u.cache.get/put` where you currently reference the global. Tests can inject a no-op or custom cache if needed:

```go
func NewAA55UDPWithCache(cache *responseCacheT /* other args */) *AA55UDP {
    return &AA55UDP{
        // ...
        cache: cache,
    }
}
```

This keeps functionality identical but reduces the “hidden state” mental load.

---

### 3. Deduplicate PDU hex parsing

`buildPDUFromHex` is almost certainly overlapping with the existing `parsePDUHex`. You can centralize the parsing logic and keep only thin wrappers.

For example, extract a shared helper:

```go
func pduFromHex(s string, wantLen int, context string) ([]byte, error) {
    clean := strings.ReplaceAll(s, " ", "")
    b, err := hex.DecodeString(clean)
    if err != nil {
        return nil, fmt.Errorf("aa55udp: invalid %s %q: %w", context, s, err)
    }
    if len(b) != wantLen {
        return nil, fmt.Errorf("aa55udp: %s must be %d bytes, got %d", context, wantLen, len(b))
    }
    return b, nil
}
```

Then:

```go
func buildPDUFromHex(s string) ([]byte, error) {
    return pduFromHex(s, 6, "pdu")
}

// Existing code:
func parsePDUHex(s string) ([]byte, error) {
    return pduFromHex(s, /*existing expected length*/, "pdu")
}
```

That trims duplicated logic and makes future changes to parsing rules or error wording easier to reason about.
</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.}

[toeklk]

Clearly the log of this PR shows in my opinion a very large disadvantage of this code wrt the original one. Whereas the original code is pretty clean, simple and could be understood in a couple of minutes even by nitwits such as myself, the current code is far less readable due to the "workarounds". Just my 2 cents :-)

[andig]

conflict with master?

[toeklk]

seems to be caused by your commit from yesterday ( hash 2132d9b ). Tried to fix it, but somehow it always gets stuck when committing the merge (I had this before)?

[DevineCZ]

@toeklk : Any news about the commit to master?

[toeklk]

@toeklk : Any news about the commit to master?

The PR is still waiting for review I guess (right @andig ?)

[andig]

keep order

[andig]

This PR adds quite a bit of namespace clash to the plugin package- it seems parts of it should be moved to a aa55 package.

[andig]

With the PR, can we also re-deprecate the old wifi implementation?

[andig]

This part is a bit confusing: the parsePDU does not use the ID, also fetch doesn't. Where is the IF actually specified? As part of the PDU? If that's the case, wouldn't it make more sense to explicitly keep the ID as separate field for sake of clarity and only add it during parsePDU?

If that makes sense: even register/count could be reused and only offset would be the actual indicator that this is indeed a block read? In that case it might be a good idea to make offset a pointer so 0 can still be used.

[andig]

Just an idea for discussion. If you're happy with the refactor we can merge as-is.

[toeklk]

I'm not sure I understand what you mean (or maybe better: I'm sure I don't understand what you mean ;-)

• ParsePDU is used for the blockReads, in this case the ID is "encapsulated" in the PDU specified in the template
• BuildPDU is used for the registerReads (and uses the ID and other params) to build the PDU.
  But I'm sure you figured that out. Could you elaborate a bit on your idea?

[andig]

Instead of specifying the entire PDU for the block read could we just give the meta data (i.e. start register, count of registers and slave id)? The PDU would still be build the same way but now more need for defining the hex bytes in the template?

[toeklk]

In that case you would need to specify:

• start address of the block
• size of the block read
• offset of the register in the block read
• (optionally) number of bytes to read (could be inferred from the type) - the count

I assume you would not change the meaning of register between the goodwe templates, so you would use offset to calculate the start address and additionally specify the offset and a blockread size then? It would indeed be more similar, but the advantage of the current approach is that one can quickly see how many different PDUs are actually used IMO. No clear winner to me, but YMMV of course. In the end, the whole pull request add robustness at the lack of code readability IMO, still something that bothers me...

[andig]

I'd agree. If @toeklk doesn't object I'd update the PR accordingly. It would still need a test after this change though.

[toeklk]

No objection from my side (at all), and I think @DevineCZ will be happy to test, right?

[andig]

Made a suggestion that should be worth testing. Imho this is indeed a bit clearer than before and even doesn't require manually calculating the block offset.

[toeklk]

Template looks very clean and logical! Tested on my dt (not et!) for regressions, and seems to work fine.

[andig]

Much appreciated, thank you. Great PR!

[DevineCZ]

With the PR, can we also re-deprecate the old wifi implementation?

Please keep the old WiFi implementation for now, as soon as this change is committed and the new version of evcc is compiled, I will test it. If it is usable, I will let you know and the old implementation could be removed. But not yet. The old WiFi implementation is the only one that works for me in the live version of evcc.