There is an issue about the "toggle_feature()" after mcu reset

[WindyYam]

The toggle_feature() in the begin() must be called only when the feature is "toggled off" otherwise it get turned off. This happens when I uses a bk2425 chip and press the arduino reset button, the toggle_feature in the init runs second time and it's turned off in my case it blocks in write() forever.

according to datasheet when feature is toggled off the register "FEATURE" could not be written as anything written and then read will get 0. so there should be something like this in begin() :

//test if feature activate is toggled
write_register(FEATURE, 0x01);
if(!read_register(FEATURE))
toggle_features();

[Avamander]

Usually people don't reset their MCU without resetting the peripherals. Have you thought about doing that instead?

[WindyYam]

Usually people don't reset their MCU without resetting the peripherals. Have you thought about doing that instead?

For what I googled there is no way to reset nrf24l01 other than manage it's power supply. So to keep "rf24.begin" behaves the same after every reset the "toggle_feature()" should be taken into consideration .

[wmarkow]

I think that calling begin() should always push the RF24 chip (or a set of its registers) into the same state.

Usually people don't reset their MCU without resetting the peripherals.

@Avamander , from one point of view I could say that calling rf24.begin() should programmatically reset RF24 chip. We could call that a software reset.

[wmarkow]

Another question is: "what toggle_features() actually does? It is defined as:

void RF24::toggle_features(void)
{
    beginTransaction();
    _SPI.transfer( ACTIVATE );
    _SPI.transfer( 0x73 );
    endTransaction();
}

where variable ACTIVATE is defined as:

#define ACTIVATE      0x50

If I understand correctly, this method tries to execute ACTIVATE command with 0x73 as a parameter. In this RF24 datasheet on the page 48 (Section 8.3.1 SPI Commands) there is no such a command which command word has value 0x50 (such a command doesn't exist). Or am I missing something? So what actually toggle_features() does?

[wmarkow]

I see that toggle_features() has been introduce by commit 79628e7 in 2011.

[WindyYam]

You can see the ACTIVATE command on nRF24L01 Product Specification V2 datasheet Page 46, it's basically a switch for some features

[wmarkow]

Thanks, this link is more helpful :)

This command complicates a bit the logic. We can not track the value of that command in local variable to cache if the features are active or not, because after CPU reset (when RF24 is still powered up) we will loose that value anyway.
We need some kind of sophisticated mechanism to find out in runtime if those feature are active or not. Moreover I would get rid of this toggle_features() method (it is private so we can do it without changing the API) and introduce a new called activate_features(); it is more descriptive and we have sure that - after we call it - the features will be always activated.

[WindyYam]

There is actually a trick to show that, from datasheet it says "The R_RX_PL_WID, W_ACK_PAYLOAD, and
W_TX_PAYLOAD_NOACK features registers are
initially in a deactivated state; a write has no
effect, a read only results in zeros on MISO"
so if ACTIVATE is off those register bit can not be modified and read always get zero. so the trick is

//test if feature activate is toggled
write_register(FEATURE, 0x01);
if(!read_register(FEATURE))
toggle_features();

this will ensure ACTIVATE is on

[wmarkow]

@WindyYam, in my previous comment I have forgot to mention your proposed fix from your first post.
We write 0x01 to FEATURE register and read back its value:

• if the read value is 0x01 then the features are already activated
• if the read value is 0x00 then the features are deactivated and we need to activate them

The side effect of this is that the method will activate the features and by the way will enable EN_DYN_ACK as well.

[wmarkow]

@WindyYam, I have created a commit in my fix_for_#401 branch of my forked version of RF24 library. Could you please review and retest?

[wmarkow]

I wanted to retest the solution proposed by @WindyYam. I'm a bit surprised because I'm not able to reproduce the original issue. It looks like on my nRF24L01 chips the features are always activated and toggle_features() does nothing. Resetting the Arduino board has no impact - it always works. I even commented out this toggle_features() in RF24.begin() and the dynamic payload length and payload with ACK seem to work correctly. The RF24printDetails method after my setup always shows something like this:

DYNPD/FEATURE	 = 0x3f 0x06

so the features seem to be enabled even if toggle_features() has been commented out.

Maybe my nRF24L01 chips confirm to the Product Specification V1? According to V1 specs, the FEATURE register is accessible always (page 63 - actually there is no information that I need to unlock access to that register) and there is no need to send ACTIVATE command. V1 specs knows nothing about ACTIVATE command.
However nRF24L01 Product Specification V2 datasheet knows about ACTIVATE command (page 46) and on page 58 there is annotation on FEATURE register, that ACTIVATE command must be send to unlock access to that register.

Or maybe my devices are a cheap clones of the original device?

Very interesting.

[Avamander]

I'm very vary of changing what works perfectly on original nRF24L01+'s. The current implementation does. API changes are possible but they have to go and sit on another major version change branch until I can be convinced there are no regressions. If some function were just added that would be way easier to release.

[WindyYam]

@wmarkow I dont have genuine nrf24 modules, I have bunch of "nrf24l01" that turn out some of them to be bk2425, others most probably si24. The issue happens when I'm using the no-ack feature(enableDynamicAck(false) and write with the no ack flag)

To be better acceptable maybe add function like "bool isFeatureActivated()" ?

[wmarkow]

I'm very vary of changing what works perfectly on original nRF24L01+'s. The current implementation does.

I feel like calling RF24.begin() twice - even on original nRF24L01+ - will in fact disable the features and you will have the issue reported by @WindyYam. The same effect seems to be reproducible when you reset Arduino with reset button while RF24 chip is still powered up. I couldn't reproduce the issue because it seems that my chips behave different than in Specs V2, however they seem to work correctly with Spec V1. Unfortunately I do not have any original nRF24L01+ chips, so I can not retest this :(

API changes are possible but they have to go and sit on another major version change branch until I can be convinced there are no regressions.

Right, I agree. But we are talking about toggle_features() which is private method and called only from begin(), so I think that we can modify toggle_features() (or even remove it) but we need to provide some substitution that will be called from begin() and activate (but disable) the features (this is the expected behavior after power up the system and calling begin() once). API will not be then changed and the #401 will be fixed. Please consider this:

• when you call begin() for the second time, the features are toggled
• when you start using **Arduino's reset button, the features are toggled
• every time you flash the new software to Arduino, Arduino is restarted and the features is toggled...

... and you may have some issues with those features: you actually may not know if they are enabled or disabled and your software may not work correctly.

@WindyYam, I have prepared some quick implementation of bool isFeatureActivated(). This function is a tricky function as well, because it must check if the features are activated but it must not change the current settings of FEATURE register (so we do not change the current settings that are made by library user). Other words the FEATURE register must have the same value after calling this function. I have ended up with something tricky like this:

bool RF24::isFeatureActivated()
{
   uint8_t features = read_register(FEATURE);
   if(features != 0)
   {
      // Features are different than zero, so it means that they must be accessible
      return true;
   }

   // Features are zero. We do not know if they are activated. To check that toggle features and read the value again.
   toggle_features();
   uint8_t features2 = read_register(FEATURE);
   if(features2 != 0)
   {
      // Ok now the features are activated. Roll back the features activation state.
      toggle_features();
      // and return false
      return false;
   }

   // Roll back the features activation state
   toggle_features();

   // Features are still zero - twice. That means that the real value for FEATURES is zero. During those two checks above,
   // the features were enabled once. But we still do not know if they are now activated. Make a final test.
   write_register(FEATURE, 0x01);
   if(read_register(FEATURE) == 0)
   {
      // Features are not activated. The write has no effect.
      return false;
   }

   // Features are activated and its value must be zero.
   write_register(FEATURE, 0x00);
   return true;
}

[WindyYam]

@wmarkow I think that has gone way too complex. Just save the register before write 0x01, and then read

