diff --git a/charger/ocpp/cp.go b/charger/ocpp/cp.go index f200eceae9..32cb62d3e8 100644 --- a/charger/ocpp/cp.go +++ b/charger/ocpp/cp.go @@ -22,10 +22,11 @@ type CP struct { id string - connected bool - bootTimer *time.Timer // timeout for BootNotification wait after WebSocket connect - connectC chan struct{} - meterC chan struct{} + connected bool + bootTimer *time.Timer // timeout for BootNotification wait after WebSocket connect + bootTriggered bool + connectC chan struct{} + meterC chan struct{} // configuration properties PhaseSwitching bool @@ -153,21 +154,34 @@ func (cp *CP) onTransportConnect() { defer cp.mu.Unlock() cp.stopBootTimer() - cp.bootTimer = time.AfterFunc(Timeout, cp.onBootTimeout) + cp.bootTriggered = false + + // The timer pointer itself identifies this connection: stopBootTimer or a + // later onTransportConnect replaces it, so delayed callbacks below detect a + // stale connection by finding cp.bootTimer no longer equal to their timer. + var timer *time.Timer + timer = time.AfterFunc(Timeout, func() { + cp.onBootTimeout(timer) + }) + cp.bootTimer = timer - // Proactively trigger BootNotification after a short delay. - // This helps chargers that don't send it spontaneously (e.g. Wallbox FW 6.x). - // The TriggerMessage is sent directly via the OCPP instance, bypassing the - // Connected() check which would fail at this point. + // Proactively trigger BootNotification after a short delay. This helps + // chargers that don't send it spontaneously (e.g. Wallbox FW 6.x) or stay + // silent on a reconnect because they already consider themselves accepted + // (e.g. Webasto NEXT). The TriggerMessage is sent directly via the OCPP + // instance, bypassing the Connected() check which would fail at this point. time.AfterFunc(TriggerBootDelay, func() { - cp.mu.RLock() - // If BootNotification already arrived or timer was cancelled (disconnect), - // there is nothing to do. - if cp.bootTimer == nil || cp.BootNotificationResult != nil { - cp.mu.RUnlock() + cp.mu.Lock() + // Nothing to do if this connection is gone (disconnect or new connect) or + // the BootNotification already arrived (both clear/replace bootTimer). + if cp.bootTimer != timer { + cp.mu.Unlock() return } - cp.mu.RUnlock() + // Mark the solicited BootNotification so OnBootNotification treats it as a + // handshake rather than a physical reboot. + cp.bootTriggered = true + cp.mu.Unlock() cp.log.DEBUG.Printf("proactively triggering BootNotification") @@ -187,14 +201,16 @@ func (cp *CP) onTransportConnect() { } // onBootTimeout is called when the BootNotification wait timer expires. -func (cp *CP) onBootTimeout() { +func (cp *CP) onBootTimeout(timer *time.Timer) { cp.mu.Lock() - if cp.bootTimer == nil { - // timer was cancelled by disconnect or BootNotification + if cp.bootTimer != timer { + // timer was cancelled by disconnect/BootNotification or superseded by a + // newer connection cp.mu.Unlock() return } cp.bootTimer = nil + cp.bootTriggered = false cp.mu.Unlock() cp.log.DEBUG.Printf("boot notification timeout, proceeding") diff --git a/charger/ocpp/cp_core.go b/charger/ocpp/cp_core.go index d7f5441001..849ddf98ae 100644 --- a/charger/ocpp/cp_core.go +++ b/charger/ocpp/cp_core.go @@ -21,26 +21,34 @@ func (cp *CP) OnBootNotification(request *core.BootNotificationRequest) (*core.B } cp.mu.Lock() + prev := cp.BootNotificationResult cp.BootNotificationResult = request + // A BootNotification evcc solicited to complete the connection handshake is + // not a charger reboot - unless its content changed (e.g. a new firmware + // version), which means the charger actually restarted and setup must re-run. + triggered := cp.bootTriggered && prev != nil && *prev == *request + cp.bootTriggered = false cp.stopBootTimer() cp.mu.Unlock() // mark charge point as ready for communication cp.connect(true) - // Notify the reboot monitor (and the initial Setup). The channel is - // buffered (size 1) and coalescing: if an older notification is still - // queued, drop it so the consumer always re-initializes against the most - // recent BootNotification. This matters when a charge point reboot-loops - // (e.g. issue #30113) faster than the monitor consumes notifications, or - // before the monitor has started at all - the channel must never overflow. - select { - case <-cp.bootNotificationRequestC: - default: - } - select { - case cp.bootNotificationRequestC <- request: - default: + if !triggered { + // Notify the reboot monitor (and the initial Setup). The channel is + // buffered (size 1) and coalescing: if an older notification is still + // queued, drop it so the consumer always re-initializes against the most + // recent BootNotification. This matters when a charge point reboot-loops + // (e.g. issue #30113) faster than the monitor consumes notifications, or + // before the monitor has started at all - the channel must never overflow. + select { + case <-cp.bootNotificationRequestC: + default: + } + select { + case cp.bootNotificationRequestC <- request: + default: + } } return res, nil diff --git a/charger/ocpp/cp_core_test.go b/charger/ocpp/cp_core_test.go index b68624ee83..041764475f 100644 --- a/charger/ocpp/cp_core_test.go +++ b/charger/ocpp/cp_core_test.go @@ -239,3 +239,94 @@ func TestMonitorRebootOnlyOnce(t *testing.T) { require.Eventually(t, func() bool { return callCount.Load() == 1 }, time.Second, 10*time.Millisecond, "setup should be called exactly once") } + +func TestTriggeredBootNotificationDoesNotNotifyRebootMonitor(t *testing.T) { + log := util.NewLogger("test") + cp := NewChargePoint(log, "test-cp") + boot := core.BootNotificationRequest{ + ChargePointModel: "Model", + ChargePointVendor: "TestVendor", + FirmwareVersion: "1.0.0", + } + // unchanged metadata from a previous connection plus a solicited boot + cached := boot + cp.BootNotificationResult = &cached + cp.bootTriggered = true + + current := boot + _, err := cp.OnBootNotification(¤t) + require.NoError(t, err) + assert.True(t, cp.Connected()) + assert.False(t, cp.bootTriggered, "flag should be consumed") + assert.Equal(t, ¤t, cp.BootNotificationResult, "metadata should be refreshed") + + select { + case req := <-cp.bootNotificationRequestC: + t.Fatalf("unchanged triggered BootNotification should not notify reboot monitor, got %s", req.ChargePointModel) + default: + } +} + +// TestTriggeredBootNotificationWithChangedIdentityNotifiesRebootMonitor covers a +// charger that actually restarted (e.g. firmware update) but whose boot arrives as +// a solicited one: the changed content must still trigger a setup re-initialization. +func TestTriggeredBootNotificationWithChangedIdentityNotifiesRebootMonitor(t *testing.T) { + log := util.NewLogger("test") + cp := NewChargePoint(log, "test-cp") + cp.BootNotificationResult = &core.BootNotificationRequest{ + ChargePointModel: "Model", + ChargePointVendor: "TestVendor", + FirmwareVersion: "1.0.0", + } + cp.bootTriggered = true + + _, err := cp.OnBootNotification(&core.BootNotificationRequest{ + ChargePointModel: "Model", + ChargePointVendor: "TestVendor", + FirmwareVersion: "1.1.0", // updated firmware + }) + require.NoError(t, err) + assert.True(t, cp.Connected()) + + select { + case req := <-cp.bootNotificationRequestC: + assert.Equal(t, "1.1.0", req.FirmwareVersion) + default: + t.Fatal("changed triggered BootNotification should notify reboot monitor") + } +} + +// TestBootTimeoutClearsTriggeredFlag ensures a solicited BootNotification that never +// arrives does not leak the bootTriggered flag past the connection: the boot timeout +// must clear it so the next connection's spontaneous boot is treated as a reboot. +func TestBootTimeoutClearsTriggeredFlag(t *testing.T) { + log := util.NewLogger("test") + cp := NewChargePoint(log, "test-cp") + cp.bootTriggered = true + + timer := time.AfterFunc(time.Hour, func() {}) + cp.bootTimer = timer + cp.onBootTimeout(timer) + + assert.False(t, cp.bootTriggered, "flag must be cleared on boot timeout") + assert.True(t, cp.Connected()) +} + +func TestSpontaneousBootNotificationNotifiesRebootMonitor(t *testing.T) { + log := util.NewLogger("test") + cp := NewChargePoint(log, "test-cp") + + _, err := cp.OnBootNotification(&core.BootNotificationRequest{ + ChargePointModel: "Rebooted", + ChargePointVendor: "TestVendor", + }) + require.NoError(t, err) + assert.True(t, cp.Connected()) + + select { + case req := <-cp.bootNotificationRequestC: + assert.Equal(t, "Rebooted", req.ChargePointModel) + default: + t.Fatal("spontaneous BootNotification should notify reboot monitor") + } +} diff --git a/charger/ocpp/cp_setup.go b/charger/ocpp/cp_setup.go index 12b1c7a677..67e33bc468 100644 --- a/charger/ocpp/cp_setup.go +++ b/charger/ocpp/cp_setup.go @@ -107,30 +107,6 @@ func (cp *CP) Setup(ctx context.Context, meterValues string, meterInterval time. } } - // BootNotification is normally received before Setup runs (we wait for it - // after WebSocket connect). Only trigger it as fallback for chargers that - // didn't send it (e.g. timeout-based connection without reboot). - cp.mu.RLock() - hasBootResult := cp.BootNotificationResult != nil - cp.mu.RUnlock() - - if !hasBootResult && cp.HasRemoteTriggerFeature { - if err := cp.TriggerMessageRequest(0, core.BootNotificationFeatureName); err != nil { - cp.log.DEBUG.Printf("failed triggering BootNotification: %v", err) - } - - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(Timeout): - cp.log.DEBUG.Printf("BootNotification timeout") - case res := <-cp.bootNotificationRequestC: - cp.mu.Lock() - cp.BootNotificationResult = res - cp.mu.Unlock() - } - } - // autodetect measurands if meterValues == "" && meterValuesSampledDataMaxLength > 0 { sampledMeasurands := cp.tryMeasurands(desiredMeasurands, KeyMeterValuesSampledData)