enhancing radio datatype for non-nRF24 HW

[2bndy5]

This is related to using nRF5x HW under the guise of RF24 API. Currently, the approach to use TMRh20's nrf_to_nrf lib is to modify the network layers c'tor. But, this leaves users no option to use the internal nRF5x radio for BLE and an external nRF24 for ESB.

My first impression was to overload the network layers' c'tors for nRF5x boards. This way the original behavior still exists, and the new behavior (to support nRF5x radios) is optionally available on the right HW. However, the problem is that the radio datatype (RF24) does not properly describe an nrf_to_nrf object. Remember, RF24 class is the base class for the API. If one inherits from the RF24 class, then they get otherwise unnecessary functionality (private/protected methods like SPI transactions).

How do we properly expand HW radio support?

My first guess is to have RF24 class inherit from a base class that declares the abstract public API, namely the functionality required to port the RF24 API to new HW radios. But this might be a breaking change as changing the network/mesh c'tor to use a abstracted datatype for the radio will break older installed versions of RF24* network layers. I almost opened this on RF24 repo because the abstracted API class would likely live in the RF24 lib. This approach feels similar to what the RadioHead lib had done.

I also had a thought about declaring typedefs for compatible RF24-API objects, but my expertise in C is lacking. So, I'm not sure if this idea is plausible at all.

[2bndy5]

If we go with an abstracted API class, then we could eventually incorporate support for Texas Instruments' chips that can replicate the ESB protocol. That's not a priority though.

This might be the beginning of v2.x for the RF24 stack...

[Avamander]

In theory modern C++ provides plenty of functionality to implement different back-ends to provide the same "interface" so to say.

This would also in theory allow clone/semi-clone support.

[2bndy5]

In theory modern C++ provides plenty of functionality to implement different back-ends to provide the same "interface" so to say.

Do you have alternate suggestions? I'm looking for something that is the most backward compatible, but I keep landing on polymorphism.

This would also in theory allow clone/semi-clone support.

Absolutely! It would be better to have a separate implementation lib for the RFM7x chips because the setup operations are so much different from the nRF24L01.

[2bndy5]

I hesitate to say this because I haven't researched it. An abstracted API (+ pipe count config) might even allow porting the network layers to HW radios that use a different spectrum entirely (ie RFM69).

[Avamander]

Do you have alternate suggestions? I'm looking for something that is the most backward compatible, but I keep landing on polymorphism.

Though I'm not entirely sure about Arduino IDE and its latest C++ version, but the InfiniTime project is probably a good example how hardware abstraction can be done with newer C++: InfiniTimeOrg/InfiniTime#1387 (comment)

[2bndy5]

Interesting. I think you'll find that different Arduino cores use (or are pinned to) specific versions of compilers. I'm not sure we can guarantee C++20 compatibility in Arduino. There doesn't seem to be a way for an Arduino lib to specify compiler args like -std=c++20.

The selection at build time of the actual interface is done in header files

This has worked well for us in the past for a SPI bus. This doesn't seem geared to building with more than 1 type of HW radio driver. What if you wanted to broadcast 1 network only on channel 90 with 1 radio, and another network only on channel 97 with a different radio?

[Avamander]

This doesn't seem geared to building with more than 1 type of HW radio driver. What if you wanted to broadcast 1 network only on channel 90 with 1 radio, and another network only on channel 97 with a different radio?

Fully optional really, you can utilize concepts without selecting a specific implementation compile-time. The important part in that case is that you can, without a clusterf**k of #ifdefs. The Basically section really highlights the key components.

There are lower C++ version concepts that could be used as well, like templates.

[2bndy5]

No ifdef soup is appealing. I think the deciding factor may be size impact. Since I can't accurately predict how that will go, we may have to prototype the ideas on seperate branches and open dummy PRs to get the report from the Arduino CI. That would also surely highlight differences between toolchains used by different Arduino cores.

[TMRh20]

