lib: location: Use k_sem_give() when cancelling Wi-Fi location #24867
Conversation
NordicBuilder
added
the
changelog-entry-required
|
Test still needs som massaging |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: c892b0d7674d18d99c000b1ef9ae4ce318cb1e5d more detailssdk-nrf:
Github labels
List of changed files detected by CI (2)Outputs:ToolchainVersion: a7529a11f4 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
simensrostad
force-pushed
the
bug/scan_wifi
branch
from
3717972 to
a8f170d
Compare
If location is cancelled during Wi-Fi search, the semaphore blocking the location positioning workqueue is not released due to k_sem_reset() being used instead of k_sem_give(). This prevents subsequent location requests to start Wi-Fi scans. This commit changes k_sem_reset() to k_sem_give() and adds a test for this scenario. Signed-off-by: Simen S. Røstad <[email protected]>
simensrostad
force-pushed
the
bug/scan_wifi
branch
from
a8f170d to
c892b0d
Compare
simensrostad
changed the title
simensrostad
added
bug
|
You can find the documentation preview for this PR here. |
| /* Next time location is requested, the Wi-Fi scan semaphore should be available again | ||
| * and scan can be started again. | ||
| */ | ||
| __cmock_net_mgmt_NET_REQUEST_WIFI_SCAN_ExpectAndReturn(0); |
There was a problem hiding this comment.
The test doesn't pass. You need to do this here too:
net_mgmt_NET_REQUEST_WIFI_SCAN_occurred = false;
The change will make this test pass but the state information is "wrong" within the library so there are some later tests that will fail likely because there is a pending WiFi scan ongoing.
Also, it would be nice that the test would succeed with the change to k_sem_give from k_sem_reset, and fail without it. I don't think that's the case now.
| { | ||
| if (scan_wifi_ready != NULL) { | ||
| k_sem_reset(scan_wifi_ready); | ||
| k_sem_give(scan_wifi_ready); |
There was a problem hiding this comment.
Is there a log which would show a scenario that this fixes. I can imagine what it fixes but I think we would need to fix the caller side. We are likely doing operations that we shouldn't do after the location request has been cancelled, such as taking the semaphore for waiting for Wi-Fi scan results.
There was a problem hiding this comment.
I found some logs from Jira.
k_sem_give makes things a little better although k_sem_reset is more appropriate in cancel operations in general. In this case, k_sem_take is not interested in the return value which is why k_sem_give happens to work better.
We can merge this once the test uses similar timing as the original problem. However, we would need additional investigation on whether method_cloud_location_positioning_work_fn still has some issues.
| #if defined(CONFIG_LOCATION_METHOD_WIFI_NET_IF_UPDOWN) | ||
| ret = scan_wifi_startup_interface(wifi_iface); | ||
| if (ret) { | ||
| LOG_ERR("Failed to start Wi-Fi interface: %d", ret); |
There was a problem hiding this comment.
This is a duplicate log to what scan_wifi_startup_interface logs, right?
If location is cancelled during Wi-Fi search, the semaphore
blocking the location positioning workqueue is not released due to
k_sem_reset() being used instead of k_sem_give(). This prevents
subsequent location requests to start Wi-Fi scans.
This commit changes k_sem_reset() to k_sem_give() and adds a test
for this scenario.
LRCS-343