lorenzodonini / ocpp-go

Open Charge Point Protocol implementation in Go
MIT License
262 stars 125 forks source link

Chargepoint: Handling connection drops #141

Closed andig closed 2 years ago

andig commented 2 years ago

Consider this simple client:

chargePoint := ocpp16.NewChargePoint(chargePointId, nil, nil)

// Set a handler for all callback functions
handler := new(ChargePointHandler)
chargePoint.SetCoreHandler(handler)

go func() {
    for err := range chargePoint.Errors() {
        fmt.Println(err)
    }
}()

// Connects to central system
if err := chargePoint.Start(url); err != nil {
    log.Fatal(err)
}

log.Printf("connected to central system at %v", url)
bootConf, err := chargePoint.BootNotification("model1", "vendor1")
if err != nil {
    log.Fatal(err)
} else {
    log.Printf("status: %v, interval: %v, current time: %v", bootConf.Status, bootConf.Interval, bootConf.CurrentTime.String())
}

select {}

I've noticed that no errors are logged when the websocket connection goes away, even if it can't be re-established.

What is the best practice to diagnose/handle connection issues?

lorenzodonini commented 2 years ago

Errors are logged by the library if you enable verbose logging.

The correct way to handle a disconnection would be to register a handler on the lower layer:

dispatcher := ocppj.NewDefaultClientDispatcher(ocppj.NewFIFOClientQueue(0))
endpoint := ocppj.NewClient(chargePointId, nil, dispatcher, nil, core.Profile, localauth.Profile, firmware.Profile, reservation.Profile, remotetrigger.Profile, smartcharging.Profile)
endpoint.SetOnReconnectedHandler(func() {
    // no params here, just an event            
})
endpoint.SetOnDisconnectedHandler(func(err error) {
    // analyze disconnect cause
})
chargePoint := ocpp16.NewChargePoint(chargePointId, endpoint, nil)
// your application code

This should deliver the information you are looking for (propagated directly from the websocket).

Note that by default the charge point will try to reconnect forever, unless you explicitly invoke the Stop method.

andig commented 2 years ago

Thank you for the clarification. Unfortunately, I can't get the messaging working. When I use your approach, the chargepoint will never successfully connect to the server:

dispatcher := ocppj.NewDefaultClientDispatcher(ocppj.NewFIFOClientQueue(0))
endpoint := ocppj.NewClient(chargePointId, nil, dispatcher, nil, core.Profile, localauth.Profile, firmware.Profile, reservation.Profile, remotetrigger.Profile, smartcharging.Profile)
endpoint.SetOnReconnectedHandler(func() {
    fmt.Println("reconnect")
})
endpoint.SetOnDisconnectedHandler(func(err error) {
    fmt.Println("disconnect")
})
chargePoint := ocpp16.NewChargePoint(chargePointId, endpoint, nil)

Instead, this works just fine:

chargePoint := ocpp16.NewChargePoint(chargePointId, nil, nil)
andig commented 2 years ago

Btw, noticed there's a different default for NewFIFOClientQueue in NewClient vs NewChargePoint.

andig commented 2 years ago

Think I found it. NewChargePoint adds the 1.6 Subprotocol to the websocket client which NewClient doesn't do. So this will work, but needs the entire websocket client setup (but not the dispatcher):

client := ws.NewClient()

client.AddOption(func(dialer *websocket.Dialer) {
    // Look for v1.6 subprotocol and add it, if not found
    alreadyExists := false
    for _, proto := range dialer.Subprotocols {
        if proto == types.V16Subprotocol {
            alreadyExists = true
            break
        }
    }
    if !alreadyExists {
        dialer.Subprotocols = append(dialer.Subprotocols, types.V16Subprotocol)
    }
})

endpoint := ocppj.NewClient(chargePointId, client, nil, nil, core.Profile, localauth.Profile, firmware.Profile, reservation.Profile, remotetrigger.Profile, smartcharging.Profile)
endpoint.SetOnReconnectedHandler(func() {
    fmt.Println("reconnect")
})
endpoint.SetOnDisconnectedHandler(func(err error) {
    fmt.Println("disconnect")
})
chargePoint := ocpp16.NewChargePoint(chargePointId, endpoint, client)
lorenzodonini commented 2 years ago

I see, this one is actually a bit nasty, since the websocket client is being auto-generated whenever you create a custom ocppj client. This prevents you (and the top-level package) from setting any additional options.

Thanks a lot for submitting a solution so quickly. I will definitely include it, as it solves the problem at hand, but just fyi I will change some of your changes later on. My main critique points are:

  1. the current default subprotocol is just a fallback (v1.6 currently), but it shouldn't really be there, since multiple subprotocols are supported (this is a critique to the current implementation and has nothing to do with your suggested changes btw)
  2. the DefaultSubprotocol you added should not be returned by the websocket layer, because that layer has no prior knowledge of the actual protocol being run. It should always be the top-level package to pass down the supported sub-protocol value
  3. the option function is unnecessarily verbose for a simple setter and should be abstracted away, similarly to the server's AddSupportedSubprotocol counterpart
    • your suggestion is a step in the right direction, but I plan on making it dynamic
lorenzodonini commented 2 years ago

Btw, noticed there's a different default for NewFIFOClientQueue in NewClient vs NewChargePoint.

This was most likely a mistake on my side. I was probably conflicted whether I should allow unlimited queue size by default when creating a client or limit it to a reasonable amount and it ended up like that. But I think I'll streamline it to unlimited queue size.

lorenzodonini commented 2 years ago

FYI @andig https://github.com/lorenzodonini/ocpp-go/pull/147. This should improve upon the points I mentioned above. Also, dependency cycles are not a thing, since the subProtocol version is only passed from either the ocpp16 or the ocpp2 packages.

In your specific case, you'd have to do adjust your code to:

wsClient := ws.NewClient()
wsClient.SetRequestedSubProtocol(types.V16Subprotocol) // the new line
dispatcher := ocppj.NewDefaultClientDispatcher(ocppj.NewFIFOClientQueue(0))
// Pass everything to the ocppj layer
endpoint := ocppj.NewClient(chargePointId, wsClient, dispatcher, nil, core.Profile, localauth.Profile, firmware.Profile, reservation.Profile, remotetrigger.Profile, smartcharging.Profile)