Tinkerforge WARP WebSocket API (BC)

[marcelGoerentz]

This is PR is the successor of #26334

It implements a websocket for receiving the data from the charger instead of polling it

[sourcery-ai]

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

Security issues:

• Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)

General comments:

• The hasFeature method infers capabilities from runtime values like power, energy, and tagId, which are legitimately zero/empty at startup or during operation; consider basing feature detection on explicit charger capabilities or API responses instead of value heuristics to avoid mis-detecting features (e.g. disabling meter or NFC support until non-zero data appears).
• The runWS reconnect loop ignores the context cancellation and will continue reconnecting forever even after cancel is called; wrapping the loop body in a select on ctx.Done() or otherwise breaking out when the context is canceled would make shutdown behavior cleaner.
• The energy manager configuration (EnergyManagerURI, EnergyManagerUser, EnergyManagerPassword) and corresponding fields like emURI/emHelper are initialized but never used after being set; either wire them into actual requests (e.g. for phase switching) or remove them to avoid confusing dead configuration.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `hasFeature` method infers capabilities from runtime values like `power`, `energy`, and `tagId`, which are legitimately zero/empty at startup or during operation; consider basing feature detection on explicit charger capabilities or API responses instead of value heuristics to avoid mis-detecting features (e.g. disabling meter or NFC support until non-zero data appears).
- The `runWS` reconnect loop ignores the context cancellation and will continue reconnecting forever even after `cancel` is called; wrapping the loop body in a `select` on `ctx.Done()` or otherwise breaking out when the context is canceled would make shutdown behavior cleaner.
- The energy manager configuration (`EnergyManagerURI`, `EnergyManagerUser`, `EnergyManagerPassword`) and corresponding fields like `emURI`/`emHelper` are initialized but never used after being set; either wire them into actual requests (e.g. for phase switching) or remove them to avoid confusing dead configuration.

## Individual Comments

### Comment 1
<location> `charger/warp-ws.go:539-548` </location>
<code_context>
+    w.log.INFO.Printf("meter value_ids mapped: %+v", idx)
+}
+
+func (w *WarpWS) updateMeterValues(res []float64) {
+	highestIndex := max(w.valuesMap.CurrentImExSumL1Index, w.valuesMap.VoltageL2NIndex, w.valuesMap.VoltageL3NIndex,
+	w.valuesMap.CurrentImExSumL1Index, w.valuesMap.CurrentImExSumL2Index, w.valuesMap.CurrentImExSumL3Index,
+	w.valuesMap.PowerImExSumIndex, w.valuesMap.EnergyAbsImSumIndex)
+
+	if len(res) < highestIndex + 1 {
+		return
+	}
+
+    w.voltL[0] = res[w.valuesMap.VoltageL1NIndex]
+	w.voltL[1] = res[w.valuesMap.VoltageL2NIndex]
+	w.voltL[2] = res[w.valuesMap.VoltageL3NIndex]
+	w.currL[0] = res[w.valuesMap.CurrentImExSumL1Index]
+	w.currL[1] = res[w.valuesMap.CurrentImExSumL2Index]
+	w.currL[2] = res[w.valuesMap.CurrentImExSumL3Index]
+	w.power = res[w.valuesMap.PowerImExSumIndex]
+	w.energy = res[w.valuesMap.EnergyAbsImSumIndex]
+}
+
</code_context>

<issue_to_address>
**issue (bug_risk):** `updateMeterValues` omits `VoltageL1NIndex` from the bounds check and duplicates `CurrentImExSumL1Index`.

`highestIndex` should also consider `VoltageL1NIndex`, otherwise the bounds check can pass while `VoltageL1NIndex` is out of range and cause a panic when indexing `res`. Also, `CurrentImExSumL1Index` appears twice in the `max(...)` call; removing the duplicate will make the intent clearer.
</issue_to_address>

