Automatic DST support #3072

nmaggioni · 2018-04-19T00:36:49Z

This PR covers the automatic handling of Daylight Saving Time, based on NIST's specs.

Calculations were inspired by 1 and 2.

This behavior can be toggled ON and OFF via a new CLI setting: tz_automatic_dst. If set to OFF/False, DST is simply ignored.

~~Documentation is planned to be added in a separate PR alongside some lines about tz_offset (currently not mentioned at all in the docs, only in release notes).~~ Docs included.

digitalentity · 2018-04-19T00:48:26Z

src/main/common/time.c

@@ -51,6 +52,7 @@ PG_REGISTER_WITH_RESET_TEMPLATE(timeConfig_t, timeConfig, PG_TIME_CONFIG, 0);



You're adding fields to the parameter group. Please also increase version number in the declaration PG_REGISTER_WITH_RESET_TEMPLATE

Rookie mistake, my bad.

No worries, I keep forgetting to do this myself 😉

Is there any more work left to be done on this? Sorry to be bothering, it seems that I cannot manually mark this request as solved.

digitalentity · 2018-04-19T01:00:18Z

Unfortunately this won't get into 1.9.1, but we'll merge it as soon as we get 1.9.1 out of the door. Can you please amend this or prepare a new PR with documentation?

nmaggioni · 2018-04-19T01:12:47Z

CLI docs done! Should these options also be described elsewhere?

fiam · 2018-04-19T11:26:46Z

Very welcome addition! I think we either need a few more options or a smarter algorithm which takes the GPS location into account. DST change doesn't happen on same day in all places. You can see the full list here https://en.wikipedia.org/wiki/Daylight_saving_time_by_country, but the biggest problem we need to account for is the difference between US and EU.

nmaggioni · 2018-04-19T12:45:01Z

That's true, different offsets should be handled. Messing with GPS data is too much in my opinion, tracking nations & states could quickly turn into an absurd chore... What about a list of special cases?

In the link that you provided I see that Last Sunday March - Last Sunday October is the most common combination: another configuration option could be added to handle other common cases. I, however, don't think that extremely specific offsets should be included in such a list (e.g.: Greenland does Saturday before last Sunday March at 22:00 local time on - Saturday before last Sunday October at 23:00 local time on); this approach could be comprehensive, but sub-optimal.

Another option could be letting users set a specific start/end time and date in a subset of ISO 8601 formats, such as MM-DDThh:mm / 07-16T19:20. This looks easily parseable and feedable into a dateTime_t structure. The downside of this approach is that the burden of specifying the correct dates falls on end users - though we may safely assume that they know how DST behaves in their country, thus making this a non-issue.

If nobody has better ideas I'll try to implement this last solution as a char[] setting.

P.S.: could anybody help me in figuring out why the name CLI setting is not present in the settings' YAML file page but is in the systemConfig_t structure? It is the only char[] setting that I thought I could check the implementation of. Should I just skip including the new tz_automatic_dst_{start,end} settings in the YAML file, or maybe just don't specify a type for them inside it?

stronnag · 2018-04-19T15:44:24Z

Why not just do this in the configurator (set /clear) the extant dst variable. The desktop OS already knows about the global DST rules.

nmaggioni · 2018-04-19T16:26:21Z

@stronnag Could you elaborate? This way you'd depend on the configurator (that is to say: you'll need a PC) to enable/disable DST, or did I get it wrong? It looks a bit too much tight coupling to me, I'd rather keep this feature not configurator-dependant.

stronnag · 2018-04-19T17:37:46Z

Well the user, or at least the majority of them, have a pretty strong dependency on / coupling to the configurator anyway.

So my though process was along the lines that either we end up with a perhaps large amount of code to maintain (and sometimes update) to provide global coverage, or we put a burden on the user to set up some rules which the FC has to parse. The time switch does not happen on the same calendar day each year, so perhaps you end up with something as arcane as the old POSIX TZ rules, for example TZ=EST5EDT,M3.2.0/02:00,M11.1.0/02:00. Otherwise, the user would have to use the configurator to change the absolute calendar dates anyway, so the configurator might as well just do the work. And we're always cognisant of the demands on flash, particularly on F3, and many places don't change the clocks and don't need to expend flash.

