RFM69 Library SPI_HAS_TRANSACTION question

[Robert]

RFM69 Library  SPI_HAS_TRANSACTION questions

Felix,

Long time ago; SPI_HAS_TRANSACTION and SPI_HAS_NOTUSINGINTERRUPT were defined in the SPI library to conditionally activate some functions, typically to avoid interrupt conflict while transferring SPI data during an ISR.
The RFM69 library is now improved by taking the SPI management out of the ISR routine, so the above definition shall not be used anymore.
However it is still in use ...
But because SPI_HAS_TRANSACTION is by default true, I don't understand the usage of it in the current library.
The only place where it could be an option is in the RFM69::select function (see at the end of this memo), but the "else" condition will never be true because of the "ifdef" .
Having say that this also means the SPI clock will always be 8MHz for all kind of boards

Code Select Expand

#ifdef SPI_HAS_TRANSACTION
  _settings = SPISettings(8000000, MSBFIRST, SPI_MODE0);
#endif

As test I did remove all "ifdef .. endif"  SPI_HAS_TRANSACTION statements leaving of course the reference to the SPI settings and transactions  and I have no receive/transmit issues.
So I believe the SPI_HAS_TRANSACTION could be removed, if there is no good read to adapt the SPI speed according board types (beside ESP8266 / EP32 running at 160MHz rate  compared to 16 MHz for a ATmega328p, but that is another story)

Regards
Robert

Code Select Expand

void RFM69::select() {
#if defined (SPCR) && defined (SPSR)
  // save current SPI settings
  _SPCR = SPCR;
  _SPSR = SPSR;
#endif

#ifdef SPI_HAS_TRANSACTION
  _spi->beginTransaction(_settings);
#else
  // set RFM69 SPI settings explicitly
  _spi->setDataMode(SPI_MODE0);
  _spi->setBitOrder(MSBFIRST);
  #if defined(__STM32F1__)
    _spi->setClockDivider(SPI_CLOCK_DIV8);
  #elif defined(__arm__)
    _spi->setClockDivider(SPI_CLOCK_DIV16);
  #else
    _spi->setClockDivider(SPI_CLOCK_DIV2);
  #endif
#endif
  digitalWrite(_slaveSelectPin, LOW);
}

[Robert]

And more ...
Because there is no interrupt issue anymore the SPI::beginTransaction and endTransaction are useless
and the RFM69::select () and  RFM69::unselect () may be resumed as:

Code Select Expand

// select the RFM69 transceiver (save SPI settings, set CS low)
void RFM69::select() {
#if defined (SPCR) && defined (SPSR)
  // save current SPI settings
  _SPCR = SPCR;
  _SPSR = SPSR;
#endif
  digitalWrite(_slaveSelectPin, LOW);
}

Code Select Expand

// unselect the RFM69 transceiver (set CS high, restore SPI settings)
void RFM69::unselect() {
  digitalWrite(_slaveSelectPin, HIGH);  
  // restore SPI settings to what they were before talking to RFM69
#if defined (SPCR) && defined (SPSR)
  SPCR = _SPCR;
  SPSR = _SPSR;
#endif
}

NB:   _settings = SPISettings(8000000, MSBFIRST, SPI_MODE0); must be of course kept in RFM69::initialize

[Felix]

The first thought is, it is probably safe to remove it...

The backstory is that the SPI_HAS_TRANSACTION is there for older SPI versions I suppose, where there is no _spisettings.
If I understand your proposal correctly, removing the SPI_HAS_TRANSACTION, and just leaving the calls assuming SPI_HAS_TRANSACTION is always true and the library the latest, then there's the possibility it will fail to compile when someone uses it with some older SPI library/Arduino core. Or say if for some reason they didn't want transactions and would undefine SPI_HAS_TRANSACTION.

But because SPI_HAS_TRANSACTION is by default true, I don't understand the usage of it in the current library.
The only place where it could be an option is in the RFM69::select function (see at the end of this memo), but the "else" condition will never be true because of the "ifdef" .

