Repository: evcc-io/evcc (go)

Issue:
OCPP: Move transaction logic to StatusNotification callback
              Starting a new transaction in the "Finishing" state would never allow the cable to be unplugged again.

@andig needsTransaction() should be moved from the Status() API to the StatusNotification callback on the connector so that it can be called immediately before the charger's internal connection timeout expires.

_Originally posted by @premultiply in https://github.com/evcc-io/evcc/issues/15552#issuecomment-2323573075_
            

Pull request description:
Ocpp: async RemoteStartTransaction by StatusNotification
Fixes #15857
Fixes https://github.com/evcc-io/evcc/issues/15552#issuecomment-2323587994

Patch:
diff --git a/charger/ocpp.go b/charger/ocpp.go
index 4319ff02..ddf87a60 100644
--- a/charger/ocpp.go
+++ b/charger/ocpp.go
@@ -2,7 +2,6 @@ package charger
 
 import (
 	"cmp"
-	"errors"
 	"fmt"
 	"math"
 	"slices"
@@ -14,6 +13,7 @@ import (
 	"github.com/evcc-io/evcc/util"
 	"github.com/lorenzodonini/ocpp-go/ocpp1.6/core"
 	"github.com/lorenzodonini/ocpp-go/ocpp1.6/types"
+	"github.com/samber/lo"
 )
 
 // OCPP charger implementation
@@ -21,12 +21,10 @@ type OCPP struct {
 	log     *util.Logger
 	cp      *ocpp.CP
 	conn    *ocpp.Connector
-	idtag   string
 	phases  int
 	enabled bool
 	current float64
 
-	remoteStart    bool
 	stackLevelZero bool
 	lp             loadpoint.API
 }
@@ -58,7 +56,6 @@ func NewOCPPFromConfig(other map[string]interface{}) (api.Charger, error) {
 		RemoteStart    bool
 	}{
 		Connector:      1,
-		IdTag:          defaultIdTag,
 		MeterInterval:  10 * time.Second,
 		ConnectTimeout: 5 * time.Minute,
 	}
@@ -118,7 +115,7 @@ func NewOCPPFromConfig(other map[string]interface{}) (api.Charger, error) {
 //go:generate go run ../cmd/tools/decorate.go -f decorateOCPP -b *OCPP -r api.Charger -t "api.Meter,CurrentPower,func() (float64, error)" -t "api.MeterEnergy,TotalEnergy,func() (float64, error)" -t "api.PhaseCurrents,Currents,func() (float64, float64, float64, error)" -t "api.PhaseVoltages,Voltages,func() (float64, float64, float64, error)" -t "api.PhaseSwitcher,Phases1p3p,func(int) error" -t "api.Battery,Soc,func() (float64, error)"
 
 // NewOCPP creates OCPP charger
-func NewOCPP(id string, connector int, idtag string,
+func NewOCPP(id string, connector int, idTag string,
 	meterValues string, meterInterval time.Duration,
 	stackLevelZero, remoteStart bool,
 	connectTimeout time.Duration,
@@ -157,21 +154,19 @@ func NewOCPP(id string, connector int, idtag string,
 		return nil, fmt.Errorf("invalid connector: %d", connector)
 	}
 
-	conn, err := ocpp.NewConnector(log, connector, cp)
-	if err != nil {
-		return nil, err
+	if remoteStart {
+		idTag = lo.CoalesceOrEmpty(idTag, cp.IdTag, defaultIdTag)
 	}
 
-	if idtag == defaultIdTag && cp.IdTag != "" {
-		idtag = cp.IdTag
+	conn, err := ocpp.NewConnector(log, connector, cp, idTag)
+	if err != nil {
+		return nil, err
 	}
 
 	c := &OCPP{
 		log:            log,
 		cp:             cp,
 		conn:           conn,
-		idtag:          idtag,
-		remoteStart:    remoteStart,
 		stackLevelZero: stackLevelZero,
 	}
 
@@ -191,13 +186,6 @@ func (c *OCPP) Connector() *ocpp.Connector {
 	return c.conn
 }
 
-func (c *OCPP) effectiveIdTag() string {
-	if idtag := c.conn.IdTag(); idtag != "" {
-		return idtag
-	}
-	return c.idtag
-}
-
 // Status implements the api.Charger interface
 func (c *OCPP) Status() (api.ChargeStatus, error) {
 	status, err := c.conn.Status()
@@ -205,18 +193,6 @@ func (c *OCPP) Status() (api.ChargeStatus, error) {
 		return api.StatusNone, err
 	}
 
-	if c.conn.NeedsAuthentication() {
-		if c.remoteStart {
-			// lock the cable by starting remote transaction after vehicle connected
-			if err := c.initTransaction(); err != nil {
-				c.log.WARN.Printf("failed to start remote transaction: %v", err)
-			}
-		} else {
-			// TODO: bring this status to UI
-			c.log.WARN.Printf("waiting for local authentication")
-		}
-	}
-
 	switch status {
 	case
 		core.ChargePointStatusAvailable,   // "Available"
@@ -312,22 +288,6 @@ func (c *OCPP) Enable(enable bool) error {
 	return err
 }
 
-func (c *OCPP) initTransaction() error {
-	rc := make(chan error, 1)
-	err := ocpp.Instance().RemoteStartTransaction(c.cp.ID(), func(resp *core.RemoteStartTransactionConfirmation, err error) {
-		if err == nil && resp != nil && resp.Status != types.RemoteStartStopStatusAccepted {
-			err = errors.New(string(resp.Status))
-		}
-
-		rc <- err
-	}, c.effectiveIdTag(), func(request *core.RemoteStartTransactionRequest) {
-		connector := c.conn.ID()
-		request.ConnectorId = &connector
-	})
-
-	return ocpp.Wait(err, rc)
-}
-
 // setCurrent sets the TxDefaultChargingProfile with given current
 func (c *OCPP) setCurrent(current float64) error {
 	err := c.conn.SetChargingProfile(c.createTxDefaultChargingProfile(math.Trunc(10*current) / 10))
diff --git a/charger/ocpp/connector.go b/charger/ocpp/connector.go
index d2363061..34bd49e0 100644
--- a/charger/ocpp/connector.go
+++ b/charger/ocpp/connector.go
@@ -1,6 +1,7 @@
 package ocpp
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -31,9 +32,11 @@ type Connector struct {
 	txnCount int // change initial value to the last known global transaction. Needs persistence
 	txnId    int
 	idTag    string
+
+	remoteIdTag string
 }
 
-func NewConnector(log *util.Logger, id int, cp *CP) (*Connector, error) {
+func NewConnector(log *util.Logger, id int, cp *CP, idTag string) (*Connector, error) {
 	conn := &Connector{
 		log:          log,
 		cp:           cp,
@@ -41,6 +44,8 @@ func NewConnector(log *util.Logger, id int, cp *CP) (*Connector, error) {
 		clock:        clock.New(),
 		statusC:      make(chan struct{}, 1),
 		measurements: make(map[types.Measurand]types.SampledValue),
+
+		remoteIdTag: idTag,
 	}
 
 	err := cp.registerConnector(id, conn)
@@ -71,6 +76,24 @@ func (conn *Connector) TriggerMessageRequest(feature remotetrigger.MessageTrigge
 	})
 }
 
+func (conn *Connector) remoteStartTransactionRequest() {
+	rc := make(chan error, 1)
+	err := Instance().RemoteStartTransaction(conn.cp.ID(), func(resp *core.RemoteStartTransactionConfirmation, err error) {
+		if err == nil && resp != nil && resp.Status != types.RemoteStartStopStatusAccepted {
+			err = errors.New(string(resp.Status))
+		}
+
+		rc <- err
+	}, conn.remoteIdTag, func(request *core.RemoteStartTransactionRequest) {
+		connector := conn.id
+		request.ConnectorId = &connector
+	})
+
+	if err := Wait(err, rc); err != nil {
+		conn.log.ERROR.Printf("failed to start remote transaction: %v", err)
+	}
+}
+
 func (conn *Connector) SetChargingProfile(profile *types.ChargingProfile) error {
 	return Instance().SetChargingProfileRequest(conn.cp.ID(), conn.id, profile)
 }
@@ -166,6 +189,12 @@ func (conn *Connector) NeedsAuthentication() bool {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
 
+	return conn.isWaitingForAuth()
+}
+
+// isWaitingForAuth checks if meter values are outdated.
+// Must only be called while holding lock.
+func (conn *Connector) isWaitingForAuth() bool {
 	return conn.status != nil && conn.txnId == 0 && conn.status.Status == core.ChargePointStatusPreparing
 }
 
diff --git a/charger/ocpp/connector_core.go b/charger/ocpp/connector_core.go
index 4531ac9c..c45f54e4 100644
--- a/charger/ocpp/connector_core.go
+++ b/charger/ocpp/connector_core.go
@@ -37,6 +37,14 @@ func (conn *Connector) StatusNotification(request *core.StatusNotificationReques
 		conn.log.TRACE.Printf("ignoring status: %s < %s", request.Timestamp.Time, conn.status.Timestamp)
 	}
 
+	if conn.isWaitingForAuth() {
+		if conn.remoteIdTag != "" {
+			defer conn.remoteStartTransactionRequest()
+		} else {
+			conn.log.DEBUG.Printf("waiting for local authentication")
+		}
+	}
+
 	return new(core.StatusNotificationConfirmation), nil
 }
 
diff --git a/charger/ocpp/cp_core.go b/charger/ocpp/cp_core.go
index 8dee46f3..ddb6dcb8 100644
--- a/charger/ocpp/cp_core.go
+++ b/charger/ocpp/cp_core.go
@@ -15,7 +15,6 @@ var (
 )
 
 func (cp *CP) Authorize(request *core.AuthorizeRequest) (*core.AuthorizeConfirmation, error) {
-	// TODO check if this authorizes foreign RFID tags
 	res := &core.AuthorizeConfirmation{
 		IdTagInfo: &types.IdTagInfo{
 			Status: types.AuthorizationStatusAccepted,
@@ -28,7 +27,7 @@ func (cp *CP) Authorize(request *core.AuthorizeRequest) (*core.AuthorizeConfirma
 func (cp *CP) BootNotification(request *core.BootNotificationRequest) (*core.BootNotificationConfirmation, error) {
 	res := &core.BootNotificationConfirmation{
 		CurrentTime: types.Now(),
-		Interval:    60, // TODO
+		Interval:    60,
 		Status:      core.RegistrationStatusAccepted,
 	}
 
@@ -52,16 +51,11 @@ func (cp *CP) StatusNotification(request *core.StatusNotificationRequest) (*core
 		return nil, ErrInvalidRequest
 	}
 
-	if request.ConnectorId == 0 {
-		return new(core.StatusNotificationConfirmation), nil
+	if conn := cp.connectorByID(request.ConnectorId); conn != nil {
+		return conn.StatusNotification(request)
 	}
 
-	conn := cp.connectorByID(request.ConnectorId)
-	if conn == nil {
-		return nil, ErrInvalidConnector
-	}
-
-	return conn.StatusNotification(request)
+	return new(core.StatusNotificationConfirmation), nil
 }
 
 func (cp *CP) DataTransfer(request *core.DataTransferRequest) (*core.DataTransferConfirmation, error) {
diff --git a/util/modbus/modbus.go b/util/modbus/modbus.go
index 00434600..1c881f8f 100644
--- a/util/modbus/modbus.go
+++ b/util/modbus/modbus.go
@@ -63,6 +63,7 @@ func (s *Settings) String() string {
 
 type meterConnection struct {
 	meters.Connection
+	proto Protocol
 	*logger
 }
 
@@ -71,23 +72,28 @@ var (
 	mu          sync.Mutex
 )
 
-func registeredConnection(key string, newConn meters.Connection) *meterConnection {
+func registeredConnection(key string, proto Protocol, newConn meters.Connection) (*meterConnection, error) {
 	mu.Lock()
 	defer mu.Unlock()
 
 	if conn, ok := connections[key]; ok {
-		return conn
+		if conn.proto != proto {
+			return nil, fmt.Errorf("connection already registered with different protocol: %s", key)
+		}
+
+		return conn, nil
 	}
 
 	connection := &meterConnection{
 		Connection: newConn,
+		proto:      proto,
 		logger:     new(logger),
 	}
 
 	newConn.Logger(connection.logger)
 	connections[key] = connection
 
-	return connection
+	return connection, nil
 }
 
 // NewConnection creates physical modbus device from config
@@ -111,8 +117,6 @@ func NewConnection(uri, device, comset string, baudrate int, proto Protocol, sla
 }
 
 func physicalConnection(proto Protocol, cfg Settings) (*meterConnection, error) {
-	var conn *meterConnection
-
 	if (cfg.Device != "") == (cfg.URI != "") {
 		return nil, errors.New("invalid modbus configuration: must have either uri or device")
 	}
@@ -131,28 +135,24 @@ func physicalConnection(proto Protocol, cfg Settings) (*meterConnection, error)
 		}
 
 		if proto == Ascii {
-			conn = registeredConnection(cfg.Device, meters.NewASCII(cfg.Device, cfg.Baudrate, cfg.Comset))
+			return registeredConnection(cfg.Device, Ascii, meters.NewASCII(cfg.Device, cfg.Baudrate, cfg.Comset))
 		} else {
-			conn = registeredConnection(cfg.Device, meters.NewRTU(cfg.Device, cfg.Baudrate, cfg.Comset))
+			return registeredConnection(cfg.Device, Rtu, meters.NewRTU(cfg.Device, cfg.Baudrate, cfg.Comset))
 		}
 	}
 
-	if cfg.URI != "" {
-		cfg.URI = util.DefaultPort(cfg.URI, 502)
+	uri := util.DefaultPort(cfg.URI, 502)
 
-		switch proto {
-		case Udp:
-			conn = registeredConnection(cfg.URI, meters.NewRTUOverUDP(cfg.URI))
-		case Rtu:
-			conn = registeredConnection(cfg.URI, meters.NewRTUOverTCP(cfg.URI))
-		case Ascii:
-			conn = registeredConnection(cfg.URI, meters.NewASCIIOverTCP(cfg.URI))
-		default:
-			conn = registeredConnection(cfg.URI, meters.NewTCP(cfg.URI))
-		}
+	switch proto {
+	case Udp:
+		return registeredConnection(uri, Udp, meters.NewRTUOverUDP(uri))
+	case Rtu:
+		return registeredConnection(uri, Rtu, meters.NewRTUOverTCP(uri))
+	case Ascii:
+		return registeredConnection(uri, Ascii, meters.NewASCIIOverTCP(uri))
+	default:
+		return registeredConnection(uri, Tcp, meters.NewTCP(uri))
 	}
-
-	return conn, nil
 }
 
 // NewDevice creates physical modbus device from config


Test patch:
diff --git a/charger/ocpp/connector_test.go b/charger/ocpp/connector_test.go
index cc6bd8cd..ba0d8091 100644
--- a/charger/ocpp/connector_test.go
+++ b/charger/ocpp/connector_test.go
@@ -24,7 +24,7 @@ type connTestSuite struct {
 
 func (suite *connTestSuite) SetupTest() {
 	suite.cp = NewChargePoint(util.NewLogger("foo"), "abc")
-	suite.conn, _ = NewConnector(util.NewLogger("foo"), 1, suite.cp)
+	suite.conn, _ = NewConnector(util.NewLogger("foo"), 1, suite.cp, "")
 
 	suite.clock = clock.NewMock()
 	suite.conn.clock = suite.clock