Add EVSE Master (Besen, Telestar, Morec, Sync chargers)#28359
Add EVSE Master (Besen, Telestar, Morec, Sync chargers)#28359maxkna111 wants to merge 16 commits intoevcc-io:masterfrom
Conversation
Adds support for EVSE Master compatible charging stations (e.g. Morec) that use the proprietary UDP protocol on port 28376. The device is auto-discovered via its periodic Login broadcast — only the serial number (printed on the label) and the app password are required in the config; no IP address is needed. Implements api.Charger, api.Meter, api.MeterEnergy, api.PhaseCurrents and api.PhaseVoltages. Uses a singleton shared UDP listener (same pattern as the KEBA UDP integration). Protocol reverse-engineered from https://github.com/johnwoo-nl/emproto. Tested on a real Morec station.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The EVSEMaster type starts a long-lived run goroutine and exposes a done channel but never provides a Close/Stop method to signal shutdown or to unsubscribe from the listener, which can lead to goroutine and subscription leaks over the process lifetime.
- In NewEVSEMaster, on the 60s discovery timeout you unsubscribe from the listener but do not signal wb.done, so the run goroutine continues blocked on wb.recv forever; consider closing done (and optionally recv) in the timeout path as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The EVSEMaster type starts a long-lived run goroutine and exposes a done channel but never provides a Close/Stop method to signal shutdown or to unsubscribe from the listener, which can lead to goroutine and subscription leaks over the process lifetime.
- In NewEVSEMaster, on the 60s discovery timeout you unsubscribe from the listener but do not signal wb.done, so the run goroutine continues blocked on wb.recv forever; consider closing done (and optionally recv) in the timeout path as well.
## Individual Comments
### Comment 1
<location path="charger/evsemaster.go" line_range="98" />
<code_context>
+ }
+ lst.Subscribe(serial, wb.recv)
+
+ go wb.run()
+
+ // Block until the EVSE has broadcast, we've logged in, and the first
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor timeout leaks the background goroutine because `done` is never closed on error
If the 60s wait in `NewEVSEMaster` times out you call `lst.Unsubscribe(serial)` and return, but the `run()` goroutine continues because `wb.done` is never closed. That leaves a leaked goroutine still blocked on `wb.recv` and managing timers/state. Ensure `wb.done` is closed (and/or provide a `Close`/`Stop` to invoke here) so the goroutine exits on timeout.
</issue_to_address>
### Comment 2
<location path="charger/evsemaster/protocol.go" line_range="108-115" />
<code_context>
+ for _, b := range buf[:totalLen-4] {
+ sum += uint32(b)
+ }
+ if uint16(sum%0xFFFF) != binary.BigEndian.Uint16(buf[totalLen-4:]) {
+ return nil, fmt.Errorf("checksum mismatch")
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unpack validates header and checksum but not the tail marker
You already validate the header and checksum, but you never confirm that the final two bytes equal `packetTail` (0x0f02). Please add a `packetTail` check after the checksum comparison so malformed or misaligned packets are rejected.
```suggestion
// Verify checksum
var sum uint32
for _, b := range buf[:totalLen-4] {
sum += uint32(b)
}
if uint16(sum%0xFFFF) != binary.BigEndian.Uint16(buf[totalLen-4:]) {
return nil, fmt.Errorf("checksum mismatch")
}
// Verify packet tail marker
if binary.BigEndian.Uint16(buf[totalLen-2:]) != packetTail {
return nil, fmt.Errorf("invalid packet tail")
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add skiptest requirement to template so TestTemplates doesn't try to instantiate the charger without a real device on the network - Add Close() method to stop background goroutine and unsubscribe listener - Close wb.done on 60s discovery timeout to prevent goroutine leak - Validate packetTail marker in Unpack to reject malformed packets - Fix whitespace lint: remove leading newline before switch statement - Fix gofmt: align comments in PackChargeStart to match surrounding block
54594b2 to
6a99460
Compare
- Use registry.AddCtx and thread ctx through constructor and run()
goroutine; replace done chan with ctx.Done() for lifecycle management
- Extract Connection type into evsemaster package (serial+password+send
logic belongs in the protocol package, not the outer charger)
- Rename Listener.WriteTo -> Send
- Remove separate reloginTick; merge heartbeat timeout detection into
keepaliveTick (single ticker, simpler logic)
- Rename maxAmps -> current to match evcc conventions
- Remove evsemaster: prefix from all error messages (logger adds context)
- Use api.ErrTimeout for timeout/offline conditions throughout
- Only update wb.current after successful MaxCurrent send
- Return api.ErrTimeout from meter methods when status is nil
- Add EXPOSE 28376/udp to Dockerfile
- var cc = struct{} style in FromConfig
ce26945 to
2e8b7b0
Compare
|
had to do a couple more mods to stabilize the implementation and have faster feedback in the ui upon state changes |
- Wait up to 10s for first EVSE broadcast on startup; log a warning if none arrives but still return successfully so evcc does not fail to load. The charger comes online automatically once the EVSE broadcasts. - Simplify nil guard in Status/Enabled: time.Since(zero) already exceeds 30s so the redundant status==nil check is dropped. - Remove GetMaxCurrent (api.CurrentGetter): it only mirrored the commanded current back to evcc, adding no new information.
54ec1f5 to
d6ba741
Compare
- Remove loggedIn field: updatedAt is the single source of truth for data freshness; the blocking constructor guarantees it is set before returning, making loggedIn redundant - Block hard on init: constructor waits up to 60s for the first ACStatus and returns api.ErrTimeout on failure, consistent with all other evcc chargers (evcc retries globally) - Introduce evsemasterTimeout const (60s) shared by the constructor and all staleness checks - Enable/MaxCurrent no longer guard on loggedIn: they just send commands; send() silently drops if evseAddr is nil (device not yet seen) - Replace status == nil checks in meter methods with unified updatedAt staleness check - Add debug logs on every timeout path to aid diagnostics
965e86b to
725fae6
Compare
|
ready for review 🤞 |
|
Did a round of simplifications/idiomatic changes- could you check if it still works? |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
currentfield inEVSEMasteris written inMaxCurrentand read inEnablewithout synchronization, which can cause a data race under concurrent access; consider guarding it with the existing mutex or using an atomic value. - In
NewEVSEMaster, only the serial length is validated while the actual hex validity is deferred toPacket.Pack; validating the serial as hex early would surface configuration errors with clearer feedback. PackSetCurrent’s comment states the range 6–32 A but the function doesn’t enforce it andMaxCurrentforwards any value directly; consider validating/clamping the allowed range or returning an error to avoid sending unsupported currents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `current` field in `EVSEMaster` is written in `MaxCurrent` and read in `Enable` without synchronization, which can cause a data race under concurrent access; consider guarding it with the existing mutex or using an atomic value.
- In `NewEVSEMaster`, only the serial length is validated while the actual hex validity is deferred to `Packet.Pack`; validating the serial as hex early would surface configuration errors with clearer feedback.
- `PackSetCurrent`’s comment states the range 6–32 A but the function doesn’t enforce it and `MaxCurrent` forwards any value directly; consider validating/clamping the allowed range or returning an error to avoid sending unsupported currents.
## Individual Comments
### Comment 1
<location path="charger/evsemaster.go" line_range="90-99" />
<code_context>
+// ACStatus is received. Returns api.ErrTimeout if the EVSE does not respond
+// within 60 seconds – check serial, password, and that the charger is on the
+// same network segment (UDP broadcast does not cross VLANs).
+func NewEVSEMaster(ctx context.Context, serial, password string) (*EVSEMaster, error) {
+ log := util.NewLogger("evsemaster")
+
+ if len(serial) != 16 {
+ return nil, fmt.Errorf("serial must be a 16-character hex string, got %q", serial)
+ }
+
+ conn, err := evsemaster.NewConnection(log, serial, password)
+ if err != nil {
+ return nil, err
+ }
+
+ wb := &EVSEMaster{
+ log: log,
+ conn: conn,
+ current: 6,
+ data: util.NewMonitor[*evsemaster.ACStatus](evsemasterTimeout),
+ }
+
+ done := make(chan struct{})
+ go wb.run(ctx, done)
+
+ select {
+ case <-done:
+ return wb, nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor timeout branch leaks the background goroutine and UDP resources
In `NewEVSEMaster`, the `time.After(evsemasterTimeout)` branch returns `api.ErrTimeout` without cancelling the context, so the `run` goroutine and UDP connection keep running even though construction failed.
Please derive a scoped context with timeout inside the constructor (e.g. `ctx, cancel := context.WithTimeout(ctx, evsemasterTimeout)`) and ensure it is cancelled both on timeout and on early error, so the background goroutine and resources are reliably cleaned up when you return an error.
</issue_to_address>
### Comment 2
<location path="charger/evsemaster.go" line_range="144" />
<code_context>
+ wb.conn.Subscribe(recv)
+ defer wb.conn.Unsubscribe()
+
+ for tick := time.NewTicker(15 * time.Second); ; {
+ select {
+ case <-ctx.Done():
</code_context>
<issue_to_address>
**issue (bug_risk):** Ticker is never stopped, which can leak resources on context cancellation
In `run`, the ticker created in the `for` loop is never stopped, so when `ctx.Done()` triggers and the function returns, the ticker goroutine and resources remain. Over time, especially with repeated creation/teardown, this can accumulate.
Prefer:
```go
tick := time.NewTicker(15 * time.Second)
defer tick.Stop()
for {
select {
// ...
}
}
```
This preserves behaviour while ensuring the ticker is cleaned up on exit.
</issue_to_address>
### Comment 3
<location path="charger/evsemaster.go" line_range="61" />
<code_context>
+
+ mu sync.RWMutex
+ data *util.Monitor[*evsemaster.ACStatus]
+ current int // last value set by MaxCurrent
+
+ // evseAddr is the EVSE's source address (e.g. 192.168.1.100:11938).
</code_context>
<issue_to_address>
**issue (bug_risk):** `current` is accessed from multiple goroutines without synchronization, causing a data-race risk
`current` is written in `MaxCurrent` and read in `Enable` (via `PackChargeStart`) without using `mu`. Since these can run in different goroutines, please either guard all accesses to `current` with `mu` or switch `current` to an `atomic.Int32`/`Int64` to ensure `PackChargeStart` sees a consistent value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@andig thanks for your contribition
exactly at 10 seconds mark after evcc spins up which i didn't exactly understand where that limit is coming from (it would help if this limit was slightly higher to deal with this issue other than that everything else is working as expected thoughts? |
|
So the code is quite minimal now but doesn't work. Isn't that nice :( I'd really like to find out why without reverting. There is no 10s delay involved anywhere. Afaiu the code, the charger should only ever be returned once fully functional, i.e. addr received and status received. There are different messages though! Is it possible to receive CmdACStatus but not have seen a login yet? In that case we probably shouldn't close the May that be it? See last commit for surfacing the error. |
|
i will try to test things in the coming days as am getting busy |
What does this add?
Support for EVSE Master app compatible charging stations (e.g. Sync, Besen, Telestar, Morec) that use the proprietary UDP protocol on port 28376.
How it works
The EVSE periodically broadcasts a Login packet (cmd
0x0001) from its own ephemeral port to our port 28376. The integration learns the device's IP and port from that broadcast source address — so only the serial number (printed on the device label) and the password (set in the EVSE Master app) are needed in config. No IP address required.A singleton shared UDP listener routes packets to subscribers by serial number, following the same pattern as the existing KEBA UDP integration.
Interfaces implemented
api.Charger(Status, Enabled, Enable, MaxCurrent)api.Meter(power)api.MeterEnergy(total energy)api.PhaseCurrentsapi.PhaseVoltagesFiles
charger/evsemaster/protocol.gocharger/evsemaster/listener.gocharger/evsemaster.gotemplates/definition/charger/evsemaster-udp.yamlTesting
Tested on a real Sync station with a car plugged in. Status, power, voltage, current readings all working. Charge start/stop and MaxCurrent confirmed working.
Notes