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

[hal] SPI: fixes potential deadlock on Gen3 platforms. #2091

Closed
wants to merge 1 commit into from

Conversation

XuGuohui
Copy link
Member

Problem

Gen3 Device may get blocked when priority of the thread doing SPI transmission higher than the thread doing USB Serial logging.

Solution

Check and handle SPI NRF_SPIM_EVENT_END while waiting SPI transaction to be completed.

Example App

#include "Particle.h"

SerialLogHandler logHandler(115200, LOG_LEVEL_ALL);

SYSTEM_THREAD(ENABLED);
SYSTEM_MODE(SEMI_AUTOMATIC);

Thread *spi_thread;

__SPISettings spi_settings(5*MHZ, MSBFIRST, SPI_MODE0);

uint32_t spi_count = 0;

void spi_thread_f(void) {
    while (true) {
        uint8_t rx_buf[64];
        SPI.beginTransaction(spi_settings);
        SPI.transfer(NULL, rx_buf, sizeof(rx_buf), NULL);
        SPI.endTransaction();
        spi_count++;
        delay(1);
    }
}

void setup() {
    SPI.setDataMode(SPI_MODE0);
    SPI.setClockSpeed(5, MHZ);
    SPI.begin();

    Serial.begin(115200);
    while(!Serial.isConnected());

    spi_thread = new Thread("spiTest", spi_thread_f, OS_THREAD_PRIORITY_CRITICAL);

    pinMode(D7, OUTPUT);
}

void loop() {
    Log.info("heartbeat: %lu", spi_count);
    delay(1);

    static system_tick_t start = millis();
    static bool toggle = false;
    if (millis() - start >= 1000) {
        start = millis();
        if (toggle) {
            digitalWrite(D7, HIGH);
        } else {
            digitalWrite(D7, LOW);
        }
        toggle = !toggle;
    }
}

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
Copy link
Member

@XuGuohui Let's fix this properly as we've discussed previously by overriding CRITICAL_REGION_ENTER()/EXIT() to use PRIMASK instead of altering NVIC registers.

@YutingYou
Copy link
Contributor

@XuGuohui Let's fix this properly as we've discussed previously by overriding CRITICAL_REGION_ENTER()/EXIT() to use PRIMASK instead of altering NVIC registers.

It might potentially affect BLE protocol which is based on the timer0 and other interrupts.

@avtolstoy
Copy link
Member

avtolstoy commented May 12, 2020

It might potentially affect BLE protocol which is based on the timer0 and other interrupts.

Which will not get blocked. We are already using this approach throughout DeviceOS. See how HAL_disable_irq() is implemented for Gen 3. We may need to use priority 4 vs 2 here specifically though.

@XuGuohui
Copy link
Member Author

We may need to use priority 4 vs 2 here specifically though.

I still got a SOS when use priority 2 building against the latest develop branch. I tested on Argon.

@XuGuohui
Copy link
Member Author

We have another workaround that makes more sense to fix the issue. This PR will be closed and superseded by another PR when it is ready.

@XuGuohui XuGuohui closed this May 13, 2020
@avtolstoy avtolstoy mentioned this pull request May 13, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants