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

Guard cellular_command() with a global lock #1415

Merged
merged 3 commits into from Jan 22, 2018

Conversation

@sergeuz
Copy link
Member

commented Oct 26, 2017

Problem

This PR fixes #1298 by guarding the implementation of cellular_command() with the Cellular HAL's global lock.

Steps to Test

Flash the following application to an Electron, and ensure that repeated attempts to update the device OTA don't cause communication errors and hard faults.

#include "application.h"

SYSTEM_THREAD(ENABLED)

namespace {

const Serial1LogHandler logHandler(115200, LOG_LEVEL_INFO, {
    { "app", LOG_LEVEL_ALL }
});

const unsigned INTERVAL = 500;
unsigned lastMillis = 0;

void onFirmwareUpdate() {
    lastMillis = millis();
}

} // namespace

void setup() {
    System.on(firmware_update_pending, onFirmwareUpdate);
}

void loop() {
    if (lastMillis != 0) {
        if (!Particle.connected()) {
            Log.info("disconnected");
            lastMillis = 0;
        } else if (millis() - lastMillis >= INTERVAL) {
            Log.info("sending command");
            Cellular.command("AT\r\n");
            lastMillis = millis();
        }
    }
}

References

  • [CH8908]

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)

@sergeuz sergeuz added the bug label Oct 26, 2017

@sergeuz sergeuz added this to the 0.8.0 milestone Oct 26, 2017

@sergeuz sergeuz requested a review from technobly Oct 26, 2017

int MDMParser::command(const char* cmd, _CALLBACKPTR cb, void* param, system_tick_t timeout)
{
LOCK();
send(cmd, strlen(cmd));

This comment has been minimized.

Copy link
@technobly

technobly Nov 14, 2017

Member

This should use MDMParser::sendFormated to ensure it is cancellable with if (_cancel_all_operations) return 0;

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 14, 2017

Author Member

hm, but why is there a semantic difference between send() and sendFormatted() in regards to the _cancel_all_operations flag? Is it an oversight or a designed behavior?

This comment has been minimized.

Copy link
@technobly

technobly Nov 14, 2017

Member

all send operations in mdm_hal start with sendFormatted(), so that's where I targeted the cancel operation, instead of down the line in the lower level send() function.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 14, 2017

Author Member

ok, so every logical modem operation starts with a call to sendFormatted() and send() is mostly used to send remaining data, such as the contents of a datagram buffer. If so, could we reduce the size of the buffer used by sendFormatted()? Currently it allocates 1K on stack just to format an AT command and I couldn't find any place in the code where such a large buffer might be needed. Most of the commands sent via sendFormatted() don't even use any formatting at all and are as short as e.g. sendFormated("AT+CGSN\r\n").

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 14, 2017

Author Member

I suggest we use a similar approach as in Print::printf(), i.e. allocate a small buffer initially and then use a variable length array if it appears that the formatted data doesn't fit into the original buffer: https://github.com/spark/firmware/blob/develop/wiring/src/spark_wiring_print.cpp#L271

This comment has been minimized.

Copy link
@technobly

technobly Nov 14, 2017

Member

That method looks fine, and we have another place in cellular_command with char buf[256]; that could use a similar treatment.

This comment has been minimized.

Copy link
@sergeuz

sergeuz Nov 14, 2017

Author Member

I'll add an overloaded sendFormatted() taking va_list as an argument in order to not duplicate the code.

@m-mcgowan m-mcgowan self-requested a review Nov 20, 2017

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

Just remembered we have this scheme as well, but I do prefer the global lock so you don't have to know if a peripheral is thread safe, or is a shared resource and think about trying to acquire a lock before using it. https://prerelease-docs.particle.io/reference/firmware/electron/#synchronizing-access-to-shared-system-resources

@m-mcgowan @sergeuz I'm needing this change to be merged asap :)

@technobly technobly modified the milestones: 0.8.0, 0.8.0-rc.2 Jan 5, 2018

@towynlin

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

👍 Let's get this released ASAP.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2018

Good point, @technobly. It would be nice to have a scoped locking for Cellular as well, because for certain commands the modem can reply with a prompt asking the caller to provide a continuation of a command. In this case the atomicity provided by Cellular at the method level is not enough. However, I think that this PR can still be merged as is, since it addresses a somewhat separate issue, where sending of a command and receiving of a response are not done atomically even for a single and complete command.

@technobly

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

Good point as well @sergeuz. The specific case I need involves using Cellular.command for one command that does not involve a prompt. So I believe this PR will fix that situation as it currently stands. I want to make sure that when the system makes a call for something like a MDMParser::socketSendTo that requires a prompt, that it will not allow Cellular.command() to interrupt it. I think it should, because those calls are wrapped with a LOCK(). But if I wanted to do the same thing with Cellular.command() it would require something like the WITH_LOCK() API. I can see this becoming a future requirement quickly, so please prioritize that feature.

@sergeuz sergeuz force-pushed the fix/issue_1298 branch from 367d054 to 5ad7fe6 Jan 9, 2018

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

@technobly Added support for WITH_LOCK(Cellular) and rebased this branch on develop.

@m-mcgowan m-mcgowan merged commit a463736 into develop Jan 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
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.