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

[keeramis]

Problem

When cloud connecting, the RGB LED doesn't go back to (blinking) green when de-registered from network, or when having DNS test failures

Solution

The cloud signaling procedure is correct, however, the LED doesn't reflect the same. The LED stays in blinking cyan, although the system state has backed off from "Cloud Connecting" state.

Steps to Test

• Run any app that allows connecting to cloud, with serial logging enabled helpfully
• As soon as "Cloud connecting" is seen (device must have started blinking cyan), disconnect the module from network (probably by removing antenna). This needs to be done after "Cloud: Connecting" and before "Cloud socket connected" states
• The device struggles at that point to connect, and likely gets a CEREG: x deregistered URC
• The device should be blinking green (instead of blinking cyan)

Example App

void setup() {
}

void loop() {
}

References

ch64095

---

Completeness

• User is totes amazing for contributing!
• Contributor has signed CLA (Info here)
• Problem and Solution clearly stated
• Run unit/integration/application tests on device
• Added documentation
• Added to CHANGELOG.md after merging (add links to docs and issues)

[keeramis]

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).

[keeramis]

ch64095.txt

[sergeuz]

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

[keeramis]

@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?

[keeramis]

+CEREG: 2 cellular deregistration* URC.

[sergeuz]

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 👍

[keeramis]

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