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

Automatic DST support #3072

Merged
merged 12 commits into from May 20, 2018

Conversation

Projects
None yet
5 participants
@nmaggioni
Copy link
Contributor

commented Apr 19, 2018

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.

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

This comment has been minimized.

Copy link
@digitalentity

digitalentity Apr 19, 2018

Member

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

This comment has been minimized.

Copy link
@nmaggioni

nmaggioni Apr 19, 2018

Author Contributor

Rookie mistake, my bad.

This comment has been minimized.

Copy link
@digitalentity

digitalentity Apr 19, 2018

Member

No worries, I keep forgetting to do this myself 😉

This comment has been minimized.

Copy link
@nmaggioni

nmaggioni Apr 20, 2018

Author Contributor

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 digitalentity added this to the 2.0 milestone Apr 19, 2018

@digitalentity

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

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

@fiam

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

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 nth 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

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

@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
Copy link
Member

left a comment

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.

typedef enum {
DST_EU,
DST_USA,
} tz_dst_country_e;

This comment has been minimized.

Copy link
@fiam

fiam Apr 20, 2018

Member

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.

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;

This comment has been minimized.

Copy link
@fiam

fiam Apr 20, 2018

Member

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.

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

This comment has been minimized.

Copy link
@fiam

fiam Apr 20, 2018

Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

@fiam Done! Thanks for the very helpful pointers.

Flash usage enhancements
Includes `tz_dst_country` removal and docs update

@nmaggioni nmaggioni force-pushed the nmaggioni:tz_automatic_dst branch from a523616 to 6ad6cc3 Apr 20, 2018

nmaggioni and others added some commits Apr 22, 2018

Merge pull request #3209 from iNavFlight/de_stale_bot
Add probot.stale for automatically managing inactive issues
@nmaggioni

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

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

@fiam
Copy link
Member

left a comment

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

{
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; }

This comment has been minimized.

Copy link
@fiam

fiam May 18, 2018

Member

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)

@@ -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) {

This comment has been minimized.

Copy link
@fiam

fiam May 18, 2018

Member

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.

#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 comment has been minimized.

Copy link
@fiam

fiam May 18, 2018

Member

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.

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++) {

This comment has been minimized.

Copy link
@fiam

fiam May 18, 2018

Member

Space between for and (

@nmaggioni

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

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

@fiam

This comment has been minimized.

Copy link
Member

commented May 18, 2018

@nmaggioni It looks good to me. Thanks!

@fiam

This comment has been minimized.

Copy link
Member

commented May 18, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2018

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

@digitalentity digitalentity changed the base branch from master to development May 19, 2018

@digitalentity digitalentity dismissed stale reviews from fiam and themself May 19, 2018

Rebase on 'development' required.

@digitalentity

This comment has been minimized.

Copy link
Member

commented May 19, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2018

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@MRC3742

This comment has been minimized.

Copy link

commented May 22, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

@MRC3742 Can reproduce, I'm investigating.

@nmaggioni nmaggioni restored the nmaggioni:tz_automatic_dst branch May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.