Locking for shared SPI bus

[GUVWAF]

In Meshtastic, we’re using a modified HAL for spiBeginTransaction()/spiEndTransaction() to acquire and release a lock to prevent access to the bus when using another peripheral at the same time (e.g. a TFT screen). However, occasionally we still got SPI errors back from the radio, which was caused by pulling the CS pin low in standby() before the SPI transaction. To solve this, we have to lock around all calls to RadioLib.
Would it be an idea to create an SPI lock and unlock callback to Module, which can be optionally set, to protect the SPI transactions as well as pulling the CS pin?

[jgromes]

This is a bit tricky, because pulling the CS pin low on SX126x is required to get it from sleep mode, and it actually has nothing to do with any SPI transactions (on a side note it should probably be released before the SPI transaction call, maybe that is the root cause?). So I think this is not actually a system-level issue, locking from your own HAL is the preferred solution from my point of view.

How exactly would this work? We would let user set these lock/unlock callbacks and call them before any IO operation if set?

[GUVWAF]

Yes, it being pulled low even during spiBeginTransaction() (where it was waiting for the lock to be released) probably makes it worse. However, even when pulling it low and then releasing it again could cause issues (especially when it wasn’t in sleep mode), right? When the CS pin is pulled low, the radio is expecting that the commands on the SPI bus are intended for it, but in this case they are not.

We would let user set these lock/unlock callbacks and call them before any IO operation if set?

Yes, that’s what I had in mind. I agree doing it from the HAL is also a good solution, but specifically for the “raw” CS pull, this is problematic. We'd need to add another lock for digitalWrite() (but only really when it's the CS pin) and the state should be tracked to ensure no double locking happens.

A separate solution to pull and release the CS pin safely when dealing with a shared SPI bus would also work for us.

[jgromes]

I see - and how about we change that CS pull for a dummy SPI transaction that does not actually send anything? Then it would pull the CS pin by doing this dummy transaction which is guarded. The SPI bus (including the CS pin) is definitely a shared resource, whereas other GPIO pins accessed via digitalWrite are typically not.

Manually toggling the CS pin from the radio class is a bit of a hack anyway; it should only be handled by the Module class. The good news is that it is only used a couple of times in the codebase, so getting rid of it shouldn't be that hard.

[mverch67]

I just commented out the mentioned code in standby() and it works better than before in a multi-threaded application.

//  if(wakeup) {
//    // pull NSS low to wake up
//    this->mod->hal->digitalWrite(this->mod->getCs(), this->mod->hal->GpioLevelLow);
//  }

The GPIO is already pulled low in the next call to function SPIwriteStream() so pulling it low outside of the transaction lock is obviously wrong and superfluous but removing it seems to solve the problem.

[jgromes]

The reason for this "obviously wrong and superfluous" line of code is well documented within git blame - it was added to fix #740. The issue at hand is that SPI transaction must wait for BUSY line to go down, which remains high in sleep mode. Hence the CS gets pulled first to wake up the device, as specified by the SX1261 datasheet. I would take a while to familiarize myself with the background a bit before posting such bold statements ;)

[mverch67]

In #740 it looks that the reported issue occurred in an edge case when using the additional busy/GPIO parameter what we don't use in our project. This edge case relates to this code:

  // ensure GPIO is low
  if(waitForGpio) {
    if(this->gpioPin == RADIOLIB_NC) {
      this->hal->delay(50);
    } else {
      RadioLibTime_t start = this->hal->millis();
      while(this->hal->digitalRead(this->gpioPin)) {
        this->hal->yield();
        if(this->hal->millis() - start >= this->spiConfig.timeout) {
          RADIOLIB_DEBUG_BASIC_PRINTLN("GPIO pre-transfer timeout, is it connected?");
          #if !RADIOLIB_STATIC_ONLY
            delete[] buffOut;
          #endif
          return(RADIOLIB_ERR_SPI_CMD_TIMEOUT);
        }
      }
    }
  }

So I think this if(wakeup) section in standby() may only be executed if this additional GPIO is used otherwise it's indeed superfluous. I just tested replacing the wrong section by