### Comment 2
<location> `charger/warp-ws.go:27-36` </location>
<code_context>
+	*request.Helper
+	log       *util.Logger
+	uri       string
+	valuesMap warp.MeterValuesIndices
+	skipLegacy bool
+
+	mu sync.RWMutex
+
+	// evse
+	status     api.ChargeStatus
+	maxCurrent int64
+
+	// meter
+	power  float64
+	energy float64
+	currL  [3]float64
+	voltL  [3]float64
+	meterIndex uint
+
+	// rfid
</code_context>

<issue_to_address>
**issue (bug_risk):** `meterIndex` appears configurable but is never set from the parsed config.

`WarpWS` defines `meterIndex` and uses it in subscription topics, but `NewWarpWSFromConfig` never assigns the parsed `EnergyMeterIndex` to it. This leaves `meterIndex` at 0 and ignores any configured meter index. If configuration is intended here, initialize `wb.meterIndex` from `cc.EnergyMeterIndex`.
</issue_to_address>

### Comment 3
<location> `charger/warp-ws.go:236` </location>
<code_context>
+	}
+}
+
+func (w *WarpWS) handleEvent(data []byte) {
+	var evt warpEvent
+	if err := json.Unmarshal(data, &evt); err != nil {
</code_context>

<issue_to_address>
**issue (complexity):** Consider splitting the large WebSocket event handler into small, focused helpers and encapsulating the meter mapping/legacy logic into its own type to keep WarpWS simpler and more modular.

You can reduce complexity meaningfully in two focused places without changing behaviour: the WebSocket event handling and the meter mapping/legacy handling.

---

### 1. Split `handleEvent` into dedicated handlers

`handleEvent` is doing many unrelated things. You can keep the same switch on topic but delegate quickly to focused helpers. That keeps `WarpWS` readable and makes extensions local.

Example refactor:

```go
func (w *WarpWS) handleEvent(data []byte) {
	var evt warpEvent
	if err := json.Unmarshal(data, &evt); err != nil {
		w.log.ERROR.Printf("ws decode: %v", err)
		return
	}
	w.log.TRACE.Printf("Received WARP event with topic: %s and payload: %v", evt.Topic, string(evt.Payload))

	w.mu.Lock()
	defer w.mu.Unlock()

	switch evt.Topic {
	case "charge_tracker/current_charge",
		"evse/state",
		"evse/external_current":
		w.handleEvseEvent(evt)

	case "meter/values",
		"meter/all_values":
		w.handleLegacyMeterEvent(evt)

	case fmt.Sprintf("meters/%d/value_ids", w.meterIndex),
		fmt.Sprintf("meters/%d/values", w.meterIndex):
		w.handleNewMeterEvent(evt)

	case "nfc/config":
		w.handleNfcEvent(evt)

	case "power_manager/state",
		"power_manager/low_level_state":
		w.handleEmEvent(evt)
	}
}
```

Then implement small helpers that just contain the old case bodies:

```go
func (w *WarpWS) handleEvseEvent(evt warpEvent) {
	switch evt.Topic {
	case "charge_tracker/current_charge":
		var c warp.ChargeTrackerCurrentCharge
		if err := json.Unmarshal(evt.Payload, &c); err != nil {
			w.log.ERROR.Printf("charge_tracker decode: %v", err)
			return
		}
		w.tagId = c.AuthorizationInfo.TagId

	case "evse/state":
		var s warp.EvseState
		if err := json.Unmarshal(evt.Payload, &s); err != nil {
			w.log.ERROR.Printf("evse/state decode: %v", err)
			return
		}
		switch s.Iec61851State {
		case 0:
			w.status = api.StatusA
		case 1:
			w.status = api.StatusB
		case 2:
			w.status = api.StatusC
		}

	case "evse/external_current":
		var c struct{ Current int64 `json:"current"` }
		if err := json.Unmarshal(evt.Payload, &c); err == nil {
			w.maxCurrent = c.Current
		}
	}
}
```

Other handler stubs are analogous:

```go
func (w *WarpWS) handleLegacyMeterEvent(evt warpEvent) { /* move meter/values + meter/all_values here */ }
func (w *WarpWS) handleNewMeterEvent(evt warpEvent)    { /* move meters/<idx>/value_ids + values here */ }
func (w *WarpWS) handleNfcEvent(evt warpEvent)         { /* move nfc/config here */ }
func (w *WarpWS) handleEmEvent(evt warpEvent)          { /* move power_manager/* here */ }
```

This keeps logic identical but flattens the big switch and localizes responsibilities.

---

### 2. Encapsulate meter mapping and `skipLegacy` into a helper

The meter mapping (`valuesMap`, `skipLegacy`, `updateMeterValueIds`, `updateMeterValues`) is low‑level and tightly coupled to `WarpWS`. Encapsulating it into a small struct will reduce surface area and hide the `skipLegacy` coupling.

Minimal refactor:

```go
type meterMapper struct {
	indices    warp.MeterValuesIndices
	skipLegacy bool
	log        *util.Logger
}

func (m *meterMapper) handleLegacyValues(vals []float64, power *float64, energy *float64, voltL, currL *[3]float64) {
	if m.skipLegacy {
		return
	}
	if len(vals) >= 6 {
		(*voltL)[0], (*voltL)[1], (*voltL)[2] = vals[0], vals[1], vals[2]
		(*currL)[0], (*currL)[1], (*currL)[2] = vals[3], vals[4], vals[5]
	}
}

func (m *meterMapper) updateValueIDs(ids []int) {
	// move body of updateMeterValueIds here, using m.indices/m.log
}

func (m *meterMapper) updateValues(vals []float64, power *float64, energy *float64, voltL, currL *[3]float64) {
	// move body of updateMeterValues here, using m.indices
}
```

`WarpWS` then holds:

```go
type WarpWS struct {
	*request.Helper
	log *util.Logger
	// ...
	meterIndex uint
	meter      meterMapper
	// ...
}
```

And `handleLegacyMeterEvent` / `handleNewMeterEvent` become:

```go
func (w *WarpWS) handleLegacyMeterEvent(evt warpEvent) {
	switch evt.Topic {
	case "meter/values":
		if w.meter.skipLegacy {
			return
		}
		var mvals warp.MeterValues
		if err := json.Unmarshal(evt.Payload, &mvals); err != nil {
			w.log.ERROR.Printf("meter/values decode: %v", err)
			return
		}
		w.power = mvals.Power
		w.energy = mvals.EnergyAbs

	case "meter/all_values":
		var vals []float64
		if err := json.Unmarshal(evt.Payload, &vals); err != nil {
			w.log.ERROR.Printf("meter/all_values decode: %v", err)
			return
		}
		w.meter.handleLegacyValues(vals, &w.power, &w.energy, &w.voltL, &w.currL)
	}
}

func (w *WarpWS) handleNewMeterEvent(evt warpEvent) {
	switch evt.Topic {
	case fmt.Sprintf("meters/%d/value_ids", w.meterIndex):
		if !w.meter.skipLegacy {
			w.meter.skipLegacy = true
		}
		var ids []int
		if err := json.Unmarshal(evt.Payload, &ids); err != nil {
			w.log.ERROR.Printf("value_ids decode: %v", err)
			return
		}
		w.meter.updateValueIDs(ids)

	case fmt.Sprintf("meters/%d/values", w.meterIndex):
		var vals []float64
		if err := json.Unmarshal(evt.Payload, &vals); err != nil {
			w.log.ERROR.Printf("values decode: %v", err)
			return
		}
		w.meter.updateValues(vals, &w.power, &w.energy, &w.voltL, &w.currL)
	}
}
```

This keeps all functionality (including the `skipLegacy` behaviour) but constrains the meter‑specific complexity to a focused type, making `WarpWS` easier to navigate and reason about.
</issue_to_address>

### Comment 4
<location> `charger/warp-ws.go:175` </location>
<code_context>
	wsURL := strings.Replace(w.uri, "http://", "ws://", 1) + "/ws"
</code_context>

<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.

*Source: opengrep*
</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.}

[marcelGoerentz]

@andig Ich hoffe ich habe alle Punkte zur Zufriedenheit abgearbeitet.

[sourcery-ai]

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

Security issues:

• Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)

General comments:

• In MeterMapper, the log field is never initialized and is dereferenced in UpdateValueIDs, which will panic if MeterMapper is used without a logger; either inject a logger on construction or guard against nil before logging.
• The interface assertion var _ api.ChargerEx = (*Warp2)(nil) in warp-ws.go looks like a copy-paste typo and should probably assert against *WarpWS instead to ensure the intended type implements api.ChargerEx.
• You introduce the FeaturePhaseSwitch constant but WarpWS.hasFeature does not handle it, so phase-switch support is never detected and the corresponding logic in NewWarpWSFromConfig will not behave as intended; consider wiring this feature flag into hasFeature or adjusting the detection logic.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `MeterMapper`, the `log` field is never initialized and is dereferenced in `UpdateValueIDs`, which will panic if `MeterMapper` is used without a logger; either inject a logger on construction or guard against `nil` before logging.
- The interface assertion `var _ api.ChargerEx = (*Warp2)(nil)` in `warp-ws.go` looks like a copy-paste typo and should probably assert against `*WarpWS` instead to ensure the intended type implements `api.ChargerEx`.
- You introduce the `FeaturePhaseSwitch` constant but `WarpWS.hasFeature` does not handle it, so phase-switch support is never detected and the corresponding logic in `NewWarpWSFromConfig` will not behave as intended; consider wiring this feature flag into `hasFeature` or adjusting the detection logic.

