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

Fixes Particle.connect() behavior in SEMI_AUTOMATIC mode #1403

Closed
wants to merge 3 commits into from

Conversation

@avtolstoy
Copy link
Member

avtolstoy commented Oct 6, 2017

submission notes
**Important:** Please sanitize/remove any confidential info like usernames, passwords, org names, product names/ids, access tokens, client ids/secrets, or anything else you don't wish to share.

Please Read and Sign the Contributor License Agreement ([Info here](https://github.com/spark/firmware/blob/develop/CONTRIBUTING.md)).

You may also delete this submission notes header if you'd like. Thank you for contributing!

Problem

Particle.connect() still does not work correctly in SEMI_AUTOMATIC mode. Original issues: #973 #1399

Solution

Instead of blocking Particle.connect() call immediately as implemented in #1145, only the next invocation of loop() should be blocked until the device connects to the cloud.

Steps to Test

  • app/particle_connect_semi_automatic_issue_1399

Example App

N/A

References


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)
@avtolstoy avtolstoy requested a review from m-mcgowan Oct 6, 2017
@m-mcgowan m-mcgowan added this to the 0.7.0 milestone Oct 31, 2017
if(threaded || SPARK_WLAN_SLEEP || !spark_cloud_flag_auto_connect() || spark_cloud_flag_connected() || SPARK_WIRING_APPLICATION || (system_mode()!=AUTOMATIC))
{
if(threaded || !SPARK_FLASH_UPDATE)
do {

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 1, 2017

Contributor

what's the purpose of the do/while(false) - isn't it the same as a regular block?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 21, 2017

Author Member

We need to short-circuit the execution at line https://github.com/spark/firmware/pull/1403/files#diff-17aa9c74822820227ce1b5db3aab8d76R527 We can change that to goto if it looks better :)

if (system_mode()!=SAFE_MODE)
setup();
SPARK_WIRING_APPLICATION = 1;
if (semi_automatic_connecting(threaded)) {

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Nov 1, 2017

Contributor

The diff isn't so clear in github, but logically, I imagine this is the primary change - to not invoke loop while connecting in semi-automatic mode.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Nov 21, 2017

Author Member
@m-mcgowan

This comment has been minimized.

Copy link
Contributor

m-mcgowan commented Nov 21, 2017

@avtolstoy, could you please review my comments above and rebase+fix conflicts. Thanks!

@avtolstoy avtolstoy force-pushed the fix/connect-semi-automatic branch from 9b37236 to 3948773 Nov 21, 2017
@avtolstoy

This comment has been minimized.

Copy link
Member Author

avtolstoy commented Nov 21, 2017

rebased on develop.

@m-mcgowan m-mcgowan modified the milestones: 0.7.0, 0.7.0-rc.7 Jan 18, 2018
@m-mcgowan m-mcgowan self-requested a review Jan 18, 2018
@ScruffR

This comment has been minimized.

Copy link
Contributor

ScruffR commented Jan 30, 2018

I'm still not sure why we need to block after a Particle.connect() at all?
IMO this is just a "fix" to support the ignorance of the caller.

Especially when using SYSTEM_THREAD(ENABLED) - but also single-threaded - why on earth would loop() need to be prevented from running again till connected.
I guess most people who spend some minimum amount of time to plan their projects would be aware of waitFor(), waitUntil() and if (Particle.connected()) would prefer Particle.connect() to behave as fire-and-forget and just guard their own code against lost (= indistinguishable from "not yet regained") connection.

If anything then waitFor() needs to fully function as expected even in single-threaded mode (see comment here #1028 (comment))

One point that would be useful tho' would be a guard against multiple calls of Particle.connect() interfering with already running connection attempts.

@technobly

This comment has been minimized.

Copy link
Member

technobly commented Jan 30, 2018

@avtolstoy just checking that we are not blocking loop when running in MULTI-THREADED mode in any system mode, is that right?

We are only fixing semi automatic from not blocking loop when called in SINGLE--THREADED mode.

@ScruffR

This comment has been minimized.

Copy link
Contributor

ScruffR commented Jan 30, 2018

If this is a non-issue for multi-threaded mode just the better, but still I can't quite see the rationale for this

Instead of blocking Particle.connect() call immediately as implemented in #1145, only the next invocation of loop() should be blocked until the device connects to the cloud.

I might misinterpret the part: "until the device connects", but when I interpret it as "until the device has connected", then I see some risk but no advantage in this.
If for any reason Particle.connect() can't succeede I'd have to wait-out the timeout periode to do any of the other work that'd not rely on the actual cloud connection.

Given a setting where Particle.connect() is the last instruction in loop() the behaviour - for the users point of view - would still be as if Particle.connect() would be blocking, so nothing really won, or not?

I know this fits to the (currently) documented behaviour of SEMI_AUTOMATIC mode, but I still don't see the benefit and rather think this is due to legacy before the advent of SYSTEM_THREAD and options like waitXXX().

Sorry for being awkward 😊, but I just think to recall some discussion back in Spark Core times where Mat mentioned that Spark.connect() wouldn't be blocking but merely set some system flags for the system tasks between loop() would act upon, but I can't provide and evidence for this (might just be dementia)


Update: (no dementia)
https://community.particle.io/t/create-time-out-for-spark-connect/10351/17?u=scruffr

So it appears that at first the docs were wrong but the behaviour was as most people would have liked it (despite the erronous docs) and now we are about to make the system fit the docs rather than the other way round.

@technobly

This comment has been minimized.

Copy link
Member

technobly commented Jan 31, 2018

The original spec for AUTOMATIC, SEMI_AUTOMATIC and MANUAL is here:
#216

Particle.connect() in SEMI_AUTOMATIC and SINGLE-THREADED mode was always designed to be a blocking call.


Now that we have MULTI-THREADED mode, can we not just use that for unblocking things and let SINGLE-THREADED mode be mostly blocking as it was originally intended to be?


why on earth would loop() need to be prevented from running again till connected.

Here are some thoughts, it's not as simple as just running loop() because loop() is most important. In the single-threaded world, If we are supposed to be connected to the Cloud, and are not yet, that Cloud connection needs to be established and there is work to be done. Wi-Fi must be connected, and the Cloud handshake needs to occur. After that we maintain a heartbeat every ~15s but also process any incoming/outgoing messages every time loop() ends. If we go off and run loop() before those initial things happen, we could be taking a very long time to connect to the Cloud depending on what's happening in loop(). This is however the entire reason for multi-threaded mode, so loop() can run while all of those background activities are blocking and doing their thing.

@ScruffR

This comment has been minimized.

Copy link
Contributor

ScruffR commented Jan 31, 2018

Thanks Brett, for the additional thoughts 👍
The question remains, why did this not seem to pose a problem from October 2014 to who knows when after April 2015 as to be read in the thread linked above?
As it seemed it was an unexpected but working "solution" and Mat didn't seem to see any issue with that behaviour at any time.

This whole discussion might be only academic once multi-threading will come out of beta and virtually becomes standard mode. However if this is the way to go, some definitive statement about the expected blocking periode (especially when not already WiFi connected) and possibly some means to change that timeout periode would be needed IMO (since such things as waitFor() can't be used).
Also means to interrupt the connection process should be documented.
Also what happens after the first time the call times out? Will there be automatic retries? If, how many and when?

For the users point of view Particle.connect() often hides away WiFi.connect() and hence the connection time stated in the docs (~1sec) does not really tell the whole story.
It's only true when already WiFi.ready() and internet available, but that's what users often trip up on.
By making Particle.connect() blocking that way, you also make the implicit WiFi.connect() also blocking although it's not meant to be but causes most of the issues users complain about when complaining about Particle.connect().

BTW, if a major factor in the rationale for making it blocking is that user code supposedly might do stuff that potentially interferes with the ongoing connection process, why not just call out that risk in the docs and hand back the responsibility not to do such things to the user?
After all, that's already what's done with warning not to block for more than 10sec without calling Particle.process() or delay() - why not in this case too?

And about the part "WiFi must be connected", why is then WiFi.connect() not documented as blocking in the same way when called independently?
Actually WiFi.connect() is mostly blocking, but not completely. From time to time it hands back control to user code (AFAICT whenever a timeout occures before a new automatic attempt is started - otherwise WiFi.connecting() - which was there before multi-threading - would have been absolutely obsolete as it never could be checked). This way user code gets the chance to intervene. Not so with the blocking Particle.connect() as I currently see it being layed out.

@technobly

This comment has been minimized.

Copy link
Member

technobly commented Jan 31, 2018

I'm sorry but I don't have the answers you seek at this time. I agree more needs to be documented. There's also MANUAL mode as well in SINGLE-THREADED operation. This is the one you want if you want to emulate a slow MULTI-THREADED operation.

SEMI_AUTOMATIC is meant to just allow you to do some things in setup() before the connection process takes place, after which Particle.connect() essentially turns it into AUTOMATIC mode.

Can you show me some code that you rely on Particle.connect() not to block in SEMI_AUTOMATIC / SINGLE-THREADED mode?

@ScruffR

This comment has been minimized.

Copy link
Contributor

ScruffR commented Jan 31, 2018

(@technobly) Brett, it's not really that some of my code relies on the non-blocking behaviour as I do know my way around most the issues, but I'm rather playing advocate for a different school of thought (although I maybe shouldn't 😊 just gets me into trouble to defend my - or worse someone else's -
stance 😳).
While I'm fully prepared to accept meaningful or even pragmatic/opportunistic rules, no rule should be treated as a dogma and stand beyond challenge.
This is what I think drives science and innovation.

The reason for the whole discussion was the "long standing (Oct. 2014 till 0.6.1-rc.1)" - so called - bug which "suddenly" got "fixed" causing users to get puzzled why good working code (despite the fact it neglected the docs, but was backed by semi-official "afaict"-statements) stopped working.
Such examples are found in the thread I linked to in my original issue reports
https://community.particle.io/t/electron-firmware-alerting-depending-on-a-variable/36206/9?u=scruffr
But as I understood members like @aeris-ming or @elcojacobs (their GitHub tags) who have filed or commented on related issues might have some actual project that gave them grieve due to the change.
I also recall other discussions on the forum that led up to my issue report, but I can't currently link to any of them.

@aeris-ming

This comment has been minimized.

Copy link

aeris-ming commented Feb 2, 2018

@ScruffR Please download and watch this vido "https://pan.baidu.com/s/1bqrGXLP"
When the usr code be blocked our product stop work I really have no idea how to explain this to my customer

@aeris-ming

This comment has been minimized.

Copy link

aeris-ming commented Feb 2, 2018

@ScruffR And Usr code will be block with system firmware 0.5.3 0.5.4 0.6.1 0.7.0.rc6 0.8.0.rc1

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

m-mcgowan commented Feb 5, 2018

Cherry-picked to 0.7.0-rc.7 branch, and that merged to develop for 0.8.0-rc.2 release. (I probably should have rebased instead so Github maintained the tracking, but that's hindsight.)

@incorvia

This comment has been minimized.

Copy link

incorvia commented May 28, 2018

I've tested in order to confirm that when SYSTEM_THREAD(ENABLED) is set Particle.connect does not block in multi-threading mode. I also see now:

"We are only fixing semi automatic from not blocking loop when called in SINGLE--THREADED mode."

This does seem to be implemented the way i'd expect to deleting my previous rant :)

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