nRF24 / RF24Mesh

OSI Layer 7 Mesh Networking for RF24Network & nrf24L01+ & nrf52x devices
http://nrf24.github.io/RF24Mesh
GNU General Public License v2.0
421 stars 154 forks source link

Problem with setting PA_LEVEL at start of Mesh #242

Closed TMRh20 closed 1 month ago

TMRh20 commented 1 month ago

So this one has been bugging me for a while, the problem is that

  1. There is no way to set the PA Level in the RF24Mesh begin() function
  2. But RF24Mesh calls radio.begin() in that function which resets the PA Level to RF24_PA_MAX https://github.com/nRF24/RF24Mesh/blob/7827e87a01c2de1d63db5f040a981f681b2eada1/RF24Mesh.cpp#L26-L36

In the examples for example, the PA Level is set prior to calling mesh.begin(), but this will just be undone by the call to radio.begin() within mesh.begin() https://github.com/nRF24/RF24Mesh/blob/7827e87a01c2de1d63db5f040a981f681b2eada1/examples/RF24Mesh_Example/RF24Mesh_Example.ino#L49-L69

I see one of 3 scenarios:

  1. Add to the RF24Meshbegin() function, so it can set the PA Level after calling radio.begin()
  2. Remove radio.begin() from RF24Mesh. This adds a some more complications, as it essentially changes the API, requiring users to call radio.begin() prior to every mesh.begin() call.
  3. Leave as is, users can set the PA Level after calling mesh.begin()

I kind of prefer option 1, but would prefer option 2 if it wasn't breaking existing code.

If we choose option one, should the default be PA_MIN or PA_MAX ?

2bndy5 commented 1 month ago

I prefer option 2 since it is consistent with using RF24Network::begin(). There's also a deprecated RF24Network::begin(uint8_t _channel, uint16_t _node_address) which I neglected to remove when releasing v2.0. So, I think we're a bit overdue for some breaking changes.

I seem to remember a rejected PR (#210) about this that instigated changing RF24::begin() so that the PA level was not altered every time it was called. Looking at RF24::_init_radio(), I see https://github.com/nRF24/RF24/commit/8ad2886b846375fca5cfcda74da784d3452d9148 in the git blame.

TMRh20 commented 1 month ago

Hmm, I had a feeling you'd prefer option 2 :laughing:

I didn't realize this was fixed at the RF24 level. I guess the change isn't really required then?

2bndy5 commented 1 month ago

I guess the change isn't really required then?

At a glance, no. Did you recently have some troubling experience that you attributed to this?

TMRh20 commented 1 month ago

Heh, the troubling experience was always adjusting my code to set the PALevel AFTER starting the mesh and again if mesh.begin() was called, which I see is not necessary anymore.

2bndy5 commented 1 month ago

I was suspecting an ebyte module; they don't seem to respect the user-specified PA level.