-
Notifications
You must be signed in to change notification settings - Fork 724
implement iso8601 TimeSpan formatting #190
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -462,7 +462,7 @@ bool DateTime::isValid() const { | |
| */ | ||
| /**************************************************************************/ | ||
|
|
||
| char *DateTime::toString(char *buffer) { | ||
| char *DateTime::toString(char *buffer) const { | ||
| uint8_t apTag = | ||
| (strstr(buffer, "ap") != nullptr) || (strstr(buffer, "AP") != nullptr); | ||
| uint8_t hourReformatted = 0, isPM = false; | ||
|
|
@@ -713,7 +713,7 @@ bool DateTime::operator==(const DateTime &right) const { | |
| @return Timestamp string, e.g. "2020-04-16T18:34:56". | ||
| */ | ||
| /**************************************************************************/ | ||
| String DateTime::timestamp(timestampOpt opt) { | ||
| String DateTime::timestamp(timestampOpt opt) const { | ||
| char buffer[25]; // large enough for any DateTime, including invalid ones | ||
|
|
||
| // Generate timestamp according to opt | ||
|
|
@@ -765,6 +765,161 @@ TimeSpan::TimeSpan(int16_t days, int8_t hours, int8_t minutes, int8_t seconds) | |
| /**************************************************************************/ | ||
| TimeSpan::TimeSpan(const TimeSpan ©) : _seconds(copy._seconds) {} | ||
|
|
||
| /*! | ||
| @brief Create a new TimeSpan from an iso8601 formatted period string | ||
|
|
||
| If the string is entirely malformed (doesn't begin with P or -) this will | ||
| construct a TimeSpan of 0 seconds. If parsing fails before the end of the | ||
| string, this constuctor will interpret its argument as the longest | ||
| well-formed prefix string. | ||
|
|
||
| For example, the string P5M10~ will parse the same as P5M | ||
|
|
||
| @param iso8601 the formatted string | ||
| */ | ||
| TimeSpan::TimeSpan(const char *iso8601) : _seconds(0) { | ||
| int32_t sign = 1; | ||
| const char *cursor = iso8601; | ||
| if (*cursor == '-') { | ||
| sign = -1; | ||
| cursor++; | ||
| } | ||
| if (*cursor == 'P') { | ||
| cursor++; | ||
| } else { | ||
| return; | ||
| } | ||
|
|
||
| char *rest; | ||
| long num = strtol(cursor, &rest, 10); | ||
| cursor = rest; | ||
| if (*cursor == 'D') { | ||
| _seconds += sign * SECONDS_PER_DAY * num; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can overflow.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it can. What should I do about it? Detect overflow, return 0 on failure, and add this failure mode to the docs?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure. It seems to me that the overflow cannot be detected without significant extra complexity. :-(
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you feel about using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe a simple fix would be to build the absolute value of the duration as a _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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) |
||
| cursor++; | ||
| } | ||
|
|
||
| if (*cursor == 'T') { | ||
| cursor++; | ||
| num = strtol(cursor, &rest, 10); | ||
| cursor = rest; | ||
| } else if (*cursor == '\0') { | ||
| return; | ||
| } else { | ||
| // error: malformed iso8601 | ||
| return; | ||
| } | ||
|
|
||
| if (*cursor == 'H') { | ||
| _seconds += sign * SECS_PER_MIN * MINS_PER_HOUR * num; | ||
| cursor++; | ||
| num = strtol(cursor, &rest, 10); | ||
| cursor = rest; | ||
| } | ||
|
|
||
| if (*cursor == 'M') { | ||
| _seconds += sign * SECS_PER_MIN * num; | ||
| cursor++; | ||
| num = strtol(cursor, &rest, 10); | ||
| cursor = rest; | ||
| } | ||
|
|
||
| if (*cursor == 'S') { | ||
| _seconds += sign * num; | ||
| } | ||
| } | ||
|
|
||
| /*! | ||
| @brief Formats this TimeSpan according to iso8601 | ||
|
|
||
| See toCharArray for more details on the format | ||
|
|
||
| @return String of formatted TimeSpan | ||
| */ | ||
| String TimeSpan::toString() const { | ||
| constexpr size_t buflen = 19; | ||
| char buf[buflen]; | ||
| this->toCharArray(buf, buflen); | ||
| return String(buf); | ||
| } | ||
|
|
||
| // Equivalent to left - right unless that would underflow, otherwise 0 | ||
| // | ||
| // 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) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
} |
||
| if (left > right) { | ||
| return left - right; | ||
| } else { | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| /*! | ||
| @brief Formats this TimeSpan according to iso8601 | ||
|
|
||
| Fails (returning 0) if buf is too small to store the result. buf size >= 19 | ||
| will never fail. | ||
| Example: TimeSpan t(32, 23, 54, 11) formats to "P32DT23H54M11S". | ||
|
|
||
| @param buf char array to write output. Always contains trailing '\0'. | ||
| @param len the length of buf. Must be at least 1. | ||
| @return number of bytes written excluding trailing '\0' or 0 on failure | ||
| */ | ||
| size_t TimeSpan::toCharArray(char *buf, size_t len) const { | ||
| size_t written = 0; | ||
|
|
||
| if (_seconds == 0) { | ||
| written += snprintf(buf, len, "PT0S"); | ||
| if (written >= len) { | ||
| return 0; | ||
| } else { | ||
| return written; | ||
| } | ||
| } | ||
|
|
||
| // Keep sign separate from tmp to prevent overflow caused by -1 * INT_MIN | ||
| int8_t sign = _seconds > 0 ? 1 : -1; | ||
| int32_t tmp = _seconds; | ||
| int8_t seconds = sign * (tmp % SECS_PER_MIN); | ||
| // Fold in sign - while dividing by SECS_PER_MIN, tmp can no longer overflow | ||
| tmp /= sign * SECS_PER_MIN; | ||
| int8_t minutes = tmp % MINS_PER_HOUR; | ||
| tmp /= MINS_PER_HOUR; | ||
| int8_t hours = tmp % HOURS_PER_DAY; | ||
| int16_t days = tmp / HOURS_PER_DAY; | ||
|
|
||
| if (_seconds < 0) { | ||
| written += snprintf(buf + written, sat_sub(len, written), "-P"); | ||
| } else { | ||
| written += snprintf(buf + written, sat_sub(len, written), "P"); | ||
| } | ||
|
|
||
| if (days > 0) { | ||
| written += snprintf(buf + written, sat_sub(len, written), "%dD", days); | ||
| } | ||
|
|
||
| if (hours > 0 || minutes > 0 || seconds > 0) { | ||
maxbla marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| written += snprintf(buf + written, sat_sub(len, written), "%s", "T"); | ||
|
|
||
| if (hours > 0) { | ||
| written += snprintf(buf + written, sat_sub(len, written), "%dH", hours); | ||
| } | ||
| if (minutes > 0) { | ||
| written += snprintf(buf + written, sat_sub(len, written), "%dM", minutes); | ||
| } | ||
| if (seconds > 0) { | ||
| written += snprintf(buf + written, sat_sub(len, written), "%dS", seconds); | ||
| } | ||
| } | ||
|
|
||
| if (written >= len) { | ||
| return 0; | ||
| } else { | ||
| return written; | ||
| } | ||
| } | ||
|
|
||
| /**************************************************************************/ | ||
| /*! | ||
| @brief Add two TimeSpans | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ class DateTime { | |
| DateTime(const __FlashStringHelper *date, const __FlashStringHelper *time); | ||
| DateTime(const char *iso8601date); | ||
| bool isValid() const; | ||
| char *toString(char *buffer); | ||
| char *toString(char *buffer) const; | ||
|
|
||
| /*! | ||
| @brief Return the year. | ||
|
|
@@ -145,7 +145,7 @@ class DateTime { | |
| TIMESTAMP_TIME, //!< `hh:mm:ss` | ||
| TIMESTAMP_DATE //!< `YYYY-MM-DD` | ||
| }; | ||
| String timestamp(timestampOpt opt = TIMESTAMP_FULL); | ||
| String timestamp(timestampOpt opt = TIMESTAMP_FULL) const; | ||
|
|
||
| DateTime operator+(const TimeSpan &span); | ||
| DateTime operator-(const TimeSpan &span); | ||
|
|
@@ -215,6 +215,9 @@ class TimeSpan { | |
| TimeSpan(int32_t seconds = 0); | ||
| TimeSpan(int16_t days, int8_t hours, int8_t minutes, int8_t seconds); | ||
| TimeSpan(const TimeSpan ©); | ||
| TimeSpan(const char *iso8601); | ||
| String toString() const; | ||
| size_t toCharArray(char *buf, size_t len) const; | ||
|
|
||
| /*! | ||
| @brief Number of days in the TimeSpan | ||
|
|
@@ -228,21 +231,23 @@ class TimeSpan { | |
| e.g. 4 days, 3 hours - NOT 99 hours | ||
| @return int8_t hours | ||
| */ | ||
| int8_t hours() const { return _seconds / 3600 % 24; } | ||
| int8_t hours() const { | ||
| return _seconds / (SECS_PER_MIN * MINS_PER_HOUR) % HOURS_PER_DAY; | ||
| } | ||
| /*! | ||
| @brief Number of minutes in the TimeSpan | ||
| This is not the total minutes, it includes days/hours | ||
| e.g. 4 days, 3 hours, 27 minutes | ||
| @return int8_t minutes | ||
| */ | ||
| int8_t minutes() const { return _seconds / 60 % 60; } | ||
| int8_t minutes() const { return _seconds / SECS_PER_MIN % MINS_PER_HOUR; } | ||
| /*! | ||
| @brief Number of seconds in the TimeSpan | ||
| This is not the total seconds, it includes the days/hours/minutes | ||
| e.g. 4 days, 3 hours, 27 minutes, 7 seconds | ||
| @return int8_t seconds | ||
| */ | ||
| int8_t seconds() const { return _seconds % 60; } | ||
| int8_t seconds() const { return _seconds % SECS_PER_MIN; } | ||
| /*! | ||
| @brief Total number of seconds in the TimeSpan, e.g. 358027 | ||
| @return int32_t seconds | ||
|
|
@@ -254,6 +259,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 | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you feel about making these either global or public?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought, |
||
| }; | ||
|
|
||
| /** DS1307 SQW pin mode settings */ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.