So I've been playing around with templates, trying to find a solution to this, and it looks kind of ugly. I'm starting to thing that a bit of ifdef soup might be appealing at this point.
Main issues so far with templates:

1. Still need to use a bunch of defines to specify the constructor and radio objects.
2. Needs lots of additional code to implement
3. Requires changes to any libraries that use the templated library

I don't pretend to fully know what I'm doing here, I'm just learning about templates, but it seems like it creates more problems than its worth.

See the templateSupport branches of RF24Network and RF24Mesh for some incomplete but functional template code with Arduino Pro Mini and NRF52 devices.

[2bndy5]

To be fair, C++ concepts are a kind of template that shouldn't require much change in higher layer abstractions. I've never played with these because I learned C++ before the C++20 std was introduced.

Inheritance might be "cleaner" as for code clutter. My main concern is increasing compile size. It has been a while since I tried polymorphism in C++...

My initial idea was something like this

// in RF24_ABS_API.h (located in RF24 repo)

class RF24_API
{
  virtual bool begin() = 0;
};

// in RF24.h
#include "RF24_ABS_API.h"

class RF24 : public RF24_API
{
  override bool begin();
};

// in nrf_to_nrf.h
#include "RF24_ABS_API.h"

class nrf_to_nrf : public RF24_API
{
  override bool begin();
};

// in RF24Network.h
#include "RF24_ABS_API.h"

class RF24Network
{
  RF24Network(RF24_API* radio, ...);
};

[TMRh20]

I'm still seeing some of the same issues with that approach, things largely still need to be defined, and when switching from RF24 to nrf_to_nrf on the NRF52, I still need to manually add a #define the way its set up right now.

I've got the overrides working for begin() with slightly modified syntax (bool begin(void) override;), but the RF24Network(RF24_API* radio, ...); line is causing problems with the constructor, not being able to convert nrf_to_nrf or RF24 type to RF24_API type. Could be because I'm just learning about overrides now and don't really know how to implement this lol.

[2bndy5]

You might have to type cast the nrf_to_nrf obj (or the RF24 obj) when passing it to the c'tor:

RF24Network network((RF24_API*)&radio);

But this might have consequences. Typically, you'd type cast a derivative (AKA "downcast") to access the base class' API. I'm hoping that the pure virtual methods defined in RF24_API (like bool begin() = 0) would forward back to their derived overrides. Again, it's been a while since I played with polymorphism.

No matter what we do here, it would require a breaking change throughout the RF24 stack in terms of instantiating the objects that use the radio HW.

[TMRh20]

You might have to type cast the nrf_to_nrf obj

Err, that just gives a different variation of the same error.

No matter what we do here, it would require a breaking change throughout the RF24 stack

Not if we just use #defines

I'm starting to think that either way it will be a compromise on how we would ideally like it to work.

[2bndy5]

If use defines then it limits the type of radio HW used in a single application, right? I was hoping there was a way to allow various/multiple radio HW types in the same app (eg 1 nRF52 network on channel X and another nRF24 network on channel Y). I don't really have a problem with the ifdef soup, but I was hoping for a more versatile solution.

[TMRh20]

If use defines then it limits the type of radio HW used in a single application, right?

Yeah, but same with templates as far as I can figure.

One thing I found, even with templates, if you have two objects RF24& radio and nrf_to_nrf& radio, you need to initialize them both in a single constructor. This negates the ability to have separate calls to RF24Network<RF24> network( radio); and RF24Network<nrf_to_nrf> network1(radio1); it would have to be something like RF24Network<nrf_to_nrf> network(radio,radio1);

I've struggled over a day of coding trying to make it work with templates and think I've given up. Learned a lot about templates though lol.

[2bndy5]

I don't really follow. You should be able to have separate instantiated objects based on the template param. I just starting to look at what you did in the template-support branch, and I think it could be improved... In what you pushed, you're not really using the template param:

RF24Network/RF24Network.cpp

Lines 74 to 75 in aeb12d7

	template <>
	RF24Network<RF24>::RF24Network(RF24& _radio) : radio(_radio), next_frame(frame_queue)

RF24Network/RF24Network.cpp

Lines 60 to 61 in aeb12d7

	template <>
	RF24Network<nrf_to_nrf>::RF24Network(nrf_to_nrf& _radio) : radio(_radio), next_frame(frame_queue)

Typically it would be more like:

template<typename RF24_like>
RF24Network<RF24_like>::RF24Network(RF24_like& _radio) : radio(_radio), next_frame(frame_queue)

Where the c'tor would be declared in the header file like so:

template<typename RF24_like>
class RF24Network
{
    RF24Network(RF24_like& radio);
    // ...
    RF24_like& radio;
};

And, I think each function definition for the RF24Network class would also need the added template<typename RF24_like> prefixed in the implementation file (RF24Network.cpp)

template<typename RF24_like>
RF24Network<RF24_like>::begin() { /* ... */ }

[2bndy5]

Should also be able to set the default value for the RF24_like param to RF24.

template<typename RF24_like = RF24>
class RF24Network
{
    RF24Network(RF24_like& radio);
    // ...
    RF24_like& radio;
};

But, depending on the C++ std used (specifically C++17), a user could instantiate with a blank set of template args:

RF24 radio(7, 8);
RF24Network<> network(radio);

// with C++17 or later
RF24Network network(radio);

// using non-default template param value
nrf_to_nrf radio1;
RF24Network<nrf_to_nrf> network(radio1);

[TMRh20]

I think it could be improved

Hahaha, you think?

I'll have to take some time and think about this, maybe try again.

[2bndy5]

I'm playing with it now (locally). To avoid merge conflicts, I'll post a fair warning if I end up pushing changes to that branch -- or maybe I'll start my own branch.

[2bndy5]

Ok, I got it to compile with proper use of template args, but the Adafruit nRF52 Arduino core doesn't use C++17 or newer, so users will have to specify a blank set of template args.

I haven't actually got the HW setup to test this, but the modified helloworld_tx example compiles fine (disregarding warnings about nrf_to_nrf src code).

#include <SPI.h>
#include <RF24.h>
#include <nrf_to_nrf.h>
#include <RF24Network.h>

RF24 radio(7, 8);  // nRF24L01(+) radio attached using Getting Started board
nrf_to_nrf radio1;  // nRF24L01(+) radio attached using Getting Started board

RF24Network<> network(radio);  // Network uses that radio
RF24Network<nrf_to_nrf> network1(radio1);  // Network uses that radio

I also hit a snag in which the compiler wasn't aware of the possible template arg values (even with a default value set), but this SO answer helped satisfy the "undefined reference" errors. Basically, I had to add the following to the end of the RF24Network.cpp file:

// ensure the compiler is aware of the possible datatype for the template class
template class RF24Network<RF24>;
#ifdef ARDUINO_ARCH_NRF52
template class RF24Network<nrf_to_nrf>;
#endif

[2bndy5]

My attempt is on template-attempt2 branch. The Linux CI also indicates that I need to update the python wrapper since I adjusted the definitions for Linux as well.

I'm not sure if it is worth it since this templating tactic could be used to support other external radio HW on Linux. But, I think we can typedef (& macro sub) the template stuff out

class RF24;
#if defined RF24_LINUX
typedef ESB_RADIO RF24;
    #define ESB_RADIO_TEMPLATE_SPEC
#else
    #define ESB_RADIO_TEMPLATE_SPEC template<class ESB_RADIO>
#endif

And just use the macro ESB_RADIO_TEMPLATE_SPEC instead of the template<class ESB_RADIO> decl.

[TMRh20]

Hmm, this ain't so different from what I was trying to do, except that it works! The only problem I ran into so far is that it gives linker errors when I try to use nrf_to_nrf on my XIAO BLE Sense 52840 with the MBED core The other core works fine as does the Feather 52840. Tried changing the defines to accommodate and no luck.

This is a big step towards getting this issue sorted out!

[2bndy5]

it gives linker errors when I try to use nrf_to_nrf on my XIAO BLE Sense 52840 with the MBED core

The Seeduino MBed core uses a slightly more specific -D arg for the chip:

-DARDUINO_ARCH_NRF52840 -MMD -mcpu=cortex-m4 -mfloat-abi=softfp -mfpu=fpv4-sp-d16 -DARDUINO=10607 -DARDUINO_SEEED_XIAO_NRF52840 -DARDUINO_ARCH_MBED -DARDUINO_ARCH_MBED -DARDUINO_LIBRARY_DISCOVERY_PHASE=0

Nonetheless, it is fixed by adding to the #if defined condition:

-#ifdef ARDUINO_ARCH_NRF52
+#if defined(ARDUINO_ARCH_NRF52) || defined(ARDUINO_ARCH_NRF52840)

BTW, the nrf_to_nrf lib is flagged as incompatible with the mbed core because of the arch specified in the library.properties:

WARNING: library nrf_to_nrf claims to run on nordicnrf52, nrf52 architecture(s) and may be incompatible with your current board which runs on mbed architecture(s).

But, I don't see a adequate solution unless mbednrf52 is acceptable. I think the core blandly specifies mbed as its arch. This is probably due to the fact that all mbed arduino cores use the same repo as a foundation.

[TMRh20]

Nonetheless, it is fixed by adding to the #if defined condition:

Could have sworn I tried that, but must have typed it wrong or something :/

[2bndy5]

I still don't know what to do about Linux. It would seem an ideal candidate for non-RF24L01 HW support, but exposing a specialized template class in boost.python is not a trivial change. So far, my efforts to update the individual wrapper have failed to compile.

[2bndy5]

FYI, the cmake-based auto-installer has the ability to select a branch for each layer. But this functionality was never exposed to the CLI; it was actually used for testing/developing. I'm guessing it would be a good idea to ask the user for a branch name (defaulting to master) before it installs the lib. Instead of a branch name, I think it could also use a git tag, but I think the installer has to do a git fetch --all to checkout a tagged commit.

The cmake-based installer will only work for versions that have CMake support. If we try to fix that the install script would have to become a lot longer, so it could fallback to the old build/install system (if no root CMakeLists.txt file is present).

[TMRh20]

Lets not get too complicated here, I don't think we need to worry too much about older versions, as long as the newer versions have CMake support, we should be OK.

Also I just checked Platform IO registry and RF24 was still at v1.4.5 for some reason so I pushed the current code as 1.4.6. The other libs are up-to-date. Just something we need to check on after our next releases.

[2bndy5]

That's odd. It is supposed to get deployed to PIO registry in the PIO CI when a release is published. I'll have to investigate...

I'm reviewing the cmake installer script, and it was written well enough to not have a significant re-write for the old build system. I'm currently playing around with this idea now. I'm not sure how we could have the old installer forward to the new one though.

[2bndy5]

The installer script mods are at the .github repo's install-versions branch. It doesn't seem to work with WSL using RF24-v1.3.11 and RF24Network-v1.0.15 (linker ignores arm-specific .so file for building RF24Network), but I pushed the branch to test it on an actual Linux (32bit) device.

[2bndy5]

I just checked Platform IO registry and RF24 was still at v1.4.5 for some reason

The logs for that CI run are too old (github removes logs older than 90 days). So, I can't see if it tried deploy to PIO. Given that the CI published the pkg to v1.4.6 release's assets, I think the problem may have been in the CI step's conditional trigger/skip.

Since I updated all the CI to be re-usable workflows (post v1.4.6), I think I still need to add the deploy-release arg to the validate_lib_json step (for all RF24* repos):

  validate_lib_json:
    uses: nRF24/.github/.github/workflows/validate_deploy_platformio.yaml@main
    secrets: inherit
    with:
      deploy-release: ${{ github.event_name == 'release' }}

