nRF24 / RF24Mesh

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

`renewAddress()` returns zero on timeout #211

Closed 2bndy5 closed 2 years ago

2bndy5 commented 2 years ago

I think I know why the examples originally interpreted a zero value returned from renewAddress() as a failure to connect.

https://github.com/nRF24/RF24Mesh/blob/535c38f070c2e538c6f690a18d0c0ee8e5e699c8/RF24Mesh.cpp#L280-L281

If the while loop exits, then the network address is returned which should be 04444 if not connected to mesh.

Proposal

Instead of returning the master node's address (I assume it was originally meant to be a boolean false), the timeout condition should just break out of the while loop (returning 04444).

Alternative Considerations

Otherwise the examples would need to be re-written to interpret both 0 and 04444 as a resulting failure from renewAddress(), but that seems to over-complicate the user code a bit. I'm erring with the "break while" idea since the docs state the function returns a network address (not a boolean).

We could go in the other direction and just make the function strictly return a boolean because users can fetch the assigned address from RF24Mesh::mesh_address. But, this approach might break existing code in the wild if expecting the network address instead of a boolean.

TMRh20 commented 2 years ago

Err, now I know why hot-swapping would sometimes work with the old examples. It WAS returning 0. I agree, it should just break out of the loop and return MESH_DEFAULT_ADDRESS if it fails.

2bndy5 commented 2 years ago

Mind if I just push a simple commit to master? I don't see this function in RF24Ethernet or RF24Gateway src.

I just noticed the Gateway examples should be updated about renewAddress() return data. The Ethernet examples have already been updated.

TMRh20 commented 2 years ago

That'd be great. I'm not sure what you mean regarding the Gateway examples, they just call renewAddress() without looking at the return data from what I can see. They use failure handling to restart the mesh if required.

2bndy5 commented 2 years ago

The ncurses examples use the return value like a bool. They literally do

bool ok = True;

// ...

if ((ok = mesh.rennewAddress())) {
    // ...
}
TMRh20 commented 2 years ago

LOL, I must be blind. I just looked at that thinking it was all cool.