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

Enhance requestAddress by looping through poll responses #238

Merged
merged 4 commits into from
Jun 9, 2024

Conversation

TMRh20
Copy link
Member

@TMRh20 TMRh20 commented Jun 8, 2024

  • When beginning to request an address, the requesting node will gather poll responses. Utilize all poll responses: If an address request fails, continue looping through available poll responses until all address requests at that level have been exhausted

Old Behaviour Example:

  • Node sends NETWORK_POLL
  • Node recieves multiple Poll Responses
  • If response received from one or more nodes, request an address, else return 0
  • If address response fails, return 0

New Behaviour Example:

  • Node sends NETWORK_POLL
  • Node recieves multiple Poll Responses
  • If response received from one or more nodes, request an address, else loop through poll requests
  • If address response fails, loop through available poll responses
  • Return 0 if all poll responses have been exhausted

Still testing this new code, but it basically involves moving one bracket and changing some return statements to continues.

- When beginning to request an address, the requesting node will gather poll responses. Utilize all poll responses: If an address request fails, continue looping through available poll responses until all address requests at that level have been exhausted
- Move gotResponse variable into loop so it defaults to 0
@2bndy5
Copy link
Member

2bndy5 commented Jun 8, 2024

It's hard to believe we didn't pick up on this before. I can't think of any disadvantages. Maybe users might observe a slower mesh connecting performance when the mesh is well populated, but this feels like the proper approach regardless.

@TMRh20
Copy link
Member Author

TMRh20 commented Jun 8, 2024

Haha, I know right? The whole polling system was built on having multiple polls, so why we didn't exploit that fact before is beyond me. I think it should offer better performance in most situations, since one problem with a populated mesh is receiving polls from the wrong 'level' when poll responses are delayed.

@TMRh20 TMRh20 marked this pull request as ready for review June 8, 2024 18:33
@TMRh20 TMRh20 requested a review from 2bndy5 June 8, 2024 18:33
@2bndy5
Copy link
Member

2bndy5 commented Jun 9, 2024

Looking at the whole function body, I think we can optimize the while loop that collects poll responses.

RF24Mesh/RF24Mesh.cpp

Lines 325 to 356 in 7c30deb

while (1) {
#if defined(MESH_DEBUG)
bool goodSignal = radio.testRPD();
#endif
if (network.update() == NETWORK_POLL) {
memcpy(&contactNode[pollCount], &network.frame_buffer[0], sizeof(uint16_t));
if (pollCount > 0 && contactNode[pollCount] != contactNode[pollCount - 1]) { //Drop duplicate polls to help prevent duplicate requests
++pollCount;
}
else {
++pollCount;
}
IF_MESH_DEBUG(printf_P(PSTR("%u: MSH Poll %c -64dbm\n"), millis(), (goodSignal ? '>' : '<')));
} // end if
if (millis() - timr > 55 || pollCount >= MESH_MAXPOLLS) {
if (!pollCount) {
IF_MESH_DEBUG(printf_P(PSTR("%u: MSH No poll from level %d\n"), millis(), level));
return 0;
}
else {
IF_MESH_DEBUG(printf_P(PSTR("%u: MSH Poll OK\n"), millis()));
break;
}
}
MESH_CALLBACK
} // end while
IF_MESH_DEBUG(printf_P(PSTR("%u: MSH Got poll from level %d count %d\n"), millis(), level, pollCount));

#if defined(MESH_DEBUG)
    bool goodSignal = radio.testRPD();
#endif
    while (millis() - timr < 55 && pollCount < MESH_MAXPOLLS) {
        if (network.update() == NETWORK_POLL) {
            uint16_t contact = 0;
            memcpy(&contact, &network.frame_buffer[0], sizeof(contact));

            // Drop duplicate polls to help prevent duplicate requests
            bool isDupe = false;
            for (uint8_t i = 0; i < pollCount; ++i) {
                if (contact == contactNode[i]) {
                    isDupe = true;
                    break;
                }
            }
            if (!isDupe) {
                contactNode[pollCount] = contact;
                ++pollCount;
            }

            IF_MESH_DEBUG(printf_P(PSTR("%u: MSH Poll %c -64dbm\n"), millis(), (goodSignal ? '>' : '<')));
        } // end if

        MESH_CALLBACK
    } // end while

    IF_MESH_DEBUG(printf_P(PSTR("%u: MSH Got poll from level %d count %d\n"), millis(), level, pollCount));
    if (!pollCount) return 0;

@2bndy5
Copy link
Member

2bndy5 commented Jun 9, 2024

BTW, I think it is more performant to

uint32_t timeout = millis() + 225;
while (millis() < timeout) { /* ... */ }

instead of

uint32_t timr = millis();
while (millis() - timr < 225) { /* ... */ }

because the arithmetic for calculating the time diff is abstracted away from each iteration of the loop.

- Per @2bndy5 in #238
- Change debug prints: Remove millis calls and improve human readability
- Adjust timeout calculations to be calculated out of loop
- Change main polling loop per @2bndy5
@TMRh20
Copy link
Member Author

TMRh20 commented Jun 9, 2024

Updated per your suggestions but slightly modified:

  • Had to remove millis() from printf calls due to corruption of the output again, similar to RF24Network
  • Made printfcalls more human readable
  • Put testRPD debug info inside Poll loop
  • Put RPD results debug info inside !isDupe logic

RF24Mesh.cpp Outdated Show resolved Hide resolved
RF24Mesh.cpp Show resolved Hide resolved
RF24Mesh.cpp Show resolved Hide resolved
- Use same timeout variable
- Add return 0 if 0 polls received
@TMRh20 TMRh20 merged commit f97a9bf into master Jun 9, 2024
17 checks passed
@TMRh20 TMRh20 deleted the EnhanceReqAddress branch June 9, 2024 12:27
2bndy5 pushed a commit that referenced this pull request Jun 9, 2024
* Enhance request address

- When beginning to request an address, the requesting node will gather poll responses. Utilize all poll responses: If an address request fails, continue looping through available poll responses until all address requests at that level have been exhausted

* Move gotResponse variable

- Move gotResponse variable into loop so it defaults to 0

* - Further improve requestAddress

- Per @2bndy5 in #238
- Change debug prints: Remove millis calls and improve human readability
- Adjust timeout calculations to be calculated out of loop
- Change main polling loop per @2bndy5

* Mods per @2bndy5 in #238

- Use same timeout variable
- Add return 0 if 0 polls received
@2bndy5
Copy link
Member

2bndy5 commented Jun 9, 2024

FYI, I cherry picked the merge commit from this PR onto the 1.x branch. If I overstepped (or somehow screwed it up), then it can be reverted.

@TMRh20
Copy link
Member Author

TMRh20 commented Jun 10, 2024

I keep forgetting about the 1.x branch. Glad you remembered!

@2bndy5 Are we about due for a release crusade? Its been around a year since the last release...

@2bndy5
Copy link
Member

2bndy5 commented Jun 10, 2024

Yeah, we probably should. Although, the only real changes that affect Arduino platform (which are directly influenced by our tagged commits/releases) are the network layers. The RF24 lib itself just has doc & example updates; the rest is specific to Linux (nRF24/RF24@v1.4.8...master).

I gotta get back to investigating the piwheels' 32-bit build problem with pyRF24 deployments (nRF24/pyRF24#61)...

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.

2 participants