search

lorenzodonini / ocpp-go

Open Charge Point Protocol implementation in Go
MIT License
275 stars 126 forks source link

Pass ws.Channel to ocppj callbacks #70

Closed michaelbeaumont closed 3 years ago

michaelbeaumont commented 3 years ago

For my purposes at least, it's enough to have the NewChargePointHandler accept ws.Channels, so I haven't changed all of the request callbacks. WDYT? I'll update 2.0 if it looks good. @lorenzodonini

lorenzodonini commented 3 years ago

I like the idea and changes look ok. I just have two considerations:

  1. not convinced about the ChargePointHandler naming, simply because in OCPP 2.0 charge points are suddenly called charging stations. I would imagine this naming will be kept in future versions, so I would suggest making this "future-proof" from the start. WDYT?
  2. I was trying to keep the ocppXX layer as independent from the websocket layer as possible (users don't even see it in normal usage). With these changes the separation becomes less obvious. I would love not to extend the ws.Channel interface with too many accessors in the future, because the point is to not access the websocket layer directly, but keep the ocppXX layer as transparent as possible.

Otherwise I'm fine with adding this as a breaking change for an upcoming release (once the tests are passing).

michaelbeaumont commented 3 years ago
  1. Fixed
  2. I've added a wrapper interface so that at least the transport level is opaque to ocpp1.6
lorenzodonini commented 3 years ago
  1. 👍
  2. Nice!

Assuming we're not adding any functionality to the ChargePointConnection and ChargingStationConnection interfaces, you could even go the extra mile and remove chargePointConnection and chargingStationConnection structs entirely. There's no real need for them, since ws.Channel conforms to the two interfaces you defined (apart from the function naming, but that can be simplified on the websocket layer). Your call on this one.