Create EU DST example #308
Conversation
As discussed in Issue adafruit#307
examples/EU DST example
Outdated
| @@ -0,0 +1,52 @@ | |||
| /* EU DST example */ | |||
There was a problem hiding this comment.
This deserves a few more explanations, stating for instance that:
-
The RTC is kept in local winter time. This is not obvious at first sight, and is quite non-standard (common practice is to keep the RTC either in UTC or in local time).
-
This code is for CET only. It can work across Europe if one is willing to accept the DST change to be one hour off.
There was a problem hiding this comment.
Very relevant suggestions.
Point 1 appears to be the key to make it a very simple procedure.
Point 2 The hour is now set to 1 and 3. This can be adapted to your local time (for instance at 2 and 4) if applicable. Therefore it is an example.
I would love to add these point to a comment in the proposed code, but have problems to find out how. (This is the very first pull request I ever do).
There was a problem hiding this comment.
This can be adapted to your local time
I believe a comment explaining how to do so would be of some value.
examples/EU DST example
Outdated
|
|
||
| #define USEDST true // Use DST (true or false) | ||
|
|
||
| DateTime now; |
There was a problem hiding this comment.
This should be local to loop().
There was a problem hiding this comment.
It is also used in the setup part.
There was a problem hiding this comment.
I cannot see the variable now used in setup().
There was a problem hiding this comment.
I think that is not wise to do.
- It is a compiler directive and good practice to declare these directives on the top of the code.
- If you have more complex date/time functionality, it is handy to have this boolean available in the complete program
There was a problem hiding this comment.
Boolean? I am talking about the variable now. I agree that the macro USEDST belongs to the top of the file.
examples/EU DST example
Outdated
| b = DateTime(n.year(), 3, 31, 3, 0, 0); b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Last sunday in march 1:00 | ||
| e = DateTime(n.year(), 10, 31, 3, 0, 0); e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // Last sunday in October 3:00 |
There was a problem hiding this comment.
You have here two calls to dayOfTheWeek(), which is quite an expensive function. You could easily call this only once, and only in March or October: you don't need all this on any other month.
There was a problem hiding this comment.
I don't understand. It is called only twice. For March as well as for October.
There was a problem hiding this comment.
It is always called twice. What I mean is that, most of the year, you don't need to call dayOfTheWeek() at all. From the 25th to the 31st of October, you do have to call dayOfTheWeek() once, but you do not care about the last day of March. Conversely, in March you do not care about the last day of October.
Here is a version of the function (untested) that never calls dayOfTheWeek() more than once, and only does so 14 days per year:
// Return the given DateTime adjusted for DST.
// The provided DateTime is assumed to be in UTC+01:00 (i.e. CET, with no DST correction).
// The result will be in CET, with the DST correction applied if needed.
DateTime dstclock(DateTime n) {
bool isDst;
if (!USEDST) {
isDst = false;
} else if (n.month() < 3 || (n.month() == 3 && n.day() < 25)) {
isDST = false; // winter time before 25 March
} else if (n.month() == 3) {
// DST starts on last Sunday of March at 02:00 UTC+01:00
uint8_t dow = n.dayOfTheWeek();
isDst = (dow == 0 && n.hour() >= 2) || (dow > 0 && n.day() - dow >= 25);
} else if (n.month() < 10 || (n.month == 10 && n.day() < 25)) {
isDst = true; // summer time before 25 October
} else if (n.month() == 10) {
// DST ends on last Sunday of October at 02:00 UTC+01:00
uint8_t dow = n.dayOfTheWeek();
isDst = (dow == 0 && n.hour() < 2) || (dow > 0 && n.day() - dow < 25);
} else { // n.month() > 10
isDst = false; // winter time after October
}
if (isDst)
n = n + TimeSpan(0, 1, 0, 0);
return n;
}This is more source code but, being an if/else chain, only one of the inner blocks is executed.
I just noticed that, given that this function expects its input to always be in UTC+01:00 (CET winter time), the DST switch always happens at 02:00.
There was a problem hiding this comment.
I prefer:
b = DateTime(n.year(), 3, 31, 1, 0, 0); // Begin of DST: March 31 1:00 (am) if (month(n) = 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Last sunday in March 1:00 (am) e = DateTime(n.year(), 10, 31, 3, 0, 0); // End of DST: October 31 3:00 (am) if (month(n) = 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // Last sunday in October 3:00 (am)
Much simpler and - since it is an example - more understandable.
There was a problem hiding this comment.
OK, makes a lot of sense! Note, however, that the time switch should not happen at 01:00 or 03:00, but always at 02:00. This is because, as you have now documented, the RTC is kept in winter time all year long, so this function expects its parameter n to be in CET winter time (UTC+01:00). In Europe, DST transitions always happen at 01:00 UTC.
examples/EU DST example
Outdated
|
|
||
| DateTime n, b, e; | ||
|
|
||
| n = rtc.now(); | ||
| b = DateTime(n.year(), 3, 31, 3, 0, 0); b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Last sunday in march 1:00 | ||
| e = DateTime(n.year(), 10, 31, 3, 0, 0); e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // Last sunday in October 3:00 | ||
|
|
||
| if (USEDST && (n > b) && (n < e)) n = n + TimeSpan(0,1,0,0); // adjust standard time if within summertime | ||
| return n; |
There was a problem hiding this comment.
Avoid code duplication:
| DateTime n, b, e; | |
| n = rtc.now(); | |
| b = DateTime(n.year(), 3, 31, 3, 0, 0); b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Last sunday in march 1:00 | |
| e = DateTime(n.year(), 10, 31, 3, 0, 0); e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // Last sunday in October 3:00 | |
| if (USEDST && (n > b) && (n < e)) n = n + TimeSpan(0,1,0,0); // adjust standard time if within summertime | |
| return n; | |
| return dstclock(rtc.now()); |
There was a problem hiding this comment.
It is not duplicated. For both the begin and the end date the last sunday is relevant. So the function should be called twice.
There was a problem hiding this comment.
What I mean is that getclock() could be reduced to this:
DateTime getclock() {
return dstclock(rtc.now());
}
examples/EU DST example
Outdated
| if (USEDST && (rtc.now() != getclock())) n = n - TimeSpan(0,1,0,0); // if summertime then adjust to the standard time or | ||
| if (USEDST && (rtc.now() != dstclock(rtc.now()))) n = n - TimeSpan(0,1,0,0); // if summertime then adjust to the standard time or |
There was a problem hiding this comment.
You should not rely on rtc.now() here: if the user is calling this it's because the RTC is not set.
There was a problem hiding this comment.
Valid point! rtc.adjust(n); should be added to line 36.
(Stil figgering out how to adjust the propsed code)
There was a problem hiding this comment.
DST support v2.txt
I was not able to find out how to incorporate your valueable review comments into the code. I am very sorry about that! So I included a new version with the changes as a result of your review.
There was a problem hiding this comment.
For amending a pull request, you can simply push extra commits to your “patch-1” branch.
Version 3 with all comments processed
There was a problem hiding this comment.
I just noticed the weird name of the file. It should be renamed in accordance with the Arduino sketch specification: avoid the space character, add the extension .ino, and move it to a directory with the same name (but without the extension). Also, the word example is redundant for a sketch belonging to a collection of examples.
I suggest: EU DST example → DST_Europe/DST_Europe.ino
This is important for the CI pipeline to build-test your example. You may notice that the build job simply ignored it. Had the file been properly named, CI would have warned that it doesn't compile.
examples/EU DST example
Outdated
| @@ -0,0 +1,59 @@ | |||
| /* EU DST example */ | |||
| /* version 3: dd 14-10-2024 */ | |||
There was a problem hiding this comment.
This date format is ambiguous. Prefer the ISO date format: 2024-10-14. Or, better yet, remove the line: the history of this file is in the git repository anyways.
examples/EU DST example
Outdated
| DateTime b, e; | ||
|
|
||
| b = DateTime(n.year(), 3, 31, 1, 0, 0); // Begin of DST: set on March 31 1:000 (am) | ||
| if (month(n) = 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March |
There was a problem hiding this comment.
This doesn't compile. You mean:
| if (month(n) = 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March | |
| if (n.month() == 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March |
examples/EU DST example
Outdated
| b = DateTime(n.year(), 3, 31, 1, 0, 0); // Begin of DST: set on March 31 1:000 (am) | ||
| if (month(n) = 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March | ||
| e = DateTime(n.year(), 10, 31, 3, 0, 0); // End of DST: set on October 31 3:00 (am) | ||
| if (month(n) = 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October |
There was a problem hiding this comment.
Same here:
| if (month(n) = 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October | |
| if (n.month() == 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October |
examples/EU DST example
Outdated
|
|
||
| void setclock(DateTime n) { // if the clock is set during summertime then adjust the clock to standard time | ||
|
|
||
| if (USEDST && (n != dstclock(n)) n = n - TimeSpan(0,1,0,0); // if summertime then adjust to the standard time |
There was a problem hiding this comment.
There are mismatched parentheses here:
| if (USEDST && (n != dstclock(n)) n = n - TimeSpan(0,1,0,0); // if summertime then adjust to the standard time | |
| if (USEDST && n != dstclock(n)) n = n - TimeSpan(0,1,0,0); // if summertime then adjust to the standard time |
examples/EU DST example
Outdated
| // initialise the rtc | ||
| rtc.begin(); | ||
| // if (rtc.lostPower()) setclock(DateTime(F(__DATE__), F(__TIME__))); // Set date and time for use with the DS3231 RTC | ||
| if (!rtc.isrunning()) setclock(DateTime(F(__DATE__), F(__TIME__))); // Set date and time for use with the DS1307 RTC |
There was a problem hiding this comment.
This doesn't compile: the class RTC_DS3231 doesn't have a method named isrunning(). You probably want to comment-out this line and uncomment the previous one. Alternatively, comment-out the current declaration of rtc and uncomment RTC_DS1307 rtc;.
examples/EU DST example
Outdated
| void loop() { | ||
| now = getclock(); // read the time from the RTC and adjust for DST or | ||
| // now = dstclock(rtc.now()); // read the time from the RTC and adjust for DST | ||
| } |
There was a problem hiding this comment.
It would be nice if this example did something visible, just like all the others do. The simplest would be to initialize the serial port in setup(), then:
void loop() {
DateTime now = getclock(); // read the time from the RTC and adjust for DST
Serial.println(now.timestamp());
delay(1000);
}There was a problem hiding this comment.
This time it compiles :-) and a nice demonstration is added.
This time it compiles :-) and a DST demonstration on the serial port is added.
Minor improvements, combined with new filename and directory
There was a problem hiding this comment.
Thanks for fixing the file name. However, you created a new copy of the file, instead of moving (git mv) the old one. Now the old one has to be removed (with git rm).
I tested this new version with RTC_Millis, adding some printout for the October transition. The serial output is a nice demonstration of the DST functionality! Here is what I got:
2024-03-31 00:59:57
2024-03-31 00:59:58
2024-03-31 00:59:59
2024-03-31 01:00:00
2024-03-31 02:00:01
2024-03-31 02:00:02
...
2024-10-27 03:59:57
2024-10-27 03:59:58
2024-10-27 03:59:59
2024-10-27 03:00:00
2024-10-27 03:00:01
2024-10-27 03:00:02
This is wrong for a few reasons:
- The March transition should happen at 02:00 winter time = 03:00 summer time. At lest for CET. This transition at 01:00 winter time would be correct for WET, but this is only a small part of Europe (PT, IE and UK).
- Even for WET, the transition is one second too late. The clock should jump from 00:59:59 to 02:00:00.
- The October transition should happen at 03:00 summer time = 02:00 winter time. At least for CET. This transition at 04:00 summer time would be correct for EET (FI, GR, RO, BG...).
Here is what the correct transitions would be for CET:
2024-03-31 01:59:57
2024-03-31 01:59:58
2024-03-31 01:59:59
2024-03-31 03:00:00
2024-03-31 03:00:01
2024-03-31 03:00:02
...
2024-10-27 02:59:57
2024-10-27 02:59:58
2024-10-27 02:59:59
2024-10-27 02:00:00
2024-10-27 02:00:01
2024-10-27 02:00:02
examples/DST_Europe/DST_Europe.ino
Outdated
| b = DateTime(n.year(), 3, 31, 1, 0, 0); // Begin of DST: set on March 31 1:000 (am) | ||
| if (n.month() == 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March | ||
| e = DateTime(n.year(), 10, 31, 3, 0, 0); // End of DST: set on October 31 3:00 (am) | ||
| if (n.month() == 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October |
There was a problem hiding this comment.
As I already explained a couple of times, the DST transitions should happen at 02:00 RTC time, not at 01:00 or 03:00.
examples/DST_Europe/DST_Europe.ino
Outdated
| e = DateTime(n.year(), 10, 31, 3, 0, 0); // End of DST: set on October 31 3:00 (am) | ||
| if (n.month() == 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October | ||
|
|
||
| if (USEDST && (n > b) && (n < e)) n = n + TimeSpan(0,1,0,0); // adjust to standard time if within summertime |
There was a problem hiding this comment.
The test n > b should be n >= b, otherwise the transition happens one second too late.
examples/DST_Europe/DST_Europe.ino
Outdated
| DateTime getclock() { // Retrieve the (DST adjusted) date and time | ||
|
|
||
| DateTime n, b, e; | ||
|
|
||
| n = rtc.now(); | ||
| b = DateTime(n.year(), 3, 31, 1, 0, 0); // Begin of DST: set on March 31 1:000 (am) | ||
| if (n.month() == 3) b = DateTime(n.year(), 3, 31 - b.dayOfTheWeek(), 1, 0, 0); // Begin of DST: adjusted to last sunday in March 1:00 (am) when actual month is March | ||
| e = DateTime(n.year(), 10, 31, 3, 0, 0); // End of DST: set on October 31 3:00 (am) | ||
| if (n.month() == 10) e = DateTime(n.year(), 10, 31 - e.dayOfTheWeek(), 3, 0, 0); // End of DST: adjusted to last sunday in October 3:00 (am) when actual month is October | ||
|
|
||
| if (USEDST && (n > b) && (n < e)) n = n + TimeSpan(0,1,0,0); // adjust to standard time if within summertime | ||
| return n; | ||
| }; |
There was a problem hiding this comment.
Replace this function with
DateTime getclock() { // Retrieve the (DST adjusted) date and time
return dstclock(rtc.now());
};There was a problem hiding this comment.
I realized that this function could be replaced with:
#define getclock() dstclock(rtc.now())
Do you want me to make that change?
There was a problem hiding this comment.
I have a slight preference for a function over a #define, as a function is more type-safe and more C++-idiomatic. But I don't care that much, and I am fine with whatever option you prefer.
examples/DST_Europe/DST_Europe.ino
Outdated
|
|
||
| // initialise the serial port | ||
| Serial.begin(BAUD); | ||
| Wire.begin(); |
There was a problem hiding this comment.
No need to initialize Wire here: this is done by RTClib. Besides, there would be no point in initializing Wire after rtc.begin().
examples/DST_Europe/DST_Europe.ino
Outdated
| Serial.print(now.timestamp(DateTime::TIMESTAMP_DATE)); Serial.print(" "); // print the date | ||
| Serial.println(now.timestamp(DateTime::TIMESTAMP_TIME)); // print the time | ||
|
|
||
| // show DST corrected time (only when in DST period, otherwise this equals the standard time) |
There was a problem hiding this comment.
Contrary to what the comment says, the lines below are not conditioned on the clock being in DST period.
There was a problem hiding this comment.
----------------- New start ------------------
Actual standard time: 2024-10-14 16:21:11
Actual DST corrected time: 2024-10-14 17:21:11 <- corrected because in DST
----------------- New start ------------------
Actual standard time: 2024-11-14 16:21:11
Actual DST corrected time: 2024-11-14 16:21:11 <- not corrected because not in DST
Replaced by newer verion on other location
I sincerely hope all the comments are processed this time.
Improved comments and replacment of getclock procedure by a define statement. Because improving code readability with a #define is more elegant than with an extra function that consumes memory and cycles.
The DSTclock procedure is made visible so that it can be used to convert time from and to DST time. However, if the need exists to convert the actual time to standard time, this procedure did not function correctly. This is improved in this new version.
|
As discussed in Issue #307