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

Feature/time isvalid #1135

Merged
merged 8 commits into from Nov 28, 2016

Conversation

@avtolstoy
Copy link
Member

commented Oct 6, 2016

Fixes issues #1116 and #965

Wiring functions added:

  • Time.isValid()
  • Particle.syncTimePending()
  • Particle.syncTimeDone()
  • Particle.timeSyncedLast()

This PR also fixes an issue with how RTC on Photon and Electron is configured. The year may only be double digit (00 - 99).

Tests added to wiring/no_fixture:

  • TIME_13_syncTimePending_syncTimeDone_when_disconnected
  • TIME_14_timeSyncedLast_works_correctly
  • TIME_15_SyncTimeInAutomaticMode
  • TIME_16_SyncTimeInManualMode
  • TIME_17_RestoreSystemMode
  • TIME_18_TimeChangedEvent

New functions documentation copied from firmware.md :

isValid()

Used to check if current time is valid. This function will return true if:

  • Time has been set manually using Time.setTime()
  • Time has been successfully synchronized with the Particle Cloud. The device synchronizes time with the Particle Cloud during the handshake. The application may also manually synchronize time with Particle Cloud using Particle.syncTime()
  • Correct time has been maintained by RTC.{{#unless core}} See information on Backup RAM (SRAM) for cases when RTC retains the time. RTC is part of the backup domain and retains its counters under the same conditions as Backup RAM.{{/unless}}

NOTE: When {{device}} is running in AUTOMATIC mode {{#unless core}}and threading is disabled {{/unless}} this function will block if current time is not valid and there is an active connection to Particle Cloud. Once {{device}} synchronizes the time with Particle Cloud or the connection to Particle Cloud is lost, Time.isValid() will return its current state. This function is also implicitly called by any Time function that returns current time or date (e.g. Time.hour()/Time.now()/etc).

Particle.syncTimeDone()

Returns true if there is no syncTime() request currently pending or there is no active connection to Particle Cloud. Returns false when there is a pending syncTime() request.

Particle.syncTimePending()

Returns true if there a syncTime() request currently pending. Returns false when there is no syncTime() request pending or there is no active connection to Particle Cloud.

Particle.timeSyncedLast()

Used to check when time was last synchronized with Particle Cloud.

// SYNTAX
Particle.timeSyncedLast();
Particle.timeSyncedLast(timestamp);

Returns the number of milliseconds since the device began running the current program when last time synchronization with Particle Cloud was performed.

This function takes one optional argument:

  • timestamp: time_t variable that will contain a UNIX timestamp received from Particle Cloud during last time synchronization
// EXAMPLE
#define ONE_DAY_MILLIS (24 * 60 * 60 * 1000)

void loop() {
  time_t lastSyncTimestamp;
  unsigned long lastSync = Particle.timeSyncedLast(lastSyncTimestamp);
  if (millis() - lastSync > ONE_DAY_MILLIS) {
    unsigned long cur = millis();
    Serial.printlnf("Time was last synchronized %lu milliseconds ago", millis() - lastSync);
    if (lastSyncTimestamp > 0)
    {
      Serial.print("Time received from Particle Cloud was: ");
      Serial.println(Time.timeStr(lastSyncTimestamp));
    }
    // Request time synchronization from Particle Cloud
    Particle.syncTime();
    // Wait until {{device}} receives time from Particle Cloud (or connection to Particle Cloud is lost)
    waitUntil(Particle.syncTimeDone);
    // Check if synchronized successfully
    if (Particle.timeSyncedLast() >= cur)
    {
      // Print current time
      Serial.println(Time.timeStr());
    }
  }
}

EDIT:

  • time_changed event added
  • When resuming session on Electron if for some reason time was not synchronized during handshake, send time request again

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)

Features

  • New Time API's! Time.isValid() | Particle.syncTimePending() | Particle.syncTimeDone() | Particle.timeSyncedLast() [Fixes #1116] [Fixes #965]

@avtolstoy avtolstoy added this to the 0.7.x milestone Oct 6, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2016

Very thoroughly documented!

Some thoughts:

  • Is there a system event for time changes? That way, application code can be actively notified when the time changes during a sync.
  • isValid() is blocking - what's the rationale for that? I would have expected it not to block since it's only a sniff of the current status. I would have imagined getTime() to block since then we are trying to get the active time, which might be in flux. We might also add an overload that disables any blocking and just returns the current time as it is.
  • What's the distinguishing factor between syncTimeDone and syncTimePending? As far as I can tell, they are inverses of each other. That's also fine, sincewaitUntil doesn't have an easy way to invert predicates, but a description of the expected use cases would be helpful.
  • On the electron, it's possible that Particle.syncTime() never syncs if all server CoAP responses are lost. The system should try to do what it can to ensure the sync will eventually happen. This is possibly outside the scope of this PR, and something to address elsewhere.
@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2016

@m-mcgowan
Thanks for comments!

Is there a system event for time changes? That way, application code can be actively notified when the time changes during a sync.

I like the idea of the system event for time changes. Should an event be fired if time is set manually using Time.setTime()?

isValid() is blocking - what's the rationale for that? I would have expected it not to block since it's only a sniff of the current status. I would have imagined getTime() to block since then we are trying to get the active time, which might be in flux. We might also add an overload that disables any blocking and just returns the current time as it is.

Time.isValid() is only blocking in AUTOMATIC mode with threading disabled. The rationale was that it's internally used by Time.now() (and as a consequence by all the other functions like Time.hour() etc) and in AUTOMATIC mode we want to ensure that when Time functions are accessed, the time is already valid. It also only blocks if there is a pending syncTime() request and current time is not valid.

What's the distinguishing factor between syncTimeDone and syncTimePending? As far as I can tell, they are inverses of each other. That's also fine, sincewaitUntil doesn't have an easy way to invert predicates, but a description of the expected use cases would be helpful.

Exactly as you say, they are inverses of each other to make it easier to use them with waitFor()/waitUntil(). The documentation lists an example usage for both of those functions.

On the electron, it's possible that Particle.syncTime() never syncs if all server CoAP responses are lost. The system should try to do what it can to ensure the sync will eventually happen. This is possibly outside the scope of this PR, and something to address elsewhere.

According to how the reliable channel is implemented (regarding confirmable requests), when several retransmissions occur and there is no reply the cloud connection is reset. I think the only issue is that when Electron tries to connect again and resumes the session instead of performing a handshake it will not issue a new syncTime() request. Perhaps we should add a check there that if time is not valid and Particle.timeLastSynced() reports 0, a time synchronization request should be sent again? If the application is manually calling Particle.syncTime() and wants to ensure that the time request succeeded, Particle.timeLastSynced() can be used to check for that. There is an example in the docs.

@avtolstoy avtolstoy force-pushed the feature/time-isvalid branch from 0589f93 to 66201fb Oct 22, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

I like the idea of the system event for time changes. Should an event be fired if time is set manually using Time.setTime()

Yes, I think so. The important thing is that time as changed. We might also include a reason for change as part of the event so the application can distinguish external changes from internal updates via Time.setTime().

I think the only issue is that when Electron tries to connect again and resumes the session instead of performing a handshake it will not issue a new syncTime() request. Perhaps we should add a check there that if time is not valid and Particle.timeLastSynced() reports 0, a time synchronization request should be sent again?

I like the idea of synching the time if it's not valid once the cloud is connected.

@avtolstoy avtolstoy force-pushed the feature/time-isvalid branch from 66201fb to bd5fe2d Oct 30, 2016

@technobly technobly self-assigned this Nov 21, 2016

technobly added 2 commits Nov 22, 2016
Merge branch 'develop' into feature/time-isvalid
 Conflicts:
	communication/src/protocol.cpp
	communication/src/protocol.h
	communication/src/spark_protocol_functions.cpp

system_tick_t spark_sync_time_last(time_t* tm, void* reserved)
{
SYSTEM_THREAD_CONTEXT_SYNC(spark_sync_time_pending(reserved));

This comment has been minimized.

Copy link
@technobly

technobly Nov 23, 2016

Member

Should this be SYSTEM_THREAD_CONTEXT_SYNC(spark_sync_time_last(time_t* tm, void* reserved));

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 23, 2016

Author Member

It should! We should also update #1146 after merging, so that this would have been caught.

(void)reserved;
return protocol->time_last_synced(tm);
}

This comment has been minimized.

Copy link
@technobly

technobly Nov 23, 2016

Member

Should this be SparkProtocol instead of ProtocolFacade here?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 23, 2016

Author Member

Good catch. The two functions above should also be changed:

int spark_protocol_set_connection_property(ProtocolFacade* protocol, unsigned property_id,
                                           unsigned data, void* datap, void* reserved)
{
int spark_protocol_command(ProtocolFacade* protocol, ProtocolCommands::Enum cmd, uint32_t data, void* reserved)

@technobly technobly merged commit 6319fbb into develop Nov 28, 2016

2 checks passed

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

@technobly technobly deleted the feature/time-isvalid branch Nov 28, 2016

@technobly technobly removed their assignment Nov 29, 2016

@technobly technobly modified the milestones: 0.7.x, 0.6.1 Nov 29, 2016

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