Support ISO 8601 Duration formatting for RTClib's TimeSpan

[maxbla]

• Arduino board: N/a (but I am testing code regularly on an Arduino Mega 2560)
• Arduino IDE version: 1.8.13

This library provides support for ISO 8601 formatted DateTimes through the toString and timestamp methods, but doesn't have a similar option for TimeSpans. ISO 8601 specifies two formats for durations. (Durations appear to be basically the same thing as this library's TimeSpans)

I would like to add methods that format TimeSpans according to ISO 8601's PnYnMnDTnHnMnS or P<date>T<time> formats, both a zero allocation version (not sure what to call it, maybe format?) and one that returns a std::string called toString. I would also add a TimeSpan constructor taking one argument of const char *iso8601duration. Do you think this feature belongs in this library and would you consider merging a PR containing the changes I laid out?

[drak7]

That sounds like a great addition!

[edgar-bonet]

Without extra context (namely the starting point of the interval), these formats can be ambiguous. “P1M” is supposed to be a one-month duration, but how long is that? 30 days? 31?

The Wikipedia article you linked to references the documentation of the Java Duration class. It is worth noting that this class doesn't deal with the ambiguous units “year” and “month”, and only handles days, hours, minutes and seconds. Days are ambiguous too, if you consider DST changes. However, the documentation of the parse() method specifically states that days are “considered to be exactly 24 hours”. And toString() does not use days, and would write "PT48H" (48 hours) instead of "P2D" (2 days).

[a method] that returns a std::string called toString

std::string is not quite standard in Arduino land. People use String instead. And that would be inconsistent with DateTime::toString(), which is the zero allocation formatter. Maybe you could use overloads:

• toString(char * buffer): zero-allocation version
• toString(void): returns a String

[maxbla]

std::string is not quite standard in Arduino land. People use String instead

Yeah, I misspoke above. I meant String.

the Java Duration class... doesn't deal with the ambiguous units “year” and “month”, and only handles days, hours, minutes and seconds.

I think this would be a good policy for this library as well. Specifically only outputting in days and smaller and failing to parse if the Duration string includes months or larger. I'm comfortable including days because I think it is pretty clear this library doesn't do daylight savings time, but I would be document this behavior in toString. This brings up the question of how to fail parsing, as the idiomatic way for C++ constructors to fail is by throwing (but not in Arduino land!). I think a named constructor returning std::optional<TimeSpan> would work, but that uses C++17 features, which might be too new?

Maybe you could use overloads

Yeah, I could, and your overloads are better than my proposed names. I rather dislike overloads because they make FFI to languages without overloads (C, Python, Go, Rust) annoying. How about

• void toCharArray(char * buffer): zero-allocation version
• String toString(void): returns a String

Also expect a pull request incoming, where you can make further comments.

[edgar-bonet]

how to fail parsing [...]

That's an issue. A few of the DateTime constructors suffer from the same problem. The ones that parse strings don't even have special handling for invalid inputs, and end up building garbage. It's not ideal, but it's cheap. I guess it could be said that the result of building from invalid strings is “unspecified”, which is not nearly as bad as “undefined”. The constructor that takes the numeric parameters (year, month, day, hour, minute, second) returns an invalid object, which is the cheapest approach in this case. Its documentation contains the following:

Warning
If the provided parameters are not valid (e.g. 31 February), the constructed DateTime will be invalid.

See also
The isValid() method can be used to test whether the constructed DateTime is valid.

You could take that last approach if a specif value of TimeSpan was reserved as invalid which, AFAIK, is not the case so far. Or you could take the cheap approach of not trying to validate the input.

std::optional<TimeSpan> would work, but that uses C++17 features, which might be too new?

Indeed. The Arduino IDE (version 1.8.13) passes the flag -std=gnu++11 to the compiler. This selects the GNU dialect of C++11.

[overloads] make FFI to languages without overloads (C, Python, Go, Rust) annoying.

Well, in the context of Arduino, I guess only C is concerned.

Not sure this should be a concern. From what I have seen, current practice on Arduino seem to be to use overloads. The Arduino core has lots of them. It often calls C from C++, but never the other way around.