search

golioth / golioth-zephyr-sdk

Golioth SDK For Zephyr
https://www.golioth.io
Apache License 2.0
66 stars 19 forks source link

Add asyncronous firmware report API #354

Closed szczys closed 1 year ago

szczys commented 1 year ago

This PR adds an asynchronous API that is safe to call from the system_client thread. This way the firmware state report can be made from the golioth_on_connect() callback.

github-actions[bot] commented 1 year ago

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

https://golioth-zephyr-sdk-doxygen-dev--pr354-szczys-add-async-7dwul4nl.web.app

(expires Wed, 12 Apr 2023 16:28:03 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a389eefadf4b4b68a539327b3459dd66c142cf49

mniestroj commented 1 year ago

@szczys I've merged first commit, which introduces async API. Second commit is still there. The issue I have with it is that it changes sync to async API use, so one is tested instead of the other. I think for this sample sync API makes more sense.

szczys commented 1 year ago

The issue I have with it is that it changes sync to async API use, so one is tested instead of the other.

This was also a concern of mine that both the sync and async be tested. In this commit, I have changed the initial firmware report to async, but sync is still used when reporting the other other states (DOWNLOADING, DOWNLOADED, UPDATING).

The issue I have with this change is that it makes what is already a somewhat complex sample and adds more complexity, really just for the purpose of testing the async function. (It also demonstrates the function, but my intent was to include the new code into the existing testing routine.)

Maybe we should think about automated OTA testing (Hardware in the Loop via CI/CD) and move this change there?

mniestroj commented 1 year ago

The issue I have with this change is that it makes what is already a somewhat complex sample and adds more complexity,

Agree. Let's leave this sample minimal (and even simplify if possible).

Maybe we should think about automated OTA testing (Hardware in the Loop via CI/CD) and move this change there?

Definitely a good idea. We have a LightDB specific tests application, which invokes various APIs for that service: https://github.com/golioth/golioth-zephyr-sdk/tree/main/tests/lightdb. An equivalent for DFU would be great. In such dedicated application we can focus on using and testing all APIs, without keeping it easy for first-time users to understand.