Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

write(...) returns false on sucess unless stopListening() is called first. #734

Closed
asafteirobert opened this issue Feb 25, 2021 · 5 comments

Comments

@asafteirobert
Copy link

asafteirobert commented Feb 25, 2021

2bndy5@5ba9b5f#diff-d0133d6229b2d23257dc3cd4fb7822b96ae4b8f54d1095ca23abf223582f7968R661
This change has the side effect of making write(...) return false on success unless stopListening() is called first (to correctly set EN_RXADDR).

This is a regression as my previous code used to work correctly without the call.
Auto ACK is documented to be enabled by default so it's excepted that ACKs work correctly, but by closing all receiving pipes by default in begin(), ACKs are never received.

The way this fails is quite hard to debug, since "write(...) returns false on success" is a common issue.

I was pulling my hair out for 3 days going through the usual "make sure your chip is powered properly", "make sure your connections are done properly", "it's probably just a fake chip".

  nrfRadio.begin();
  nrfRadio.setChannel(NRF_CHANNEL);
  nrfRadio.setPALevel(RF24_PA_MAX);
  nrfRadio.setDataRate(RF24_250KBPS);
  nrfRadio.enableAckPayload();
  nrfRadio.enableDynamicPayloads();
  nrfRadio.setRetries(15, 15);
  nrfRadio.openWritingPipe(NRF_PIPE);
  //nrfRadio.stopListening(); <<< unless I call this, the chip is left in a weird state where ACKs don't work even though ACKs should be enabled by default.
@2bndy5
Copy link
Member

2bndy5 commented Feb 25, 2021

According to the datasheet, the data pipes 0 & 1 are open after PoR (power-on-reset), but I chose to close them since stopListening() will open pipe 0 (the TX pipe) weather auto-ack is enabled or not. The lib's docs insist that you call stopListening() before your first call to write() specifically because underlying pipe 0 management.

There are 2 reasons this could be a problem:

  1. Your code calls begin() more than once. This is not normal, and any scenario that requires begin() be called multiple times on the same SPI bus would be an edge case.
  2. You reset the MCU, and your program (that behaves like a TX only node) never calls stopListening(). Again, the lib's docs/examples say that stopListening() should be called before write*().

Remember that not all MCU's cut power to the attached peripherals when the MCU's reset button is pressed. This is why I made begin() brute force a soft reset on the radio. (that and a soft reset helps address #401)

PS - The docs/examples show proper usage of stopListening() prior to the commit you referenced, so there was no backward compatibility concerns.

@asafteirobert
Copy link
Author

Making begin() force a soft reset is a reasonable goal, I'm not saying revert the change. The fix mentioned in #735 sounds good.

My main problem with the issue is that by default it leaves the chip in a state no one wants. (ACKs are enabled but just don't work). The library should avoid leaving the chip in a state that doesn't make any sense. Either set it by default in TX mode with ACKs disabled, or TX mode with ACKs enabled, or RX mode. "TX mode with ACKs enabled but actually unable to actually receive ACKs" makes no sense.
This is a high level library(used a lot by hobbyists and beginners) so when reasonable it should "just work", and try to be written in a way that avoids problems or makes them obvious.

It's reasonable(even if incorrect) to assume that stopListening() should only need to be called if startListening() was called first.

@2bndy5
Copy link
Member

2bndy5 commented Feb 26, 2021

Excellent feedback! I have a few notes though.

My main problem with the issue is that by default it leaves the chip in a state no one wants

Actually, I wanted this state when testing the new examples on multiple radios driven by different MCUs. I found that without an initial inactive state, my payloads were getting intercepted by radios that weren't supposed to be active, and hitting the reset button or re-uploading the program/sketch didn't help.

try to be written in a way that avoids problems or makes them obvious

The naming convention wasn't my choice, but we work with what maniacBug provided from initial release (for backward compatibility).

It's reasonable (even if incorrect) to assume that stopListening() should only need to be called if startListening() was called first.

Again, this is a naming convention problem that can't be properly addressed without breaking backward compatibility. The *Listening() functions do more than just manage the radio's RX role, as I mentioned about pipe 0 in my first response concerning the TX role. When you dig deeper into the datasheet, you'll realize that auto-ack feature quickly toggles the radio from TX mode to RX mode to receive the ACK packets (on pipe 0) and then back to TX mode.

I'm glad the fix in #735 pleases you, but the fact remains: calling stopListening() before write*() has been mandated for a very long time.

@2bndy5
Copy link
Member

2bndy5 commented Feb 26, 2021

Merged #735. Closing this. Expect changes in next release

@2bndy5 2bndy5 closed this as completed Feb 26, 2021
@asafteirobert
Copy link
Author

Thanks for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants