Added compatibility for RFM7X and bk242x-style nRF24 clones including… #754

microtronics · 2021-03-26T14:23:41Z

… example

2bndy5 · 2021-03-26T16:02:56Z

This looks like a copy and paste job. You should be more mindful of putting other people's names on your work. That lib you copied from uses MIT license, but this lib uses GNU license (there's a subtle difference), so an MIT license might be required to be added.

Are you willing to keep maintaining this extra support? Because this is probably going to require your help to troubleshoot in the future. We don't own any of these modules to verify this work.

microtronics · 2021-03-26T16:34:01Z

The important part are the changes to RF24.cpp. The MIT license is the most open license and allows practically everything. Plus I referenced this code in both the example and in the RF24_config. The example code only contains the properly configured library and the calls to it.

Since I have personal interest in this feature, I can maintain it. The important changes are rather small though (hard to find nonetheless). @Avamander stated in an old Issue in RF24Mesh that he liked support for these modules

2bndy5 · 2021-03-26T20:27:35Z

Since I have personal interest in this feature, I can maintain it

That's good news. I'm just trying to avoid disappointing someone that might need help. @Avamander would never turn down support for more devices. Ever.

I will leave a review when I get the time to look through it all.

microtronics · 2021-03-26T22:03:54Z

Great! I'll wait until this is merged and then add the changes to RF24Network as well

2bndy5

You still have a lot of work to do here. Remember that the RF24 library supports all/various Arduino platforms (not just ARDUINO_ARCH_AVR) and Linux platforms.

So Far, this PR doesn't add support for the RFM7x and bk252x radio modules. Rather this PR simply patches the config file for compatibility with a dependent library that isn't able to install in the Arduino IDE (nor PlatformIO); the other library needs to exist in the sketch folder like you've done in the new example here.

2bndy5 · 2021-03-26T21:08:09Z

RF24_config.h

@@ -25,6 +25,10 @@
 //#define SPI_UART    // Requires library from https://github.com/TMRh20/Sketches/tree/master/SPI_UART
 //#define SOFTSPI     // Requires library from https://github.com/greiman/DigitalIO

+//This introduces some changes to make the library work with RFM73/75 and other bk242x derivates. See examples/rfm7xAndBk242xCompatiblity
+//Those modules require additional initialization which can be done with https://github.com/jnk0le/RFM7x-lib
+//#define RF24_COMPATIBILITY_MODE_FOR_RFM7X_BK242X


I would prefer to use RF24_FOR_RFM7X_BK242X as the macro name. I know some would prefer wordiness, but think of the beginners who would have to memorize and declare this in their project.

2bndy5 · 2021-03-26T21:14:09Z

RF24_config.h

@@ -25,6 +25,10 @@
 //#define SPI_UART    // Requires library from https://github.com/TMRh20/Sketches/tree/master/SPI_UART
 //#define SOFTSPI     // Requires library from https://github.com/greiman/DigitalIO

+//This introduces some changes to make the library work with RFM73/75 and other bk242x derivates. See examples/rfm7xAndBk242xCompatiblity
+//Those modules require additional initialization which can be done with https://github.com/jnk0le/RFM7x-lib


If this PR still requires the RFM7x lib then it should be listed as a dependency, but RFM7X is not able to install in the Arduino IDE (nor its Library Manager).

If the RFM7X lib is just a reference to originating work, then this needs to be re-worded to avoid confusion.

2bndy5 · 2021-03-26T21:53:41Z

examples/rfm7xAndBk242xCompatiblity/rfm7xAndBk242xCompatiblity.ino

+#include "RF24.h"
+
+// See https://github.com/jnk0le/RFM7x-lib
+#include "rfm7x.h"


This include should be automatically done in RF24_config.h using #if defined (RF24_FOR_RFM7X_BK252X.

The rest of this example is identical to GettingStarted.ino (excluding my comment about the init code in setup()). Any radio module that's supported should not be limited to 1 example. Have you tested the other examples to see if they're all as compatible as this is?

I re-wrote and simplified all the examples to avoid having too many examples to maintain. If we change anything to GettingStarted, then we'd also have to go into this example and make the same changes. Copy-n-Paste skills or no, our time is worth more than that. Allowing this example is asking for other contributors to submit their own circumstantial code examples (its a snowball effect).

2bndy5 · 2021-03-26T21:59:19Z

examples/rfm7xAndBk242xCompatiblity/rfm7x_config.h

@@ -0,0 +1,499 @@
+#ifndef RFM7X_CONFIG_H_


No license nor authorship. Also this file won't be available in the docs (we use Doxygen to generate the docs), this missing comment block also needs a @file command.

Much of the rest of this file isn't necessary if the libraries are combined properly.

2bndy5 · 2021-03-26T22:01:52Z

examples/rfm7xAndBk242xCompatiblity/rfm7xAndBk242xCompatiblity.ino

+  rfm7x_io_init();
+  spi_init();
+  if (rfm7x_is_present()) {
+    Serial.println("RFM7X connected, initializing");
+    //this code initializes the required registers for the clones. Especially bank1
+    rfm7x_init();
+    delay(2);
+    rfm7x_toggle_reg4();
+    delay(1);
+  }


This code should reside in RF24::begin(). Since its use is conditional to the RF24_FOR_RFM7X_BK242X macro, the code should be encapsulated in
#if defined (RF24_FOR_RFM7X_BK242X) if (rfm7x_is_present()) { IF_SERIAL_DEBUG(printf_P(PSTR("RFM7X connected, initializing\r\n"))); //this code initializes the required registers for the clones. Especially bank1 rfm7x_init(); delay(2); rfm7x_toggle_reg4(); delay(1); } #else // !defined (RF24_FOR_RFM7X_BK242X)

rfm7x_io_init() and spi_init() should also be consolidated into RF24::begin(). Please try to find a way that these libraries could be better combined. Otherwise, I suggest you simply submit a link (that we could add to the docs homepage) to a (your) personal repo that demonstrates using both libraries independently.

2bndy5 · 2021-03-26T22:06:28Z

examples/rfm7xAndBk242xCompatiblity/rfm7x_hardware.h

@@ -0,0 +1,65 @@
+#ifndef RFM7X_HARDWARE_H_


No license nor authorship. Also this file won't be available in the docs (we use Doxygen to generate the docs), this missing comment block also needs a @file command.

Much of the rest of this file isn't necessary if the libraries are combined properly.

2bndy5 · 2021-03-26T22:09:26Z

examples/rfm7xAndBk242xCompatiblity/rfm7x.cpp

+//Compiler          : GCC
+//Author            : [email protected]
+//                    https://github.com/jnk0le
+//License           : MIT


This is why we need a copy of the MIT License. I quote directly from the license aggreement:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

2bndy5 · 2021-03-26T22:10:01Z

examples/rfm7xAndBk242xCompatiblity/rfm7x.cpp

+//**************************************************************
+//Compiler          : GCC
+//Author            : [email protected]
+//                    https://github.com/jnk0le


Did you adapt this or are you deflecting ownership?

2bndy5 · 2021-03-26T22:11:39Z

examples/rfm7xAndBk242xCompatiblity/rfm7x.h