## Individual Comments

### Comment 1
<location> `charger/warp-ws.go:397` </location>
<code_context>
+		return w.currL[0] != 0 || w.voltL[0] != 0
+	case warp.FeatureNfc:
+		return w.tagId != ""
+	default:
+		w.log.TRACE.Printf("feature missing: %s", f)
+		return false
</code_context>

<issue_to_address>
**issue (bug_risk):** FeaturePhaseSwitch is never reported as available, so phase-switch support based on WARP3 native feature is effectively disabled.

`hasFeature` currently only handles `FeatureMeter`, `FeatureMeterPhases`, and `FeatureNfc`; everything else hits `default`, logs "feature missing", and returns `false`. As a result, the `FeaturePhaseSwitch` path in `NewWarpWSFromConfig` is never taken and the EM fallback (or nothing, if EM URI is empty) is always used. You should add an explicit `case warp.FeaturePhaseSwitch` with a reliable detection heuristic (or expose a dedicated feature-detection call) so the new phase-switch feature flag actually works.
</issue_to_address>

### Comment 2
<location> `charger/warp/meter_mapper.go:38` </location>
<code_context>
+	}
+
+	if len(missing) > 0 {
+		m.log.ERROR.Printf("missing required meter value IDs: %v", missing)
+		return
+	}
</code_context>

<issue_to_address>
**issue (bug_risk):** MeterMapper assumes `log` is non-nil; if not initialized, this will panic on first missing-ID error.

In `UpdateValueIDs`, `m.log.ERROR.Printf` is called even though `WarpWS.meter` is currently a zero-value `warp.MeterMapper`, and I don’t see `log` being set anywhere. That makes a nil-pointer panic likely when required IDs are missing.

Please either always construct `MeterMapper` with a non-nil logger (e.g. `meter: warp.MeterMapper{log: log}` in `NewWarpWS`) or add a nil check around the logging call:

```go
if m.log != nil {
    m.log.ERROR.Printf("missing required meter value IDs: %v", missing)
}
```
</issue_to_address>

### Comment 3
<location> `charger/warp-ws.go:386` </location>
<code_context>
+	w.is3Phase = s.Is3phase
+}
+
+func (w *WarpWS) hasFeature(f string) bool {
+	w.mu.RLock()
+	defer w.mu.RUnlock()
</code_context>

<issue_to_address>
**issue (complexity):** Consider replacing the heuristic-based `hasFeature` implementation with explicit capability flags set in the event handlers to make feature detection more straightforward and predictable.

You can simplify the feature detection by replacing the heuristic `hasFeature` logic with explicit capability flags that are set in the event handlers. This reduces the hidden coupling to event order and makes `NewWarpWSFromConfig` easier to reason about without changing behavior.

### 1. Add explicit capability flags

```go
type WarpWS struct {
    *request.Helper
    log *util.Logger
    uri string

    mu sync.RWMutex

    // evse
    status     api.ChargeStatus
    maxCurrent int64

    // meter
    power      float64
    energy     float64
    currL      [3]float64
    voltL      [3]float64
    meterIndex uint
    meter      warp.MeterMapper

    // rfid
    tagId     string
    nfcConfig warp.NfcConfig

    // capabilities
    hasMeter       bool
    hasMeterPhases bool
    hasNfc         bool

    // energy manager
    emState    warp.EmState
    emLowLevel warp.EmLowLevelState
    is3Phase   bool
    emURI      string
    emHelper   *request.Helper

    // config
    current int64
    cancel  context.CancelFunc
}
```

### 2. Set flags in the handlers where the data is first successfully decoded

```go
func (w *WarpWS) handleLegacyMeterEvent(evt warpEvent) {
    switch evt.Topic {
    case "meter/values":
        var m warp.MeterValues
        if !w.decode(evt.Payload, &m, "meter/values") {
            return
        }
        w.power = m.Power
        w.energy = m.EnergyAbs
        w.hasMeter = true

    case "meter/all_values":
        var vals []float64
        if !w.decode(evt.Payload, &vals, "meter/all_values") {
            return
        }
        copy((w.voltL)[:], vals[:3])
        copy((w.currL)[:], vals[3:6])
        w.hasMeterPhases = true
    }
}

func (w *WarpWS) handleMetersEvent(evt warpEvent) {
    switch evt.Topic {
    case fmt.Sprintf("meters/%d/value_ids", w.meterIndex):
        var ids []int
        if !w.decode(evt.Payload, &ids, "value_ids") {
            return
        }
        w.meter.UpdateValueIDs(ids)

    case fmt.Sprintf("meters/%d/values", w.meterIndex):
        var vals []float64
        if !w.decode(evt.Payload, &vals, "values") {
            return
        }
        w.meter.UpdateValues(vals, &w.power, &w.energy, &w.voltL, &w.currL)
        w.hasMeter = true
        w.hasMeterPhases = true
    }
}

func (w *WarpWS) handleChargeTracker(payload json.RawMessage) {
    var c warp.ChargeTrackerCurrentCharge
    if !w.decode(payload, &c, "charge_tracker") {
        return
    }
    w.log.TRACE.Printf("nfc: tag detected: %s", c.AuthorizationInfo.TagId)
    w.tagId = c.AuthorizationInfo.TagId
    w.hasNfc = true
}
```

### 3. Simplify `hasFeature`

```go
func (w *WarpWS) hasFeature(f string) bool {
    w.mu.RLock()
    defer w.mu.RUnlock()

    switch f {
    case warp.FeatureMeter:
        return w.hasMeter
    case warp.FeatureMeterPhases:
        return w.hasMeterPhases
    case warp.FeatureNfc:
        return w.hasNfc
    default:
        w.log.TRACE.Printf("feature missing: %s", f)
        return false
    }
}
```

This keeps the existing decorator-based wiring and function closures intact, but removes the implicit “non-zero means available” heuristic and the hidden dependency on which events arrive first, making the code path in `NewWarpWSFromConfig` easier to follow and reason about.
</issue_to_address>

### Comment 4
<location> `charger/warp-ws.go:175` </location>
<code_context>
	wsURL := strings.Replace(w.uri, "http://", "ws://", 1) + "/ws"
</code_context>

<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.

*Source: opengrep*
</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]

