-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Tinkerforge WARP HTTP API (BC) #26334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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! |
mfuchs1984
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comment.
|
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. |
54b2390 to
2f3b7dd
Compare
|
|
|
Also mit WEM + WARP2 pro geht bei mir jetzt alles. Tests mit WARP3 stehen noch aus.... Soll ich smart auch testen? Hätte unter umständen Zugriff auf eine... Sonst habe ich nur pro. |
Gerne alles testen, aber vermutlich ist nur noch die WARP3 interessant. Ob Smart oder Pro macht in der Implementierung keinen Unterschied. |
|
Also WEM Adresse muss man nach wie vor bei der WARP3 eintragen damit die auch Phasenumschaltung kann. |
Aber hier ganz unten steht doch, dass es ohne geht: https://www.tinkerforge.com/de/blog/all-information-for-the-new-warp3-charger/ Oder meinst du in EVCC funktioniert es noch nicht richtig? |
|
Ja, ich meine evcc. Atm ist es verwirrend, dass der Nutzer bei WEM die Adresse der Wallbox nochmal eintragen muss, damit 1p3p zur Verfügung steht. |
|
Kannst du mal den Output von Ich habe das Gefühl, dass die
Ja, das repariere ich. |
|
Klar: curl http://192.168.179.250/info/features
["rtc","evse","cp_disconnect","button_configuration","rgb_led","ethernet","firmware_update","meters","nfc","phase_switch","meter","meter_all_values","meter_phases"]und curl http://192.168.179.250/power_manager/state
{"config_error_flags":0,"external_control":0} |
|
Danke, genau wie gedacht. Hab es schon repariert. |
|
Ich habe jetzt folgende Wallboxen getestet:
Alle haben jetzt wie erwartet funktioniert und es kam auch kein Fehler mehr. @andig Aus meiner Sicht kann das rein. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a new HTTP API for Tinkerforge WARP chargers as an alternative to the existing MQTT-based implementation. The MQTT API is marked as deprecated but remains supported for backward compatibility.
Key changes:
- New HTTP-based charger implementation with support for both WARP and WARP3 models
- Automatic fallback from newer
metersAPI to legacymeterAPI - Phase auto-switching control differs between WARP models (always disabled for WARP3, configurable via Energy Manager for WARP)
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/definition/charger/tinkerforge-warp3.yaml | Marks MQTT-based WARP3 template as deprecated |
| templates/definition/charger/tinkerforge-warp3-smart.yaml | Marks MQTT-based WARP3 Smart template as deprecated |
| templates/definition/charger/tinkerforge-warp3-http.yaml | New HTTP-based template for WARP3 chargers with phase auto-switch disabled |
| templates/definition/charger/tinkerforge-warp.yaml | Marks MQTT-based WARP template as deprecated |
| templates/definition/charger/tinkerforge-warp-http.yaml | New HTTP-based template for WARP chargers with Energy Manager support |
| charger/warp2-mqtt_decorators.go | Auto-generated decorator file for MQTT implementation |
| charger/warp2-mqtt.go | MQTT implementation moved from previous location |
| charger/warp/types.go | Adds new feature constants and meter value type definitions |
| charger/warp-http_decorators.go | Auto-generated decorator file for HTTP implementation |
| charger/warp-http.go | New HTTP API implementation with meters API support and fallback logic |
| // metersValueIds returns an array of 6 indices mapping meter value IDs to their positions | ||
| // in the values array: [VoltageL1N, VoltageL2N, VoltageL3N, CurrentImExSumL1, CurrentImExSumL2, CurrentImExSumL3] |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function comment is inaccurate. It states the function returns an array of 6 indices, but it actually returns a map containing 8 meter value IDs (including power and energy values, not just the 6 voltage/current values mentioned).
| // metersValueIds returns an array of 6 indices mapping meter value IDs to their positions | |
| // in the values array: [VoltageL1N, VoltageL2N, VoltageL3N, CurrentImExSumL1, CurrentImExSumL2, CurrentImExSumL3] | |
| // metersValueIds returns a map from meter value IDs to their positions in the values array. | |
| // It covers the following IDs: VoltageL1N, VoltageL2N, VoltageL3N, CurrentImExSumL1, CurrentImExSumL2, | |
| // CurrentImExSumL3, PowerImExSum, and EnergyAbsImExSum. |
| var phases func(int) error | ||
| var getPhases func() (int, error) | ||
| if wb.emHelper != nil { | ||
| if res, err := wb.emState(); err == nil && res.ExternalControl != 1 { |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition 'res.ExternalControl != 1' uses a magic number. This should be compared against a named constant to make the intent clearer. Consider defining constants for external control states in the warp types file.
| if res, err := wb.emState(); err == nil && res.ExternalControl != 1 { | |
| const externalControlEnabled = 1 | |
| if res, err := wb.emState(); err == nil && res.ExternalControl != externalControlEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ich stimme Copilot nicht zu, aber es wäre schön konsistent mit entweder 0 oder 1 zu vergleichen, aber nicht zu mischen.
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDCurrentImExSumL3]; !ok { | ||
| return metersValues, fmt.Errorf("current L3 value ID not found") | ||
| } else if idx >= len(res) { | ||
| return metersValues, fmt.Errorf("current L3 index out of range: idx=%d, len(values)=%d", idx, len(res)) | ||
| } else { | ||
| metersValues.CurrentImExSumL3 = res[idx] | ||
| } | ||
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDPowerImExSum]; !ok { | ||
| return metersValues, fmt.Errorf("power value ID not found") | ||
| } else if idx >= len(res) { | ||
| return metersValues, fmt.Errorf("power index out of range: idx=%d, len(values)=%d", idx, len(res)) | ||
| } else { | ||
| metersValues.PowerImExSum = res[idx] | ||
| } | ||
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDEnergyAbsImExSum]; !ok { | ||
| return metersValues, fmt.Errorf("energy value ID not found") | ||
| } else if idx >= len(res) { | ||
| return metersValues, fmt.Errorf("energy index out of range: idx=%d, len(values)=%d", idx, len(res)) | ||
| } else { | ||
| metersValues.EnergyAbsImExSum = res[idx] | ||
| } | ||
|
|
||
| return metersValues, nil | ||
| } | ||
|
|
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metersValues function has significant code duplication with 8 nearly identical if-else blocks for extracting values. This reduces maintainability and makes the code harder to modify. Consider refactoring this into a helper function that takes the value ID as a parameter and returns the value or error.
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDCurrentImExSumL3]; !ok { | |
| return metersValues, fmt.Errorf("current L3 value ID not found") | |
| } else if idx >= len(res) { | |
| return metersValues, fmt.Errorf("current L3 index out of range: idx=%d, len(values)=%d", idx, len(res)) | |
| } else { | |
| metersValues.CurrentImExSumL3 = res[idx] | |
| } | |
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDPowerImExSum]; !ok { | |
| return metersValues, fmt.Errorf("power value ID not found") | |
| } else if idx >= len(res) { | |
| return metersValues, fmt.Errorf("power index out of range: idx=%d, len(values)=%d", idx, len(res)) | |
| } else { | |
| metersValues.PowerImExSum = res[idx] | |
| } | |
| if idx, ok := wb.metersValuesMap[warp.MetersValueIDEnergyAbsImExSum]; !ok { | |
| return metersValues, fmt.Errorf("energy value ID not found") | |
| } else if idx >= len(res) { | |
| return metersValues, fmt.Errorf("energy index out of range: idx=%d, len(values)=%d", idx, len(res)) | |
| } else { | |
| metersValues.EnergyAbsImExSum = res[idx] | |
| } | |
| return metersValues, nil | |
| } | |
| if value, err := metersValueOrError(wb.metersValuesMap, res, warp.MetersValueIDCurrentImExSumL3, "current L3"); err != nil { | |
| return metersValues, err | |
| } else { | |
| metersValues.CurrentImExSumL3 = value | |
| } | |
| if value, err := metersValueOrError(wb.metersValuesMap, res, warp.MetersValueIDPowerImExSum, "power"); err != nil { | |
| return metersValues, err | |
| } else { | |
| metersValues.PowerImExSum = value | |
| } | |
| if value, err := metersValueOrError(wb.metersValuesMap, res, warp.MetersValueIDEnergyAbsImExSum, "energy"); err != nil { | |
| return metersValues, err | |
| } else { | |
| metersValues.EnergyAbsImExSum = value | |
| } | |
| return metersValues, nil | |
| } | |
| func metersValueOrError(indexMap map[int]int, values []float64, id int, valueName string) (float64, error) { | |
| idx, ok := indexMap[id] | |
| if !ok { | |
| return 0, fmt.Errorf("%s value ID not found", valueName) | |
| } | |
| if idx >= len(values) { | |
| return 0, fmt.Errorf("%s index out of range: idx=%d, len(values)=%d", valueName, idx, len(values)) | |
| } | |
| return values[idx], nil | |
| } |
| // CurrentPower implements the api.Meter interface | ||
| func (wb *WarpHTTP) metersCurrentPower() (float64, error) { | ||
| values, err := wb.metersValues() | ||
|
|
||
| return values.PowerImExSum, err | ||
| } | ||
|
|
||
| // TotalEnergy implements the api.MeterEnergy interface | ||
| func (wb *WarpHTTP) metersTotalEnergy() (float64, error) { | ||
| values, err := wb.metersValues() | ||
|
|
||
| return values.EnergyAbsImExSum, err | ||
| } | ||
|
|
||
| // currents implements the api.PhaseCurrrents interface | ||
| func (wb *WarpHTTP) metersCurrents() (float64, float64, float64, error) { | ||
| values, err := wb.metersValues() | ||
| if err != nil { | ||
| return 0, 0, 0, err | ||
| } | ||
|
|
||
| return values.CurrentImExSumL1, values.CurrentImExSumL2, values.CurrentImExSumL3, nil | ||
| } | ||
|
|
||
| // voltages implements the api.PhaseVoltages interface | ||
| func (wb *WarpHTTP) metersVoltages() (float64, float64, float64, error) { | ||
| values, err := wb.metersValues() | ||
| if err != nil { | ||
| return 0, 0, 0, err | ||
| } | ||
|
|
||
| return values.VoltageL1N, values.VoltageL2N, values.VoltageL3N, nil | ||
| } |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metersCurrentPower, metersTotalEnergy, metersCurrents, and metersVoltages functions all call metersValues separately, which makes separate HTTP requests for each. This is inefficient and could lead to performance issues. Consider caching the result or having a single function that returns all values at once to reduce the number of HTTP requests.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| cc := struct { | ||
| URI string | ||
| User string | ||
| Password string | ||
| EnergyManagerURI string | ||
| EnergyManagerUser string | ||
| EnergyManagerPassword string | ||
| DisablePhaseAutoSwitch bool | ||
| EnergyMeterIndex uint | ||
| }{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cc := struct { | |
| URI string | |
| User string | |
| Password string | |
| EnergyManagerURI string | |
| EnergyManagerUser string | |
| EnergyManagerPassword string | |
| DisablePhaseAutoSwitch bool | |
| EnergyMeterIndex uint | |
| }{} | |
| var cc struct { | |
| URI string | |
| User string | |
| Password string | |
| EnergyManagerURI string | |
| EnergyManagerUser string | |
| EnergyManagerPassword string | |
| DisablePhaseAutoSwitch bool | |
| EnergyMeterIndex uint | |
| } |
| currentPower = wb.metersCurrentPower | ||
| totalEnergy = wb.metersTotalEnergy | ||
| currents = wb.metersCurrents | ||
| voltages = wb.metersVoltages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metersVoltages vs meterVoltages ist arg subtil. Die ifs sind eher ein if featureA else if featureB statt der nil Vergleiche?
| err := wb.GetJSON(uri, &status) | ||
| if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| err := wb.GetJSON(uri, &status) | |
| if err != nil { | |
| if err := wb.GetJSON(uri, &status); err != nil { |
| func (wb *WarpHTTP) meterCurrentPower() (float64, error) { | ||
| var res warp.MeterValues | ||
| uri := fmt.Sprintf("%s/meter/values", wb.uri) | ||
| err := wb.GetJSON(uri, &res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use caching instead of single requests per meter. Or does the Warp support the cache headers? Either cache result or use memcache transport.
| warp.MetersValueIDEnergyAbsImExSum: false, | ||
| } | ||
|
|
||
| var indices = make(map[int]int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use lo.Intersect and/or lo.Diff
| return indices, nil | ||
| } | ||
|
|
||
| func (wb *WarpHTTP) metersValues() (warp.MetersValues, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of duplicated code; refactor
| - name: user | ||
| - name: password | ||
| - name: meterIndex | ||
| default: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| default: 0 |
| default: 0 | ||
| required: true | ||
| description: | ||
| de: Stromzähler Index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kennt der Anwender den "Index"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich glaube ja, man kann die in der UI anlegen. Üblicherweise ist aber nur Index 0 interessant, der eingebaute Zähler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich glaube ja, man kann die in der UI anlegen. Üblicherweise ist aber nur Index 0 interessant, der eingebaute Zähler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kannst du auch über die API auslesen, dann muss der Nutzer das nicht konfigurieren: https://docs.warp-charger.com/docs/interfaces/mqtt_http/api_reference/evse?hardwareType=any#evse_meter_config_any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reicht es dann nicht sich fest auf Index 0 zu beschränken? Der Anwendungszweck/Verwendung ist hier ja ohnehin festgelegt.
Weniger ist mehr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Es könnte theoretisch Nutzer geben, die den Index verstellt haben. Mir wäre aber nicht bekannt, dass irgendjemand das schonmal getan hat, außer wir für interne Tests. Deshalb halte ich das für hinreichend nischig, dass EVCC das nicht unterstützen muss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raus damit ;) Kann ja in der Go Config drin bleiben, das ist das Default dann 0.
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