So it occurred to me that the configurator could be updated to manage the existing mechanism. After all, the host already knows the rules and the configurator could be programmed to compare the current TZ offset with that in the FC and ask the user if she'd like to adjust the setting for the clocks changing. If it's anything like my household, it would just be another device to adjust.

But I really have no strong views either way.

nmaggioni · 2018-04-19T17:54:42Z

That would be clever, and as you said the user would simply have another device to adjust the time of - not too much of a chore. That could also be compatible with the EU/USA presets that I'm working on, and if a user lives outside these two areas he/she will disable automatic adjustments and (eventually) use the configurator to update the absolute offset.

fiam · 2018-04-19T18:24:38Z

I think we could add a couple of variables which indicate the offset in hours from the start of the year when DST starts/stops (max value is 24*366, so it fits in 16 bits). When connecting from the configurator, we could retrieve those and check them against the host computer. If they match, nothing else needs to be done. If they don’t, we can present a dialog to the user asking if they should be updated.

This just needs one connection from the configurator until the user switches time zones/DST points, etc... While we're at it, we could also do the same for the timezone, since right now users have to set it manually.

This will have very minimal impact on firmware size, but the configurator needs a non-trivial amount of work. I'm only a bit wary of storing the offset in hours and then some country starts switching at xx:30, so maybe we should consider storing minutes like we do for the timezone, but that needs 32 bits.

Thoughts?

stronnag · 2018-04-19T18:30:12Z

as the changeover typically takes place in the early hours of the morning, we probably don't need to worry about the possibility of 1/2 hours.

nmaggioni · 2018-04-19T20:12:11Z

I've implemented @fiam's first suggestion before seeing this last idea.

Leaving the configurator aside for the moment and assuming that everything is done via CLI, what's an easy way for the user to calculate hours since the start of the year and up to the n_th weekday of a given month? Sounds like an easy task for a script but difficult for an human, but I could be missing something evident.

fiam · 2018-04-20T09:27:33Z

@nmaggioni Storing the hours would be only useful for "machine assisted" usage. Calculating the hours manually would be a pain.

With that said, I also like this approach that you've submitted right now. I think it could be merged with a few changes, I'll post a review in a few minutes.

fiam

Please, guard all the new code and settings with #if defined(RTC_AUTOMATIC_DST) and define it for targets with flash size > 128 in src/main/target/common.h. F3 targets are very close to their size limit, so we'll need to start disabling features for them soon.

fiam · 2018-04-20T09:29:25Z

src/main/common/time.h

+typedef enum {
+    DST_EU,
+    DST_USA,
+} tz_dst_country_e;


Name this tz_automatic_dst_e with the following values: TZ_AUTO_DST_OFF, TZ_AUTO_DST_EU, TZ_AUTO_DST_USA. Adjust settings.yaml accordingly.

fiam · 2018-04-20T09:29:49Z

src/main/common/time.h

 typedef struct timeConfig_s {
    int16_t tz_offset; // Offset from UTC in minutes, might be positive or negative
+    bool tz_automatic_dst; // Automatically handle DST or ignore it
+    tz_dst_country_e tz_dst_country;
 } timeConfig_t;


Rather than 2 variables, use just one uint8_t named tz_automatic_dst which can take the values OFF (default), US,EU, etc... That way we use a single byte for storage and users just need to change one setting to make it work. Put a comment at the right indicating that the value comes from tz_automatic_dst_e.

fiam · 2018-04-20T09:32:58Z

src/main/common/time.c