why? just

json.Unmarshal

into

[]warpEvent

[marcelGoerentz]

Not possible since we don't get a JSON Array here, we get concatenated JSON Objects, that looks like: {...}{...}{...}

[andig]

@rtrbt any chance getting only a single json message per websocket message? This kind of bundling makes it unnecessarily complicated.

[marcelGoerentz]

After connecting to the websocket we get 2 Frames with a bunch of JSON objects concatenated as shown in my previous comment.
That's why I've implemented the little state machine.

So it would be great if we could get at least a JSON Array with the 2 first frames.

[andig]

This can be cut down to ~60% of the current code size.

[andig]

Is it required to decode all structs into individual values instead of just storing the struct?

[marcelGoerentz]

Obviously its not. I will store the structs instead.

[marcelGoerentz]

@andig please check again.

[sourcery-ai]

Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:

Security issues:

• Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections. (link)

General comments:

• In hasFeature, the mutex is held while performing the HTTP GET to /info/features; consider releasing the lock before doing network I/O and only locking around reading/updating w.features to avoid blocking other goroutines on slow or failing requests.
• In run, wsURL is built by replacing only http:// with ws://; for HTTPS endpoints this will leave https:// unchanged and break WebSocket connectivity, so adjust the scheme mapping to also handle https://→wss://.
• The post helper always builds requests using w.uri even for power_manager/* paths; when a separate pmURI is configured (Energy Manager), those requests should use pmURI to actually target the power manager instead of the charger URI.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `hasFeature`, the mutex is held while performing the HTTP GET to `/info/features`; consider releasing the lock before doing network I/O and only locking around reading/updating `w.features` to avoid blocking other goroutines on slow or failing requests.
- In `run`, `wsURL` is built by replacing only `http://` with `ws://`; for HTTPS endpoints this will leave `https://` unchanged and break WebSocket connectivity, so adjust the scheme mapping to also handle `https://`→`wss://`.
- The `post` helper always builds requests using `w.uri` even for `power_manager/*` paths; when a separate `pmURI` is configured (Energy Manager), those requests should use `pmURI` to actually target the power manager instead of the charger URI.

## Individual Comments

### Comment 1
<location> `charger/warp-ws.go:416-422` </location>
<code_context>
+	return w.statusFromEvseStatus(), nil
+}
+
+func (w *WarpWS) StatusReason() (api.Reason, error) {
+	w.mu.RLock()
+	defer w.mu.RUnlock()
+	if w.statusFromEvseStatus() == api.StatusB && w.evse.UserEnabled && w.evse.UserCurrent == 0 {
+		return api.ReasonWaitingForAuthorization, nil
+	}
+	return api.ReasonUnknown, nil
+}
+
</code_context>

<issue_to_address>
**issue (bug_risk):** StatusReason can deadlock due to re-entrant RLock on the same mutex

`StatusReason` holds `w.mu.RLock()` and then calls `statusFromEvseStatus`, which calls `getField` and attempts to `RLock` the same mutex again. Since `sync.RWMutex` is not re-entrant, this will deadlock. You could either avoid locking here and rely on `getField` for all reads (including `evse.UserEnabled` / `evse.UserCurrent`), or adjust `statusFromEvseStatus` so it doesn’t call `getField` when invoked under an existing lock.
</issue_to_address>

### Comment 2
<location> `charger/warp-ws.go:184` </location>
<code_context>
	wsURL := strings.Replace(w.uri, "http://", "ws://", 1) + "/ws"
</code_context>

<issue_to_address>
**security (javascript.lang.security.detect-insecure-websocket):** Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.

*Source: opengrep*
</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.}