WiFi location service with sample #708

mniestroj · 2024-12-11T19:14:54Z

No description provided.

github-actions · 2024-12-11T19:15:43Z

Visit the preview URL for this PR (updated for commit a0996d7):

https://golioth-firmware-sdk-doxygen-dev--pr708-location-wifi-mz17ai5t.web.app

_{(expires Fri, 17 Jan 2025 15:22:17 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a9993e61697a3983f3479e468bcb0b616f9a0578}

include/golioth/location/wifi_zephyr.h

include/golioth/location/wifi.h

github-actions · 2024-12-16T18:45:36Z

Code Coverage

Package	Line Rate	Branch Rate	Health
include.golioth	75%	50%	✔
port.linux	62%	34%	➖
port.utils	58%	46%	➖
port.zephyr	58%	25%	➖
src	69%	31%	➖
Summary	67% (2719 / 4035)	31% (1126 / 3671)	➖

codecov · 2024-12-16T18:46:06Z

Codecov Report

Attention: Patch coverage is 6.12245% with 92 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/location.c	0.00%	43 Missing ⚠️
src/location_wifi.c	0.00%	36 Missing ⚠️
src/coap_client_libcoap.c	30.00%	6 Missing and 1 partial ⚠️
src/coap_client.c	0.00%	3 Missing ⚠️
src/coap_client_zephyr.c	50.00%	2 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
src/coap_client.c	`45.57% <0.00%> (-0.46%)`	⬇️
src/coap_client_zephyr.c	`57.28% <50.00%> (-0.25%)`	⬇️
src/coap_client_libcoap.c	`54.40% <30.00%> (-0.35%)`	⬇️
src/location_wifi.c	`0.00% <0.00%> (ø)`
src/location.c	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

szczys · 2024-12-17T15:12:02Z

src/coap_client.c

@@ -261,6 +261,34 @@ static enum golioth_status golioth_coap_client_set_internal(struct golioth_clien
    return GOLIOTH_OK;
 }

+enum golioth_status golioth_coap_client_post(struct golioth_client *client,


Do we have anywhere else that we use _post in the user API? I ask because this is a departure from the _set and _set_block API naming we currently have. I wonder if _set_payload or something along those lines would be a good fit?

Yeah... I was thinking about that myself and considered set_payload / set_with_response / set_with_rsp, etc.

My first implementation was about extending set with payload (and payload_size) arguments. This seemed to be obvious choice for internals of our SDK and was the cleanest path forward, without introducing new API (like _post). This turned out to be very verbose for public API that SDK users consume. So... decided to leave those APIs without payload.

Next approach was to implement either _set() equivalent with _set_with_rsp() to describe that payload is provided in that case. That however was very loosely related to our existing _set() APIs, like lightdb_*_set(). Then I was thinking about whether _set_*() is a good choice for API that sends and receives something. More natural is the _post() nomenclature, which is very well known in case of HTTP, CoAP and in general in the world of RESTful APIs.

Decided to go with _post() API, since that maps very good to internal CoAP implementation without introducing new nomenclature. It does what POST method does technically... so it sends data and receives data. It is definitely more than set word describes.

So if we are about changing the (internal) APIs, my bet would be to use _post() in most places and just expose _set() for high-level APIs, like lightdb, where it makes sense. Even for stream service which just sends data now... I think it makes more sense to use golioth_stream__push() or golioth_stream__post() instead of golioth_stream__set().

I also like using _post because it more closely matches our use of _get and _delete elsewhere. I don't love the idea of us now exposing a set_cb and a post_cb as part of the public API though. The post_cb looks very similar to the get_cb (identical?) and the set_cb is currently used for POST and DELETE operations. I wonder if moving to something more like "callback with response" and "callback without response" would make more sense?

This is a good discussion! Here are my thoughts:

I like the idea of using _post() for the internal API. The use of set() here always felt a little odd here, since we are doing POST operations and the other APIs also use the CoAP REST operation names (GET, OBSERVE, DELETE, etc.). We can keep set() for the higher level APIs that don't return a payload like LightDB State and Stream. This change would be out of scope for this PR.

For this PR, I think adding the new API works fine.

I agree with Dan that we have a lot of overlap in the callbacks. I'm working on a change to proxy the callbacks through the upper half client as part of my hackday / holiday hours project. For that change, I'm using one catch-all callback between the lower and upper-half clients. We could do something similar for user callbacks (i.e. use one callback and set the payload to NULL if it is irrelevant for the operation). Or we could rename the callbacks as Dan suggested. Another idea is proxying all callbacks through the higher level services and then using service-specific callbacks. I think that will be easier for users, but it does add a lot of indirection to the callbacks (lower client => upper client => service => user code).

szczys · 2024-12-17T15:14:26Z

src/coap_client.c

+        .content_type = content_type,
+        .callback_post = callback,
+        .arg = callback_arg,
+        .callback_is_post = true,


Perhaps not relevant to this PR, but this callback_is_post member makes me think of the is_observe member we have in some other params structs. Perhaps these could be unified in a type member that all params structs have with an enum that specifies is_post, is_observe, etc.

Sure, I would 100% agree if we would have them in the same structure. Do you think it makes sense to introduce type now even if we have 2 types at most for each struct?

Even if we would switch to types, then I think that observe and post should be defined in separate enum, i.e. separate namespace. They are just different things, so I would prefer to not put them in single location to prevent confusion.

I think you're right about keeping namespaces separate. This is something to think about in future work but I don't think we should make any changes yet.

include/golioth/location.h

hasheddan

Nice work @mniestroj! Left a few comments below.

include/golioth/location.h

include/golioth/location/wifi.h

hasheddan · 2024-12-19T18:21:20Z

src/coap_client.c

@@ -261,6 +261,34 @@ static enum golioth_status golioth_coap_client_set_internal(struct golioth_clien
    return GOLIOTH_OK;
 }

+enum golioth_status golioth_coap_client_post(struct golioth_client *client,


I also like using _post because it more closely matches our use of _get and _delete elsewhere. I don't love the idea of us now exposing a set_cb and a post_cb as part of the public API though. The post_cb looks very similar to the get_cb (identical?) and the set_cb is currently used for POST and DELETE operations. I wonder if moving to something more like "callback with response" and "callback without response" would make more sense?

examples/zephyr/location/wifi/CMakeLists.txt

sam-golioth

Great work @mniestroj! Can we add a twister test for the new sample? I'd like us to get into the habit of adding tests alongside new code rather than adding them retroactively.

include/golioth/location.h

src/Kconfig

include/golioth/location.h

src/location_wifi.c

examples/zephyr/location/wifi/CMakeLists.txt

examples/zephyr/location/wifi/src/wifi_playback.c

sam-golioth · 2025-01-02T22:49:32Z

src/coap_client.c

@@ -261,6 +261,34 @@ static enum golioth_status golioth_coap_client_set_internal(struct golioth_clien
    return GOLIOTH_OK;
 }

+enum golioth_status golioth_coap_client_post(struct golioth_client *client,


This is a good discussion! Here are my thoughts:

I like the idea of using _post() for the internal API. The use of set() here always felt a little odd here, since we are doing POST operations and the other APIs also use the CoAP REST operation names (GET, OBSERVE, DELETE, etc.). We can keep set() for the higher level APIs that don't return a payload like LightDB State and Stream. This change would be out of scope for this PR.

For this PR, I think adding the new API works fine.

I agree with Dan that we have a lot of overlap in the callbacks. I'm working on a change to proxy the callbacks through the upper half client as part of my hackday / holiday hours project. For that change, I'm using one catch-all callback between the lower and upper-half clients. We could do something similar for user callbacks (i.e. use one callback and set the payload to NULL if it is irrelevant for the operation). Or we could rename the callbacks as Dan suggested. Another idea is proxying all callbacks through the higher level services and then using service-specific callbacks. I think that will be easier for users, but it does add a lot of indirection to the callbacks (lower client => upper client => service => user code).

This allows to pass callback and parse returned payload at application layer. Signed-off-by: Marcin Niestroj <[email protected]>

sam-golioth

Changes look good @mniestroj! Left a couple comments about using the location request struct while processing the response. Still missing a twister test for the sample as well.

src/location.c

Introduce support for Golioth WiFi location service. Signed-off-by: Marcin Niestroj <[email protected]>

Add a sample with WiFi access points collected over the city trip. Signed-off-by: Marcin Niestroj <[email protected]>

This driver is helpful for testing without access to hardware with WiFi. Add also native_sim overlays with preconfigured wifi_playback driver. Signed-off-by: Marcin Niestroj <[email protected]>

Signed-off-by: Marcin Niestroj <[email protected]>

mniestroj · 2025-01-10T15:24:43Z

Still missing a twister test for the sample as well.

Working on that :) Which platforms do we want to support? I'm targeting native_sim for now, due to possiiblity to test with playback data. For real WiFi capable platforms we don't really have a reliable way of testing with accesspoint.

sam-golioth · 2025-01-10T15:27:50Z

Still missing a twister test for the sample as well.

Working on that :) Which platforms do we want to support? I'm targeting native_sim for now, due to possiiblity to test with playback data. For real WiFi capable platforms we don't really have a reliable way of testing with accesspoint.

I'm good with just native_sim to start 👍

We might be able to test with other platforms later, since we don't actually care that the location data is accurate, just that we are able to use the APIs, and that the sample code compiles and runs as expected. Making sure the location data is accurate is for the cloud team to test 🙂

mniestroj · 2025-01-10T16:35:02Z

Twister sample test implemented in #725 (it requires dev environment for now due to Location backend service).

sam-golioth

Awesome work @mniestroj!

mniestroj requested review from sam-golioth, szczys and hasheddan and removed request for sam-golioth December 11, 2024 19:14

hasheddan reviewed Dec 11, 2024

View reviewed changes

include/golioth/location/wifi_zephyr.h Outdated Show resolved Hide resolved

include/golioth/location/wifi.h Outdated Show resolved Hide resolved

mniestroj force-pushed the location-wifi branch 11 times, most recently from ff80383 to 623ec88 Compare December 16, 2024 18:33

mniestroj force-pushed the location-wifi branch 6 times, most recently from 83f680a to 1abbd41 Compare December 17, 2024 13:12

szczys reviewed Dec 17, 2024

View reviewed changes

mniestroj force-pushed the location-wifi branch from 1abbd41 to 3732f21 Compare December 17, 2024 15:36

szczys approved these changes Dec 18, 2024

View reviewed changes

mniestroj force-pushed the location-wifi branch from 3732f21 to b300e5f Compare December 18, 2024 20:26

mniestroj force-pushed the location-wifi branch 5 times, most recently from 3d255a2 to b95f216 Compare December 19, 2024 15:21

mniestroj requested a review from hasheddan December 19, 2024 15:31

mniestroj force-pushed the location-wifi branch from b95f216 to ad5601c Compare December 19, 2024 15:37

hasheddan reviewed Dec 19, 2024

View reviewed changes

mniestroj force-pushed the location-wifi branch 2 times, most recently from 7355624 to 8b77188 Compare January 2, 2025 15:00

sam-golioth reviewed Jan 2, 2025

View reviewed changes

coap_client: introduce callback for CoAP POST API with response

3b565a5

This allows to pass callback and parse returned payload at application layer. Signed-off-by: Marcin Niestroj <[email protected]>

mniestroj force-pushed the location-wifi branch 5 times, most recently from ccc2394 to 1f49b15 Compare January 9, 2025 13:26

mniestroj requested a review from sam-golioth January 9, 2025 19:09

sam-golioth reviewed Jan 9, 2025

View reviewed changes

src/location.c Outdated Show resolved Hide resolved

src/location.c Outdated Show resolved Hide resolved

mniestroj added 4 commits January 10, 2025 16:21

location: wifi: initial support

ba3d991

Introduce support for Golioth WiFi location service. Signed-off-by: Marcin Niestroj <[email protected]>

examples: zephyr: location: add simple WiFi location demo

8dd5bba

Add a sample with WiFi access points collected over the city trip. Signed-off-by: Marcin Niestroj <[email protected]>

examples: zephyr: location: introduce wifi_playback driver

8a08831

This driver is helpful for testing without access to hardware with WiFi. Add also native_sim overlays with preconfigured wifi_playback driver. Signed-off-by: Marcin Niestroj <[email protected]>

examples: zephyr: location: wifi: add README

a0996d7

Signed-off-by: Marcin Niestroj <[email protected]>

mniestroj force-pushed the location-wifi branch from 1f49b15 to a0996d7 Compare January 10, 2025 15:21

sam-golioth approved these changes Jan 10, 2025

View reviewed changes

mniestroj merged commit c2ac22d into main Jan 11, 2025
134 of 140 checks passed

mniestroj deleted the location-wifi branch January 11, 2025 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiFi location service with sample #708

WiFi location service with sample #708

mniestroj commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

szczys Dec 17, 2024

mniestroj Dec 17, 2024

hasheddan Dec 19, 2024

sam-golioth Jan 2, 2025

szczys Dec 17, 2024

mniestroj Dec 17, 2024

szczys Dec 18, 2024

hasheddan left a comment

hasheddan Dec 19, 2024

sam-golioth left a comment

sam-golioth Jan 2, 2025

sam-golioth left a comment

mniestroj commented Jan 10, 2025

sam-golioth commented Jan 10, 2025 •

edited

Loading

mniestroj commented Jan 10, 2025

sam-golioth left a comment

WiFi location service with sample #708

WiFi location service with sample #708

Conversation

mniestroj commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 • edited Loading

github-actions bot commented Dec 16, 2024 • edited Loading

Code Coverage

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-golioth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-golioth left a comment

Choose a reason for hiding this comment

mniestroj commented Jan 10, 2025

sam-golioth commented Jan 10, 2025 • edited Loading

mniestroj commented Jan 10, 2025

sam-golioth left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading

sam-golioth commented Jan 10, 2025 •

edited

Loading