refactor: migrate to `stopListening(txAddress)` from `openWritingPipe()` #1030

2bndy5 · 2025-04-26T22:53:33Z

stopListening() now writes the TX address to pipe 0 (for reading and writing). This deprecates the need for openWritingPipe() by offering an overloaded stopListening(txAddress).

All examples (and the python wrapper) have been updated to use the new public stopListening(txAddress) accordingly.

Important

RF24Network should be updated to use the new stopListening(txAddress) also. This approach should retain current performance in networking layers while still satisfying #1028.

The main take away is no duplicate SPI transactions when switching to TX mode and (re)setting the TX address. This aims to fix the performance regression in #1029.

github-actions · 2025-04-26T23:03:16Z

Memory usage change @ a04c324

Board	flash	%	RAM for global variables	%
`arduino:avr:nano`	❔ -132 - +44	-0.43 - +0.14	💚 -18 - 0	-0.88 - 0.0
`arduino:samd:mkrzero`	❔ -48 - +28	-0.02 - +0.01	💚 -32 - 0	-0.1 - 0.0

Click for full report table

Board	`examples/GettingStarted` flash	%	`examples/GettingStarted` RAM for global variables	%	`examples/AcknowledgementPayloads` flash	%	`examples/AcknowledgementPayloads` RAM for global variables	%	`examples/ManualAcknowledgements` flash	%	`examples/ManualAcknowledgements` RAM for global variables	%	`examples/StreamingData` flash	%	`examples/StreamingData` RAM for global variables	%	`examples/MulticeiverDemo` flash	%	`examples/MulticeiverDemo` RAM for global variables	%	`examples/InterruptConfigure` flash	%	`examples/InterruptConfigure` RAM for global variables	%	`examples/scanner` flash	%	`examples/scanner` RAM for global variables	%	`examples/encodeRadioDetails` flash	%	`examples/encodeRadioDetails` RAM for global variables	%
`arduino:avr:nano`	-16	-0.05	0	0.0	-20	-0.07	0	0.0	-18	-0.06	0	0.0	-18	-0.06	0	0.0	-132	-0.43	-18	-0.88	-24	-0.08	0	0.0	44	0.14	0	0.0	-2	-0.01	0	0.0
`arduino:samd:mkrzero`	-4	-0.0	0	0.0	-4	-0.0	0	0.0	-8	-0.0	0	0.0	-8	-0.0	0	0.0	-48	-0.02	-32	-0.1	-8	-0.0	0	0.0	28	0.01	0	0.0	-4	-0.0	0	0.0

Click for full report CSV

Board,examples/GettingStarted<br>flash,%,examples/GettingStarted<br>RAM for global variables,%,examples/AcknowledgementPayloads<br>flash,%,examples/AcknowledgementPayloads<br>RAM for global variables,%,examples/ManualAcknowledgements<br>flash,%,examples/ManualAcknowledgements<br>RAM for global variables,%,examples/StreamingData<br>flash,%,examples/StreamingData<br>RAM for global variables,%,examples/MulticeiverDemo<br>flash,%,examples/MulticeiverDemo<br>RAM for global variables,%,examples/InterruptConfigure<br>flash,%,examples/InterruptConfigure<br>RAM for global variables,%,examples/scanner<br>flash,%,examples/scanner<br>RAM for global variables,%,examples/encodeRadioDetails<br>flash,%,examples/encodeRadioDetails<br>RAM for global variables,%
arduino:avr:nano,-16,-0.05,0,0.0,-20,-0.07,0,0.0,-18,-0.06,0,0.0,-18,-0.06,0,0.0,-132,-0.43,-18,-0.88,-24,-0.08,0,0.0,44,0.14,0,0.0,-2,-0.01,0,0.0
arduino:samd:mkrzero,-4,-0.0,0,0.0,-4,-0.0,0,0.0,-8,-0.0,0,0.0,-8,-0.0,0,0.0,-48,-0.02,-32,-0.1,-8,-0.0,0,0.0,28,0.01,0,0.0,-4,-0.0,0,0.0

RF24.h

examples/MulticeiverDemo/MulticeiverDemo.ino

RF24.cpp

TMRh20 · 2025-04-27T05:05:09Z

I have one suggestion about this, since it changes the API so much.

uint8_t rxNodeAddress[6] = "1Node";
memcpy(radio.txAddress, rxNodeAddress, 5);
radio.stopListening();

Why not change it so the user just calls the following:

uint8_t rxNodeAddress[6] = "1Node";
radio.stopListening(rxNodeAddress);

This way users don't need to know about or use the memcpy function or openWritingPipe.

*EDIT: It could also allow use to keep the old stopListening() function in place, for when users don't want the bother of overwriting the pipe0 rx address.

2bndy5 · 2025-04-27T05:33:12Z

That could work. It is a little less intuitive because

the name stopListening() (thanks maniacBug 😜).
it kind of assumes the TX address will be dynamic.

We could even overload stopListening() where

stopListening(void) remains the same but deprecated along with openWritingPipe(). This would leave existing code unhindered by the performance regression I'm trying to resolve here.
stopListening(uint8_t*) would additionally cache/restore the TX address to satisfy the pipe 0 bug.

setting the address was separate from activating TX mode for a reason though

Is it unusual to set the TX address before actually engaging TX mode?
What if the TX address doesn't change every time the radio enters TX mode? In most of our examples, the address doesn't have to be globally scoped. In actual user production code, where the TX address is static, each "prep TX mode" function wouldn't necessarily have access to the buffer that configures the TX address.
1. If the TX address doesn't change, the user might think that stopListening(void) would work the same as stopListening(uint8_t*). Then they'd instinctively invoke the deprecated behavior.

2bndy5 · 2025-04-27T05:39:28Z

Would this be a bad time to phase in the newer grammar asTx() and asRx() that I landed on in the rf24-rs port?

TMRh20 · 2025-04-27T05:39:50Z

Is it unusual to set the TX address before actually engaging TX mode?

It kind of is, it makes things more difficult to address in RF24Network. I really have to look at the order of operations to see how this will work out.

What if the TX address doesn't change every time the radio enters TX mode?

Thats why we leave the old stopListening and openWritingPipe in place, so users can just use the old functionality.

Here is what I am playing around with:

void RF24::stopListening()
{

    ce(LOW);

    //delayMicroseconds(100);
    delayMicroseconds(static_cast<int>(txDelay));
    if (ack_payloads_enabled) {
        flush_tx();
    }

    config_reg = static_cast<uint8_t>(config_reg & ~_BV(PRIM_RX));
    write_register(NRF_CONFIG, config_reg);

#if defined(RF24_TINY) || defined(LITTLEWIRE)
    // for 3 pins solution TX mode is only left with additional powerDown/powerUp cycle
    if (ce_pin == csn_pin) {
        powerDown();
        powerUp();
    }
#endif

    write_register(EN_RXADDR, static_cast<uint8_t>(read_register(EN_RXADDR) | _BV(pgm_read_byte(&child_pipe_enable[0])))); // Enable RX on pipe0

}

/****************************************************************************/

void RF24::stopListening(uint8_t address[5])
{
    stopListening();
    write_register(TX_ADDR, address, addr_width);
    write_register(RX_ADDR_P0, address, addr_width);

}

/****************************************************************************/

void RF24::stopListening(uint64_t address)
{
    stopListening();
    write_register(TX_ADDR, reinterpret_cast<uint8_t*>(&address), addr_width);
    write_register(RX_ADDR_P0, reinterpret_cast<uint8_t*>(&address), addr_width);

}

I think most users are already avoiding pipe 0 because of this bug.

2bndy5 · 2025-04-27T05:42:15Z

I think most users are already avoiding pipe 0 because of this bug.

Without a doubt, ~~we are~~ I am totally overthinking this but for good reasons this time.

2bndy5 · 2025-04-27T05:51:05Z

Is it unusual to set the TX address before actually engaging TX mode?

It kind of is, it makes things more difficult to address in RF24Network. I really have to look at the order of operations to see how this will work out.

Its a little difficult for me to remember the order of ops. In the examples, I noticed that most of them set the addresses to the pipes first and then call stopListening() in setup(). I often confuse the mandate about startListening() being called after setting the RX addresses as if it applies similarly to stopListening().

It only works out in the examples because they often call stopListening() to idle the radio after master or slave roles complete.

EDIT: I think if we publicly expose the ce() function, the network layer would be less reliant on stopListening() to exit RX mode.

TMRh20 · 2025-04-27T05:58:45Z

This is what I changed in RF24Network

template<class radio_t>
bool ESBNetwork<radio_t>::write_to_pipe(uint16_t node, uint8_t pipe, bool multicast)
{
    bool ok = false;

    // Open the correct pipe for writing.
    // First, stop listening so we can talk
    if (!(networkFlags & FLAG_FAST_FRAG)) {
        if(pipe){
          radio.stopListening();
        }else{
          radio.stopListening(pipe_address(node, pipe));    // << **** CHANGED THIS
        }
            
    }

Now I'm getting upwards of 27KB/s with the same tests. Not sure what was going on before, but this is more inline with the capabilities of the radios.

2bndy5 · 2025-04-27T06:09:17Z

Now I'm getting upwards of 27KB/s with the same tests. Not sure what was going on before, but this is more inline with the capabilities of the radios.

That's with only 1 hop or a multitude?

// First, stop listening so we can talk

Honestly, this could be done quicker with just toggling the CE pin. I find it weird this lib hides the CE pin capability and the STATUS flags. Every time I write a nRF24L driver from scratch, I always expose these as a fail-safe for advanced usage; networking and streaming consecutive payloads definitely qualify as advanced usage.

TMRh20 · 2025-04-27T06:15:25Z

That's direct, node 1 to node 0 at 27KB/s with RF24Network. Kind of the speed I was expecting before.
What about the need to config PRIM_RX and the EN_RXADDR register? Isn't that required to stopListening() ?

2bndy5 · 2025-04-27T06:17:27Z

What about the need to config PRIM_RX and the EN_RXADDR register? Isn't that required to stopListening() ?

Yeah, but lumping that in with CE management is what mandates the confusing order of operations.

TMRh20 · 2025-04-27T06:18:34Z

Well in that case, lets make CE() public!

2bndy5 · 2025-04-27T06:27:59Z

Should I combine this PR changes with your stopListening() overload approach? Or start a new branch?

It still doesn't hurt to have the TX address public too. I can't think of a point where this lib (or other network layers) needs exclusive access/control over it.

FYI, rust doesn't have implicit function overloading like in C++. Python 3 had to be patched to support overloaded functions (making it an explicit opt-in feature/syntax in pure python).

TMRh20 · 2025-04-27T06:29:54Z

Whichever way is easiest I guess.
I agree.

TMRh20 · 2025-04-27T06:39:18Z

Would this be a bad time to phase in the newer grammar asTx() and asRx() that I landed on in the rf24-rs port?

Missed this comment, but to me those names are more confusing than startListening/stopListening. Maybe I'm just old and averse to change :p

2bndy5 · 2025-04-27T06:43:09Z

Fair enough. After writing examples with radio.as_tx() or radio.as_rx(), I kind of prefer it now. Not to mention the convenient brevity.

related to nRF24/RF24#1028 but takes a different approach from nRF24/RF24#1029 (more like nRF24/RF24#1030)

2bndy5 · 2025-04-30T20:05:48Z

I found a problem with the overloaded stopListening(txAddress) approach.

radio.stopListening(txAddress);      // write the TX address to RX_ADDR_P0
radio.openReadingPipe(0, rxAddress); // write the RX address to RX_ADDR_P0
// !!! still in TX mode but RX pipe 0 address is not the TX address

My gut reaction is to use the cached config_reg and the given pipe number in openReadingPipe() to prevent overwriting the TX address while still in TX mode.
So, this

RF24/RF24.cpp

Lines 1651 to 1656 in 7125fb4

    
           if (child < 2) { 
        
               write_register(pgm_read_byte(&child_pipe[child]), address, addr_width); 
        
           } 
        
           else { 
        
               write_register(pgm_read_byte(&child_pipe[child]), address, 1); 
        
           }

gets changed to

        if (child > 1) {
            write_register(pgm_read_byte(&child_pipe[child]), address, 1);
        }
        else if (static_cast<bool>(config_reg & _BV(PRIM_RX)) || child != 0) {
            write_register(pgm_read_byte(&child_pipe[child]), address, addr_width);
        }

Fortunately, the cached pipe0_reading_address will be restored to pipe 0 when startListening() is called.

RF24/RF24.cpp

Lines 1158 to 1161 in 7125fb4

    
           // Restore the pipe0 address, if exists 
        
           if (_is_p0_rx) { 
        
               write_register(RX_ADDR_P0, pipe0_reading_address, addr_width); 
        
           }

Following the state of the EN_RXADDR register in this scenario, I think we should be fine because both openReadingPipe() and stopListening() open RX pipe 0, and startListening() won't close it because _is_p0_rx is true.

explain logic in comment

user code should ultimately specify address(es) regardless of initialization

for plus variants only

2bndy5 · 2025-05-03T07:35:48Z

This one needs diligent review. My brain is sore from ~~overthinking~~ analyzing the order of function calls that might cause erroneous radio setup.

TMRh20 · 2025-05-03T09:43:39Z

Well good news so far, this doesn't seem to break any old code. I'm running this on all my RPis & a couple Arduinos to test. Will move on to testing the new API soon.

2bndy5 · 2025-05-03T09:57:56Z

I don't think it would even if you use pipe 0 to RX (the real crux of the problem). If pipe 0 is used, then there should just be an extra/duplicate write to RX_ADDR_P0 register (from openWritingPipe()). With the overloaded stopListening(tx_address), the duplicate write to that register is removed.

I must admit, your stopListening() overload idea is exactly what this needed.

RF24.cpp

TMRh20

Arduino sketches all seem to check out now:

gettingStarted (pipe0, pipe1, pipe2, etc working)
interruptConfigure - good
AckknowledgementPayloads - good
Multiceiver - good

RF24Network, Mesh, Ethernet & Gateway examples all functioning well also

2bndy5 · 2025-05-03T11:17:46Z

I'm too tired to think. Mind if this sits overnight (my time). I'll give this another code review pass tomorrow and we can do the merge crusade.

I still think the network layers would be more performant with the overloaded stopListening() instead of openWritingPipe(). But the rationale escapes me right now.

TMRh20 · 2025-05-03T11:21:16Z

Yeah, I guess the DPL bug has existed forever, one more night won't hurt

I still think the network layers would be more performant with the overloaded stopListening() instead of openWritingPipe(). But the rationale escapes me right now.

I'll take a look at it today also.

2bndy5 · 2025-05-03T18:53:49Z

I 'm going to deprecate that new stopListening(uint64_t) because using an integer as an address has a couple disadvantages:

The MSB (unique address prefix for each pipe) oddly goes last in an integer. This only confuses users that never heard of "Endianness". In declaring a buffer, the MSB intuitively goes first.
Even if a 5-byte buffer is byte-aligned to 6 bytes, there's still less memory allocated when compared to a 64-bit integer.

Both of these disadvantages are briefly explained (with example snippets) in the migration guide, so we shouldn't have to repeat ourselves when people ask.

PS - I get why you added it; it makes migration practically a swapping of function names. But, the address passed needs to be changed into a buffer to complete the migration. There would also be a duplicate stopListening(void) call that is no longer required if stopListening(address) is already called.

review migration guide again

TMRh20 · 2025-05-03T21:55:45Z

Yeah, I should probably have a look at RF24Network some time and see about converting from using uint64_t for addressing.

Soo, release crusade?

2bndy5 · 2025-05-03T22:00:04Z

Yeah, I should probably have a look at RF24Network some time and see about converting from using uint64_t for addressing.

Already done in nRF24/RF24Network#251. I'm testing that branch now (against another RPi running just master branches).

Soo, release crusade?

Sure.

2bndy5 · 2025-05-03T22:01:15Z

If you're going to do it, remember RF24 gets a minor bump, not a patch.

2bndy5 added the enhancement label Apr 26, 2025

2bndy5 commented Apr 26, 2025

View reviewed changes

RF24.h Outdated Show resolved Hide resolved

2bndy5 commented Apr 26, 2025

View reviewed changes

examples/MulticeiverDemo/MulticeiverDemo.ino Outdated Show resolved Hide resolved

2bndy5 commented Apr 26, 2025

View reviewed changes

RF24.cpp Outdated Show resolved Hide resolved

2bndy5 commented Apr 27, 2025

View reviewed changes

RF24.cpp Outdated Show resolved Hide resolved

2bndy5 added a commit to nRF24/rf24-rs that referenced this pull request Apr 27, 2025

refactor!: replace open_tx_pipe() with tx_address

8a5f2d0

related to nRF24/RF24#1028 but takes a different approach from nRF24/RF24#1029 (more like nRF24/RF24#1030)

2bndy5 mentioned this pull request Apr 27, 2025

Re-send lost fragments nRF24/RF24Network#247

Open

2bndy5 marked this pull request as draft April 28, 2025 06:59

This comment was marked as resolved.

Sign in to view

2bndy5 force-pushed the alt-pipe0-tx-fix branch from 08624c0 to 7125fb4 Compare April 28, 2025 21:21

2bndy5 force-pushed the alt-pipe0-tx-fix branch 2 times, most recently from b2d7a97 to f9fa18b Compare April 30, 2025 23:42

2bndy5 marked this pull request as ready for review May 1, 2025 00:31

2bndy5 added 9 commits May 2, 2025 23:37

use correct buffer size in multiciever demos.

d1d1795

missed an autoAckPL example

d5b6ca3

overload stopListening(tx_address)

e6fd948

openReadingPipe(0) should not overwrite TX address in TX mode

62f1449

explain logic in comment

update examples

d1a0168

try initializing the cached addresses in declaration

5e52cdb

don't init cached addresses

e0fe474

user code should ultimately specify address(es) regardless of initialization

revert exposing cached TX address

f9d6957

add to migration guide

79ea02d

2bndy5 force-pushed the alt-pipe0-tx-fix branch from feb8fd1 to a7bfa41 Compare May 3, 2025 07:27

stopConstCarrier() restore cached TX address

6061761

for plus variants only

2bndy5 force-pushed the alt-pipe0-tx-fix branch from a7bfa41 to 6061761 Compare May 3, 2025 07:31

2bndy5 requested a review from TMRh20 May 3, 2025 07:34

TMRh20 reviewed May 3, 2025

View reviewed changes

RF24.cpp Outdated Show resolved Hide resolved

revert openWritingPipe(buf)

c9dbb87

TMRh20 approved these changes May 3, 2025

View reviewed changes

Add uint64_t versions of new stoplistening

7beba48

2bndy5 added 2 commits May 3, 2025 12:30

deprecate stopListening(uint64_t)

472d5f8

review migration guide again

upgrade doxygen version

a04c324

2bndy5 merged commit d9417e0 into master May 3, 2025
152 checks passed

2bndy5 deleted the alt-pipe0-tx-fix branch May 3, 2025 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate to `stopListening(txAddress)` from `openWritingPipe()` #1030

refactor: migrate to `stopListening(txAddress)` from `openWritingPipe()` #1030

2bndy5 commented Apr 26, 2025 •

edited

Loading

github-actions bot commented Apr 26, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025

This comment was marked as resolved.

2bndy5 commented Apr 30, 2025 •

edited

Loading

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

TMRh20 left a comment

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

2bndy5 commented May 3, 2025 •

edited

Loading

refactor: migrate to stopListening(txAddress) from openWritingPipe() #1030

refactor: migrate to stopListening(txAddress) from openWritingPipe() #1030

Conversation

2bndy5 commented Apr 26, 2025 • edited Loading

github-actions bot commented Apr 26, 2025 • edited Loading

TMRh20 commented Apr 27, 2025 • edited Loading

2bndy5 commented Apr 27, 2025

setting the address was separate from activating TX mode for a reason though

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025 • edited Loading

2bndy5 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025 • edited Loading

TMRh20 commented Apr 27, 2025 • edited Loading

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025

2bndy5 commented Apr 27, 2025 • edited Loading

TMRh20 commented Apr 27, 2025

TMRh20 commented Apr 27, 2025 • edited Loading

2bndy5 commented Apr 27, 2025

This comment was marked as resolved.

2bndy5 commented Apr 30, 2025 • edited Loading

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

TMRh20 left a comment

Choose a reason for hiding this comment

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

TMRh20 commented May 3, 2025

2bndy5 commented May 3, 2025

2bndy5 commented May 3, 2025 • edited Loading

refactor: migrate to `stopListening(txAddress)` from `openWritingPipe()` #1030

refactor: migrate to `stopListening(txAddress)` from `openWritingPipe()` #1030

2bndy5 commented Apr 26, 2025 •

edited

Loading

github-actions bot commented Apr 26, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 27, 2025 •

edited

Loading

TMRh20 commented Apr 27, 2025 •

edited

Loading

2bndy5 commented Apr 30, 2025 •

edited

Loading

2bndy5 commented May 3, 2025 •

edited

Loading