sandeepmistry / arduino-LoRa

An Arduino library for sending and receiving data using LoRa radios.
MIT License
1.65k stars 630 forks source link

NSS pull down can be after SPI begin transaction #513

Open CCCanyon opened 3 years ago

CCCanyon commented 3 years ago

I had some problem with LoRa giving no response when the bus is connected with another SPI device with hardware SCK pull-up.

I suspected the culprit is the 1.8V long bump there (NSS vs SCK): LoRa_NSS_SCK_OLED-connected

This problem was fixed by moving the line for NSS low after beginTransaction in singleTransfer():

uint8_t LoRaClass::singleTransfer(uint8_t address, uint8_t value)
{
  uint8_t response;

  _spi->beginTransaction(_spiSettings);
  digitalWrite(_ss, LOW);
  _spi->transfer(address);
  response = _spi->transfer(value);
  _spi->endTransaction();
  digitalWrite(_ss, HIGH);

  return response;
}
IoTThinks commented 3 years ago

Seem correct to me in my first sight. Nice find. If we share the same SPI with other SPI devices, we need to set CS/SS low to select a SPI device.

What if other codes can wrongly set multiple CS/SS to low in the same time too? As the code execution may not be sequential.

CCCanyon commented 3 years ago

Seem correct to me in my first sight. Nice find. If we share the same SPI with other SPI devices, we need to set CS/SS low to select a SPI device.

I did not add any code .I "moved" NSS low after _spi->beginTransaction(_spiSettings).

What if other codes can wrongly set multiple CS/SS to low in the same time too? As the code execution may not be sequential.

The case when other codes might went wrong is not the concern of this library. In this thread we just need to make this library work properly.

LurkingKiwi commented 2 years ago

I found this problem too, the existing code (not yet fixed in April 2022) violates the SPI usage rules, and leaves a problem where an interrupt can occur between NSS low and beginTransaction() blocking interrupts. If the ISR accesses the same chip then the interrupted call fails as NSS was put back up by the ISR calling endTransaction(). If it accesses a different chip, both chips will receive the communication from the ISR in parallel with bad results.