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

Notify the cloud about keepalive interval changes #1908

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented Sep 6, 2019

Problem

User applications can change the keepalive interval of the cloud connection via the Particle.keepAlive() method. The interval can also be changed by the system depending on the cellular provider and/or the type of the external connectivity used by the gateway device in a mesh network.

At present, the cloud has no means to determine the keepalive interval set on a particular device, and this affects the ability of the cloud to track the online status of the device accurately.

Solution

Publish a special private event (particle/device/keepalive) containing the actual value of the keepalive interval every time the interval changes.

Steps to Test

Flash the example application to a device. Subscribe to the device's events and verify that keepalive events get published when the keepalive interval is changed.

Note: The cloud-side handler for the keepalive events is not yet implemented, so this test requires running a local instance of the device service, which has particle/device/keepalive added to the list of known internal events.

Example App

#include "application.h"

SYSTEM_MODE(MANUAL);

namespace {

int cloudStatus = cloud_status_disconnected;
bool done = false;

void cloudStatusChanged(system_event_t event, int param) {
    cloudStatus = param;
}

void waitCloudStatus(int status) {
    while (cloudStatus != status) {
        Particle.process();
    }
}

} // unnamed

void setup() {
    System.on(cloud_status, cloudStatusChanged);
}

void loop() {
    if (done) {
        return;
    }

    // The default keepalive interval should be published after the protocol handshake
    Particle.connect();
    waitCloudStatus(cloud_status_connected);
    delay(1000);

    // This should generate a new keepalive event
    Particle.keepAlive(10);

    // This should not generate a new event, since the interval has not changed
    Particle.keepAlive(10);

    // Disconnect from the cloud
    delay(1000);
    Particle.disconnect();
    waitCloudStatus(cloud_status_disconnected);

    // Set the interval before establishing a connection to the cloud. The interval should be
    // published after the protocol handshake
    Particle.keepAlive(20);
    Particle.connect();
    waitCloudStatus(cloud_status_connected);

    // Disconnect from the cloud
    delay(1000);
    Particle.disconnect();
    waitCloudStatus(cloud_status_disconnected);

    // Set the interval while the handshake is in progress. Two events should be published during
    // the handshake. Note: At present, the events may be delivered in the wrong order
    Particle.connect();
    waitCloudStatus(cloud_status_connecting);
    // Run one more iteration of the system loop to let the system start the handshake
    Particle.process();
    Particle.keepAlive(30);

    // Disconnect from the cloud
    delay(1000);
    Particle.disconnect();
    waitCloudStatus(cloud_status_disconnected);

    done = true;
}

@@ -358,6 +359,7 @@ constexpr const char KEY_RESTORE_EVENT[] = "spark/device/key/restore";
constexpr const char DEVICE_UPDATES_EVENT[] = "particle/device/updates/";
constexpr const char FORCED_EVENT[] = "forced";
constexpr const char UPDATES_PENDING_EVENT[] = "pending";
constexpr const char KEEPALIVE_INTERVAL_EVENT[] = "particle/device/keepalive";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device service presently already has the [spark/particle]/session/timeout event, which was made before we had UDP, so the name is now somewhat confusing, but it serves to feed the IdleChecker about how long to consider the device offline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks! I also noticed that the device service expects the interval to be in seconds, while the current code sends it in milliseconds. Are we sure it's safe if Device OS will start publishing session/timeout events? cc @JamesHagerman

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-mcgowan @JamesHagerman Pinging here again. Should I change the event name to particle/session/timeout?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For visibility: we discussed this internally and decided to use the new event name for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-mcgowan Does the rest of the PR look good to you?

@sergeuz sergeuz requested a review from m-mcgowan October 2, 2019 19:08
@@ -34,11 +42,30 @@ class Pinger
* USER SYS NO
* USER USER YES
*/
// TODO: It feels that this logic should have been implemented in the system layer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. The comms layer just provides the timeout and the system layer manages the multiple stakeholders that may choose to change it. Similarly to how we manage the LED.

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

Successfully merging this pull request may close these issues.

2 participants