Closed 2bndy5 closed 1 year ago
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...
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.
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.
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).
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++: https://github.com/InfiniTimeOrg/InfiniTime/pull/1387#issuecomment-1370932346
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?
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 concept
s without selecting a specific implementation compile-time. The important part in that case is that you can, without a clusterf**k of #ifdef
s. The Basically
section really highlights the key components.
There are lower C++ version concepts that could be used as well, like template
s.
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.
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:
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.
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++...
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.
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.
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.
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.
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.
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: https://github.com/nRF24/RF24Network/blob/aeb12d7890ded17ad21a1383b2816f376b233ebc/RF24Network.cpp#L74-L75 https://github.com/nRF24/RF24Network/blob/aeb12d7890ded17ad21a1383b2816f376b233ebc/RF24Network.cpp#L60-L61
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() { /* ... */ }
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);
I think it could be improved
Hahaha, you think?
I'll have to take some time and think about this, maybe try again.
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.
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
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.
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!
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.
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 :/
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.
Ok, I had to refactor the pyRF24Network bindings... Should be back to working order (using template class).
I just had a thought that might maintain backward compatibility in user code.
ESB_Network
.typedef ESB_Network<RF24> RF24Network;
typedef ESB_Network<nrf_to_nrf> RF52Network;
This compiles fine when used like so in the examples:
#include <SPI.h>
#include <RF24.h>
#include <nrf_to_nrf.h>
#include <RF24Network.h>
RF24 radio(7, 8); // nRF24L01(+) radio attached
nrf_to_nrf radio1; // nRF52840 radio
RF24Network network(radio); // Network that uses radio
RF52Network network1(radio1); // Network that uses radio1
If this works, it still qualifies as a major change (v2.0.0). The docs might also be a bit off-putting as all doc'd API will be referring to the renamed ESB_Network
class.
I haven't fully considered how this will impact RF24Mesh and other higher layers, but I imagine we'd want to do the same template tactic for those layers as well.
That’s pretty cool If it can also be backwards compatible! Will have to play around more with this all as soon as I get a chance. I would still be scratching my head trying to figure out basic templates.
I'm pushing the renaming changes now (had to re-work the docs a bit). Templates are something that takes practice, and this library isn't simple enough for a learning experience with templates (best to start with a dead simple exercise).
@Avamander We haven't been trying with concepts because the compiler used for the nRF52 cores doesn't have C++20 features enabled. I'm still considering alternatives since I'm pretty sure C++11 std is a common minimum. As suspected, the language standards used varies per core/board, but I found
-std=gnu++14
in the Seeeduino mbed core's cxxflags.txt-std=gnu11
in the Seeeduino mbed core's cflags.txtIf we can't rely on a minimum standard, then I think maybe the declared template specializations should adequately limit the supported types of template param values: https://github.com/nRF24/RF24Network/blob/0b4cd337bd13d5c3fec9a4958c98b78188626468/RF24Network.cpp#L1290-L1293 While this isn't robust for a multitude of radio HW abstractions, it seems feasible for our current purposes.
So with the RF24Mesh changes, everything is working great for me, but it looks like the constructor for RF24Mesh with nrf_to_nrf is a bit horrific:
ESB_Mesh <ESB_Network<nrf_to_nrf>, nrf_to_nrf> mesh(radio, network);
is what I have working...
Is there any way to shorten this up? Probably not but thought I'd ask lol. Still getting used to ESB_Network and ESB_Mesh naming conventions, but I think that is a pretty good choice, but I'd prefer it without the underscore.
When it comes all the way to RF24Ethernet, its going to get ugly.
The underscore can go. I'm not committed to the names, I just went with what first popped in my head. If feeling playful, we could even use SkyNet & SkyMesh 🤣. Were you thinking of something more like RFx\
Is there any way to shorten this up? ... When it comes all the way to RF24Ethernet, its going to get ugly.
The typedefs don't just serve as backward compatibility. I have also used the tactic to distinguish between radio type. The following should work as it currently is:
nrf_to_nrf radio;
RF52Network network(radio);
RF52Mesh mesh(radio, network);
As for shortening up the template spec used in the underlying srcs, I'm nor sure. My gut says no, but we might be able to use typedef
s instead. I have to think on it. Being familiar with templates, the nested sets of template vars make sense to me.
While we're still discussing experimental changes, I think RF24Mesh lib could actually inherit from RF24Network instead of wrapping around it.
network
and radio
private members in the mesh layernetwork.update()
exposed when mesh.update()
is only needed.network.write()
would still be accessible given that the mesh.write()
signatures don't resemble network.write()
overrides.This would definitely be a breaking change despite the typedef'd templates:
RF24 radio(7, 8);
RF24Mesh mesh(radio);
The impact of this might be too radical on the other layers though. I'm currently resigned to "don't fix what ain't broke".
I dropped the nRF52 commits to RF24Ethernet master and moved them into a separate branch (NRF52-support). You may have to do a git reset
though. The following assumes your local clone is on master branch:
cd RF24Ethernet
git reset --hard 0649526c311255ec34ee0e5666df65c5a990e9d8
git pull
I'll be pushing the template idea to RF24Ethernet in a new branch (template-support) ~shortly~.
Is there a reason that RF24EthernetClass is not named RF24Ethernet? My template approach may obsolete that but break examples which would need to be updated like so:
RF24 radio(7, 8);
RF24Network network(radio);
RF24Ethernet ethernet(radio, network);
// now use `ethernet` obj instead of an obj named `RF24Ethernet`
I can still use the RF24EthernetClass
name as a typedef, but I thought I'd ask first.
EDIT: Oh, I see RF24Ethernet
is instantiated as an extern
obj and called by friend
class' definitions. This makes the template approach much more complicated in RF24Ethernet.
Yeah, I had a quick look at RF24Ethernet yesterday, after seeing your ESBMesh and Network changes, I thought I would give RF24Ethernet a look. Didn't get very far lol.
we could even use SkyNet & SkyMesh
Hahaha, I think we would be taking our chances there. I don't want to be 'terminated'
I kind of like the ESBRadio, ESBNetwork, ESBMesh names, it more properly describes them than RF24 or NRF52 prefixes.
I thought I would give RF24Ethernet a look. Didn't get very far lol.
On the plus side, this issue may have given me a inexcusable reason to familiarize myself with RF24Ethernet. It does seem rather fragile to expect the user code to instantiate an object exactly named RF24Ethernet. Fair warning: I might start pestering you with questions on that repo.
Well if you can't figure it out, hopefully I can answer them. RF24Ethernet involved a lot of copy/paste from https://github.com/ntruchsess/arduino_uip now found at https://github.com/UIPEthernet/UIPEthernet
Thanks, that sheds a lot of light on the design decisions. For now I'll just stick to applying templates to the ethernet layer.
Concerning RF24Ethernet/ethernet_comp.h:
#define Ethernet RF24Ethernet
#define EthernetClient RF24Client
#define EthernetServer RF24Server
#define EthernetUDP RF24UDP
I foresee a problem when using these with the RF52Ethernet
specialization. We might have to discourage their usage going forward with the template approach. I see they're used in some examples which will only work when using RF24Ethernet specialization. However an app that combines both RF24Ethernet
& RF52Ethernet
will have to use more explicit references (not the above aliasing definitions).
Within a templated class' (non-static) method update()
, the extern
declared obj's tick()
should invoke RF24Ethernet.tick()
or RF52Ethernet.tick()
depending on how the extern ESBEthernetClass
was instantiated.
#ifndef RF24_TAP
template<class mesh_t, class network_t, class radio_t>
void ESBEthernetClass<mesh_t, network_t, radio_t>::update()
#else
template<class network_t, class radio_t>
void ESBEthernetClass<network_t, radio_t>::update()
#endif
{
Ethernet.tick(); // !!! can only resolve to RF24Ethernet.tick(); because
// of the static definitions in RF24Ethernet/ethernet_comp.h
}
Using the this
pointer isn't an option because ESBEthernetClass::tick()
is a static function.
I honestly don't see a way to maintain the current API structure of RF24Ethernet while applying the template approach because of all the static members. I think a custom fork/branch is needed for use with nrf_to_nrf and other radio HW abstractions. See also this SO answer about calling static members from a templated member.
TBH, my first impression is that RF24EthernetClass had a flawed design because the static tick()
function calls on externally declared objects (trusting that the user code follows a precise convention). After seeing how UIPEthernet is designed, I'm not sure what a better (template-friendly) design would be.
I guess this isn't that big of a deal. RF24Ethernet isn't nearly as popular as the lower layers of the stack, and I don't see many users wanting to run two radios at a time with RF24Ethernet, so we could probably skip the templating of RF24Ethernet and just stick with defines. That is unless you are wanting to re-design it, I think that is a little bit out of the scope of what I can take on right now, and I don't expect you to do everything yourself. It is fun watching you design code though, its like watching a skilled artist doing his thing, and getting to play around with the results.
Ok, I think I have a good basic handle on how templates work now, thanks to all your examples with code I already understood. Its so much easier to follow a working example than random examples on the internet. I've tested what I can and think we can merge this stuff as soon as you think its ready @2bndy5 .
I'm ready to move on this also. I'm only cautious because this would mean releasing a v2.0.0 tag. And I think there should be a brief tutorial page somewhere (probably in RF24Network) about migrating to v2. I say brief because it shouldn't be a major impediment, but there are caveats:
Any third party libs that extend the network/mesh layer may also need to be updated to incorporate the new templated class prototypes.
template<class radio_t>
class ESBNetwork;
template<class network_t, class radio_t>
class ESBMesh;
The entire template tactic might also need be applied to third party libs, which as we've seen in RF24Ethernet may be a non-starter.
I haven't played around with this much, but I think that third party libs can use the backward-compatible typedef in their template spec if they use typename
instead of class
as the template arg's datatype:
template<typename network_t, typename mesh_t>
class ESBGateway
And also inform the compiler what types they intend to support
template class ESBGateway<RF24Network, RF24Mesh>;
Yeah, I'm good with all that.
And I think there should be a brief tutorial page somewhere
Probably on the main docs page and in the readme on GitHub, in both RF24Mesh and Network, its hard to get people to read things sometimes. This would be temporary, and eventually we could move it to its own page. This is probably something I can handle, I would just need a bit of time to put it together.
The .github/docs uses jekyl to render MD files as a webpage. I'm not sure how handy it is for multiple pages given the current setup (which scrambled together rather clumsily).
Please also note that the auto-installers do not do a version check like most other pkg managers.
I just had a thought: We should keep a separate branch for v1.x in case we need to backport updates. This would be helpful to those that can't/won't migrate to v2, but it would mostly benefit PlatformIO users. I'm not sure if we can retro-release tags for Arduino IDE. I think we can if needed, but we'd be better to do a retro-release (v1.x) before a modern release (v2.x) that way the "latest" release is of v2.x variety.
Great idea, if you would take care of the releases since you did most of the work I think we are ready for 1.x releases all around.
OK, docs are in the template branches if you want to review. I have RF24Ethernet changes for Mesh/Network v2.0 ready to go as well, but I will PR once the other v2.0 changes are in place.
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).
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.
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.
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 annrf_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
typedef
s for compatible RF24-API objects, but my expertise in C is lacking. So, I'm not sure if this idea is plausible at all.