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.

Sign up for GitHub

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

Jump to bottom

[Gen3] Fix LED behavior in case of network failure in Cloud Connecting state #2210

Merged
merged 1 commit into from
Oct 9, 2020
Merged

[Gen3] Fix LED behavior in case of network failure in Cloud Connecting state #2210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions system/src/system_task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,10 @@ void establish_cloud_connection()
// the last error diagnostic is used only for communication errors since we cannot mix
// HAL and communication error codes without specifying the category of an error
WARN("Cloud socket connection failed: %d", connect_result);
if (connect_result == SYSTEM_ERROR_NETWORK) {
LED_SIGNAL_STOP(CLOUD_HANDSHAKE);
LED_SIGNAL_STOP(CLOUD_CONNECTING);
}
Copy link
Contributor Author

@keeramis keeramis Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A couple of comments:

  • Choosing to change LED behavior only with SYSTEM_ERROR_NETWORK because the other errors don't necessarily lead to network failure. As per my understanding, network failure should be the one case that should get the LED back to green.

  • The LED behavior also seem to largely depend on these states here such as these :

    device-os/system/src/system_network_manager.cpp

    Line 402 in 70f4f9a

    LED_SIGNAL_START(NETWORK_CONNECTED, BACKGROUND);

    There is an edge case where the device enters "Cloud Connecting" state, and fails DNS test, but it has not yet gotten a deregistration URC, in which case it is in IP_CONFIGURED state so the above code makes it breathe green, which is rightfully the correct state because it is in IP_CONFIGURED state. Once it gets the deregistration URC, it blinks green as expected. Please see this log as well. Just highlighting an edge case for reference (and it looks OK from my understanding).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ch64095.txt

Copy link
Member

@sergeuz sergeuz Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC, this issue is specific to Gen 3 platforms. On Gen 2, the system gets notified that it's no longer connected to the network and proceeds to update the LED (as well as the system state) accordingly via a call to cloud_disconnect(): https://github.com/particle-iot/device-os/blob/develop/system/src/system_network_internal.h#L677-L680.

Perhaps, rather than providing a workaround for just one specific case, where creating the cloud socket or "connecting" it to the server fails (note that UDP sockets are connection-less), we should investigate why the system doesn't update the cloud connection state accordingly when it's no longer connected to the network? I think we have a bigger problem here rather than just LED indication, and it probably dates back to mesh times when we, for some reason, weren't able to determine reliably which network interface among the available ones is used for the cloud connection.

cc @avtolstoy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sergeuz

investigate why the system doesn't update the cloud connection state accordingly

Which state should the system update the cloud connectivity status to? Probably go back to "cloud disconnect" and restart the connection since we see a +CEREG: 2 URC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+CEREG: 2 cellular deregistration* URC.

Copy link
Member

@sergeuz sergeuz Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I discussed this PR with Andrey, and whereas it's possible that we're not cancelling the current ongoing connection attempt on network disconnection events in certain cases, trying to investigate and address that in the scope of this specific issue would be too much. The change introduced in this PR is perfectly fine: the establish_cloud_connection() function starts the LED indication and it should perform a cleanup and stop the indication if it wasn't able to establish a connection. I'm going to approve this PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okie thanks Sergey. Please let me know if we need to open a separate story for the broader correction.

SPARK_CLOUD_SOCKETED = 0;

diag->status(CloudDiagnostics::DISCONNECTED);
Expand Down