Add Kathrein charger #20412
Add Kathrein charger #20412
Conversation
|
Mir ist aufgefallen, dass sich die Funktionen „NewKathreinFromConfig“ und „NewKathrein“ auf das Modbus-RTU-Protokoll beziehen. Die Kathrein-Wallbox lässt sich jedoch nur über das Modbus-TCP-Protokoll ansprechen (ähnlich wie z.B. keba-modbus.go). Siehe auch Absatz „Communication Interface“ im Modbus-Register-Dokument von Kathrein. Und als Unit-ID sollte 0x00 gesetzt werden. |
|
Folgende Punkte sind mir noch aufgefallen: Phasen bei Definition der Variablen Currents und Powers nicht korrekt nummeriert (L1 / L2 / L3): Beim Setzen von MaxCurrentMillis liefert "current" einen Float64-Wert in A, der noch in mA umgerechnet werden muss, also: TotalEnergy: Außerdem bin ich mir nicht sicher, ob die Umrechnung des Registerwerts im Format float32 nach float64 so korrekt ist (kommt an mehreren Stellen vor): |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
api/chargemodestatus.go:57
- The variable 's1' is used but its origin is unclear; if this is unintended, consider using the appropriate variable (possibly 's') or ensuring 's1' is properly defined.
return ChargeStatus(s1), nil
Everything must still be tested, I have not come to it yet // Alles muss hier getestet werden. Leider ist bei mir Zeit im Moment sehr begrenzt |
|
Die Umrechnung eines 32-Bit-Registerwertes (float32) nach float64 geht offenbar so: |
charger/kathrein.go
Outdated
| } | ||
|
|
||
| enabled, err := wb.Enabled() | ||
| if err == nil && enabled { |
charger/kathrein.go
Outdated
| } | ||
|
|
||
| // Switch phases | ||
| _, err = wb.conn.WriteSingleRegister(kathreinRegSetpointRelais, u) |
charger/kathrein.go
Outdated
| case 0x0007: | ||
| return 3, nil | ||
| default: | ||
| return 0, err |
There was a problem hiding this comment.
Gibts diesen Fall?
Und: err ist hier immer nil!
There was a problem hiding this comment.
It may happen? This may prevent simply catching any value, like Kathrein implementing a new return value (like 2phase switching?) in the future without breaking the evcc since any new value will be graciously ignored by evcc. Granted, err is "nil". This will be changed.
charger/kathrein.go
Outdated
| // GetPhases implements the api.PhaseGetter interface | ||
| func (wb *Kathrein) GetPhases() (int, error) { | ||
| b, err := wb.conn.ReadHoldingRegisters(kathreinRegSetpointRelais, 1) | ||
|
|
charger/kathrein.go
Outdated
| fmt.Printf("Device - Timestamp (local):\t%d\n", b) | ||
| } | ||
|
|
||
| if b, err := wb.conn.ReadHoldingRegisters(kathreinRegL1Voltage, 2); err == nil { |
There was a problem hiding this comment.
Können wir die ganzen Messwerte (und ihre zugehörigen Konstanten) bitte weg lassen? Entweder werden sie schon ausgelesen oder sie sind für evcc nicht relevant.
There was a problem hiding this comment.
Yeah, I just wanted to make sure I was reading them correctly when first testing it. I will clean it up once I have a successful test
|
@TheMagnetar great PR, looking almost ready. Can I kindly ask you to contact info@evcc.io? |
|
PS.: what's still missing is a template to make the implementation accessible. |
|
@TheMagnetar Vorschlag: `template: kathrein
|
Done |
|
Failing check: Wäre hier nicht "id:0" korrekt? @premultiply |
|
Sieht so aus. Dafür muss die 1 aus der defaults.yaml raus ODER hier sollte ausnahmsweise mal einfach nur |
|
Laut Manual ignoriert die Box ohnehin die Modbus-ID also ist es völlig egal was dort eingetragen wird. Kann also auch 1 sein. |
|
Supi. Was fehlt denn dann hier noch? |
|
In der kathrein.go (Zeile 150) und im Template kathrein.yaml ist aber "ID:0" gesetzt. |
|
Wie gesagt. Das geht halt so nicht. Und wenns eh egal ist können wir das such ignorieren. |
|
Mein Kommentar zielte auf die Frage ab, ob man das dann nicht auch in den beiden Dateien anpassen müsste. Wir hatten die ID auf 0 gesetzt aufgrund der Aussage im Modbus-Dokument von Kathrein: "At the moment, the Unit-ID is ignored, but 0x00 (Broadcast) is recommended for future compatibility." |
Reviewer's GuideThis pull request introduces support for the Kathrein wallbox by adding a new charger implementation that communicates via Modbus TCP. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @TheMagnetar - I've reviewed your changes - here's some feedback:
- Please ensure the license header in
charger/kathrein.gomatches the project's standard MIT license. - Consider making the enabling of EMS control (
kathreinRegEMSControlRegister) configurable instead of always enabling it during initialization.
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.
|
|
||
| var _ api.Diagnosis = (*Kathrein)(nil) | ||
|
|
||
| // Diagnose implements the api.Diagnosis interface |
There was a problem hiding this comment.
issue (complexity): Consider creating a helper function to encapsulate the register reading and printing logic in the Diagnose method.
Consider abstracting the repeated read/print logic into a helper that accepts a conversion function. For example, you could add a helper like this:
func (wb *Kathrein) readAndPrint(reg uint16, count uint16, label string, conv func([]byte) string) {
if b, err := wb.conn.ReadHoldingRegisters(reg, count); err == nil {
fmt.Printf("%s:\t%s\n", label, conv(b))
}
}Then, update Diagnose() to call this helper. For instance:
func (wb *Kathrein) Diagnose() {
wb.readAndPrint(kathreinRegHeader, 1, "Header - Mapping Version",
func(b []byte) string {
return fmt.Sprintf("%d", b[1])
})
wb.readAndPrint(kathreinRegDeviceInfo, 1, "Device - Info",
func(b []byte) string {
return fmt.Sprintf("%d", b[1])
})
wb.readAndPrint(kathreinRegDeviceNumber, 8, "Device - Number",
func(b []byte) string { return string(b) })
// …continue for remaining registers…
}This refactoring minimizes duplication, reduces nesting, and preserves all functionality.
Since this is my first contribution to this project and it is not yet tested, I have marked this PR it as draft to gather feedback.
Summary by Sourcery
Add support for Kathrein electric vehicle chargers with Modbus TCP integration
New Features:
Documentation:
Chores: