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

Graceful cloud disconnect #1269

Closed
wants to merge 3 commits into from

Conversation

@avtolstoy
Copy link
Member

commented Mar 6, 2017

Firmware should terminate cloud connection in a graceful manner when possible to ensure that confirmable messages have been received by the server. Relevant issue: #1165

TODO: This PR could make use of #1245 (mainly the Particle.publish() change that makes it returns a Future) for on-device tests.

Review only. Do not merge. Already in #1289


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)

@avtolstoy avtolstoy added this to the 0.7.0 milestone Mar 6, 2017

@avtolstoy avtolstoy referenced this pull request Apr 23, 2017
5 of 6 tasks complete

@technobly technobly requested a review from sergeuz May 4, 2017

@sergeuz
Copy link
Member

left a comment

I'm approving this since it's a review-only PR. Still, I would like to see at least important requested changes made in the feature/photon/wiced-3.7.0-7 branch, which supersedes this PR.

{
int result = 1;

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

I would always use -1 (or ProtocolError::UNKNOWN in this particular case) as a generic error code to avoid confusion with boolean values

break;
case ProtocolCommands::DISCONNECT:
result = wait_confirmable();
ack_handlers.clear();
break;
case ProtocolCommands::WAKE:
wake();
break;

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

result = 0 is missing here

return this->wait_confirmable();
}

return 0;

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

This probably should return an error in case of an unsupported command

@@ -1411,10 +1423,8 @@ bool SparkProtocol::handle_message(msg& message, token_t token, CoAPMessageType:
case CoAPMessageType::DESCRIBE:
{
int desc_flags = DESCRIBE_ALL;
if (message.len > 8 && queue[8] <= DESCRIBE_ALL) {
if (message_padding_strip(queue, message.len) > 8 && queue[8] <= DESCRIBE_ALL) {

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

I don't quite follow this. Could you add a comment describing what message_padding_strip() does in terms of the CoAP message format and/or encryption padding scheme we use? If it fixes some bug, should a similar check in the newer protocol implementation be updated as well?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 27, 2017

Author Member

I should have probably added a comment that this strips PKCS#7 padding. This is correctly handled in newer protocol implementation, see https://github.com/spark/firmware/blob/develop/communication/src/lightssl_message_channel.cpp#L76

It actually does fix an issue with #1226 not working correctly as message.len is always > 8 due to the padding.

} else {
spark_protocol_command(sp, ProtocolCommands::DISCONNECT, 0, nullptr);
}
}

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

It looks like application ACK handlers don't get cleared in case of a non-graceful disconnect. Should we add a separate command which would tear down a protocol's session without waiting for unconfirmed messages?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 27, 2017

Author Member

👍

@@ -387,7 +386,7 @@ class ManagedNetworkInterface : public NetworkInterface
WLAN_CONNECTED = 0;
WLAN_DHCP = 0;

cloud_disconnect();
cloud_disconnect_graceful();

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 18, 2017

Member

I don't think the socket should be closed gracefully here. If an application (or the system) attempts to turn off the network without disconnecting from the cloud explicitly (either gracefully or not) then it most likely wants the network to be turned off immediately, without waiting for any unconfirmed messages.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 27, 2017

Author Member

I don't remember exactly what we decided on when discussing futures and graceful disconnection, but if I remember correctly the idea was that WiFi.disconnect() and Cellular.disconnect() calls should close the connection sanely and disconnect from WiFi or cellular network, compared to WiFi.off() and Cellular.off() which would terminate the connection and turn off wireless module 'immediately'.

What would be the cons of closing cloud connection normally before disconnecting from wireless network?

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 27, 2017

Member

Yeah, I meant WiFi.off() and Cellular.off() specifically, which I think should turn off the networking immediately (at least by default). The problem with current code is that ManagedNetworkInterface::off() calls ManagedNetworkInterface::disconnect() first and thus always waits for unconfirmed messages.

@technobly

This comment has been minimized.

Copy link
Member

commented Jul 12, 2017

Closing due to changes being in released/v0.7.0-rc.1

@technobly technobly closed this Jul 12, 2017

@technobly technobly deleted the feature/graceful_disconnect branch Jul 12, 2017

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.