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

mesh.checkConnection via parent not master #250

Merged
merged 9 commits into from
Jun 30, 2024
Merged

mesh.checkConnection via parent not master #250

merged 9 commits into from
Jun 30, 2024

Conversation

TMRh20
Copy link
Member

@TMRh20 TMRh20 commented Jun 28, 2024

These changes tend to allow more nodes on a network, especially helping distant nodes (far from master node) with verifying connectivity as they only have to verify a connection with their parent node, instead of the master node. Tends to help with keeping the mesh together as nodes closer to the master node change their postition and/or address as the mesh changes physically or logically.

Since this fundamentally changes behaviour of the mesh, I've tested the changes pretty well, and its looking good for these changes. I'm seeing greater connectivity, less packet loss, and a mesh that behaves a bit better.

Its not 5-9s, but its probably about as good as it gets after 1 day of testing with most nodes upgraded to this new functionality:
PingGuage3

- Adjust mesh.checkConnection function to verify connectivity with parent only. Leave old functionality in place with a #define
- Suggestions in place per @2bndy5
closes #249
@TMRh20 TMRh20 requested a review from 2bndy5 June 28, 2024 10:14
RF24Mesh.cpp Outdated Show resolved Hide resolved
RF24Mesh_config.h Outdated Show resolved Hide resolved
RF24Mesh_config.h Outdated Show resolved Hide resolved
RF24Mesh_config.h Outdated Show resolved Hide resolved
Specifically for PRs that only change the lib src code and not examples.
use doxygen references
github-actions[bot]

This comment was marked as resolved.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot dismissed their stale review June 28, 2024 19:27

outdated suggestion

Copy link
Contributor

github-actions bot commented Jun 28, 2024

Memory usage change @ 8c494ae

Board flash % RAM for global variables %
arduino:avr:nano ❔ -152 - +70 -0.49 - +0.23 0 - 0 0.0 - 0.0
arduino:samd:mkrzero 🔺 0 - +44 0.0 - +0.02 0 - 0 0.0 - 0.0
Click for full report table
Board examples/RF24Mesh_Example
flash
% examples/RF24Mesh_Example
RAM for global variables
% examples/RF24Mesh_Example_Master_Statics
flash
% examples/RF24Mesh_Example_Master_Statics
RAM for global variables
% examples/RF24Mesh_Example_Master_To_Nodes
flash
% examples/RF24Mesh_Example_Master_To_Nodes
RAM for global variables
% examples/RF24Mesh_Example_Node2Node
flash
% examples/RF24Mesh_Example_Node2Node
RAM for global variables
% examples/RF24Mesh_Example_Node2NodeExtra
flash
% examples/RF24Mesh_Example_Node2NodeExtra
RAM for global variables
% examples/RF24Mesh_SerialConfig
flash
% examples/RF24Mesh_SerialConfig
RAM for global variables
% examples/RF24Mesh_Example_Master
flash
% examples/RF24Mesh_Example_Master
RAM for global variables
%
arduino:avr:nano -152 -0.49 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 64 0.21 0 0.0 70 0.23 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
arduino:samd:mkrzero 40 0.02 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0 40 0.02 0 0.0 44 0.02 0 0.0 0 0.0 0 0.0 0 0.0 0 0.0
Click for full report CSV
Board,examples/RF24Mesh_Example<br>flash,%,examples/RF24Mesh_Example<br>RAM for global variables,%,examples/RF24Mesh_Example_Master_Statics<br>flash,%,examples/RF24Mesh_Example_Master_Statics<br>RAM for global variables,%,examples/RF24Mesh_Example_Master_To_Nodes<br>flash,%,examples/RF24Mesh_Example_Master_To_Nodes<br>RAM for global variables,%,examples/RF24Mesh_Example_Node2Node<br>flash,%,examples/RF24Mesh_Example_Node2Node<br>RAM for global variables,%,examples/RF24Mesh_Example_Node2NodeExtra<br>flash,%,examples/RF24Mesh_Example_Node2NodeExtra<br>RAM for global variables,%,examples/RF24Mesh_SerialConfig<br>flash,%,examples/RF24Mesh_SerialConfig<br>RAM for global variables,%,examples/RF24Mesh_Example_Master<br>flash,%,examples/RF24Mesh_Example_Master<br>RAM for global variables,%
arduino:avr:nano,-152,-0.49,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,64,0.21,0,0.0,70,0.23,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0
arduino:samd:mkrzero,40,0.02,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0,40,0.02,0,0.0,44,0.02,0,0.0,0,0.0,0,0.0,0,0.0,0,0.0

Copy link
Member

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling for Arduino Nano

The generic non-master node example shows a significant decrease in flash memory (-152 bytes). For some reason the Node2Node examples both show a slight increase in flash memory (+64 and +70 bytes). It isn't apparent why the flash memory increased for the Node2Node examples, but I think the generic non-master node example is a more relevant result.

Compiling for cortex-based MCU

The ATSAMD21 showed negligible increase in flash memory (+40 to +44 bytes) on generic non-master node and Node2Node examples, but that family of processors is well equipped for any changes in compile size.

also defaults RF24_DRIVER to SPIDEV (per nRF24/RF24#971)
@2bndy5
Copy link
Member

2bndy5 commented Jun 28, 2024

I also added the ability to use the old behavior in CMake builds:

RF24Mesh/CMakeLists.txt

Lines 146 to 149 in 458e964

if(DEFINED RF24MESH_CONN_CHECK_TYPE)
message(STATUS "RF24MESH_CONN_CHECK_TYPE set to ${RF24MESH_CONN_CHECK_TYPE}")
target_compile_definitions(${LibTargetName} PUBLIC RF24MESH_CONN_CHECK_TYPE=${RF24MESH_CONN_CHECK_TYPE})
endif()

cd RF24Mesh/build
cmake .. -DRF24MESH_CONN_CHECK_TYPE=RF24MESH_CONN_CHECK_MASTER
make

I haven't actually tested this yet. It might be more effective to add the CMake option to examples_RPi/CMakeLists.txt (instead of root/lib CMakeLists.txt).

@2bndy5
Copy link
Member

2bndy5 commented Jun 28, 2024

It might be more effective to add the CMake option to examples_RPi/CMakeLists.txt (instead of root/lib CMakeLists.txt).

I think this would apply to any macro wrapped with #ifndef guards. This idea would be better suited to a different PR. For now, I'll leave it as is until I get around to investigating further. Currently, the Linux builds are working fine.

@2bndy5
Copy link
Member

2bndy5 commented Jun 28, 2024

Would it make sense to return true if checkConnection() is called on a master node?

As long as the master node's radio is idling in RX, I think this would be interpreted as "connected to the mesh." Otherwise, user code written for both master and child nodes might end up calling renewAddress() on master node if their code doesn't understand that it is running the master node. I see no considerations in renewAddress() (& its helper requestAddress()) when the node ID is set to 0.

@TMRh20
Copy link
Member Author

TMRh20 commented Jun 28, 2024

Damn, you've been busy @2bndy5

Would it make sense to return true if checkConnection() is called on a master node?

I was thinking the same thing.

Will have to look at your other comments tomorrow probably, but at a glance they all add up.

2bndy5 added a commit to nRF24/CircuitPython_nRF24L01 that referenced this pull request Jun 28, 2024
2bndy5 added a commit to nRF24/CircuitPython_nRF24L01 that referenced this pull request Jun 28, 2024
2bndy5 added a commit to nRF24/CircuitPython_nRF24L01 that referenced this pull request Jun 29, 2024
@TMRh20
Copy link
Member Author

TMRh20 commented Jun 30, 2024

Thanks for making the necessary changes!

Been testing extensively and there are some drawbacks:

  • Nodes tend to stay connected at one mesh location for longer. This can be good, but if nodes with somewhat spotty connections attach as direct child nodes of the master, they may hold their position for longer.
  • Distant nodes with a spotty connection to master may also hold their position longer, even though they are having connectivity issues.
  • These drawbacks can be somewhat mitigated by additional connection checking/renewal of addresses if connections are failing. For example, when using RF24Mesh directly, I just use a timer to verify connectivity every 10-30 seconds. It may also be beneficial to verify if writes/connections are failing and renew the address upon multiple failures. With RF24Ethernet, I've added a line to many of my nodes to always renew the address if unable to connect to MQTT.
  • Nodes can get 'lost'. If an address is set to 0 or the mesh is quickly restarted, nodes may be able to verify their connectivity with their parent node, but have no bi-directional communication with the master node. Again it may be suitable to verify connectivity with the master node periodically, or leave the master node off for long enough so all nodes are in a state of renewal to work around this issue.

@TMRh20 TMRh20 merged commit 2f5aea2 into master Jun 30, 2024
60 checks passed
@TMRh20 TMRh20 deleted the ConnCheckParent branch June 30, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential mesh.checkConnection() improvement
2 participants