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.

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

Daylight Saving Time support #1058

Merged
merged 2 commits into from
Jul 20, 2016
Merged

Daylight Saving Time support #1058

merged 2 commits into from
Jul 20, 2016

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented Jul 10, 2016

Implements initial Daylight Saving Time (DST) API proposed in #211.

Added Wiring functions:

  /* Retrieve the current DST offset that is added to the current local time when
   * Time.beginDST() has been called.
   * The default is 1 hour.
   */
  float TimeClass::getDSTOffset();
  /* Set a custom DST offset */
  void TimeClass::setDSTOffset(float offset);
  /* Add the offset from getDSTOffset() to the current time */
  void TimeClass::beginDST();
  /* Do not add the offset from getDSTOffset() to the current time */
  void TimeClass::endDST();
  /* Returns true if DST is in effect (beginDST() was called previously) */
  uint8_t TimeClass::isDST();

Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

FEATURE

  • added Daylight Saving Time support #1058 per proposed #211

@avtolstoy avtolstoy added this to the 0.6.x milestone Jul 10, 2016
@technobly technobly merged commit ddd190d into develop Jul 20, 2016
@ScruffR
Copy link
Contributor

ScruffR commented Sep 2, 2016

I'm not sure how this will actually address the issue of switching DST on and off 😕

In #211 there was talk about a way to set the switching pattern

TimeChangeRule usEDT = {"EDT", Second, Sun, Mar, 2, -240}; //UTC - 4 hours
TimeChangeRule usEST = {"EST", First, Sun, Nov, 2, -300}; //UTC - 5 hours
Timezone usEastern(usEDT, usEST);

// calls Time.zone(), Time.enableDST(), Time.setDSTOffset() using a DST setting determined from
// the current UTC time and the data in the Timezone instance.
Time.timezone(usEstern);

But I can't see any way to set an auto-switching pattern in the docs or in this PR, but that would seem to be the only thing that we are actually missing.
If I still have to decide to switch DST on or off in code, I could do that just as much via Time.zone() as usual - so what?

BTW, these values (zone, DSTOffset and the swtiching rules) should be made sticky too, to add value, otherwise we could dump (setDSTOffset(float)) in favour of beginDST(float) - since we'd have to code both lines ourselves anyway, what's the benefit?

Without the ability to set switching rules (actually not switching rules but ranges to automatically check agains to apply DST or not whenever a time is requested) and persisting the settings the whole "enhancement" here is worth little - IMHO.

@pomplesiegel
Copy link

Oh, i didn't realize that this DST implementation wouldn't automatically switch on and off based on the calendar! Unfortunately I have to agree - an implementation for adding or subtracting amounts of an hour from the current time won't help us very much as product developers.

Right now we're manually mimicking DST by changing the Time.zone() offset as an OTA function call. I don't think this feature in its current state will be be much more elegant in final application implementation.

Is there a gameplan for an automatically adjusting time based on DST calendar rules? Virtually every ioT product in the world needs this.

@technobly technobly deleted the feature/time-dst branch October 27, 2016 17:23
@ScruffR
Copy link
Contributor

ScruffR commented Nov 24, 2016

For keeping some things on the radar, I carried over some bits and bobs to issue #1161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants