[beken-72xx] Free list returned by wlan_sta_scan_result() #226
Conversation
There were a few things I didn't like about this function: 1) realloc() was called a bit too often. 2) if realloc() failed, the previous memory was not freed. 3) scanAlloc returned previous count or 255 on error. But there was no real check for error and 255 could've been used as index to null. I think it's better to simple return boolean. 4) scanAlloc was clearing memory only up to (and excluding) the new entries.
| @@ -20,7 +20,10 @@ static rtw_result_t scanHandler(rtw_scan_handler_result_t *result) { | |||
| if (!net->SSID.len) | |||
| return RTW_SUCCESS; | |||
There was a problem hiding this comment.
I can see this function always returns RTW_SUCCESS, even on error. Is this intended?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -163,7 +163,7 @@ class WiFiClass { | |||
| ); | |||
|
|
|||
| int16_t scanComplete(); | |||
| uint8_t scanAlloc(uint8_t count); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -20,7 +20,10 @@ static rtw_result_t scanHandler(rtw_scan_handler_result_t *result) { | |||
| if (!net->SSID.len) | |||
| return RTW_SUCCESS; | |||
There was a problem hiding this comment.
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.
kuba2k2
changed the title
kuba2k2
added
BK7231
|
Ok, as you can see I decided to make the scanAlloc() method return the number of succesfully allocated items. This can help if realloc() fails - then we still can use the previously allocated buffer if needed. I have tested this code shortly on bk7231, but not on realtek, since I do not own any realtek device atm (only checked compilation). |
…bretiny-eu#226)" This reverts commit 1d80b5f.
This method might return a dynamically allocated buffer which should be freed by the caller (the example code does it). Not doing this results in memory leaks.