Feature/cancel network connect on disconnect

[m-mcgowan]

Implementation for Issue #886

Todo

• Because this PR is primarily valuable for use with Multi-Threaded mode on the Electron, we need to solve the lockup issue before it should be merged.
• We should also look into what can be done to make Soft Power down function immediately with this PR while Connecting to the tower, i.e. It should cancel the connect. Some suggestions below.

---

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)

[m-mcgowan]

The latest commit ensures that the cancel operation only affects the call to Cellular.connect(). However, I'm seeing that my electron doesn't connect to the tower, and sometimes stalls entirely. Here's the test code:

#include "Particle.h"

SerialDebugOutput out;

SYSTEM_MODE(SEMI_AUTOMATIC)
SYSTEM_THREAD(ENABLED)

bool not_connecting() { return !Cellular.connecting(); }

void setup()
{
        while (!Serial.available());
        Cellular.on();
        delay(1000);
        Cellular.disconnect();
        delay(1000);
        DEBUG("app connecting");
        Cellular.connect();
        DEBUG("app connecting complete");
    delay(3000);
        DEBUG("app disconnect");
    Cellular.disconnect();
        DEBUG("app disconnect complete");
    waitUntil(not_connecting);
        DEBUG("app reconnect");
    Cellular.off();
        DEBUG("app reconnect complete");
        Cellular.on();
        Particle.connect();
}

void loop()
{

}

(EDIT: Why is Eclipse formatting so broken?)

[m-mcgowan]

This is a risky change - I've seen my electron hang. @technobly Can you confirm the same?

[technobly]

Ok, this test app works as expected... however I've commented out SYSTEM_THREAD(ENABLE)

#include "Particle.h"

SerialDebugOutput out;

SYSTEM_MODE(SEMI_AUTOMATIC)
SYSTEM_THREAD(ENABLED)

bool not_connecting() { return !Cellular.connecting(); }

void setup()
{
    while (!Serial.available());
    DEBUG_D("\r\n[ powering on modem ]\r\n\r\n");
    Cellular.on();
    DEBUG_D("\r\n[ powering on modem complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ disconnecting modem ]\r\n\r\n");
    Cellular.disconnect(); // This does nothing because the modem never connected yet
    DEBUG_D("\r\n[ disconnecting modem complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ app connecting ]\r\n\r\n");
    Cellular.connect();
    DEBUG_D("\r\n[ app connecting complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ app disconnect ]\r\n\r\n");
    Cellular.disconnect();
    DEBUG_D("\r\n[ app disconnect complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ wait until not connecting ]\r\n\r\n");
    waitUntil(not_connecting);
    DEBUG_D("\r\n[ wait until not connecting complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ powering off modem ]\r\n\r\n");
    Cellular.off();
    DEBUG_D("\r\n[ powering off modem complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ powering on modem ]\r\n\r\n");
    Cellular.on();
    DEBUG_D("\r\n[ powering on modem complete ]\r\n\r\n");
    delay(1000);

    DEBUG_D("\r\n[ connecting to cloud ]\r\n\r\n");
    Particle.connect();
    DEBUG_D("\r\n[ connecting to cloud complete ]\r\n\r\n");
}

void loop()
{

}

With theading enabled... the calls to the modem do not block the test app from proceeding to the next line, so that is why it behaves unexpectedly.

[m-mcgowan]

But the threading is essential so we can cancel the connect attempt while it is happening.

[technobly]

Some notes from testing this PR on an Electron U260

• locks up during Connect when system threading is used.
• does not allow soft power down to work properly during Connect, will eventually soft power down at the end of Connect, which could take 5 minutes.
• will cancel connect by holding SETUP for 3 seconds with system threading off.
• does not appear better in canceling connect than v0.4.8-rc.6 firmware when system threading is off.
• v0.4.8-rc.6 can perform a soft power down during Connect.

Todo

• Because this PR is primarily valuable for use with Multi-Threaded mode on the Electron, we need to solve the lockup issue before it should be merged.
• We should also look into what can be done to make Soft Power down function immediately with this PR while Connecting to the tower, i.e. It should cancel the connect.

[technobly]

Some more data... Tinker that looks like this below, does seem to work ok and not lockup during connect with multi-threading enabled.

SYSTEM_MODE(AUTOMATIC);
SYSTEM_THREAD(ENABLED);
void setup()
{
    Particle.function("digitalread", tinkerDigitalRead);
    Particle.function("digitalwrite", tinkerDigitalWrite);
    Particle.function("analogread", tinkerAnalogRead);
    Particle.function("analogwrite", tinkerAnalogWrite);
    pinMode(D7, OUTPUT);
}
void loop()
{
    digitalWrite(D7, !digitalRead(D7));
    delay(100);
}

• Soft power down still requires a full connection to the cloud before the double tap will be processed. Same is true for RSSI (single tap). This seems to be because system_handle_button_click() is only called from Spark_Idle_Events() instead of immediately in reset_button_click(). Recommending this change... @sergeuz what do you think? It seems to work well. At this point in time I don't want to move RSSI in there as well, because it's a little bit dependent on the network being connected already, so a call to it from interrupt would not be good.

void system_handle_button_click()
{
    const uint8_t clicks = button_final_clicks;
    button_final_clicks = 0;
    switch (clicks) {
    case 1: // Single click
        system_display_rssi();
        break;
    // case 2: // Double click
    //     SYSTEM_POWEROFF = 1;
    //     network.connect_cancel(true);
    //     break;
    default:
        break;
    }
}
#endif // #if Wiring_SetupButtonUX

void reset_button_click()
{
    const uint8_t clicks = button_current_clicks;
    button_current_clicks = 0;
    CLR_BUTTON_TIMEOUT();
    if (clicks > 0) {
        system_notify_event(button_final_click, clicks);
        button_final_clicks = clicks;

#if Wiring_SetupButtonUX
        // Handle soft power down immediately
        if (clicks == 2) {
            SYSTEM_POWEROFF = 1;
            network.connect_cancel(true);
        }
#endif // #if Wiring_SetupButtonUX
    }
}

[technobly]

@m-mcgowan what do you think about my Soft Power Down comments and code fix above? We also need to figure out what it is about my code above that locks up the electron during connect with multithreading enabled.

[m-mcgowan]

I don't think we should mix soft power down requiring an responsive application into this but address that separately. I wrote an app that blocked loop(), and soft power down doesn't work on 0.4.8-rc.6 either, so this is not a new issue.

The reason is because system events should be posted and fired before shutting down, and these should happen on the application thread. A refinement might be to set a flag and force a system shutdown from systick after a timeout, but let's take that as a separate issue.

[technobly]

👍 I'm ok with breaking out the Soft Power Down refinement. Edited the first post to reflect this. Would you please add the issue with your additional thoughts on a fix?

[m-mcgowan]

• I found the cause of the hang, which was what made this a dangerous PR.
• on disconnect, the device can still breathe green. It does this also on the photon. There is code to explicitly check if the disconnect was unsolicited or not, and the blue fading LED only shown on unsolicited disconnect and not when Network.disconnect() is called. This is a long standing issue not introduced by this PR.

[technobly]

@m-mcgowan tested my test app above on a U260 [edit: with multi-threading enabled] and it does not lock up now during connect :) Also cancel works during connect just fine, and can exit and connect to the Cloud thereafter. Some of the Cellular API function operations lag behind the debugging output, but everything works. Also on device wiring/no_fixture tests all pass.