search

lorenzodonini / ocpp-go

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

Parsing of local timestamps not supported - spec violation? #237

Closed andig closed 1 year ago

andig commented 1 year ago

In https://github.com/evcc-io/evcc/issues/10399 we've noticed that timestamps in the form of are rejected:

parsing time "2023-10-19T15:28:07" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "Z07:00"

https://github.com/evcc-io/evcc/issues/10399#issuecomment-1772535765 found:

In the specs of ocpp 1.6 I can read .. 3.15. Time notations This section is normative. Implementations MUST use ISO 8601 date time notation. Message receivers must be able to handle fractional seconds and time zone offsets (another implementation might use them). Message senders MAY save data usage by omitting insignificant fractions of seconds. .. and as mentioned here https://en.wikipedia.org/wiki/ISO_8601 .. If the time is in UTC, add a Z directly after the time without a space. Z is the zone designator for the zero UTC offset. .. and some lines above .. If no UTC relation information is given with a time representation, the time is assumed to be in local time. .. Could this not also be understood as that the "Z" or the other time zone strings for example "+02:00" are optional? Or is this a requirement of evcc to have all chargers and, may be, all other devices at UTC?

This seems to indicate that parsing local time zone should be possible, too?

andig commented 1 year ago

Update: noticed that DateTime supports custom format via DateTimeFormat. Unfortunately that is a global setting and cannot be adjusted per chargepoint.

lorenzodonini commented 1 year ago

RFC3339 indeed doesn't support a timestamp without explicit timezone and I oversimplified the ISO8601 support, due to it actually being a huge collection of different formats (see https://ijmacd.github.io/rfc3339-iso8601/).

I'd fix this by supporting all ISO8601 via an external lib. Shouldn't break anything and instead add more flexibility.

andig commented 1 year ago

That would imho be preferable over a fallback global format 👍🏻

lorenzodonini commented 1 year ago

Added the improved ISO8601 parsing.

The DateTimeFormat global variable is still used for sending messages: this allows to configure the lib to use a specific (if any) format for marshaling timestamps. This can be set to arbitrary format. If set to empty (""), it will use the internal Go default (RFC3339, which is ISO8601-compatible in that case).

andig commented 1 year ago

The DateTimeFormat global variable is still used for sending messages: this allows to configure the lib to use a specific (if any) format for marshaling timestamps.

Problem remains that this approach does not allow to mix different charge points. The format should be per CP instead of global in order to be useful beyond the most basic case?

lorenzodonini commented 1 year ago

Problem remains that this approach does not allow to mix different charge points. The format should be per CP instead of global in order to be useful beyond the most basic case?

You expect sending timestamps in different formats to different clients? Why? By sending an ISO-formatted timestamp to all of them it wouldn't cause any issue, assuming the charge points implement the spec correctly.

andig commented 1 year ago

Please don't get me wrong- I haven't seen this particular issue in the wild yet.

By sending an ISO-formatted timestamp to all of them it wouldn't cause any issue, assuming the charge points implement the spec correctly.

If that wasn't the case, the global fallback setting would not be needed at all, right?

lorenzodonini commented 1 year ago

If that wasn't the case, the global fallback setting would not be needed at all, right?

Totally true for servers. The fallback was originally meant for charge points, in case a vendor wishes to enforce a specific format of the ISO standard.

I'm really hoping to keep this as simple as possible tbh. If this is ever needed I can think of two ways to make this dynamic though:

  1. include a format override option in the DateTime struct itself. This would need to be passed at runtime for every message. It would offer the best flexibility but would require a bit more effort for the applications
  2. a global mapping that can be looked up during serialization. The type serialization is not linked to any state currently (pure object marhsaling operation), so the map needs to be "static". This one is uglier imo
andig commented 1 year ago

Totally true for servers. The fallback was originally meant for charge points, in case a vendor wishes to enforce a specific format of the ISO standard.

Oh yeah, I've had our server use case in mind. Let's solve this when it really becomes a problem.

Thank you for all the efforts spend with maintaining this great library!