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

Particle.connected() should return true only after handshake messages are acknowledged #1825

Merged
merged 9 commits into from Aug 3, 2019

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented Jun 14, 2019

Problem

Particle.connected() returns true immediately after the session is resumed, even if the session data became invalid for some reason and the device will in fact never receive ACKs for its handshake messages until the next connection attempt.

Solution

  1. Wait for all system- and protocol-level handshake messages to be acknowledged before changing the cloud connection state.
  2. Make Particle.publish() and other cloud functions in the Wiring API fail early if the device is not connected to the cloud.

Steps to Test

  1. Run the communication unit tests.
  2. Flash the example app and verify via the logs that Particle.connected() changes its return value only after all expected ACKs are received from the server.

Example App

#include "application.h"

SYSTEM_MODE(MANUAL)

namespace {

const SerialLogHandler logHandler(LOG_LEVEL_INFO);

void cloudStatusChanged(system_event_t event, int param) {
    if (param == cloud_status_connected) {
        Log.info("cloud_status_connected");
    }
}

bool lastConnected = true;

} // namespace

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

void loop() {
    const bool connected = Particle.connected();
    if (connected != lastConnected) {
        Log.info("Particle.connected(): %d", (int)connected);
        lastConnected = connected;
    }
    Particle.process();
}

References

  • [ch32340]

@sergeuz sergeuz requested a review from m-mcgowan June 14, 2019 10:28
@sergeuz sergeuz added this to the 1.3.1-rc.1 milestone Jun 14, 2019
@avtolstoy avtolstoy added ready to merge PR has been reviewed and tested and removed needs review labels Jul 1, 2019
@sergeuz
Copy link
Member Author

sergeuz commented Jul 29, 2019

@m-mcgowan I'd like to merge this PR, could you please take a look at it?

Copy link
Contributor

@m-mcgowan m-mcgowan left a comment

Choose a reason for hiding this comment

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

This is quite a significant change, I'm approving it since the code looks good from inspection and it's blocking other work, but I'd like to draw up a more comprehensive test plan prior to release.

@@ -56,8 +56,11 @@ static uint32_t start_ymodem_flasher_serial_speed = START_YMODEM_FLASHER_SERIAL_

ymodem_serial_flash_update_handler Ymodem_Serial_Flash_Update_Handler = NULL;

// TODO: Use a single state variable instead of SPARK_CLOUD_XXX flags
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@sergeuz
Copy link
Member Author

sergeuz commented Aug 1, 2019

I'd like to draw up a more comprehensive test plan prior to release.

What specifically do you think needs to be tested in addition to what is already covered by the unit tests and test app in this PR?

@sergeuz sergeuz merged commit 8bf8dcf into develop Aug 3, 2019
@sergeuz sergeuz deleted the fix/particle_connected branch August 3, 2019 15:50
@m-mcgowan
Copy link
Contributor

I'd like to draw up a more comprehensive test plan prior to release.

What specifically do you think needs to be tested in addition to what is already covered by the unit tests and test app in this PR?

I'd like to test the edge cases with end to end tests to be sure the integration works as expected. E.g. what happens when none of the system messages are delivered? or just some of them? I know we expect a timeout, so we should have tests to validate that.

Are we certain that a race condition doesn't occur where the response is received so quickly that we empty the message queue and consider the handshake complete when it's not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready to merge PR has been reviewed and tested
Projects
None yet
4 participants