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.

Sign up for GitHub

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

[beken-72xx] Free list returned by wlan_sta_scan_result() #226

Merged
merged 5 commits into from
Jan 6, 2024
Merged

[beken-72xx] Free list returned by wlan_sta_scan_result() #226

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1485b26
Free list returned by wlan_sta_scan_result()
szupi-ipuzs Dec 22, 2023
b0c9190
scanAlloc improvements
szupi-ipuzs Dec 23, 2023
a85c912
Corrected clearing new entries in scanAlloc
szupi-ipuzs Dec 23, 2023
c119111
scanAlloc() now returns number of allocated items
szupi-ipuzs Jan 4, 2024
7910d09
Fixed compilation issues related to goto.
szupi-ipuzs Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cores/beken-72xx/arduino/libraries/WiFi/WiFiScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ static void scanHandler(void *ctx, uint8_t param) {
}

ScanResult_adv result;
result.ApNum = 0;
result.ApList = NULL;
if (wlan_sta_scan_result(&result)) {
LT_EM(WIFI, "Failed to get scan result");
goto end;
}
LT_IM(WIFI, "Found %d APs", result.ApNum);

cls->scanAlloc(result.ApNum);
if (!scan->ap) {
if (!cls->scanAlloc(result.ApNum)) {
LT_WM(WIFI, "scan->ap alloc failed");
goto end;
}
Expand All @@ -47,6 +48,9 @@ static void scanHandler(void *ctx, uint8_t param) {
scan->running = false;
xSemaphoreGive(cDATA->scanSem);
}
if (result.ApList) {
free(result.ApList);
}
LT_HEAP_I();
return;
}
Expand Down
2 changes: 1 addition & 1 deletion cores/common/arduino/libraries/api/WiFi/WiFi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class WiFiClass {
);

int16_t scanComplete();
uint8_t scanAlloc(uint8_t count);
Copy link
Member

Choose a reason for hiding this comment

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

These are Arduino APIs, we're not supposed to change the function signatures or behavior. As unfortunate as it is (the APIs are pretty badly designed IMHO), doing otherwise may cause undesired problems with compilation if the user's code uses these methods for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was afraid of something like that.
I do not see any description of the return value, but returning the previous count does not allow this method to indicate an error. And, correct me if i'm wrong, failure to allocate a memory should be quite common on systems like that, so it is important to know that and react (eg. by retrying or at least not writing to the memory area).
So, I would strongly suggest changing the API even risking breaking the user's code.

Alternatively, if it is only the method's signature that must remain the same, I suggest returning instead the count of items that we successfully allocated. Then the user can compare it to what was requested and judge by that. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, upon checking I see that there's no such method in ESP32 at all:
https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFi/src/WiFiScan.h

Sorry for the confusion. I must have taken the wifi library from somewhere else then, because I don't think I'd ever write a function with such a weird return value.

Of course, you're free to keep your changes in this case. This function could even be made private now - no other class is supposed to use it anyway.

bool scanAlloc(uint8_t count);
void scanInit();
void scanDelete();

Expand Down
25 changes: 17 additions & 8 deletions cores/common/arduino/libraries/api/WiFi/WiFiScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,23 @@ void WiFiClass::scanDelete() {
scan = NULL;
}

uint8_t WiFiClass::scanAlloc(uint8_t count) {
uint8_t last = scan->count;
scan->count = count;
scan->ap = (WiFiScanAP *)realloc(scan->ap, count * sizeof(WiFiScanAP));
if (!scan->ap)
return 255;
memset(scan->ap + last, 0, sizeof(WiFiScanAP));
return last;
bool WiFiClass::scanAlloc(uint8_t count) {
if ((!scan->ap) || (count > scan->count)) {
auto newMem = (WiFiScanAP *)realloc(scan->ap, count * sizeof(WiFiScanAP));
if (!newMem) {
return false;
}
scan->ap = newMem;
}
if (!scan->ap) {
return false;
}
if (count > scan->count) {
// clear only new entries
memset(scan->ap + scan->count, 0, sizeof(WiFiScanAP) * (count - scan->count));
}
scan->count = count;
return true;
}

String WiFiClass::SSID(uint8_t networkItem) {
Expand Down
5 changes: 4 additions & 1 deletion cores/realtek-amb/arduino/libraries/WiFi/WiFiScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ static rtw_result_t scanHandler(rtw_scan_handler_result_t *result) {
if (!net->SSID.len)
return RTW_SUCCESS;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see this function always returns RTW_SUCCESS, even on error. Is this intended?

Copy link
Member

Choose a reason for hiding this comment

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

Probably; this is a scanning callback called by the SDK, I don't even think it cares about errors. So the return value is most likely meaningless, and returning an error code would likely cause some other problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just checked and it appears that you're right - the SDK completely ignores the value returned by this callback. So for the sake of consistency it's better to always return RTW_SUCCESS.


uint8_t last = cls->scanAlloc(scan->count + 1);
uint8_t last = scan->count + 1;
if (!cls->scanAlloc(last)) {
return RTW_ERROR;
}

scan->ap[last].ssid = strdup((char *)net->SSID.val);
scan->ap[last].auth = securityTypeToAuthMode(net->security);
Expand Down
Loading