Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 34 additions & 18 deletions charger/ocpp/cp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")

Expand All @@ -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")
Expand Down
34 changes: 21 additions & 13 deletions charger/ocpp/cp_core.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
91 changes: 91 additions & 0 deletions charger/ocpp/cp_core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(&current)
require.NoError(t, err)
assert.True(t, cp.Connected())
assert.False(t, cp.bootTriggered, "flag should be consumed")
assert.Equal(t, &current, 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a test for the case where a solicited BootNotification is the first-ever boot (no previous metadata)

The updated logic only suppresses reboot notifications when bootTriggered && prev != nil && *prev == *request. Current tests cover solicited boots with unchanged vs changed cached metadata. Please add a test for a first connection where bootTriggered == true and BootNotificationResult is nil, and assert that the reboot-monitor channel receives the request. This will lock in the behavior that first boots always notify the reboot monitor, distinguishing them from reconnects.

Suggested implementation:

func TestTriggeredBootNotificationFirstSolicitedBootNotifiesRebootMonitor(t *testing.T) {
	log := util.NewLogger("test")
	cp := NewChargePoint(log, "test-cp")

	boot := core.BootNotificationRequest{
		ChargePointModel:  "Model",
		ChargePointVendor: "TestVendor",
		FirmwareVersion:   "1.0.0",
	}

	// first-ever connection: no previous metadata, solicited boot
	cp.BootNotificationResult = nil
	cp.bootTriggered = true

	// act: handle solicited boot notification
	cp.handleBootNotificationRequest(context.Background(), &boot)

	// assert: reboot monitor is notified
	select {
	case req := <-cp.bootNotificationRequestC:
		if req.ChargePointModel != boot.ChargePointModel ||
			req.ChargePointVendor != boot.ChargePointVendor ||
			req.FirmwareVersion != boot.FirmwareVersion {
			t.Fatalf("unexpected boot notification in reboot monitor channel: got %+v, want %+v", req, boot)
		}
	case <-time.After(time.Second):
		t.Fatal("expected first solicited BootNotification to notify reboot monitor")
	}
}

// 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.

To integrate this test with the rest of your codebase:

  1. Ensure context and time are imported in cp_core_test.go if they are not already:
    • import "context"
    • import "time"
  2. Replace cp.handleBootNotificationRequest(context.Background(), &boot) with the actual helper/function you use in other tests to feed a core.BootNotificationRequest through the charge point logic (e.g., cp.OnBootNotification, cp.HandleBootNotification, etc.), matching its signature and usage pattern.
  3. If bootNotificationRequestC is a channel of core.BootNotificationRequest rather than pointers, adjust the select-case accordingly (remove the pointer dereference and update the comparisons).
  4. If you already have a shared timeout constant or helper for channel assertions in this test file, use that instead of time.Second for consistency.

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:
Comment thread
premultiply marked this conversation as resolved.
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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (testing): Avoid using a 1-hour timer in the test to reduce flakiness and stray goroutines

In TestBootTimeoutClearsTriggeredFlag, time.AfterFunc(time.Hour, func() {}) is only used to obtain a *time.Timer, but it leaves a long-lived timer and goroutine that will fire later in the background. To avoid extra goroutines and potential flakiness, create a short-lived timer and stop it immediately (e.g. timer := time.NewTimer(time.Second); timer.Stop()), or use time.AfterFunc(0, ...) and stop it right away. The delay doesn’t matter here—only the timer’s identity does—so prefer a timer that doesn’t linger.

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")
}
}
24 changes: 0 additions & 24 deletions charger/ocpp/cp_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading