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

[Request] allow non-default SPI bus by overloading begin() #743

Closed
2bndy5 opened this issue Mar 15, 2021 · 8 comments · Fixed by #750
Closed

[Request] allow non-default SPI bus by overloading begin() #743

2bndy5 opened this issue Mar 15, 2021 · 8 comments · Fixed by #750

Comments

@2bndy5
Copy link
Member

2bndy5 commented Mar 15, 2021

Overload begin() for the Arduino platform only

Using a secondary (non-default) SPI bus with the RF24 lib has been a frequently requested feature lately. Demand for this will likely only increase with newer MCUs (that expose more than 1 SPI bus to the GPIO pins). Again this will only apply to the Arduino platform since the Linux platform already offers multiple SPI bus options via the SPIDEV or BCM283x drivers.

Solution

  1. Convert the variable of type _SPI into a pointer, and set it to the default SPI object that the lib currently uses.
// in RF24.h
private:
    _SPI* _spi;
// in RF24.cpp
void RF24::RF24(...)
{
    _spi = &SPI;
    // repeat the same for SoftSPI and SPI_UART using #ifdef soup
}
  1. overload RF24::begin() with
bool RF24::begin(_SPI* spiBus)
{
    _spi = spibus;
    return begin();
}
  1. convert all calls using _SPI.func_name() to _spi->func_name(). I inadvertently made this easier by consolidating the SPI bus access functions back in new examples and more #691

Alternative Ideas

I can't really think of any alternative implementations that wouldn't be over-complicated. The solution above is a pretty standard way that many libraries use.

I don't want to use the hack proposed in #722 for compatibility with the existing alternative SPI implementations (see below in "Additional context").

Additional context

I want to maintain compatibility with the existing alternative SPI implementations, namely SoftSPI and SPI_UART. The real challenge will be sifting through the #ifdef soup and only changing the _SPI when !defined(RF24_LINUX) && defined(RF24_SPI_TRANSACTIONS) (or something similar) 😰.

I also want to satisfy #539 in a similar overloaded begin(ce_pin, csn_pin), but that's still on the back burner (for now).

@TMRh20
Copy link
Member

TMRh20 commented Mar 15, 2021

Nice, can we make it default to a specific SPI bus?
Something like:

bool RF24::begin(_SPI* spiBus = nullptr);

bool RF24::begin(_SPI* spiBus){
  if(spiBus != nullptr){
    _spi = spibus;
  }else{
    somethingElse...
  }
}

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 15, 2021

I should test it, but that idea might make the overloaded begin() ambiguous (to the compiler) compared to the original begin(void).

I just was going for something most compatible with the existing code.

UPDATE: As I suspected, gettingStarted.ino doesn't compile when the spiBus param defaults to nullptr. I suppose it would be more appropriate to overload the c'tor (for arduino only) to take a spiBus param. But I'm not sure how that idea is anymore useful than what I proposed in the OP.

Its worth noting that the SPI access now looks like the following for when RF24_LINUX is not defined.

    #else // !defined(RF24_LINUX)

    beginTransaction();
        #if defined (XMEGA_D3)
    // using _SPI as an object
    status = _SPI.transfer(R_RX_PAYLOAD);
    while (data_len--) { *current++ = _SPI.transfer(0xFF); }
    while (blank_len--) { _SPI.transfer(0xff); }

        #else // !defined(XMEGA_D3)
    // using _SPI as an datatype pointer
    status = _spi->transfer(R_RX_PAYLOAD);
    while (data_len--) { *current++ = _spi->transfer(0xFF); }
    while (blank_len--) { _spi->transfer(0xff); }

        #endif // !defined(XMEGA_D3)
    endTransaction();

    #endif // !defined(RF24_LINUX)

and begin/endTransaction() now look something like

inline void RF24::endTransaction()
{
    csn(HIGH);
    #if defined(RF24_SPI_TRANSACTIONS)
        #if defined(RF24_RPi)
    _SPI.endTransaction();
        #else // !defined(RF24_RPi)
    _spi->endTransaction();
        #endif // !defined(RF24_RPi)
    #endif // defined (RF24_SPI_TRANSACTIONS)
}

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 15, 2021

There are still some things that I really need to discuss:

  1. the python wrapper actually isn't applicable to this feature since SPIDEV and BCM283x drivers already offer multiple SPI bus options.
  2. The XMEGA_D3 macro seems to be for an AVR type board, but I'm not sure what exactly (the docs don't help me there). Does the supported board(s) offer multiple exposed HW driven SPI buses?
  3. RF24_arch_config.h (at root dir) specifies extern HardwareSPI SPI; when ARDUINO, __avr__, and __ARDUINO_X86__ are not defined. What platform is this aimed at? (Arduino Due?) I see git blames this commit from @TMRh20 that introduces ATTiny support based on work by @jscrane.
    • ATTiny platform might not be applicable to this feature request due to limited pins and the USPI implementation in the ATTinyCore by SpenceKonde.

p.s. I had to define a macro called DOXYGEN_FORCED using the Doxyfile's PREDEFINED tag, so the overloaded begin() is properly documented.

@TMRh20
Copy link
Member

TMRh20 commented Mar 16, 2021

  1. https://www.microchip.com/wwwproducts/en/ATxmega64d3
  2. No platform, that is from Maniacbug days for some unsupported devices I believe.

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 16, 2021

@TMRh20 thanks for this vital info

About 2: Looks like the ATxmega64D3 has 5 SPI buses to expose, but I'm still unsure if boards using this chip actual expose more than 1 SPI bus to the broken-out pins. Probably best to extend this feature to the ATxmega64D3 for completeness (and it would simplify the code I sampled above).

About 3: I think if I just add #define _SPI HardwareSPI after the instantiated singleton SPI is declared, that would encompass all platforms (and alternate SPI implementations) not specific to Linux with this feature.

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 16, 2021

Looking further into esp32 and esp8266, I'm finding it may be necessary to allow the user to call spi_bus.begin(<custom pin args>) before calling radio.begin(&spi_bus). This means I might have to segregate the soft reset algorithm from the original begin() (into a new private function called _init_radio()), so both RF24::begin() methods can return a bool from _init_radio().

This might also support some bitbanging SPI implementations that would be specific to certain Arduino cores (as opposed to the limited SoftSPI implementation from the frozen DigitalIO lib).

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 19, 2021

I think my overload-begin branch is ready for testing. I've modified support for Due, ATxmega64D3, Teensy, and all AVR (including SoftSPI implementation). Platform support that hasn't been modified includes anything Linux, ATTiny, LittleWire, and the SPI_UART implementation.

SPI object's begin(...) must be called prior to radio.begin(&spi_obj)

Additional documentation is pending (it may need example snippets for different Arduino cores). I plan on modifying the Arduino support page (and just link to Arduino page from new begin(_SPI*) doc)

@2bndy5
Copy link
Member Author

2bndy5 commented Mar 20, 2021

PlatformIO build
Look, its a new badge 😍 !

I had to add a new CI to test the teensy platform against the RF24 examples using PlatformIO because the TeesnyCore for the Arduino IDE isn't CLI friendly... It seems to complete faster than the Arduino CI we're already using, but that may change if I add more boards like the STM32 family.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants