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

SYSTEM_FLAG_RESET_NETWORK_ON_CLOUD_ERRORS #946

Closed
wants to merge 0 commits into from

Conversation

@sergeuz
Copy link
Member

commented Apr 7, 2016

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

@monkbroc monkbroc added the in progress label Apr 7, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2016

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

@technobly

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

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 sergeuz force-pushed the feature/reset-network-flag branch from ba9f8a2 to 033a524 Apr 17, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

Rebased on top of current develop

@m-mcgowan m-mcgowan added this to the 0.6.x milestone Apr 27, 2016

@m-mcgowan m-mcgowan self-assigned this May 3, 2016

{
uint8_t value = 0;
system_get_flag(flag, &value, nullptr);
return value;
}

inline void set_flag(system_flag_t flag, uint8_t value)
static inline void set_flag(system_flag_t flag, uint8_t value)

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan May 27, 2016

Contributor

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.

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 28, 2016

Author Member

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.

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jun 6, 2016

Contributor

Ah, thanks for clarifying! 👍

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented May 27, 2016

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

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2016

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

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2016

@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

@technobly technobly self-assigned this Jun 13, 2016

@sergeuz sergeuz force-pushed the feature/reset-network-flag branch from 033a524 to aceb2cb Jun 13, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2016

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2016

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.

@m-mcgowan m-mcgowan closed this Jun 15, 2016

@m-mcgowan m-mcgowan force-pushed the feature/reset-network-flag branch from 0765e21 to 3441237 Jun 15, 2016

@technobly technobly removed the in progress label Jun 15, 2016

@technobly technobly deleted the feature/reset-network-flag branch Oct 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.