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 planned disconnections #1899

Merged
merged 46 commits into from
Apr 16, 2020
Merged

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented Sep 3, 2019

Problem

This PR implements a mechanism for graceful disconnection from the cloud on the following events:

  • System reset (as part of the firmware update process or via System.reset(), System.dfu() and other APIs).
  • Disconnecting via Particle.disconnect().
  • Shutting down network interfaces.
  • Entering the listening mode.
  • Entering one of the sleep modes.

Upon any of the above events, the device first sends a Goodbye message to the cloud, waits for the message to be received by the cloud, and then proceeds to close the connection with the cloud. On UDP platforms the message is sent as a confirmable message and needs to be acknowledged by the server. On TCP platforms the delivery of the message is ensured by half-closing the socket and waiting for the server to close the connection.

The Goodbye message carries the information about the disconnection reason, system reset reason and duration of the sleep (if applicable). The format of the data is described here.

Important: This PR introduces the following changes in the current Device OS behavior:

  • Device OS now completely disconnects from the cloud when going into sleep. Previously, it was leaving the comms layer and cloud socket in whatever state they were prior to entering the sleep mode.
  • By default, System.reset() and other APIs no longer reset the device immediately if it's connected to the cloud.

This PR also refactors buffer appender classes (BufferAppender, BufferAppender2), so that there's only one BufferAppender class.

Steps to Test

  • Run unit tests.
  • Run the app/cloud_disconnect test app.

References

  • [ch34913]

@sergeuz sergeuz changed the title Notify the cloud about the planned disconnection Notify the cloud about planned disconnections Sep 3, 2019
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.

All good changes! There are a few areas we need to complete:

  1. documentation: the behaviour changes to cloud disconnect and system reset should be documented so that our users are aware of these changes.

  2. opt-out: if the developer doesn't want to be sending disconnection reasons to the cloud, perhaps this can be opted out, e.g. via a system flag or feature.

communication/inc/spark_protocol_functions.h Outdated Show resolved Hide resolved
communication/src/lightssl_protocol.cpp Show resolved Hide resolved
system/src/main.cpp Outdated Show resolved Hide resolved
system/src/system_mode.cpp Show resolved Hide resolved
system/src/system_network_manager_api.cpp Outdated Show resolved Hide resolved
test/unit_tests/services/varint.cpp Show resolved Hide resolved
@sergeuz
Copy link
Member Author

sergeuz commented Sep 18, 2019

opt-out: if the developer doesn't want to be sending disconnection reasons to the cloud, perhaps this can be opted out, e.g. via a system flag or feature.

How about we instead add a feature flags that disables graceful disconnections altogether? I don't see much sense in disabling Goodbye messages alone, but still waiting for other unacknowledged messages when disconnecting from the cloud

@sergeuz
Copy link
Member Author

sergeuz commented Oct 1, 2019

I noticed that graceful cloud disconnection was implemented in 3 different places, 09ecec3c363555c9d57856bab4c734b4c1e9bf76 and 32b06be653f6b7dce0c0e9a76d631ce00c4fad94 address that

@sergeuz
Copy link
Member Author

sergeuz commented Oct 2, 2019

opt-out: if the developer doesn't want to be sending disconnection reasons to the cloud, perhaps this can be opted out, e.g. via a system flag or feature

@m-mcgowan It is now possible to disable graceful cloud disconnection either globally (via the SYSTEM_FLAG_DISCONNECT_IMMEDIATELY flag), or by passing the RESET_NO_WAIT flag to System.reset() and other methods that reset the device.

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.

opt-out: if the developer doesn't want to be sending disconnection reasons to the cloud, perhaps this can be opted out, e.g. via a system flag or feature.

How about we instead add a feature flags that disables graceful disconnections altogether? I don't see much sense in disabling Goodbye messages alone, but still waiting for other unacknowledged messages when disconnecting from the cloud

