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.

Sign up for GitHub

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

implement iso8601 TimeSpan formatting #190

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

implement iso8601 TimeSpan formatting #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 157 additions & 2 deletions RTClib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -765,6 +765,161 @@ TimeSpan::TimeSpan(int16_t days, int8_t hours, int8_t minutes, int8_t seconds)
/**************************************************************************/
TimeSpan::TimeSpan(const TimeSpan &copy) : _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);
maxbla marked this conversation as resolved.
Show resolved Hide resolved
cursor = rest;
if (*cursor == 'D') {
_seconds += sign * SECONDS_PER_DAY * num;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can overflow.

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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. :-(

Copy link
Author

@maxbla maxbla Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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) {
Copy link
Author

Choose a reason for hiding this comment

The 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
Expand Down
18 changes: 13 additions & 5 deletions RTClib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 &copy);
TimeSpan(const char *iso8601);
String toString() const;
size_t toCharArray(char *buf, size_t len) const;

/*!
@brief Number of days in the TimeSpan
Expand All @@ -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
Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

/** DS1307 SQW pin mode settings */
Expand Down