Not sure which "else" you refer to...:

Code Select Expand

#ifdef SPI_HAS_TRANSACTION
  _spi->beginTransaction(_settings);
#else                            <---------- this one?

The reasoning here that this else is where the specific SPI settings would fall back if there is no SPI_HAS_TRANSACTION in the chain of compiled files and directives, it would just set things specifically. The way I see it, we do things one way if there is SPI_HAS_TRANSACTION, as everyone else seems to, and settings things manually if not (like in early versions of RFM69 library).

[Robert]

Felix,

Not actually a big issue!
My question is trying to understand the usage of SPI_HAS_TRANSACTION with this current library version expecting you could clarify it for me.
NB:
1. older version of SPI has no definition of SPI_HAS_TRANSACTION that will be considered to false. So SPI will not work with the RFM library
2. yes I refer to the #
ifdef SPI_HAS_TRANSACTION
  _spi->beginTransaction(_settings);
#else

As far as I remember (probably 2015 and it is a long story), a problem was raised because of interrupt conflict between RFM69 transceivers and others SPI devices typically the Arduino Ethernet Shield using WizNet5100, because the RFM69 Library was managing interrupted data from the ISR.
See
https://lowpowerlab.com/forum/moteino/moteino-w5100-ethernet-spi-support-spi_has_transaction/msg5132/#msg5132
and
https://lowpowerlab.com/forum/moteino/moteino-w5100-ethernet-spi-support-spi_has_transaction/75/

The solution at that time  was to use with the RFM69 library the updated SPI library of Paul Stoffregen (which is now the official one released with the Arduino IDE since 2015) conditional compilation switches indicating usage of SPI transaction in interrupt through:

#define SPI_HAS_TRANSACTION 1
and
#define SPI_HAS_NOTUSINGINTERRUPT 1

To select (typically)
SPI::usingInterrupt(uint8_t interruptNumber)
SPI::notUsingInterrupt(uint8_t interruptNumber)

The SPI library also introduces 
  SPI::beginTransaction()
  SPI::endTransaction()
That are taking care of the SREG according the interrupt mode and to be user before
SPI::transfer ()
NB:
Preliminary of course are:
  SPI.begin ()
and
  SPISettings(uint32_t clock, uint8_t bitOrder, uint8_t dataMode)

My understanding is that the SPI_HAS_INTERRUPT is not used by the RFM69 library anymore to indicate interrupt usage, but only to activate SPI functions such as:   
   _settings = SPISettings(8000000, MSBFIRST, SPI_MODE0)
   SPI::beginTransaction()
   SPI::endTransaction()
Which should not conditionally subject to the SPI_HAS_INTERRUPT because always true (at least with the Arduino SPI library) and could now be unconditionally by called.
So SPI::transfer () may be called directly, through RFM69::select() / RFM69::unselect() just managing the state of the interrupt pin (LOW/HIGH).

Having say that, removing the SPI_HAS_INTERRUPT test could be a problem while using non Arduino IDE SPI libraries such for the ESP32 / ESP8266.

I am running the RFM69 old Library (with interrupt in ISR) on AVRs but also on ESP8266 in RAM and bypassing the SPI_HAS_INTERRUPT since years now without problem.
So my next exercise  will be to test the current RFM69 Library version (with and without SPI_HAS_INTERRUPT) and see the impact on ESP8266.
Another challenge is the ESP32, but the performance and architecture of this board may requires more RFM69 library tuning (I have already RFM69 shield for for WeMOS LOLIN32, but not deeply tested).

I still have time, so to be continued..., I will let you know my findings
Anyway thanks for your reply.

Robert

[Felix]

1. older version of SPI has no definition of SPI_HAS_TRANSACTION that will be considered to false. So SPI will not work with the RFM library

I don't believe that is the case, if the SPI_HAS_TRANSACTION is not defined, then the RFM69 library will proceed with manual setting of SPI settings, instead of the new way.