I was thinking to save data. But we can add it later if needed.

communication/inc/spark_protocol_functions.h Show resolved Hide resolved
@m-mcgowan m-mcgowan self-requested a review October 22, 2019 20:50
@sergeuz
Copy link
Member Author

sergeuz commented Oct 24, 2019

I was thinking to save data. But we can add it later if needed.

@m-mcgowan hm, yeah, that would make sense. I'm not entirely happy about the SYSTEM_FLAG_DISCONNECT_IMMEDIATELY flag which I introduced in this PR. It feels that it acts too broadly and may actually cause problems on sleepy devices. Having something like Particle.abort() would be a much more flexible option.

How about I ditch that flag and introduce another one which would disable goodbye messages specifically?

@m-mcgowan
Copy link
Contributor

How about I ditch that flag and introduce another one which would disable goodbye messages specifically?

I guess my main concern is that adding the goobye message adds two key behaviour changes:

  1. disconnects take longer since they wait for the message to be ACKed
  2. disconnects require more data

When introducing features, it's not always clear how these changes impact customers.

On the basis of this, I'm suggesting adding two flags: one to disable the goodbye message entirely and another to disable waiting for it (so it's sent as a non-confirmable CoAP message.) The state of these flags will be communicated to the cloud so that it knows the device is intentionally not saying goodbye when leaving. Irish Goodbye :-)

@sergeuz
Copy link
Member Author

sergeuz commented Nov 7, 2019

I'm suggesting adding two flags: one to disable the goodbye message entirely and another to disable waiting for it (so it's sent as a non-confirmable CoAP message.)

It can be my dislike of overly configurable interfaces, but I'd keep it simple. If the overhead of this feature is not desirable, the users are free to disable it. Allowing it to be enabled but work unreliably feels like we're inventing extra work for ourselves.

The state of these flags will be communicated to the cloud so that it knows the device is intentionally not saying goodbye when leaving. Irish Goodbye :-)

That's a good point. I'll add a relevant flag to the Hello message 👍

@sergeuz sergeuz force-pushed the feature/close_msg branch 6 times, most recently from 959d9f6 to 0f73159 Compare November 13, 2019 23:25
@sergeuz
Copy link
Member Author

sergeuz commented Nov 13, 2019

@m-mcgowan I added a system flag that enables/disables sending of Goodbye messages, as well as a Hello message flag that tells the cloud whether it should expect a Goodbye message from the device. Below is the summary of the changes since your last review:

  • Protocol flags are now stored persistently in the session data. This allows the protocol implementation to detect changes in its configuration and trigger a new Hello message when the session is resumed.
  • App state is no longer calculated as a CRC32 of other CRC32 checksums, instead, all fields comprising the app state are now compared directly. This reduces the chance of a collision.
  • SYSTEM_FLAG_DISCONNECT_IMMEDIATELY was replaced with the SYSTEM_FLAG_SEND_GOODBYE flag (set to 1 by default).
  • Fixed a bug with truncated describe messages introduced in this PR.

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.

Looks good. 👍 Just some comments around clarification.

communication/src/protocol.cpp Outdated Show resolved Hide resolved
communication/inc/protocol_defs.h Outdated Show resolved Hide resolved
system/src/system_cloud_internal.cpp Outdated Show resolved Hide resolved
services/inc/appender.h Show resolved Hide resolved
@sergeuz sergeuz force-pushed the feature/close_msg branch 3 times, most recently from d58db7e to e74e4ad Compare March 18, 2020 00:07
wiring/inc/spark_wiring_cloud.h Show resolved Hide resolved
@avtolstoy avtolstoy added ready to merge PR has been reviewed and tested and removed needs review labels Apr 16, 2020
@sergeuz sergeuz merged commit 86c0259 into develop Apr 16, 2020
@sergeuz sergeuz deleted the feature/close_msg branch April 16, 2020 14:42
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ready to merge PR has been reviewed and tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants