implement iso8601 TimeSpan formatting #190

maxbla · 2020-08-03T12:41:36Z

fixes #181

I ran this test. It basically tests that serializing and constructing returns the same TimeSpan that was serialized.

int main() {
    std::default_random_engine generator;
    std::uniform_int_distribution<int32_t> distribution(-2147483648, 2147483647);
    char buf[19];
    for (int i=0; i<10000000; i++) {
        int32_t seconds = distribution(generator); 
        TimeSpan t(seconds);
        t.toCharArray(buf, 19);
        TimeSpan t1(buf);
        assert(t1._seconds == t._seconds);
    }
}

I was looking for somewhere to put tests like this, but it seems like this repository doesn't really have tests.

edgar-bonet

Just a few comments:

RTClib.cpp

maxbla · 2020-08-04T05:55:03Z

RTClib.h

@@ -231,6 +234,9 @@ class TimeSpan {

 protected:
 int32_t _seconds; ///< Actual TimeSpan value is stored as seconds
+ static constexpr int32_t SECS_PER_MIN = 60; ///< Number of seconds in a minute
+ static constexpr int32_t MINS_PER_HOUR = 60; ///< Number of minutes in an hour
+ static constexpr int32_t HOURS_PER_DAY = 24; ///< Number of hours in a day


How would you feel about making these either global or public?

I have no strong feelings either way: they certainly look like generally useful, but they are kind of trivial, and they would risk collisions in the global namespace. Maybe public would be OK. I guess you should get the opinion of a maintainer: I am just a random contributor.

Maybe a static (constexpr) global then? That way they won't cause global namespace collisions. This library as-is has the magic number antipattern. Fixing it is out of scope for this PR though.

On second thought, static constexprs won't work in this case because they are in the header. Statics in a header are textually #included, and static means limited to a "translation uint", so the constants would still be exposed in the public interface.

edgar-bonet

Looking good. Some more comments:

RTClib.cpp

edgar-bonet · 2020-08-04T09:18:10Z

RTClib.cpp

+ long num = strtol(cursor, &rest, 10);
+ cursor = rest;
+ if (*cursor == 'D') {
+ _seconds += sign * SECONDS_PER_DAY * num;


This can overflow.

Yeah, it can. What should I do about it? Detect overflow, return 0 on failure, and add this failure mode to the docs?

I'm not sure. It seems to me that the overflow cannot be detected without significant extra complexity. :-(

How would you feel about using __builtin_smull_overflow? It is supported in both gcc and clang, but it's non-standard.

__builtin_smull_overflow() looks nice, but I do not know whether it is supported by all the environments where RTClib can potentially be used.

Maybe a simple fix would be to build the absolute value of the duration as a uint32_t. That won't prevent the overflow, but at least it would avoid undefined behavior. Then, at the end of the constructor,

_seconds = sign * abs_seconds;

is guaranteed to wrap correctly to the correct signed value, as long as the result is in the range of an int32_t.

all the environments where RTClib can potentially be used

Is RTClib used outside the Arduino ecosystem? And if so, can it be used with any compiler other than Clang or GCC? (clang includes these intrinsics too)

RTClib.cpp

maxbla · 2020-10-31T01:58:48Z

What's the status of this PR? Should I do something to move it along?

edgar-bonet · 2020-10-31T09:33:33Z

I guess you could solve the two issues raised by GitHub:

This pull request is still a work in progress
This branch has conflicts that must be resolved

The conflict is pretty easy to fix:

diff --cc RTClib.cpp
index e282548,2ecffb4..3a0ff37
--- a/RTClib.cpp
+++ b/RTClib.cpp
@@@ -694,8 -713,8 +713,8 @@@ bool DateTime::operator==(const DateTim
      @return Timestamp string, e.g. "2020-04-16T18:34:56".
  */
  /**************************************************************************/
 -String DateTime::timestamp(timestampOpt opt) {
 +String DateTime::timestamp(timestampOpt opt) const {
-   char buffer[20];
+   char buffer[25]; // large enough for any DateTime, including invalid ones
  
    // Generate timestamp according to opt
    switch (opt) {

I would consider rebasing on top of master, and maybe squashing the last 9 commits together.

As for the “work in progress” status, you should be able to change it to “Ready for review”, when you are ready.

maxbla · 2021-03-10T04:13:22Z

This PR is ready for (final) review and I have time to fix any problems identified in said review.

maxbla · 2021-03-10T07:18:48Z

RTClib.cpp

+// At the time of writing, this is only used in logic in TimeSpan::toCharArray
+// This function's inclusion is very unfortunate, and if it is used anywhere
+// else, should probably use a template
+static size_t sat_sub(size_t left, size_t right) {


It feels dirty to add this function in here, but it is the cleanest way I could think to handle the repeated

if (written >= len) { buf[len - 1] = '\0'; return 0; }

edgar-bonet

LGTM. I hope it gets merged, with or without my suggestions.

RTClib.cpp

Uses iso 8601 period format. Includes a constructor from a string. Makes all toString-like methods const

edgar-bonet reviewed Aug 3, 2020

View reviewed changes

RTClib.cpp Outdated Show resolved Hide resolved

RTClib.cpp Outdated Show resolved Hide resolved

RTClib.cpp Outdated Show resolved Hide resolved

RTClib.cpp Outdated Show resolved Hide resolved

RTClib.cpp Outdated Show resolved Hide resolved

RTClib.cpp Show resolved Hide resolved

maxbla commented Aug 4, 2020

View reviewed changes

maxbla mentioned this pull request Aug 4, 2020

Fix compiler warnings #191

Closed

edgar-bonet reviewed Aug 4, 2020

View reviewed changes

maxbla force-pushed the master branch 3 times, most recently from ac9e45a to a123854 Compare March 10, 2021 03:23

maxbla marked this pull request as ready for review March 10, 2021 03:50

maxbla force-pushed the master branch 5 times, most recently from 9f002c2 to 1dca08a Compare March 10, 2021 06:34

maxbla commented Mar 10, 2021

View reviewed changes

edgar-bonet approved these changes Mar 10, 2021

View reviewed changes

RTClib.cpp Outdated Show resolved Hide resolved

add methods to format TimeSpans

3d61e6d

Uses iso 8601 period format. Includes a constructor from a string. Makes all toString-like methods const

maxbla force-pushed the master branch from 1dca08a to 3d61e6d Compare March 10, 2021 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement iso8601 TimeSpan formatting #190

implement iso8601 TimeSpan formatting #190

maxbla commented Aug 3, 2020

edgar-bonet left a comment

maxbla Aug 4, 2020

edgar-bonet Aug 4, 2020

maxbla Aug 5, 2020

maxbla Mar 15, 2021

edgar-bonet left a comment

edgar-bonet Aug 4, 2020

maxbla Aug 4, 2020

edgar-bonet Aug 4, 2020

maxbla Mar 10, 2021 •

edited

Loading

edgar-bonet Mar 10, 2021

maxbla Mar 10, 2021

maxbla commented Oct 31, 2020

edgar-bonet commented Oct 31, 2020

maxbla commented Mar 10, 2021

maxbla Mar 10, 2021

edgar-bonet left a comment

implement iso8601 TimeSpan formatting #190

Are you sure you want to change the base?

implement iso8601 TimeSpan formatting #190

Conversation

maxbla commented Aug 3, 2020

edgar-bonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgar-bonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxbla Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxbla commented Oct 31, 2020

edgar-bonet commented Oct 31, 2020

maxbla commented Mar 10, 2021

Choose a reason for hiding this comment

edgar-bonet left a comment

Choose a reason for hiding this comment

maxbla Mar 10, 2021 •

edited

Loading