SYSTEM_FLAG_RESET_NETWORK_ON_CLOUD_ERRORS

[sergeuz]

Fixes #748 by introducing additional system flag that allows to disable resetting of the network connection on cloud errors (enabled on the Core by default)

---

• Review code
• Test on device
• Add documentation
• Add to CHANGELOG.md

[sergeuz]

It would be better to merge #945 first, so I can rebase this PR on top of it.

[technobly]

Before or after merging #945 don't forget to inc these asserts:

static_assert(SYSTEM_FLAG_RESET_NETWORK_ON_CLOUD_ERRORS == 5, "system flag value");
static_assert(SYSTEM_FLAG_MAX == 6, "system flag max value");

[sergeuz]

Rebased on top of current develop

[m-mcgowan]

What's the reason behind making this static? I'd like to avoid someone writing SystemClass::set_flag() since we may in future need to maintain state, requiring an instance method, i.e. System.set_flag().

In practice, the code produced will be the same since the compiler will optimize out this when inlining the body.

[sergeuz]

The reason is that I added System::enabled() method which is const and thus requires get_flag() to be either static or const as well. Note that both get_flag() and set_flag() methods are in private section of the class, so there shouldn't be any compatibility problem if we change internal implementation later.

[m-mcgowan]

Ah, thanks for clarifying! 👍

[m-mcgowan]

Could you add docs too please. (A rebase on develop is probably needed.)

[m-mcgowan]

Just a gentle reminder @sergeuz that docs are needed before we can merge.

[m-mcgowan]

The original code in handle_cfod() makes my head hurt a bit - I'm not sure if the original intent was to be so intricate or if it's just buggy, but I love your simplification of it!

So it previously incremented the cfod_count after a failed connection - if it reached the MAX_FAILED_CONNECTS value then the SPARK_WLAN_RESET flag was set. If it hadn't reached the MAX_FAILED_CONNECTS value, then an internet check was done and if that failed, then the cfod_count was incremented again, and if that equals MAX_FAILED_CONNECTS then the connection is reset.

So now, we don't reset the connection on an internet check, but only reset the connection every MAX_FAILED_CONNECTS attempts, which is more comprehensible I feel. The failed internet connection still triggers a warning (the LED will flash orange twice) but doesn't cause the connection to be fully reset on each connection attempt, which will be welcomed on the electron where tearing down the connection fully resets the modem.

[m-mcgowan]

It's not enabled on the photon and electron by default. My comment in the issue does suggest the setting may be platform specific, but that's quite a big change in some respects so let's not let it slip by unnoticed without some discussion.

My worries are that if we remove resetting the network for Photon/Electron, that long term connectivity may suffer. A safer fallback may be to keep automatic reset in the AUTOMATIC/SEMI-AUTOMATIC modes, and disable the automatic reset in MANUAL mode, as indicated in the issue. @technobly - any thoughts on this?

[sergeuz]

The original code in handle_cfod() makes my head hurt a bit

Yeap, I was puzzled by that as well. In the current code it looks like the second increment just "speed ups" resetting of the network connection if there's no internet available. It's possible that the code had some deeper intent previously though.

Should I update the code to disable automatic reset only if the system is in MANUAL mode? If yes, do we still need the system flag to enable / disable that functionality?

[m-mcgowan]

I think for now we can leave the code as is - the user has access to the flag so can enable/disable as appropriate. The only potential change I would like to discuss is if we should turn on the reset function for the electron/photon too (to preserve the 0.5.x behavior.)

[technobly]

@sergeuz would you please rebase this against develop and let me know when it's ready to merge? I've reviewed the code and it looks good. There are currently some conflicts with SYSTEM_FLAG_PUBLISH_RESET_INFO.

Also do you have any docs for this one? I was looking at what we already have and what's missing...

Already have in Docs:

System.enableUpdates()
Enables OTA updates. Updates are enabled by default.

System.disableUpdates()
Disables OTA updates. An attempt to start an OTA update will fail.

System.updatesEnabled()
Determine if OTA updates are presently enabled or disabled.

System.updatesPending()
Indicates if there are OTA updates pending.

Missing:

// SYSTEM_FLAG_RESET_ENABLED
System.enableReset();
System.disableReset();
System.resetEnabled();

// SYSTEM_FLAG_RESET_PENDING (although this is in system events which is a different API)
System.resetPending();

// Generic API
System.enable(flag);
System.disable(flag);
System.enabled(flag);
// including a description of flags

[sergeuz]

Added API docs for the system flags. The documentation doesn't cover SYSTEM_FLAG_STARTUP_SAFE_LISTEN_MODE and SYSTEM_FLAG_WIFITESTER_OVER_SERIAL1 flags - as I understand those are not intended to be managed by applications directly.

[technobly]

SYSTEM_FLAG_WIFITESTER_OVER_SERIAL1 is used in Tinker to ensure we have Serial1 enabled for MFGing use, but then would otherwise be disabled.

#if Wiring_WiFi
STARTUP(System.enable(SYSTEM_FLAG_WIFITESTER_OVER_SERIAL1));
#endif

SYSTEM_FLAG_STARTUP_SAFE_LISTEN_MODE sounds like a potentially useful thing, but I'm having a hard time understanding what its use case is.

[m-mcgowan]

I'll merge this but without the last change to docs - the flag is meant to be internal. It was intended for the device updater, to put the device in safe mode for more reliable transfers, but I don't believe it's actually used in practice. When we build the USB vendor commands, this might be used with a special command to reboot to safe listening mode.