Vestel: decode version as utf16 #21656
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mfuchs1984 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
charger/vestel.go
Outdated
| utf16BytesToString := func(b []byte) string { | ||
| s, _ := unicode.UTF16(unicode.BigEndian, unicode.IgnoreBOM).NewDecoder().String(string(b)) | ||
| return s | ||
| } | ||
|
|
||
| fw, _, _ := strings.Cut(strings.TrimPrefix(utf16BytesToString(bytes.TrimRight(b, "\x00")), "v"), "-") |
There was a problem hiding this comment.
Also entweder auf die Funktionsdeklaration verzichten oder (vllt. besser) den ganzen Kram in eine neue Funktion verschieben?
There was a problem hiding this comment.
Kommt von hier, ggf. in die helpers / utils?
evcc/charger/daheimladen-mb.go
Line 264 in b8563f2
There was a problem hiding this comment.
charger/helper.go würde ich spontan sagen. Da ist auch schon bytesAsString() drin.
|
Mir fällt gerade auf, dass dieses encoding auch der Grund sein könnte, warum @DonRoberto1985 den RFID Tag, obwohl er richtig angezeigt wird, nicht erfolgreich verwenden konnte. Der hat in der spec den selben datentyp "String" wie die Firmware Version. |
|
Jo, Strings in Modbus sind immer ein absolutes Ratespiel. Da kann man eigentlich nur raten und grundsätzlich verlieren. |
|
Also wenn richtig angezeigt dann sollte er ja auch richtig dekodiert sein. Aber who knows. |
|
Der Versionsstring wird auch richtig angezeigt wenn man ihn printet, ein Stringvergleich schlägt aber fehl. Konvertiert man die Strings in Byte slices, sieht man wiede den Unterschied. |
|
Ich Bau das mal um. @DonRoberto1985, would you mind testing a nightly when it is ready? |
|
Bestätigung bzgl. RFID #21687 (reply in thread) |
mfuchs1984
force-pushed
the
fix/vestel_version_string
branch
from
e6db7cc to
896b59e
Compare
There was a problem hiding this comment.
Hey @mfuchs1984 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Die Version ist, wie man in #21644 sieht, als utf16 kodiert. Zudem enthält sie noch weitere Versionskomponenten, getrennt mit einem "-".
Der code kann hier gestestet werden: https://go.dev/play/p/918no4SI930