[BUG] ScanNetworksResponse returns duplicated networks and return list is limited

[AlanLCollins]

Reproduction steps

1. Create an environment with multiple (e.g four) Thread networks in different channels. Each network with multiple Thread routers. (+20 different routers distributed across multiple thread networks)
2. Perform Matter commissioning for a new device (e.g. lighting-app). So ScanNetworks command is sent from chip tool , and the accessory device returns ScanNetworksResponse.

Results:

• The new device will print in the serial console +20 MLE Discovery responses from each Router in the vicinity.
• The ScanNetworksResponse will have limited number of networks depending on platform used (Nordic =15, Silabs =5) ,and most of those entries will be duplicated networks.
• Most likely you won't get visibility of the 4 thread networks.

Proposed solution:

• remove duplicated Networks from the ScanNetworksResponse
• SDK to default the number of entries of the ScanNetworksResponse so no differences across platforms

Bug prevalence

always

GitHub hash of the SDK that was being used

SDK1.0

Platform

efr32, nrf

Platform Version(s)

No response

Anything else?

No response

[Damian-Nordic]

Isn't your problem already fixed by https://github.com/project-chip/connectedhomeip/pull/23492/files?

[AlanLCollins]

Hello @Damian-Nordic , thank you for the pointer. Yes, that code should fix the most critical issue. However I don't fully understand this section of the of code:

if (scanResponseArray[i].rssi < scanResponse.rssi)
{
    scanResponseArray[i] = scanResponseArray[--scanResponseArrayLength];
}

If the new entry is found to be a duplicate, the higher RSSI should be kept so this seems better:
scanResponseArray[i] = scanResponse.rssi;
Otherwise, in corner cases, the list will continue having duplicates.
I am in contact with @yhoyoon to deep dive into this.

[Damian-Nordic]

@AlanLCollins This code simply cancels/removes the existing entry (by replacing it with the last element and decrementing the array size) for a given PAN ID so that it can be re-added later. So it's equivalent to what you proposed, right?

[yhoyoon]

@AlanLCollins, @Damian-Nordic

I will update the answer to this issue and request that this issue be closed.
It seems to just delete the existing duplicate item while replacing it with the last item,

if (scanResponseArray[i].rssi < scanResponse.rssi)
{
    scanResponseArray[i] = scanResponseArray[--scanResponseArrayLength];
}

but it later adds and sorts the item.

scanResponseArray[scanResponseArrayLength - 1] = scanResponse;
Sorting::InsertionSort(scanResponseArray.Get(), scanResponseArrayLength,
                       [](const ThreadScanResponse & a, const ThreadScanResponse & b) -> bool { return a.rssi > b.rssi; });

I added the code above for minor optimization, so it should work well.

• ref: Fix thread commissioning fail where there are many Thread Border Routers #23492 (comment)

I already tested it in the environment below, and I confirmed that it works well through my Commissioner's log.

• 50+ Thread Border Routers
• Thread Commissionee Device
  • lock-app on nrf52840 dk
  • lock-app on silabs BRD4162A

@AlanLCollins , as I said several times on Slack, I think you ...

1. just looked at the code and didn't actually test it, or
2. tested, but looking at the scan list before sorting them from your Thread Commissionee Device's log

then, you reported this issue. You should check the sorted scan list in your Commissioner's log or your Thread Commissionee Device's log after adding scan results logging function in the right place. The Thread Commissionee Device's OpenThread stack prints the scan results, this is the results before sorting and removing duplicates from the code in my PR. If you did the test, I think you would have seen this log.

Please close this issue after fully understanding the code or testing properly. If you have additional questions, please leave a comment.

[AlanLCollins]

Closing this ticket, as the PR shared addresses the most critical issue. The extra analysis/concern of the PR itself comes from doing code review of the change. I need to come back with actual tests to be certain if there is a real issue.
Thank you all for your patience while sharing your thoughts on this matter.