Graceful cloud disconnect #1269
Conversation
| { | ||
| int result = 1; |
There was a problem hiding this comment.
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; |
| return this->wait_confirmable(); | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
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) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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?
| @@ -387,7 +386,7 @@ class ManagedNetworkInterface : public NetworkInterface | |||
| WLAN_CONNECTED = 0; | |||
| WLAN_DHCP = 0; | |||
|
|
|||
| cloud_disconnect(); | |||
| cloud_disconnect_graceful(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Closing due to changes being in |
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 aFuture) for on-device tests.Review only. Do not merge. Already in #1289
Doneness: