-
Notifications
You must be signed in to change notification settings - Fork 709
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
Wrote fixDateTime()
method
#185
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test it yet, but I fear that the various +=
operations are prone to overflow. Maybe the intermediate calculations should be done on 16-bit local variables.
RTClib.cpp
Outdated
// make month valid to prevent getDaysInMonth() returning 0 | ||
// Otherwise, infinite loop possible | ||
if (m > 12) { | ||
temp = m % 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably meant to be m / 12
temp = m % 12; | |
temp = m / 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
RTClib.cpp
Outdated
m++; | ||
if (m > 12) { | ||
yOff++; | ||
m--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be m = 1
or m -= 12
m--; | |
m -= 12; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
I did a small test that shows the effect of the
The first one is the correct, expected behavior. The second one is incorrect: it should return 2000-01-01T04:16:00. Edit: the case when either the month or the day is zero is also mishandled:
Month 0 should presumably be understood as December from the previous year, but As a side note, I find the name of the method ( |
RTClib.cpp
Outdated
*/ | ||
/**************************************************************************/ | ||
bool isLeapYear(uint16_t year) { | ||
return year % 400 == 0 || (year % 4 == 0 && year % 100 != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified isLeapYear()
and made it static to prevent misuse.
Fixed bug with `fixDateTime()` overflowing DateTime data members. Simplified `isLeapYear()` function for efficiency at the cost of not working for years of multiples of 100 and not multiples fo 400. Also wrote warning for limitation. Made `isleapYear()` static to avoid misuse. Rewrote warning for `fixDateTime()` to specify that the year becomes 2100 if fixing DateTime requires setting the year to before 2000.
Wrote
fixDateTime()
method: attempts to fix invalid DateTime object by decrementing invalid time/date components and incrementing next components to compensate. E.g. If DateTime object represents the 32 July 2019 then runningfixDateTime()
will change it to 1 August 2019.Also wrote
getDaysInMonth()
andisLeapYear()
functions. These are used byfixDateTime()
hence the inclusion.