great library - but when opening for reading on pipe0

[justinBR]

Hey, great library,
I ran into a bit of a bug when trying to open pipe 0 for reading. The problem is in the startListening() function. You have written

  if (pipe0_reading_address[0] > 0){
    write_register(RX_ADDR_P0, pipe0_reading_address, addr_width);	
  }else{
	closeReadingPipe(0);
  }

which is supposed to re-load the reading address if one has been stored in the pipe0_reading_address buffer. However, you do not check the whole address, you just check the first byte.

This will obviously cause problems if the first byte in the address to be opened starts with 0x00 as mine did. You should check the whole address.

I fixed it in my copy, so I need nothing from you. just letting you know.

[hexwell]

Had the same problem, spent a couple hours investigating the issue and came to the same conclusion.
I came to the issues to post this and found yours.
Would you mind sharing your fix / making a pull request to the library?

[justinBR]

void RF24::startListening(void)
{
 #if !defined (RF24_TINY) && ! defined(LITTLEWIRE)
  powerUp();
 #endif
  write_register(NRF_CONFIG, read_register(NRF_CONFIG) | _BV(PRIM_RX));
  write_register(NRF_STATUS, _BV(RX_DR) | _BV(TX_DS) | _BV(MAX_RT) );
  ce(HIGH);
  // Restore the pipe0 adddress, if exists
  if ((pipe0_reading_address[0] > 0) || pipe0_reading_address[1] > 0){
    write_register(RX_ADDR_P0, pipe0_reading_address, addr_width);	
  }else{
	closeReadingPipe(0);
  }

  // Flush buffers
  //flush_rx();
  if(read_register(FEATURE) & _BV(EN_ACK_PAY)){
	flush_tx();
  }

  // Go!
  //delayMicroseconds(100);
}

this doesn't check the whole address, but it checks one more byte. you can add more bytes if you need to.
the actual change is in the line
if ((pipe0_reading_address[0] > 0) || pipe0_reading_address[1] > 0)

[hexwell]

Thank you!
I am sure this would've already occurred to you but as a note for those who are kindly working on this:
With this kind of fix the "presence" detection of the address is reliant on its value, thus never allowing for the complete use of the address space. Looking at the code, some sort of flag might be required, or a note in the documentation about the unusable addresses.

[TMRh20]

I've given this some thought, and my feeling is that the code should remain the same, but the documentation and probably examples should more clearly specify this behavior. I'll look at that soon.

Please see http://maniacalbits.blogspot.com/2013/04/rf24-addressing-nrf24l01-radios-require.html for the general reasoning.

A bit of an old post, but many addresses including ones using 0xFF or 0x00 should be avoided and why carefully selected addressing can make an actual difference in outcomes. (again examples should be updated)

As my example of "good addressing", RF24Network uses 0xcc,0xc3,0x3c,0x33,0xce,0x3e,0xe3,0xec for its logical->physical addressing scheme.

It is possible to add a #define in the config file to enable/disable full address space checking, but there should be plenty of address space while avoiding the bad ones. ???

[2bndy5]

from the link specified for @TMRh20 general reasoning to not fix this:

These tips should be regarded as rules of thumb rather than absolutes.

Although, I am all for detailed documentation and examples.