if(wakeup && this->mod->getGpio() != RADIOLIB_NC) {

and this also works for us and will also not affect the edge case in #740. But keep in mind that this code (when using the additional GPIO) is still erroneous in a multi-threaded environment as the CS pin must not be low outside of an SPI transaction lock otherwise it interferes with other SPI transmissions.

[jgromes]

I like how you call "not using the BUSY pin" an edge case - that is using the device as intended ;)

If you really are not using BUSY pin, then RadioLib will try to work around that by just slapping a 50 ms delay before any SPI transaction. Will it work for all SPI transactions? No idea, but it seems to work for most of them. It is hardly optimal, but the only way to kind of make it work without BUSY. I'm guessing that you were forced into this by hardware limitations, but I would strongly suggest using BUSY whenever it is accessible.

Pulling the CS pin to get the device out of sleep mode is required by the SX126x(/SX128x/LR11xx) hardware, plain and simple. The hardware does not really care about your computational model and whether we consider the CS pin part of the shared SPI resource or not. We can sort of "tie" these things together via the dummy SPI transfer as proposed in my post above (+also making the Module::getCs() method private to discourage this in the future). But removing this wakeup call will reintroduce the issue reported in #740 to any setup that uses the BUSY pin.

[mverch67]

Oh, I just see, that we do use this GPIO as busy pin... need to find another solution then.

[GUVWAF]

I think the proposed solution of a dummy SPI transfer (which is guarded) instead of the CS pull would work for us.

[jgromes]

@GUVWAF added in b2b4c9e and tested on my test setup (while also checking that indeed just removing the NSS pull to wakeup does indeed break everything when BUSY is connected). I also applied this to LR11x0 and SX128x since they both use the same NSS pull wakeup mechanism.

[GUVWAF]

Great, thank you!

[HeadBoffin]

Very conflicted here - I've a number of contacts where I've pointed them to Meshtastic and now I get an email from one of them asking if this sort of encounter is normal. Is there a Meshtastic fork that comes with less caffeinated devs?

Wasted time this morning unravelling a Helium mapper repro that claims TTN support but just nukes the FUP, says it uses LMIC in the docs but actually RadioLib. So not enjoying Open Source today at all. Meh.

[GUVWAF]

I've a number of contacts where I've pointed them to Meshtastic and now I get an email from one of them asking if this sort of encounter is normal. Is there a Meshtastic fork that comes with less caffeinated devs?

Not sure what the context was for them, but coming here with these kind of generic accusations also doesn’t help Open Source. I think I’ve been pretty constructive here to solve an issue that has been haunting us for a long time, and so have other Meshtastic devs in the past when working with RadioLib.

[HeadBoffin]

Nothing generic here, it's all in the convo above. The observations made to me regarded the assumptions made through the lens of Meshtastic. Like it is the centre and anything that doesn't conform is bad - "obviously wrong and superfluous" and the "an edge case when using the additional busy/GPIO parameter what we don't use in our project." referencing the BUSY (DIO0) pin which does so much to facilitate LoRa radio firmware since it all began.

This particular contact is/was being encouraged to integrate Meshtastic in to their systems as a bonus, it won't be the prime focus of their system but will grow the eco-system & coverage - but are expressing concern about raising issues. So I'm being asked about alternative support channels. But perhaps not here.

[mariotti]

Good time time to donate money into meshtastic :)

[mariotti]

Indeed the commit b2b4c9e seems to have other dependencies, or better blocking other projects.

It would be nice to push it a bit.

[mariotti]

At present we have a dependency on this PR: meshtastic/firmware#3259
It is a large change which we would like to push soon.

[mariotti]

Any help needed?

[jgromes]

blocking other projects.

Which ones? As discussed above, this fix is only needed in multithreaded environments with more than one device on the same SPI bus and I don't have reports other than from meshtastic that an update is needed. I was also under the impression that there was an existing workaround that meshtastic was using.

Any help needed?

Not sure if this question is addressed to me, but no. It's been implemented, tested and pushed to master, I just don't see the need to release another patch that would effectively only add this one fix. There are other open issues and ongoing work.

[jgromes]

@mariotti FYI a new patch has just been released ;)