[2bndy5]

CI updated org-wide. 🤞🏼 I also rebased the relevant template-* branches to avoid merge conflicts.

[2bndy5]

Still having trouble getting RF24Mesh's PIO CI to install the right branch of RF24Network

Run pio pkg install -g --skip-dependencies -l 'https://github.com/nRF24/RF24Network.git#template-attempt2'
Library Manager: RF24Network@1.0.17+sha.05e7f46 is already installed

But that is the wrong SHA for RF24Network's template-attempt2 branch... It should be RF24Network@1.0.17+sha.ecba37c🤷🏼‍♂️. Hopefully this will be resolved as the template branches are merged to master (in proper order).

[TMRh20]

So are we ready to do this thing? You want me to do the 1.x releases?

[2bndy5]

Sorry I've been dragging my feet no real reason here. Yeah, we're ready for release crusade.

I didn't see anything noteable in the in the net work layer histories since our last crusade. RF24 lib got some significant updates in the PicoSDK support, but that has been in master for a while and I suspect that people are using master instead of a tagged commit with the PicoSDK because our PicoSDK instructions don't explicitly say to use tags.

[2bndy5]

@TMRh20 If you want to do the releases (should be easy with that auto-generate summary button), then I'll open the PRs and start the v1.x branches.

I almost forgot about the version bump in the lib.properties/json files.

[2bndy5]

Is it ok to remove the initial NRF52Support and templateSupport branches that you started?

[TMRh20]

I can do the version bump with the releases, you should be able to remove except for RF24Ethernet branch

[2bndy5]

RF24Gateway has only had CI changes since last release. It might be a good idea to make sure the CI deploys to PIO for RF24 v1.4.7 before continuing to publish releases for other layers because they'll all be using the same PIO validate/deploy workflow.

[2bndy5]

It deployed to PIO! I verified with the CI logs, then I got the email. 🎉

UPDATE: The network, mesh, and ethernet libs have also been accurately deployed to PIO.

[2bndy5]

v1.x branches created

I created a branch rule for each of the new v1.x branches on the network, mesh, and gateway layer. This way the git history should be respected the same as it is for the master branches.

v2.0 PRs open

I left some open as drafts to indicate that they aren't ready to merge unless the dependency layer has been merged. This is done to remind me to clean up the CI changes I used for testing.

[2bndy5]

I know this isn't a goal, but if there are any breaking API changes you want make (like renaming functions with underscores to not have underscores), then this is the chance to do so. I mention this because semantically, it makes sense to remove deprecated functions (or rename functions) in a major version bump.

On the topic of deprecated functions:
RF24Network::begin() has a deprecated overloaded function that techincally would be removed on a major version bump. However, I doubt there would be enough noticable change in compile size to deem it ok to remove.

[2bndy5]

So sorry. I'm going to have to step away again... life and whatnot. The PRs look good at a glance, I found some areas where the docstring mods should more explicitly reference the nrf_to_nrf lib. And the usual CI reverts before merging...

[TMRh20]

No rush, the 1.x release is in successfully, so whenever you get a chance for 2.0 its all good.

[2bndy5]

Ok. Its all merged. We can do a v2.0 release crusade whenever you want.

I also re-released RF24Mesh to make sure installing v1.1.10 or newer doesn't install RF24Network v2.x in PlatformIO. Older versions of RF24Mesh v1 will likely cause an error when used with RF24Network v2.x because there was nothing in RF24Mesh v1.1.9 or previous stopping PIO from using the latest release of RF24Network.

I don't really know if Arduino IDE has version checking for lib deps (see lib specs), but v2 is backward compatible enough for users to migrate. The PIO dependencies were a real pain in the a$$.

[TMRh20]

We can do a v2.0 release crusade whenever you want.

Done!