fix: improve daylight saving time handling in updateSWChange method, …#80
Conversation
There was a problem hiding this comment.
Pull request overview
Improves EU daylight saving time (summer/winter) switching in NTPClientPlus::updateSWChange() to trigger at the correct transition hour (rather than at the start of the transition date), addressing issue #79.
Changes:
- Compute a “standard-time hour” (without DST) to evaluate the DST transition moment more precisely.
- Update the last-Sunday-in-March rule to switch to DST only at/after 02:00 standard time.
- Update the last-Sunday-in-October rule to switch back to winter time only at/after 02:00 standard time (== 03:00 summer time).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (hourNoDST >= 2) | ||
| { | ||
| this->setSummertime(1); | ||
| summertimeActive = true; | ||
| } | ||
| else | ||
| { | ||
| this->setSummertime(0); | ||
| summertimeActive = false; | ||
| } |
There was a problem hiding this comment.
The Sunday switch blocks duplicate the same state-setting logic (set _summertime + summertimeActive) in both branches. This can be simplified by computing a single boolean (e.g., shouldBeSummer = hourNoDST >= 2 for March, inverse for October) and then applying it once, which reduces branching and makes future rule changes less error-prone.
| if (hourNoDST >= 2) | |
| { | |
| this->setSummertime(1); | |
| summertimeActive = true; | |
| } | |
| else | |
| { | |
| this->setSummertime(0); | |
| summertimeActive = false; | |
| } | |
| bool shouldBeSummer = (hourNoDST >= 2); | |
| this->setSummertime(shouldBeSummer ? 1 : 0); | |
| summertimeActive = shouldBeSummer; |
| if (hourNoDST >= 2) | ||
| { | ||
| this->setSummertime(0); | ||
| summertimeActive = false; | ||
| } | ||
| else | ||
| { | ||
| this->setSummertime(1); | ||
| summertimeActive = true; | ||
| } |
There was a problem hiding this comment.
Same as the March block: the last-Sunday-in-October branch repeats the same setSummertime(...) + summertimeActive = ... assignments in both paths. Consider computing the desired DST state once from hourNoDST and applying it once to avoid duplicated logic.
| if (hourNoDST >= 2) | |
| { | |
| this->setSummertime(0); | |
| summertimeActive = false; | |
| } | |
| else | |
| { | |
| this->setSummertime(1); | |
| summertimeActive = true; | |
| } | |
| bool dstActive = (hourNoDST < 2); | |
| this->setSummertime(dstActive ? 1 : 0); | |
| summertimeActive = dstActive; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ed/unsigned variables
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this->_summertime * this->secondperhour + // Summer time offset | ||
| this->_secsSince1900 + // seconds returned by the NTP server | ||
| ((millis() - this->_lastUpdate) / 1000); // Time since last update | ||
| { |
There was a problem hiding this comment.
Line has trailing whitespace after the opening brace. Please remove it to keep formatting consistent with the rest of the file (e.g., other function bodies start with a clean {).
| { | |
| { |
| // If today is Sunday (dayOfWeek == 7) then this is already the last sunday in march -> set summer time | ||
| // If today is Sunday (dayOfWeek == 7) then this is the last Sunday in March. | ||
| // Switch to summer time at 02:00 standard time (clock jumps to 03:00). | ||
| if(dayOfWeek == 7){ |
There was a problem hiding this comment.
Formatting is inconsistent with the rest of this file (most conditionals use if ( and put a space before {). Please update to the prevailing style (e.g., if (dayOfWeek == 7) {).
| if(dayOfWeek == 7){ | |
| if (dayOfWeek == 7) { |
| // If today is Sunday (dayOfWeek == 7) then this is the last Sunday in October. | ||
| // Switch back to winter time at 02:00 standard time | ||
| // (which corresponds to 03:00 local summer time). | ||
| if(dayOfWeek == 7){ |
There was a problem hiding this comment.
Formatting is inconsistent with the rest of this file (most conditionals use if ( and put a space before {). Please update to the prevailing style (e.g., if (dayOfWeek == 7) {).
| if(dayOfWeek == 7){ | |
| if (dayOfWeek == 7) { |
…fix #79