1. if read is 0, the written 0x01 would do nothing(see datasheet, write has no effect), and we know it's deactivated;
2. if read is something else, write back its value before, and we know it's activated;

bool isFeatureActivated(){
    uint8_t features = read_register(FEATURE);
    write_register(FEATURE, 0x01);
    if(!read_register(FEATURE)){   //got zero, deactivated
        return FALSE;
    }else{  //activated
        write_register(FEATURE, features);
        return TRUE;
    }
}

[wmarkow]

@WindyYam, you are right. My solution is too way complex. Yours is much simpler. Now we have an example how simple things can get over complicated :)

[TMRh20]

Bit of an old issue, but I've taken a closer look at it and unfortunately I'm probably more confused now. I made the toggle_features function public, and toggled it manually to see what was going on.

1. Essentially, the toggle_features() function seems to have no real effect on my nrf24l01+ radios
2. Removing the function call from begin() mostly makes no difference. I can disable/re-enable ack payloads and DPL at will.
3. I was able to get the radio into a state where upon reset, calling begin failed to reset the FEATURE register to 0, but its been difficult to recreate consistently. Writing anything but 0 always seems to work.

So I'm thinking the makers of these devices decided to ditch the ACTIVATE command and just make things writable. I don't have any bk2425 or SI24R1 to test with though.

So I guess the toggle_features command should probably be left as-is for the nrfs, since there is inconsistent behaviour without it, and adding code to the begin() function to support an obscure device is out of the question. It may be possible to add another function or #define that enables the full functionality per #401 (comment)

This is the one and only time I've ever heard reference to the bk2425 but I am a bit familiar with the SI24R1s, so I'd like to know how they behave in regard to the ACTIVATE command prior to making changes. If anybody with SI24R1s would like to test this out, I would mainly like to know the following:

1. What happens when toggle_features is removed from begin()? Does the FEATURES register get reset to 0? Is it still writable after?
2. What happens when toggle_features is called during runtime? Any change in enabling ACK payloads or DPL?

[2bndy5]

@Avamander

Usually people don't reset their MCU without resetting the peripherals.

This simply isn't true. The MCUs' reset buttons don't typically affect the board's regulators. Most often the reset button focuses only on the board's CPU's enable pin (which does not invoke a power-on-reset condition in the nRF24L01). Please correct me if I misunderstood this comment.

@TMRh20 This is why adjusting the RF24::begin() is the only course of action to address this issue (for any type module connected -- including non-plus variants of nRF24L01). Additionally, this issue would persist when using the python wrapper if begin() isn't adjusted because exiting the Python interpreter/REPL also does not invoke a power-on-reset condition in the nRF24L01

@wmarkow I like your solution, but adding another function (even if only private) that is only called once is a bit excessive.

I've had to address the un-intuitive manner of the reset button on my CircuitPython library. You can see how I've addressed this issue in this library (on my fork) with the 2 above mentioned commits (beware the second commit fixes faulty logic in the first). Subsequently, I added a _is_p_variant private boolean to expedite additional functionality for #640 & #641 (other issues related specifically to the non-plus nRF24L01).

[Avamander]

This simply isn't true. The MCUs' reset buttons don't typically affect the board's regulators. Most often the reset button focuses only on the board's CPU's enable pin (which does not invoke a power-on-reset condition in the nRF24L01). Please correct me if I misunderstood this comment.

@2bndy5 The meaning was that it's a weird setup that doesn't run initialization/reset code after a reset.

[2bndy5]

Ah, thanks for that clarification. I agree, it is weird that RF24 doesn't brute force a soft reset on init, but that would require dumping a whole lot more info to the nRF24L01 registers. I'm afraid @TMRh20 might err against this behavior as it seems to compromise the "optimized" description of this repo. BTW I did employ a soft reset for my CircuitPython library, but I captured the RX addresses during the lib's init (related to #496), instead of resetting them.

[2bndy5]

resolved by #691

feels good to close this one 😄