Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
andig opened this issue Oct 20, 2023 · 9 comments · Fixed by #241
Closed

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

andig opened this issue Oct 20, 2023 · 9 comments · Fixed by #241
Assignees

Comments

@andig
Copy link
Contributor

andig commented Oct 20, 2023

In evcc-io/evcc#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"

evcc-io/evcc#10399 (comment) 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 andig changed the title Parsing of local timestamps violates spec Parsing of local timestamps not supported - spec violation? Oct 20, 2023
@andig
Copy link
Contributor Author

andig commented Oct 20, 2023

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

@lorenzodonini
Copy link
Owner

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.

@lorenzodonini lorenzodonini self-assigned this Oct 23, 2023
@andig
Copy link
Contributor Author

andig commented Oct 23, 2023

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

@lorenzodonini lorenzodonini linked a pull request Oct 30, 2023 that will close this issue
@lorenzodonini
Copy link
Owner

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
Copy link
Contributor Author

andig commented Oct 31, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

andig commented Oct 31, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

andig commented Oct 31, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants