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

Wait for confirmable messages when entering the deep sleep mode #1767

Merged
merged 5 commits into from May 16, 2019

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented May 7, 2019

Problem

System.sleep(SLEEP_MODE_DEEP, ...) doesn't wait for confirmable messages to be processed before entering the deep sleep mode. If the current session data became invalid for some reason, this may put a sleepy device into an endless loop, where it wakes up, restores the session, publishes an event and goes back to sleep without ever noticing that there's no established connection with the cloud.

Solution

By default, the system should wait for all confirmable messages to be processed before entering a sleep mode. This PR also introduces a flag that allows overriding the default behavior (SLEEP_NO_WAIT).

Steps to Test

  1. Flash the following application to an Electron:
#include "application.h"

// SYSTEM_THREAD(ENABLED)

namespace {

const Serial1LogHandler logHandler(115200, LOG_LEVEL_WARN, {
    { "app", LOG_LEVEL_ALL }
});

} // namespace

void setup() {
    Log.info("setup()");
}

void loop() {
    Log.info("Particle.publish()");
    auto f = Particle.publish("my_event", PRIVATE | WITH_ACK);
    f.onSuccess([](bool) {
        Log.info("ACK received");
    }).onError([](Error) {
        Log.warn("Particle.publish() failed");
    });
    Log.info("System.sleep()");
    System.sleep(SLEEP_MODE_DEEP, 5);
}

Verify that the application gets notified about an ACK received for the published message before the device enters the deep sleep mode.

2. Apply the following patch via git apply:

diff --git a/communication/src/dtls_message_channel.cpp b/communication/src/dtls_message_channel.cpp
index dec29fb9d..be096f4c2 100644
--- a/communication/src/dtls_message_channel.cpp
+++ b/communication/src/dtls_message_channel.cpp
@@ -112,6 +112,7 @@ auto SessionPersist::restore(mbedtls_ssl_context* context, bool renegotiate, uin
 		context->state = MBEDTLS_SSL_HANDSHAKE_WRAPUP;
 		context->in_epoch = in_epoch;
 		memcpy(context->out_ctr, &out_ctr, sizeof(out_ctr));
+		context->out_ctr[0] = 'X'; // Invalidate the session data
 		memcpy(context->handshake->randbytes, randbytes, sizeof(randbytes));
 
 		context->transform_negotiate->ciphersuite_info = mbedtls_ssl_ciphersuite_from_id(ciphersuite);

Verify that the device running the same application performs the full handshake on every second boot. When the system resumes the session using invalid data, it should take no longer than 1 minute for the communication layer to fail with a timeout error while waiting for ACKs and reset the session.

Edit: A better solution for the second test case is implemented in #1776.

References

  • [ch32341]

  • [bugfix] Wait for confirmable messages when entering the deep sleep mode #1767

@sergeuz sergeuz added the bug label May 7, 2019
@sergeuz sergeuz requested a review from m-mcgowan May 7, 2019 20:42
@sergeuz sergeuz added this to the 1.2.0-rc.1 milestone May 15, 2019
@@ -65,6 +65,7 @@ test(system_sleep)
API_COMPILE({ SleepResult r = System.sleep(SLEEP_MODE_DEEP, SLEEP_NETWORK_STANDBY, 60); (void)r; });

API_COMPILE({ SleepResult r = System.sleep(SLEEP_MODE_DEEP, SLEEP_NETWORK_STANDBY); (void)r; });
API_COMPILE({ SleepResult r = System.sleep(SLEEP_MODE_DEEP, SLEEP_NETWORK_STANDBY | SLEEP_NO_WAIT); (void)r; });
Copy link
Contributor

Choose a reason for hiding this comment

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

👏


// Make sure all confirmable UDP messages are sent and acknowledged before sleeping
if (spark_cloud_flag_connected() && !(param & SYSTEM_SLEEP_FLAG_NO_WAIT)) {
Spark_Sleep();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a blocking call. Do we want to keep it like that?

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 now I'd say yes, because System.sleep() is a blocking call as well. Also, we're already waiting for confirmable messages synchronously when entering the stop mode:

Spark_Sleep();

@m-mcgowan m-mcgowan added the ready to merge PR has been reviewed and tested label May 16, 2019
@sergeuz sergeuz force-pushed the fix/wait_ack_before_sleeping branch 2 times, most recently from 62a11fb to fdd671f Compare May 16, 2019 16:19
@sergeuz sergeuz force-pushed the fix/wait_ack_before_sleeping branch from fdd671f to e7312e5 Compare May 16, 2019 16:58
@sergeuz sergeuz merged commit b14e2d9 into develop May 16, 2019
@sergeuz sergeuz deleted the fix/wait_ack_before_sleeping branch May 16, 2019 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ready to merge PR has been reviewed and tested
Projects
None yet
2 participants