Tinkerforge WARP: add WARP4 support with vehicle SoC via ISO 15118 - #31446
Conversation
|
My VW ID.4 is identified instantly and the reported SoC is correct. |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="templates/definition/charger/tinkerforge-warp4-ws.yaml" line_range="5" />
<code_context>
+products:
+ - { brand: TinkerForge, description: { generic: WARP4 Smart } }
+ - { brand: TinkerForge, description: { generic: WARP4 Pro } }
+capabilities: ["iso151182", "mA", "rfid", "1p3p", "dim"]
+requirements:
+ description:
</code_context>
<issue_to_address>
**issue (review_instructions):** The capability "iso151182" appears to be a typo and is inconsistent with the usual "iso15118" naming used in other templates.
In other charger templates, the ISO 15118 capability is typically labeled "iso15118". Using "iso151182" here will likely break capability detection and is inconsistent with the established wording in the templates directory. Please adjust this entry to match the standard capability name defined in templates/README.md / other existing templates.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `templates/**/*.yaml`
**Instructions:**
Verify that the changes comply to the rules defined in templates/README.md. Also check consistence (wording) with other files inside templates directory.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -162,6 +160,14 @@ func NewWarpWS(ctx context.Context, uri, user, pass, emURI, emUser, emPass strin | |||
| } | |||
| } | |||
|
|
|||
| // WARP4: probe the ev/state api providing vehicle data read via ISO 15118; | |||
| // no locking needed, the websocket goroutines are not started yet | |||
| var ev warp.EvState | |||
There was a problem hiding this comment.
Sorry if I have my picky day, but I don't like this either: Getting here only to test for the result somewhere else creates more confusion (and is not what I meant). Just to be sure: every other feature is detectced via HasFeature. This is not possible here?
There was a problem hiding this comment.
Unfortunately not (at least not now): the firmware only announces evse, meter(s), nfc, phase_switch etc. in info/features, the ev module does not register a feature (checked all addFeature calls in the sources). So probing the api is the only option. Reworked it to read like hasFeature: stateless probe, called where the result is used. I'll ask Tinkerforge to add a feature flag so this can switch to hasFeature eventually.
There was a problem hiding this comment.
But maybe it's worth opening a PR for adding "ev" to the info/features, so that evcc can drop the probe in the future.
There was a problem hiding this comment.
@andig, should we defer this PR until the "ev" feature is available in the WARP4 firmware?
There was a problem hiding this comment.
/cc @rtrbt any chance to get this announced as feature?
There was a problem hiding this comment.
This should be a "no-brainer". I've just opened a PR and will check this evening...
Tinkerforge/esp32-firmware#503
There was a problem hiding this comment.
Announcing the "ev" feature works fine and with the latest changes evcc still correctly identifies my car and reads the SoC.
So we'll have to wait for the Tinkerforge PR being merged...
@andig, please re-review the commit.
There was a problem hiding this comment.
This should be a "no-brainer". I've just opened a PR and will check this evening...
Tinkerforge/esp32-firmware#503
Sorry, this took a while. I've changed the PR to add the "iso15118" feature, see the PR for an explaination.
|
LGTM |
Do you want me to squash the commits in the PR or are you going to squash-merge them later? |
|
I can always squash when committing :) |
|
LGTM except that the feature is now called |
|
Thanks @rtrbt, I've changed the code accordingly and test this evening with the latest WARP4 firmware from master... |
3edce63 to
d6478c1
Compare
|
One nitpick: WARP4 chargers ship with "Read state of charge" disabled to disable the PLC modem/ISO15118 communication completely. We do this because with the modem enabled, the charger would have to wait two minutes before switching to the old charging process via IEC 61851 when charging a non-ISO vehicle, for example a Twingo. You could either
|
|
Thanks for the context @rtrbt! I would prefer documenting it over auto-enabling: switching One suggestion though: could the firmware announce Independent of that, I will extend the template documentation: vehicle identification requires ISO 15118 to be enabled, reading the SoC additionally requires "Read state of charge". |
It wouldn't if you return 0 or |
|
Ok, soc() already returns api.ErrNotAvailable when ev/state carries no data, so a stock WARP4 will fall back to the vehicle and a conditional feature announcement is cosmetic only. |
Nope that's not what features are for. A feature (almost) always indicates that a piece of hardware is available. For example the ethernet feature indicates an ethernet PHY is available, but you can still unplug the cable or deactivate ethernet via the API.
This will then also fall back to the vehicle's data if reading the SoC via iso 15118 fails? Seems to be a good idea. |
- new template tinkerforge-warp4-ws (WARP4 Smart/Pro) with iso151182 capability - warp-ws: probe ev/state endpoint and register api.Battery when available - warp-ws: subscribe to ev/state websocket events for live SoC updates - warp-ws: disable WARP phase auto switching also for warp4 devices Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mac address read via ISO 15118 is exposed in ev/state. Prefer it over the RFID tag so vehicles can be matched by their identifiers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- detect WARP4 via already fetched device type instead of probing ev/state - align soc with identify: return result first Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Probe the endpoint in NewWarpWS before the websocket goroutines start, avoiding both device name matching and init-time locking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ev module is not announced in info/features, so hasFeature cannot detect it. Probe the api statelessly in NewWarpWSFromConfig instead; evState is populated by the initial websocket push. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Requires WARP4 firmware announcing the ev feature, see Tinkerforge/esp32-firmware#503. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tinkerforge announces the feature from the iso15118 module instead of the ev module, since the ev module will also be ported to devices without a PLC modem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the separate WARP4 template with per-product entries in the combined template. SoC reading requires the built-in meter and is documented as Pro-only; vehicle identification works on both variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Rein damit? |
Vielleicht mache ich heute Abend sicherheitshalber noch einen letzten Test mit der gerade veröffentlichten Firmware?! :-D |
|
Just re-tested with firmware 2.12.2 and a fresh evcc database: Still looking good :-) |
fixes #31448
Adds support for the recently released Tinkerforge WARP4 charger. WARP4 inherits the WARP2/3 API, so charging control already works with the existing
warp-wsimplementation. What's new is ISO 15118 communication: the wallbox reads the vehicle's SoC at charge start and estimates it during the session, exposed via theev/stateAPI.tinkerforge-warp-wstemplate with per-product capabilities (iso151182on both,meteron Pro)warp-wsregistersapi.Batterywhen theiso15118feature is announced ininfo/features(WARP4 firmware 2.12.2 or later), returningapi.ErrNotAvailablewhile no vehicle SoC is known, in which case evcc falls back to the vehicle's own SoC sourceidentifyprefers the ISO 15118 MAC fromev/stateover the RFID tag, so vehicles can be matched via their configured identifiersTODO
iso15118feature (released as 2.12.2)iso15118-2anddin70121(WARP4 implements both)