The way it is written, pipe0_reading_address is declared as a 5-byte array with the first byte being 0 (uint8_t friendly NULL -- it's a classic NULL vs 0 problem). Would using the RX_ADDR_P0 reset value, 0xE7E7E7E7E7, in the declaration of pipe0_reading_address be a better workaround for testing if pipe0_reading_address has been altered? Furthermore, why not fetch the RX_ADDR_P0 contents and memcmp() that to pipe0_reading_address in startListening()? However, I understand trying to avoid the additional csDelay time for that extra SPI transaction.

[hexwell]

Thanks for the reply, @TMRh20
Very interesting read, although it seems to me that the issue explained there is a different one, I'll refer to it as the PER issue.

A note in the documentation about the PER issue would certainly be of help to users of the library, that may not be aware of it.

However, the original problem remains:
Addresses having the first byte = 0 are mistakenly interpreted as "not existing" by the snippet of code mentioned by justinBR in the first comment of this thread.

That check may coincidentally or intentionally exclude "bad addresses" concerning the PER issue.
If it is coincidental it means that the sole purpose of the check is to establish the presence of the address, which it does poorly.
If it is intentional it means that its purpose is to avoid the PER issue, but it misses quite a few cases according to the examples in the article.

In conclusion, my opinion is that:

This library should not concern with actively avoiding the PER issue with address checks, but may inform the users about it in the documentation and examples.

About the original issue:
Either the code should have a better presence check for the address, like a separate boolean flag variable, or another note in the documentation is necessary.

Of course none of this is of utmost importance, and as you say there are plenty of addresses available avoiding the ones causing problems.

[2bndy5]

@hexwell Sorry if my post seemed off topic.

Either the code should have a better presence check for the address

That is what I was trying to suggest, rather than adding another byte (or 2) of code to implement your idea of a separate boolean flag. I do appreciate the distinction you made about 2 separate issues. I'm just trying to help.

After more thought, I can foresee a problem using the register's reset value as pipe0_reading_address initial value: the MCU reset button. In my experience the reset button does not invoke a Power-on-Reset condition in the nRF24L01. Meaning, the nRF24L01's address registers would persist while the RF24 object's pipe0_reading_address would be re-initialized.

[hexwell]

@2bndy5 your comments are spot on, no worries.
My intent was to bring attention to the difference between the issues, which I'm happy was welcome; the suggestion of the boolean flag was merely an example.
I think the potential choice of implementation of the check should be up to people who posses a better knowledge of the workings of the module and the code in it's entirety than I, for lack of time, do

[2bndy5]

I'm wondering why this library forces the RX pipe 0 closed if the first byte of pipe0_reading_address is 0. Pipe 0 works fine as a receiving pipe. It doesn't make sense to me because the ESB protocol is intuitive enough to manage pipe 0 as needed for TX operations (even in RX mode). I'll probably open another issue to address this, but it might also be applicable to this issue.

[wohali]

Hey @2bndy5 , thanks for your hard work on this library - much appreciated.

If this should be in a new issue, please split it out.

Just wanted to point out that there's a really important use case for setting the first address byte to 0x00, namely protocol reverse engineering. Here's some reading if you're unfamiliar:

• https://travisgoodspeed.blogspot.com/2011/02/promiscuity-is-nrf24l01s-duty.html
• http://www.remote-exploit.org/articles/keykeriki_v2_0__8211_2_4ghz/index.html
• https://github.com/Yveaux/NRF24_Sniffer

Right now, to get around the limitations of this library, something like this is required:

radio.setAutoAck(false);
writeRegister(RF_SETUP, 0x21); // Disable PA, 250kbps, LNA enabled in a single op - no ctor to do this in one go?
radio.setPayloadSize(sz);
radio.setChannel(ch);
writeRegister(EN_RXADDR, 0x00);    // this is critical - seems no way to do this with RF24?
writeRegister(SETUP_AW, 0x00);  // though perhaps passing in a_width of 2 would work? https://github.com/nRF24/RF24/blob/2819745c727cccc2cf2308595a77c637cc84cd95/RF24.cpp#L1417
radio.openReadingPipe(0, 0xAALL);
radio.openReadingPipe(1, 0x55LL);
radio.disableCRC();
radio.startListening();
// ...

where writeRegister is a raw SPI transfer that also handles CSN toggling. Not pleasant!

I see 70786aa which looks encouraging to help with some of this - can you clarify how the use case I've outlined might be supported by this commit? I expect other language bindings (like the Python one) will need to have this variable exposed as well...) Thanks!

[2bndy5]

@TMRh20 what would you say to implementing @hexwell's bool idea (to resolve this) as a MSBit in the private member addr_width? I know I'll need to compare the code-size differences because there would be a slight computational hit in setAddressWidth(), open/closeReadingPipe(), and startListening().

[TMRh20]

I guess I’ve had enough time to think about it. :p I think it’s probably a good idea to implement it rather than leaving it as an open issue.

…

On Oct 6, 2021, at 10:48 PM, Brendan ***@***.***> wrote:  @TMRh20 what would you say to implementing @hexwell's bool idea (to resolve this) as a MSBit in the private member addr_width? I know I'll need to compare the code-size differences because there would be a slight computational hit in setAddressWidth(), open/closeReadingPipe(), and stopListening(). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[2bndy5]

I'll also compare code-sizes for using a private bool vs repeatedly doing bitwise ops on addr_width (not sure how compiler optimizations will affect this).

I'll be adding this into rp2xxx branch when I'm ready to commit. There aren't any changes on that branch that increase the code-size (compared to master).

[2bndy5]

Results (using rp2xxx branch)

code-size (in bytes) on ATSAMD21 (compiling GettingStarted.ino)

original size	w/ MSB flag	w/ bool flag
35896	35960	35904

code size (in bytes) on ATMega328P (compiling GettingStarted.ino)

original size	w/ MSB flag	w/ bool flag
6604 (pgm space) 250 (dyn mem)	6600 (pgm space) 250 (dyn mem)	6576 (pgm space) 251 (dyn mem)

code size (in bytes) on ATTiny85 (compiling the specially designed rf24ping85.ino)

original size	w/ MSB flag	w/ bool flag
3142 (47% pgm space) 66 (dyn mem)	3174 (48% pgm space) 66 (dyn mem)	3150 (47% pgm space) 67 (dyn mem)

Conclusion

As you can see, using a new dedicated bool is the way to go.

While using a dedicated bool flag does allocate 1 more byte of memory, it seems that replacing the comparison pipe0_reading_address[0] > 0 with _is_p0_rx (in startListening()) reduces program space (more so on the AVR chip). As I suspected, using bitwise math to store the flag in the addr_width's MSBit is more costly (& less legible) than its intention. Yes, the ATTiny85 will take a hit no matter what we do, but a dedicated bool is still the lesser of 2 evils.

@TMRh20 I'm going ahead with a new private bool called _is_p0_rx.