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

Gen2: network connection is established unexpectedly. #2309

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

XuGuohui
Copy link
Member

@XuGuohui XuGuohui commented Apr 22, 2021

Problem

  1. If the modem is just turned on by Network.on() and there is something wrong processing the modem events to cause a WLAN_WD to fire or somehow the SPARK_WLAN_RESET is set to reset the modem, device will try connecting to the network after the modem is reset.
  2. If the modem is just turned on by Network.on() and system sleep API is called to make device enter sleep mode, after the device wakes up it will try connecting to the network. (Gen2 with all sleep API and Gen3 with sleep 1.0 API)

Solution

Introduce a new flag to indicate if network connection should be restored after powering cycle the modem. The flag is set

  1. before the modem is reset if networking in connecting or ready:
--- a/system/src/system_network_compat.cpp
+++ b/system/src/system_network_compat.cpp
@@ -248,6 +249,7 @@ void manage_network_connection()
         {
             WARN("Resetting WLAN due to %s", (WLAN_WD_TO()) ? "WLAN_WD_TO()":((SPARK_WLAN_RESET) ? "SPARK_WLAN_RESET" : "SPARK_WLAN_SLEEP"));
             auto was_sleeping = SPARK_WLAN_SLEEP;
+            SPARK_WLAN_CONNECT_RESTORE = network_ready(0, 0, 0) || network_connecting(0, 0, 0);
             //auto was_disconnected = network.manual_disconnect();
  1. after device wakes up if network needs to be restored:
--- a/system/src/system_sleep.cpp
+++ b/system/src/system_sleep.cpp
@@ -90,7 +90,12 @@ int system_sleep_network_resume(network_interface_index index, network_status_t&
      * if single threaded, or this function is invoked synchronously by the system thread if system threading
      * is enabled. In both case, that would block the user application. Setting a flag here to unblock the user
      * application and restore the connection later. */
-    SPARK_WLAN_SLEEP = 0;
+    if (status.on) {
+        SPARK_WLAN_SLEEP = 0;
+    }
+    if (status.connected) {
+        SPARK_WLAN_CONNECT_RESTORE = 1;
+    }
 #else
     if (status.on) {

--- a/system/src/system_sleep_compat.cpp
+++ b/system/src/system_sleep_compat.cpp
 static void network_resume() {
-    // Set the system flags that triggers the wifi/cloud reconnection in the background loop
-    if (wakeupState.wifiConnected || wakeupState.wifi)  // at present, no way to get the background loop to only turn on wifi.
+    if (wakeupState.wifi) {
         SPARK_WLAN_SLEEP = 0;
-    if (wakeupState.cloud)
+    }
+    // Set the system flags that triggers the wifi/cloud reconnection in the background loop
+    if (wakeupState.wifiConnected) {
+        SPARK_WLAN_CONNECT_RESTORE = 1;
+    }
+    if (wakeupState.cloud) {
         spark_cloud_flag_connect();
+    }
 }

Finally the manage_network_connection() will handle the connection accordingly:

--- a/system/src/system_network_compat.cpp
+++ b/system/src/system_network_compat.cpp
     else
     {
-        if (!SPARK_WLAN_STARTED || (spark_cloud_flag_auto_connect() && !network_ready(0, 0, 0)))
-        {
-            // INFO("Network Connect: %s", (!SPARK_WLAN_STARTED) ? "!SPARK_WLAN_STARTED" : "SPARK_CLOUD_CONNECT && !network.ready()");
+        if (!SPARK_WLAN_STARTED) {
+            INFO("Network On: !SPARK_WLAN_STARTED");
+            network_on(0, 0, 0, 0);
+        }
+
+        if ((SPARK_WLAN_CONNECT_RESTORE || spark_cloud_flag_auto_connect()) && !network_ready(0, 0, 0)) {
+            INFO("Network Connect:%s%s", (SPARK_WLAN_CONNECT_RESTORE ? " SPARK_WLAN_CONNECT_RESTORE" : " "),
+                                         (spark_cloud_flag_auto_connect() ? " spark_cloud_flag_auto_connect()" : " "));
+            // XXX: If the auto-connect flag is not set, we used to only call network_connect() once here,
+            // even if it failed to connect to network for whatever reason. So we should clear the flag here.
+            SPARK_WLAN_CONNECT_RESTORE = 0;
             network_connect(0, 0, 0, 0);
         } else {

Steps to Test

  1. Use an electron and detach the Cellular antenna. Comment out the sleep function in the example. The device should persist to connect to network after the modem is reset due to WLAN_WD expired.
  2. Comment out the Network.connect() in the example. The device should not initiate connection after it wakes up.
  3. Run the example without commenting out anything. The device should initiate connection to network after it wakes up.

Example App

#include "Particle.h"

SYSTEM_MODE(SEMI_AUTOMATIC);
SYSTEM_THREAD(ENABLED);

Serial1LogHandler l(115200, LOG_LEVEL_ALL);
time32_t start = 0;

void setup() {
    Log.info("Push async call to system thread");
    Network.on();

    waitUntil(Network.isOn);
    delay(3s);

    Network.connect();
    delay(1s);

    Log.info("System.sleep");
    System.sleep(SystemSleepConfiguration().mode(SystemSleepMode::STOP).duration(5s));
    Log.info("System.sleep done");
}

void loop() {
}

References

N/A


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)

@XuGuohui XuGuohui added this to the 3.1.0 milestone Apr 22, 2021
@XuGuohui XuGuohui requested a review from avtolstoy April 22, 2021 08:35
@@ -40,6 +40,7 @@ uint32_t wlan_watchdog_duration = 0;

volatile uint8_t SPARK_WLAN_RESET = 0;
volatile uint8_t SPARK_WLAN_SLEEP = 0;
volatile uint8_t SPARK_WLAN_CONNECT_RESTORE = 0;
Copy link
Member

@avtolstoy avtolstoy Apr 27, 2021

Choose a reason for hiding this comment

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

How about SPARK_WLAN_CONNECT instead? Can be set in network_connect() and where appropriate as well (e.g. manage_network_connection or restored after sleep).

Fine to leave as-is as well.

Copy link
Member Author

@XuGuohui XuGuohui Apr 27, 2021

Choose a reason for hiding this comment

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

I was considering to use SPARK_WLAN_CONNECT, yet we need to set/clear it in many places to make sure its state is well set, that will even complicate the system network manager for Gen2 and easier to cause race condition under unknown conditions. So I just suffix it with _RESTORE so that it just need to be set where necessary, like resetting the modem or after sleep. The caller who has called network_off() should take the responsibility to see if the connection should be restored.

system/src/system_sleep_compat.cpp Show resolved Hide resolved
@XuGuohui XuGuohui force-pushed the fix/unexpected_connection_establishment branch from 3b701ea to 8eeb151 Compare April 28, 2021 05:51
@XuGuohui XuGuohui requested a review from avtolstoy April 28, 2021 05:56
Base automatically changed from fix/sleep_consistency to develop April 28, 2021 19:06
@XuGuohui XuGuohui force-pushed the fix/unexpected_connection_establishment branch from 8eeb151 to 644aab7 Compare April 30, 2021 13:26
@XuGuohui XuGuohui merged commit 7bcea8f into develop Apr 30, 2021
@XuGuohui XuGuohui deleted the fix/unexpected_connection_establishment branch April 30, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants