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.

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

Add settings cancel API #481

Merged
merged 2 commits into from
May 22, 2024
Merged

Add settings cancel API #481

merged 2 commits into from
May 22, 2024

Conversation

szczys
Copy link
Contributor

@szczys szczys commented May 6, 2024

Add API to cancel settings observations and free the settings service context.

Resolves https://github.com/golioth/firmware-issue-tracker/issues/521

Copy link

github-actions bot commented May 6, 2024

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

https://golioth-firmware-sdk-doxygen-dev--pr481-szczys-add-set-nhf2tzkh.web.app

(expires Wed, 29 May 2024 03:32:51 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a9993e61697a3983f3479e468bcb0b616f9a0578

@szczys szczys marked this pull request as draft May 6, 2024 18:02
@szczys szczys force-pushed the szczys/add_rpc_cancel branch 3 times, most recently from 7adb16d to 56a80b0 Compare May 8, 2024 02:07
src/settings.c Outdated
@@ -463,6 +463,21 @@ struct golioth_settings *golioth_settings_init(struct golioth_client *client)
return gsettings;
}

enum golioth_status golioth_settings_deinit(struct golioth_client *client,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the RPC PR: We can extract client from settings so this function only needs to take one argument.


golioth_sys_sem_take(connected_sem, GOLIOTH_SYS_WAIT_FOREVER);

perform_settings_registration(client);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change this function to return struct golioth_settings * then we can keep settings as a local stack variable instead of moving it to a global.

@szczys szczys force-pushed the szczys/add_rpc_cancel branch 3 times, most recently from 89d18f7 to 6fa46b1 Compare May 21, 2024 16:11
@szczys szczys force-pushed the szczys/add_settings_cancel branch from b8ec242 to 1bc20f4 Compare May 21, 2024 16:37
Base automatically changed from szczys/add_rpc_cancel to main May 21, 2024 18:11
@szczys szczys marked this pull request as ready for review May 21, 2024 18:11
@szczys szczys force-pushed the szczys/add_settings_cancel branch 2 times, most recently from a4a4e7a to 619c0ee Compare May 21, 2024 22:48
Add golioth_settings_deinit() function that cancels Settings observations
and frees the Settings context struct (used to store the registered
Settings paths).

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys force-pushed the szczys/add_settings_cancel branch from 619c0ee to 8441a37 Compare May 22, 2024 03:31
@szczys szczys requested a review from sam-golioth May 22, 2024 03:34
Copy link

Code Coverage

Type Coverage
lines 58.7% (1264 of 2152 lines)
functions 66.0% (126 of 191 functions)

Copy link
Contributor

@sam-golioth sam-golioth left a comment

Choose a reason for hiding this comment

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

Looks good!

@szczys szczys merged commit c8fecdb into main May 22, 2024
38 of 42 checks passed
@szczys szczys deleted the szczys/add_settings_cancel branch May 22, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants