Vestel: decorate RFID support #21124
Conversation
There was a problem hiding this comment.
Hey @mfuchs1984 - I've reviewed your changes - here's some feedback:
- The conditional logic in
decorateVestelfor different feature combinations is becoming extensive; consider if a more composable decorator pattern might be clearer as more features are added. - User-facing text in
vestel.yamlcontains a mix of languages in the English description (e.g., "und" instead of "and").
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
f9d9ff3 to
7800314
Compare
|
@andig die Änderung resultiert bei Wallboxen mit alter Firmware -Version leider in einem "connection reset by peer" Fehler, siehe #21048 (comment) Damit ist das Problem eher verschärft, würde daher sagen revert, außer dir fällt was ein um das Problem zu beheben. Insgesamt braucht es hier eine Entscheidung: Besitzer der E.ON Variante zum inoffiziellen Update auf die Vestel Firmware zwingen oder nicht. Wenn nicht, gäbe es noch die Möglichkeit, die RFID Funktion über das auslesen der Version zu dekorieren. |
|
…und wir können die FW Version nirgendwo auslesen? |
|
Wie wäre es wenn wir RFID explizit als Templateparameter mit Versionsangabe ausprägen? |
Doch, das geht, wird in Diagnose schon gemacht, siehe Line 329 in 1c4fe7b Sieht dann so aus Soweit ich verstehe, ist die minimal notwendige Version die |
|
Wie im Issue 21359 erwähnt, scheint es mit der Firmware version 3.156.0 zu funktionieren, wenigstens mit der Webasto-Version. #21359 (comment) |
RFID support through Modbus depends on the Vestel Firmware Version. Since updates of the charger are not easy to perform, this PR makes sure to only decorate Identify when the registers are available. Adds hints to the template requirements.
Fixes #21048, fix #21127, refs #20848