Level 2 child nodes unable to process Logical Addresses provided by Master node

[Hypecta]

Hi I've been playing around with the RF24Mesh package and found a possible issue relating to the level 2 child nodes.

Issue: Level 2 child nodes are not able to verify that the provided address from the master node is legitimate as the test_addr variable will always be 0.

File Name: rf24_mesh.py
Code Line: 202
Code: level = 0 if contact < 7 else len(oct(contact)[2:])

If level 2 child nodes (e.g. 6th/7th mesh network) were to request for an address from a level 1 child node, the bit-shifting algorithm done to derive test_addr will always be 0 as level will be 0 when contact is less than 7 (master node 0o0 & child nodes 0o1 to 0o5)

new_addr = struct.unpack("<H", self.frame_buf.message[:2])[0]
level = 0 if contact < 7 else len(oct(contact)[2:])
test_addr = new_addr & ~(0xFFFF << (level * 3))

Output of self.frame_buf.header, self.frame_buf.message, and code variables.

=== Frame ===
Header: from 0o4 to 0o4444 type 128 id 1 reserved 8
Message (Encoded): bytearray(b'$\x00')
Message (Decoded): 44

=== Variables ===
contact: 0o4
new_addr: 0o44
level: 0
test_addr: 0o0

If Line 202 of rf24_mesh.py is modified so that level is 1 for contact within the range of 1 - 7, it will allow test_addr to reflect the correct address accordingly for bit shifting to verify the address provided:

level = 0 if contact == 0 else (1 if 0 < contact < 7 else len(oct(contact)[2:]))

Output of self.frame_buf.header, self.frame_buf.message, and code variables.

=== Frame ===
Header: from 0o4 to 0o4444 type 128 id 1 reserved 8
Message (Encoded): bytearray(b'$\x00')
Message (Decoded): 44

=== Variables ===
contact: 0o4
new_addr: 0o44
level: 1
test_addr: 0o4

Do let me know if I need to provide more evidence or outputs to support this.

[2bndy5]

Please bear in mind that this was all ported from the C++ RF24Mesh, so I'll be looking back to that as a source of truth (expected behavior).

The line in question has its C++ roots here:

                    uint16_t mask = 0xFFFF;
                    newAddy &= ~(mask << (3 * getLevel(contactNode[i]))); // Get the level of contact node. Multiply by 3 to get the number of bits to shift (3 per digit)

where the C++ RF24Mesh::getLevel() is defined as

uint8_t ESBMesh<network_t, radio_t>::getLevel(uint16_t address)
{
    uint8_t count = 0;
    while (address) {
        address >>= 3;
        count++;
    }
    return count;
}

I forget why I chose to reduce the original bit-shifting calc for getLevel(), but I suspect it had something to do with python's int capacity and the fact that it is only used once in this address validation algorithm.

Unfortunately, the decision to use an octal string and count the digits to get the level didn't prove useful for nodes on level 0 (the master node). That is why there is a specific exception for level 0 nodes in this source.

I need to review this a bit more, but I feel like your solution only accounts for level 2 nodes.

1. It would be nice to have code that will reproduce this bug you describe.
2. Instead of counting characters in an octal string, maybe it is time to revisit the python port and directly use bit-shifting logic as done in the C++ source.

[2bndy5]

My instinct is to do exactly what the C++ source does. Here is the patch for that idea:

--- a/circuitpython_nrf24l01/rf24_mesh.py
+++ b/circuitpython_nrf24l01/rf24_mesh.py
@@ -183,6 +183,13 @@ class RF24MeshNoMaster(NetworkMixin):
         if not contacts:
             return False

+        def _get_level(address: int) -> int:
+            count = 0
+            while address:
+                address >>= 3
+                count += 1
+            return count
+
         new_addr = None
         for contact in contacts:
             # print("Requesting address from", oct(contact))
@@ -199,8 +206,7 @@ class RF24MeshNoMaster(NetworkMixin):
                     and self.frame_buf.header.reserved == self.node_id        
                 ):
                     new_addr = struct.unpack("<H", self.frame_buf.message[:2])[0]
-                    level = 0 if contact < 7 else len(oct(contact)[2:])       
-                    test_addr = new_addr & ~(0xFFFF << (level * 3))
+                    test_addr = new_addr & ~(0xFFFF << (_get_level(contact) * 3))
                     if test_addr != contact:
                         new_addr = None
                     else:

[2bndy5]

I have uploaded my solution to the resolve-48 branch on this repo.
@Hypecta It would be great if you could git checkout that branch and see if it works.

On first glance, the inline conditional was really just a quick hack I used during development. It should be faster to use just bit shifting math without extra conditional logic wrapping around that. This is why I'm leaning toward the solution in resolve-48 branch.

[Hypecta]

Hi @2bndy5 thanks for the fast fix on this!

I personally installed this project with pip install circuitpython-nrf24l01, hence I had to manually replace the fix you did, and it works!

My current mesh setup is able to provide addresses all the way to the last level (level 4) of nodes.

This is my DHCP output:

Along side with my level 4 child node 0o1112:

Would this suffice as it working? I'd think so!

[2bndy5]

FYI: you can install a git source using pip:

pip install git+https://github.com/nRF24/CircuitPython_nRF24L01.git@resolve-48

should install the resolve-48 branch, although I probably got a typo in there somewhere.

Would this suffice as it working? I'd think so!

Great to hear! Sorry I don't have a network setup to test this on... If it's all the same to you, I'll merge resolve-48 solution and release the bug fix (as v2.1.3).

[2bndy5]

🎉 v2.1.3 is live on pypi. If you're using the adafruit community bundle on MCU devices, then the v2.1.3 should be bundled within 24 hours or so.

BTW, I deleted the resolve-48 branch as it was merged to master.