nRF24 / RF24

OSI Layer 2 driver for nRF24L01 on Arduino & Raspberry Pi/Linux Devices
https://nrf24.github.io/RF24
GNU General Public License v2.0
2.21k stars 1.02k forks source link

CE set LOW twice consecutively in `startConstantCarrier()` #986

Open 2bndy5 opened 1 month ago

2bndy5 commented 1 month ago

https://github.com/nRF24/RF24/blob/81348ad9b100abf252de6f287a5855c18c0c9cc8/RF24.cpp#L1996-L1997 where the definition of reUseTX() is https://github.com/nRF24/RF24/blob/81348ad9b100abf252de6f287a5855c18c0c9cc8/RF24.cpp#L1325-L1331

I think the problem is that reUseTX() doesn't deactivate the CE pin before it needs to clear the MAX_RT flag. It would make sense to me if the radio is put into standby mode before clearing the MAX_RT flag.

Git blame seems a bit confusing. The consecutive ce() calls were added in https://github.com/nRF24/RF24/commit/77b1e2c1400e755d13bfd9345bd235075b056ad4, but it was closer to what I would expect beforehand in https://github.com/nRF24/RF24/commit/d0ec0c461ce6c2e1f15b334344330cdf8f993417 (except without clearing the MAX_RT flag)

I think this was a copy-n-paste oversight, but the changes are 10 years old 😆. FWIW, the CirPy lib does ce(LOW) -> clear flag -> ce(HIGH) as well.

Propsal

in startConstantCarrier():

- ce(LOW); 
  reUseTX();

then in reUseTX():

+     ce(LOW); //Re-Transfer packet 
      write_register(NRF_STATUS, _BV(MAX_RT)); //Clear max retry flag 
      write_register(REUSE_TX_PL, RF24_NOP, true); 
-     ce(LOW); //Re-Transfer packet 
      ce(HIGH); 

PS - I found this when writing unit tests to ensure expected behavior in pure rust. 😉

TMRh20 commented 1 month ago

Hmm, interesting find! I think you are probably correct in the required order of operations here.

Your rust project seems a bit ambitious, I don't know much rust, but will follow along with interest!