+
+/************************************************************************************
+    Author: [email protected]
+    https://github.com/jnk0le


Did you adapt this or are you deflecting ownership?

2bndy5 · 2021-03-26T22:11:47Z

examples/rfm7xAndBk242xCompatiblity/rfm7x.h

+/************************************************************************************
+    Author: [email protected]
+    https://github.com/jnk0le
+    This library is distributed under MIT license terms


No it is not; not in the current state of this PR

microtronics · 2021-03-26T22:34:09Z

Sorry but thats nonsense... The fix for compatibility is done in RF24.cpp as I already stated. https://github.com/jnk0le/RFM7x-lib/blob/master/examples/Arduino%20workaround/RF24.ino btw also references to RF24 as an example. The initialization procedure that is additionally required could be added to RF24, but I don't see the point in doing so. We can leave out the whole example code and just use the fix in RF24.cpp...

2bndy5 · 2021-03-26T22:49:44Z

from the comment you linked:

https://github.com/maniacbug/RF24

This library is based on ManiacBug's work, but his library only supported Arduino platforms (nor did it support PlatformIO) .

We can leave out the whole example code and just use the fix in RF24.cpp...

That would be more acceptable if you don't want to distill the necessary parts from RFM7x lib into the RF24 class. But to be clear, that doesn't provide automatic support for RFM7x/bk252x radio modules, rather it allows the use of the RFM7x lib as a local-only dependency to provide support for those radios.

microtronics · 2021-03-26T22:54:00Z

I think that's the best solution and when somebody want's to enable support they find the info that additional initialization is required with a link to RFM7x-lib.

And as I stated, I don't have any issues regarding the ACK/noACK-bit that jnk0le mentions...

2bndy5 · 2021-03-26T22:57:20Z

The docs could use some explanation how to use the RFM7x lib as a dependency. That way we have a reference to link to instead C-n-P the info on a per-issue/question basis.

As for RF24Network, please tag this PR when you're ready

2bndy5 · 2021-03-26T23:20:10Z

I don't have any issues regarding the ACK/noACK-bit that jnk0le mentions...

Its possible that the RFM7x lib disables the auto-ack feature to avoid that problem. Which is why so many uninformed people suggest disabling auto-ack when troubleshooting for possible clones/counterfeits.

2bndy5 · 2021-03-27T03:36:56Z

@microtronics you are going to hate me. After looking into the source code for the RFM7x lib, I've found that using it as a dependency will ignore (and make this RF24 lib look like it's broken for) newer features implemented in this RF24 lib (since its ManiacBug days) - namely:

incompatible with SPI_UART, SOFT_SPI and my upcoming feature in Overload begin, abstract some docs, & patch printf for mbed #750 that's pending testing on the teensy, ESP32/8266, and really any board with more than 1 exposed SPI bus.
Not to mention the RFM7x lib has hardcoded the CE and CSN pins to 9 & 10 respectively 👎🏼 . I see you changed these pins for this PR, but that solution is not acceptable enough to call the RFM7x lib compatible with this lib.
Furthermore, that RFM7x lib was meant as a research project (a very helpful one at that!), and has been development-stale for over 3 years now (don't expect updates without 3rd party involvement). None of the RFM7x lib's forks have any added improvements.
Not compatible on Linux (not even using this RF24 lib because of the hardcoded CE and CSN pins)
You neglected to mention that the RFM7x/bk242x modules have a limited range of supported frequencies.

For these reasons, I would prefer to have a more native solution to support the RFM7x radios. To save memory (storing the BANK1 register's mandatory values), we can use a macro to enable RFM7x/bk242x support.

@Avamander @TMRh20 Need a second opinion before I do the asshole thing and just close this.

TMRh20 · 2021-03-27T06:21:22Z

@2bndy5 IMHO I pretty much agree with your assessment. Supporting clones is one thing when it is a matter of adding a define or a line or two of code, but with all that this requires, I would suggest a separate fork of RF24 or a lib dedicated to these clones. We could then link to it from the documentation. I think the best advice is to avoid these clones altogether.

microtronics · 2021-03-27T09:04:35Z

I would say there are simple solutions, but whatever... I'll just keep the fixes to myself...

2bndy5 · 2021-03-27T09:14:44Z

@microtronics Sometimes the simplest solutions aren't the most sustainable. Sorry this didn't work out, but I'm still willing to add to the documentation so others can still benefit from your solution. Are you willing to submit a step-by-step rfm7x.md file written in markdown? It would need to be in the root folder of this repo, and don't worry about syntax (I'm willing to address that if needed).

microtronics · 2021-03-27T09:28:43Z

You guys seem to mix things up... First of all the RFM7X are not clones but rather counterfeits. All channels, auto-ack and so on do work and they work even together with genuine nRF24l01+
The code you are all complaining about is in the examples folder. As in "this is an example on how to init Bank1 and use RFMs with the RF24 lib".

The steps required are literally:

Init Bank1
Use the fix for the broken TX_EMPTY

I'd say it's a totally viable solution to leave Bank1 init to the users who want to use these modules and even if not, the init code could be added to RF24...

2bndy5 · 2021-03-27T10:06:09Z

The code you are all complaining about is in the examples folder

because you want us to distribute someone else's work without proper Licensing (we've been over that)

If step 2 was so simple, then why pack it into a full fledged example? A code snippet in a step-by-step tutorial would have been better (that way we're not distributing the code from someone else's library).

RFM7X are not clones but rather counterfeits

The words "clones" and "counterfeits" are often used interchangeably in these issues, so we've stopped caring about that semantic.

All channels ... work

have you verified that? even the comments from the RFM7x lib say that they have a specific range.

the init code could be added to RF24

that's what I thought this PR was doing, but you went with just copying and pasting the entire RFM7x lib instead of actually using the SPI functions in our library. BTW, its easier said than done... (please prove me wrong).

microtronics · 2021-03-27T10:16:05Z

The code you are all complaining about is in the examples folder

because you want us to distribute someone else's work without proper Licensing (we've been over that)

If step 2 was so simple, then why pack it into a full fledged example? A code snippet in a step-by-step tutorial would have been better (that way we're not distributing the code from someone else's library).

As I said, we can just drop the whole example...

All channels ... work

have you verified that? even the comments from the RFM7x lib say that they have a specific range.

Of course, why would I say it otherwise. And btw also RFM7x-lib states:

All documentations says about 83 available channels, but tests show that all 127 channels can be used. (of course, only when you are allowed to use those)

the init code could be added to RF24

that's what I thought this PR was doing, but you went with just copying and pasting the entire RFM7x lib instead of actually using the SPI functions in our library. BTW, its easier said than done... (please prove me wrong).

Most things are easier said than done, but still it's no big deal. I did that initialization manually on my old PIC-micros for AC-dimming

2bndy5 · 2021-03-27T10:21:04Z

(of course, only when you are allowed to use those)

this sounds like something in BANK1 needs tweaking

I did that initialization manually on my old PIC-micros for AC-dimming

does that mean we can expect a PR with a more native solution?

microtronics · 2021-03-27T10:26:01Z

(of course, only when you are allowed to use those)

this sounds like something in BANK1 needs tweaking

no, it doesn't...

I did that initialization manually on my old PIC-micros for AC-dimming

does that mean we can expect a PR with a more native solution?

I don't know... I'd still say that this is easier done by using a library that already does this kind of work (why reinvent the wheel?). Guess I'll get back to work on my dissertation

jnk0le · 2022-10-15T21:09:08Z

Well, that was intended as a quick hack to slap onto existing arduino codebase in case of rogue supply chains etc.
It's good to hear that it actually works.

IIRC, at some point I tried to update this to a more maintaied fork, but it wasn't gonna work without modifying innerworkings of that lib, so gave up quickly. It was probably related to "protected" retransmit counter or something else that could make an infinite loop.

Licensing issues should be fixed by this commit:
github.com/jnk0le/RFM7x-lib/commit/cd64406abb7e770041d34b5ec2b32c3763f0d4f8

I don't have any issues regarding the ACK/noACK-bit that jnk0le mentions...

Its possible that the RFM7x lib disables the auto-ack feature to avoid that problem. Which is why so many uninformed people suggest disabling auto-ack when troubleshooting for possible clones/counterfeits.

It's not disabling but inverting it (as I think it is). One of the bank 1 register has a field documented as :

Compatible mode:
0:Static compatible
1:Dynamic compatible

which should control the inversion of noACK bit. Selectable by RFM73_CONFIG_COMPATIBLE_MODE in config header.

I have cross checked that with an NRF24l01+ from aliexpress which I assumed was an SI24R1 due to instability under default PA setting for genuine nRF, corresponding to +4 or +7dBm on SI. Other module with RFX2401 was especially unstable at those +4/+7dBm, similarly bk2425 at +4dBm couldn't transmit through its RFX2401 at all (-1dBm was fine)

In yes-ACK comm, RFM with static compatible mode can't talk to the one set with dynamic compatible and vice versa. To talk to SI24R1, it requires static compatible mode set up.

2bndy5 · 2022-10-16T03:13:32Z

@jnk0le Thanks for the feedback (especially regarding the License question)! It would be easier to integrate your lib if

We could add it as a dependency for the Arduino library manager, but that would require you publish the lib in the Arduino Lib manager (see the Arduino lib index here).
The CE and CSN pins were more easily configured (say using a macro definition named RFM7X_CE_PIN or RFM7X_CSN_PIN).

Otherwise, we'd have to copy the source and modify it (like what was done here, but a as a library instead of as an example application).

As for the NOACK inversion, I see that register in the RFM73 datasheet at offset 0x0C in bank 0. Ideally, this RF24 lib would use the RFM7x lib for init only, so I think the RFM73_CONFIG_COMPATIBLE_MODE flag could be used to adjust value used when RF24::begin() configures bank 0.

In yes-ACK comm, RFM with static compatible mode can't talk to the one set with dynamic compatible and vice versa. To talk to SI24R1, it requires static compatible mode set up.

This is one of the reasons that the RF24 lib doesn't auto-enable the use of the NOACK flag by default. This is controlled by RF24::enableDynamicAck(), but using the RFM7x lib may require a switching to bank 1 to properly set the register at 0x0C (bit 0), then switching back to bank 0 to set the register 0x1D (bit 0).

However, I'm not sure if changing anything in bank 1 would require a re-init for bank 0 registers (which would further complicate the existing RF24 codebase). Its been a while since I read any of the datasheets in your RFM7x lib.

I would love to continue this discussion in #776 now that you've taken some interest. I'm kinda flattered that you commented on this because your work/research has been invaluable to RF24 for numerous reasons.

jnk0le · 2022-10-23T16:38:31Z

It would be easier to integrate your lib if
1 We could add it as a dependency for the Arduino library manager, but that would require you publish the lib in the Arduino Lib manager (see the Arduino lib index here).

Critical configuration done by modifying definitions in one file and SPI/IO by modifying 2 other files. I would say that this is not usable as an arduino lib.

2 The CE and CSN pins were more easily configured (say using a macro definition named RFM7X_CE_PIN or RFM7X_CSN_PIN).

jnk0le/RFM7x-lib@5763900

Tony added 2 commits March 26, 2021 15:15

Added compatibility for RFM7X and bk242x-style nRF24 clones including…

f3a240d

… example

applied auto-formatting

37bb377

microtronics marked this pull request as draft March 26, 2021 14:35

Tony added 2 commits March 26, 2021 16:07

more formatting

2721668

just used arduinoIDE to do formatting

6b5b8f4

microtronics marked this pull request as ready for review March 26, 2021 15:31

swapped order of init code in example

0fc3cbb

2bndy5 requested changes Mar 26, 2021

View reviewed changes

microtronics closed this Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added compatibility for RFM7X and bk242x-style nRF24 clones including… #754

Added compatibility for RFM7X and bk242x-style nRF24 clones including… #754

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021 •

edited

Loading

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021

2bndy5 left a comment

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021 •

edited

Loading

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021 •

edited

Loading

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021

2bndy5 Mar 26, 2021

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

2bndy5 commented Mar 27, 2021

TMRh20 commented Mar 27, 2021

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021 •

edited

Loading

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021

microtronics commented Mar 27, 2021

jnk0le commented Oct 15, 2022 •

edited

Loading

2bndy5 commented Oct 16, 2022

jnk0le commented Oct 23, 2022

Added compatibility for RFM7X and bk242x-style nRF24 clones including… #754

Added compatibility for RFM7X and bk242x-style nRF24 clones including… #754

Conversation

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021 • edited Loading

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021

2bndy5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2bndy5 Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2bndy5 Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

microtronics commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

2bndy5 commented Mar 26, 2021

2bndy5 commented Mar 27, 2021

TMRh20 commented Mar 27, 2021

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021 • edited Loading

microtronics commented Mar 27, 2021

2bndy5 commented Mar 27, 2021

microtronics commented Mar 27, 2021

jnk0le commented Oct 15, 2022 • edited Loading

2bndy5 commented Oct 16, 2022

jnk0le commented Oct 23, 2022

microtronics commented Mar 26, 2021 •

edited

Loading

2bndy5 Mar 26, 2021 •

edited

Loading

2bndy5 Mar 26, 2021 •

edited

Loading

2bndy5 commented Mar 27, 2021 •

edited

Loading

jnk0le commented Oct 15, 2022 •

edited

Loading