Tinkerforge WARP HTTP API (BC) #26334
Conversation
evcc-bot
added
devices
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
meterValuesthe length checklen(res) <= 5will still allow slices of length 5 and thencurrents/voltagesindex up tores[5], so this should belen(res) < 6(or equivalent) to avoid a potential out-of-bounds panic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `meterValues` the length check `len(res) <= 5` will still allow slices of length 5 and then `currents`/`voltages` index up to `res[5]`, so this should be `len(res) < 6` (or equivalent) to avoid a potential out-of-bounds panic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
OT, bitte neuer PR. |
|
Nice PR! |
lehmanju
force-pushed
the
warp-http
branch
2 times, most recently
from
d908dee to
2d664be
Compare
Hast Du dazu eine Referenz? |
Ah, missverständlich. Die ist nicht in der WARP Firmware deprecated, sondern die evcc Templates sind als deprecated markiert ;). |
|
Ah- für mich auch ok. Ohne Umwege ist egtl. immer besser! |
|
Für Setups wo neben evcc auch andere Systeme auf die Wallbox zugreifen, hat MQTT den Vorteil dass alle Clients parallel per Publish/Subscribe lauschen können, ohne die Wallbox mehrfach zu pollen. Wäre es sinnvoll, die MQTT Templates nicht als deprecated zu markieren, sondern als gleichwertige Alternative zu belassen – für Nutzer die ohnehin einen Broker betreiben? |
Die API wird ja nicht tausendfach pro Sekunde gepollt, insofern ist das denke ich kein Problem. HTTP mit JSON kann schon einiges ab, da kommt man mit evcc nicht so schnell an Grenzen. Andere Clients können ja weiterhin MQTT nutzen. Sollte sich Gegenteiliges herausstellen, kann man das deprecated ja wieder rückgängig machen. Aber besser ist glaube ich schon perspektivisch nur eine API zu pflegen. |
|
Ich nutze ein WARP2 mit WEM der die Phaseumschaltung macht.
Ich wollte das nur anmerken, da ich persönlich lieber weiter den Broker nutzen wollen würde. Ist aber nicht konstruktiv begründet, sondern nur Geschmackssache. |
|
@deadrabbit87 @mfuchs1984 gerne nochmal testen ;) |
|
Habe selbst keinen WARP charger ;) |
|
Mach ich heute Abend. |
|
@rtrbt could you kindly take a look at https://github.com/evcc-io/evcc/pull/26334/changes#diff-8a0b3f5a73d38e9a0a8367e7d12113e1ee968bd65f9e444feb12c22091c300a7R283? This looks like a horrible API. Couldn't this be returned as map in the first place? |
Nope, the /values API has to be as small as possible due to memory constraints on the charger (we support up to 7 chargers with 96 values each) and also to reduce traffic, because value IDs are stable and values change all the time. Building an index cache with the value IDs and then indexing directly into values is using the API as intended. This could however be done with one indirection less by not maintaining warp.metersValuesMap as a map, but as an array instead: result.VoltageL1N = res[0]
result.VoltageL2N = res[1]
result.VoltageL3N = res[2]
result.CurrentImExSumL1 = res[3]
result.CurrentImExSumL2 = res[4]
result.CurrentImExSumL3 = res[5]
result.PowerImExSum = res[6]
result.EnergyAbsImSum = res[7]This could be simplified more if it is possible to iterate over struct members in Go, or if you change warp.MetersValues to be an array. |
|
@lehmanju could you pick this up? |
|
Just one question, why don't we use a web socket instead of http requests? The communication will then be bidirectional and we can use most of the mqtt implementation, since the data coming from the charger will be the same. EDIT: |
|
The problem is:
So, an alternative approach would be to use a |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In hasFeature you cache an empty feature list when the initial /info/features call fails, which will permanently disable all feature-based behavior for that instance; consider surfacing the error during construction or retrying instead of silently treating it as 'no features'.
- metersValueIds currently fails charger initialization if any of the required value IDs are missing; if some devices legitimately expose only a subset, it might be more robust to degrade functionality (e.g., disable currents/voltages/energy individually) instead of returning an error for the whole charger.
- In disablePhaseAutoSwitch the error message 'disabling phase auto switch failed' is wrapped twice (once on request creation and once on Do); consider centralizing the wrapping to avoid redundant and slightly confusing error text.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In hasFeature you cache an empty feature list when the initial /info/features call fails, which will permanently disable all feature-based behavior for that instance; consider surfacing the error during construction or retrying instead of silently treating it as 'no features'.
- metersValueIds currently fails charger initialization if any of the required value IDs are missing; if some devices legitimately expose only a subset, it might be more robust to degrade functionality (e.g., disable currents/voltages/energy individually) instead of returning an error for the whole charger.
- In disablePhaseAutoSwitch the error message 'disabling phase auto switch failed' is wrapped twice (once on request creation and once on Do); consider centralizing the wrapping to avoid redundant and slightly confusing error text.
## Individual Comments
### Comment 1
<location> `charger/warp-http_decorators.go:9` </location>
<code_context>
+ "github.com/evcc-io/evcc/api"
+)
+
+func decorateWarpHTTP(base *WarpHTTP, meter func() (float64, error), meterEnergy func() (float64, error), phaseCurrents func() (float64, float64, float64, error), phaseVoltages func() (float64, float64, float64, error), identifier func() (string, error), phaseSwitcher func(int) error, phaseGetter func() (int, error)) api.Charger {
+ switch {
+ case identifier == nil && meter == nil && phaseSwitcher == nil:
</code_context>
<issue_to_address>
**issue (complexity):** Consider changing the generator to build the decorated charger by sequentially wrapping `base` once per non-nil capability instead of enumerating every interface combination in a massive switch.
You can avoid the combinatorial explosion entirely by making the generator compose decorators incrementally instead of precomputing every combination.
Right now, each `case` defines a new anonymous struct that embeds `*WarpHTTP` plus some interfaces. Instead, you can:
1. Start from `api.Charger` (`base`).
2. For each non-nil capability function, wrap the current charger value in a small anonymous struct that embeds:
- the previous `api.Charger`, and
- exactly one new interface implementation.
Because the embedded `api.Charger` already implements all previously-added interfaces, each wrapper automatically implements the union of all interfaces without needing a dedicated struct per combination.
Conceptually, your generator can produce something like this:
```go
func decorateWarpHTTP(
base *WarpHTTP,
meter func() (float64, error),
meterEnergy func() (float64, error),
phaseCurrents func() (float64, float64, float64, error),
phaseVoltages func() (float64, float64, float64, error),
identifier func() (string, error),
phaseSwitcher func(int) error,
phaseGetter func() (int, error),
) api.Charger {
var c api.Charger = base
if meter != nil {
c = &struct {
api.Charger
api.Meter
}{
Charger: c,
Meter: &decorateWarpHTTPMeterImpl{
meter: meter,
},
}
}
if meterEnergy != nil {
c = &struct {
api.Charger
api.MeterEnergy
}{
Charger: c,
MeterEnergy: &decorateWarpHTTPMeterEnergyImpl{
meterEnergy: meterEnergy,
},
}
}
if phaseCurrents != nil {
c = &struct {
api.Charger
api.PhaseCurrents
}{
Charger: c,
PhaseCurrents: &decorateWarpHTTPPhaseCurrentsImpl{
phaseCurrents: phaseCurrents,
},
}
}
// ...repeat pattern for phaseVoltages, identifier, phaseSwitcher, phaseGetter...
return c
}
```
Key points:
- You only ever create at most one anonymous struct per capability.
- The method set grows by embedding the previous `api.Charger`, so the final `c` implements all required interfaces.
- The nil case is naturally handled: if all funcs are nil, `c` stays as `base`.
Refactoring the generator to emit this sequential wrapping pattern will:
- Reduce `decorateWarpHTTP` from hundreds of nearly identical `case` branches to a short, linear function.
- Preserve all existing functionality and type behavior (the returned value still implements `api.Charger` plus whichever optional interfaces are available).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@andig any TODOs left? |
|
Ich bin nicht glücklich mit der meter(s)Values Unterscheidung, auch wenn die API so heisst. Das ist wahnsinnig subtil und visuell kaum zu unterscheiden. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@andig erledigt |
|
Ich habe eine Implementierung mit Websocket. |
|
Fänd ich gut. Der Klarheit halber: neuer PR gegen master der auf diesem hier aufbaut und den replaced? |
|
@andig ja, so würde ich es machen. Ist noch nicht ganz fertig, aber bald. |
|
Cool, danke für's übernehmen! |
|
@lehmanju Eine Frage habe ich noch, warum hast die Meters API konfigurierbar gemacht. Macht es nicht Sinn nur das erste Meter (Index 0) auszulesen? Jedes andere Meter ist nicht vom Charger und kann bei evcc direkt eingebunden werden. |
|
@marcelGoerentz kannst auch hardcoden, aber "theoretisch" ist es halt in der warp konfigurierbar. |
|
Definitiv ist hardcoded in Ordnung, wurde oben im Thread schon besprochen. Ist hier ja auch so, nur halt in der yaml. |
8 participants
Die WARP Charger von Tinkerforge implementieren inzwischen alle eine HTTP API. Dieser PR implementiert diese API analog zur MQTT API. Die MQTT API wird weiter unterstützt, ist aber als deprecated markiert. Zukünftige Nutzer können die Wallbox jetzt einfacher konfigurieren, es ist nur die Angabe einer IP/eines Hostnamens erforderlich.
Damit wird der MQTT Broker für mich dann auch endlich überflüssig und kann abgeschaltet werden.
EDIT:
power_managerFeature nicht selbst unterstützt, also nur für WARPmetersAPI, aber behaltemeterAPI als Fallback