+    dateTime_t dateTime;
+    rtcTimeToDateTime(&dateTime, t);
+    int lastSunday;
+    switch (timeConfig()->tz_dst_country) {


Once you make tz_automatic_dst an uint8_t, cast it here to tz_automatic_dst_e, so the compiler will make sure the checks are exhaustive. i.e.

switch ((tz_automatic_dst_e)timeConfig()-> tz_automatic_dst) {

nmaggioni · 2018-04-20T15:52:21Z

@fiam Done! Thanks for the very helpful pointers.

Includes `tz_dst_country` removal and docs update

Add probot.stale for automatically managing inactive issues

nmaggioni · 2018-05-18T14:29:34Z

@digitalentity Are any other corrections needed or can this be set to mergeable in 2.0?

fiam

Sorry for not reviewing these changes sooner. We've all been pretty busy getting a lot of stuff ready for 2.0.

fiam · 2018-05-18T14:50:26Z

src/main/common/time.c

 {
    rtcTime_t initialTime = dateTimeToRtcTime(dateTimeInitial);
    rtcTime_t offsetTime = rtcTimeMake(rtcTimeGetSeconds(&initialTime) + minutes * 60, rtcTimeGetMillis(&initialTime));
+    #if defined(RTC_AUTOMATIC_DST)
+    if (automatic_dst && isDST(offsetTime)) { offsetTime += 3600; }


Please, align the #if at the start of the line. Don't put the code inside the if in the same line. We use:

if (condition) { do_foo(); }

(this applies to more places in the diff, but no reason to repeat the same comment in all of them)

fiam · 2018-05-18T14:52:18Z

src/main/common/time.c

@@ -120,14 +121,84 @@ static bool rtcIsDateTimeValid(dateTime_t *dateTime)
           (dateTime->millis <= 999);
 }

-static void dateTimeWithOffset(dateTime_t *dateTimeOffset, dateTime_t *dateTimeInitial, int16_t minutes)
+#if defined(RTC_AUTOMATIC_DST)
+static int lastSundayOfMonth(int currentYear, int wantedMonth) {


Indentation and formatting need some work in all this function. Opening brace for functions should be in the next line. Indent using 4 spaces and add a space before and after , as well as arithmetic operators.

fiam · 2018-05-18T15:06:11Z

src/main/common/time.c

+#if defined(RTC_AUTOMATIC_DST)
+static int lastSundayOfMonth(int currentYear, int wantedMonth) {
+    int days[] = {31,29,31,30,31,30,31,31,30,31,30,31};
+	int m, y = currentYear, w;


This line seems very hairy. m should be declared in the for, since it's initialised and used only there. y is assigned to w (where is that variable coming from?), but it looks like you could just use currentYear a few lines down without any extra variables.

fiam · 2018-05-18T15:06:24Z

src/main/common/time.c

+    days[1] -= (y % 4) || (!(y % 100) && (y % 400));
+    w = y * 365 + (y - 1) / 4 - (y - 1) / 100 + (y - 1) / 400 + 6;
+
+	for(m = 0; m < 12; m++) {


Space between for and (

nmaggioni · 2018-05-18T16:50:10Z

@fiam Everything should be okay now, did I miss anything? You should really publish an official styleguide in the developer docs, btw.

fiam · 2018-05-18T18:10:42Z

@nmaggioni It looks good to me. Thanks!

fiam · 2018-05-18T18:11:45Z

@nmaggioni I spoke too fast, it's not compiling. You need to handle TZ_AUTO_DST_OFF in the switch. Just add:

case TZ_AUTO_DST_OFF:
    break;

And it should be good to go

nmaggioni · 2018-05-18T18:23:44Z

Whoops, my bad... Forgot about that, it's now fixed.

Rebase on 'development' required.

digitalentity · 2018-05-19T23:54:07Z

@nmaggioni new features should be done on top of development branch. Please rebase. Other than that I don't see any reasons not to merge this.

nmaggioni · 2018-05-20T00:02:17Z

Sorry, I must have fudged up something while branching locally - it's now fixed!

MRC3742 · 2018-05-22T17:52:16Z

@nmaggioni I'm not sure this is working as intended? or has it not been fully implemented?
After compiling from the latest pull of the development branch, using cli this is showing a value of 0.
It also shows an allowed range of 0 - 0 ? shouldn't it be OFF - ON?
Unable to change from 0 in CLI
FC is SPF3EVO
Running firmware 2.0.0 released on: May 22 2018 13:31:11

Entering CLI Mode, type 'exit' to return, or 'help'

get tz

tz_offset = -240
Allowed range: -1440 - 1440

tz_automatic_dst = 0
Allowed range: 0 - 0

nmaggioni · 2018-05-22T18:18:10Z

@MRC3742 Can reproduce, I'm investigating.

Automatic DST support

eae76b9

digitalentity requested changes Apr 19, 2018

View reviewed changes

digitalentity added this to the 2.0 milestone Apr 19, 2018

Increase config template version for tz_automatic_dst

dd19387

CLI docs for tz_offset and tz_automatic_dst

37c3280

DST presets for EU and USA, new tz_dst_country config

d861ee8

fiam reviewed Apr 20, 2018

View reviewed changes

Flash usage enhancements

6ad6cc3

Includes `tz_dst_country` removal and docs update

nmaggioni force-pushed the tz_automatic_dst branch from a523616 to 6ad6cc3 Compare April 20, 2018 19:37

nmaggioni and others added 3 commits April 22, 2018 16:34

Merge branch 'master' into tz_automatic_dst

48b5d0a

Add config for probot.stale

3e23e62

Merge pull request iNavFlight#3209 from iNavFlight/de_stale_bot

de00fc5

Add probot.stale for automatically managing inactive issues

Merge branch 'master' into tz_automatic_dst

bd17c41

fiam requested changes May 18, 2018

View reviewed changes

Code style & fix for last sunday of month calculations

3a34ba8

fiam previously approved these changes May 18, 2018

View reviewed changes

Handle disabling DST

baa9b4d

digitalentity previously approved these changes May 19, 2018

View reviewed changes

digitalentity changed the base branch from master to development May 19, 2018 23:52

digitalentity added the Rebase Required label May 19, 2018

Merge branch 'development' into tz_automatic_dst

5078fe3

digitalentity removed the Rebase Required label May 20, 2018

digitalentity approved these changes May 20, 2018

View reviewed changes

digitalentity merged commit e7ca794 into iNavFlight:development May 20, 2018

nmaggioni deleted the tz_automatic_dst branch May 20, 2018 07:20

nmaggioni restored the tz_automatic_dst branch May 22, 2018 19:03

nmaggioni mentioned this pull request May 22, 2018

Fix tz_automatic_dst settings table #3262

Merged

		@@ -51,6 +52,7 @@ PG_REGISTER_WITH_RESET_TEMPLATE(timeConfig_t, timeConfig, PG_TIME_CONFIG, 0);

Automatic DST support #3072

Automatic DST support #3072

Conversation

nmaggioni commented Apr 19, 2018 • edited Loading

Calculations were inspired by 1 and 2.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalentity commented Apr 19, 2018

nmaggioni commented Apr 19, 2018 • edited Loading

fiam commented Apr 19, 2018

nmaggioni commented Apr 19, 2018

stronnag commented Apr 19, 2018

nmaggioni commented Apr 19, 2018

stronnag commented Apr 19, 2018

nmaggioni commented Apr 19, 2018

fiam commented Apr 19, 2018

stronnag commented Apr 19, 2018

nmaggioni commented Apr 19, 2018 • edited Loading

fiam commented Apr 20, 2018

fiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmaggioni commented Apr 20, 2018

nmaggioni commented May 18, 2018

fiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmaggioni commented May 18, 2018

fiam commented May 18, 2018

fiam commented May 18, 2018

nmaggioni commented May 18, 2018

digitalentity commented May 19, 2018

nmaggioni commented May 20, 2018

MRC3742 commented May 22, 2018

get tz

nmaggioni commented May 22, 2018

nmaggioni commented Apr 19, 2018 •

edited

Loading

nmaggioni commented Apr 19, 2018 •

edited

Loading

nmaggioni commented Apr 19, 2018 •

edited

Loading