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.disableUpdates() operates asynchronously #1801

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Conversation

m-mcgowan
Copy link
Contributor

@m-mcgowan m-mcgowan commented May 29, 2019

This PR will be cherry-picked into another PR based on release/1.2.1 (for 1.2.1-rc.3).

Problem

For a full description see the clubhouse story. In a nutshell, System.disableUpdates() should not block the application thread to send the event to the cloud.

Solution

The internal function system_send_event gains an additional flag to indicate the event should be processed asynchronously. This is a temporary solution until we introduce futures to all synchronous system APIs so that they may be used synchronously or asynchronously as chosen by the caller.

Steps to Test

Testing was done by using the app below both with and without the asynchornous event flag handling. The connection to the cloud was interrupted by turning off the mobile hotspot and the LED observed to see if the application thread continued running. Without asynchronous events, the LED stopped blinking immediately. With asynchronous events, it continued blinking for a few seconds (blocking on queuing the asynchronous request to the system event queue - we should consider allowing the queue to allocate more resources or make some functions optional when the queue cannot accept more items.)

Example App

#include "application.h"

SYSTEM_THREAD(ENABLED);
void setup()
{
    Serial.begin();
    pinMode(D7, OUTPUT);
}

#define TIME(fn, maximum) \
    { int start = millis(); \
    fn; \
    int duration = millis()-start; \
    if (duration > maximum) { \
        Serial.printlnf("function took too long: %d ms", duration); \
    }}

//ApplicationWatchdog wd(5000, System.reset);

void loop()
{
    digitalWrite(D7, false);
    TIME(System.enableUpdates(), 2);
    delay(100);
    digitalWrite(D7, true);
    TIME(System.disableUpdates(), 2);
    delay(100);
}

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)

  • [bugfix] System.disableUpdates() operates asynchronously #1801

@m-mcgowan m-mcgowan added this to the 1.2.1-rc.2 milestone May 29, 2019
@technobly technobly modified the milestones: 1.2.1-rc.2, 1.2.1 May 31, 2019
@technobly technobly requested a review from sergeuz June 14, 2019 04:04
@technobly technobly modified the milestones: 1.2.1, 1.3.0-rc.1 Jun 14, 2019
Copy link
Member

@sergeuz sergeuz left a comment

Choose a reason for hiding this comment

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

This is fine as a temporary solution. Do you plan to implement support for "cancellable events" in the scope of this PR as well, or should we merge it as is for now?

@@ -159,7 +159,12 @@ inline uint32_t convert(uint32_t flags) {

bool spark_send_event(const char* name, const char* data, int ttl, uint32_t flags, void* reserved)
{
if (flags & PUBLISH_EVENT_FLAG_ASYNC) {
SYSTEM_THREAD_CONTEXT_ASYNC_RESULT(spark_send_event(name, data, ttl, flags, reserved), true);
Copy link
Member

Choose a reason for hiding this comment

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

For visibility: this will only work if name and data are statically allocated (which is true for the updates/enabled and updates/forced events)

@technobly technobly added ready to merge PR has been reviewed and tested and removed needs review labels Jun 14, 2019
…chronous so the system thread is not held up unnecessary.
@technobly technobly merged commit 5165c25 into develop Jun 14, 2019
@technobly technobly deleted the feature/ch33758 branch June 14, 2019 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ready to merge PR has been reviewed and tested
Projects
None yet
3 participants