search

golioth / golioth-zephyr-sdk

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

samples: move parts of prj.conf to Kconfig.defconfig #382

Closed mniestroj closed 1 year ago

mniestroj commented 1 year ago

The general idea is to reduce duplication across different samples' prj.conf files and configure common settings using Kconfig default values.

github-actions[bot] commented 1 year ago

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

https://golioth-zephyr-sdk-doxygen-dev--pr382-prj-to-kconfig-zwxijvh1.web.app

(expires Mon, 29 May 2023 15:45:15 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a389eefadf4b4b68a539327b3459dd66c142cf49

szczys commented 1 year ago

I think moving these configurations out of the prj.conf makes them less visible to the user. When I'm working on adding a feature to an existing application I often look at the prj.conf in our sample files as a clue to what Kconfig symbols may also be needed.

For the main process thread it's not too hard to figure out for yourself. But for the mbedtls options I think it's quite helpful.

mniestroj commented 1 year ago

I think moving these configurations out of the prj.conf makes them less visible to the user. When I'm working on adding a feature to an existing application I often look at the prj.conf in our sample files as a clue to what Kconfig symbols may also be needed.

For the main process thread it's not too hard to figure out for yourself. But for the mbedtls options I think it's quite helpful.

That is a good point. This is exactly the reason why I haven't done that before. The thing is... we are trying to support both vanilla Zephyr and NCS at the same time. This is almost impossible to do with prj.conf approach, since almost all mbedTLS configuration options are different in Zephyr and different in NCS (they use their own fork of mbedTLS, with their Kconfig files).

So during the work on NCS and cert-based auth, I figured that moving some options from prj.conf to Kconfig.defconfig would help cleanup the dependencies. The alternative would be to move from prj.conf to boards/BOARD.conf files, which would increase duplication (instead of decreasing it), probably making it harder to maintain in the long term.

See https://github.com/golioth/golioth-zephyr-sdk/pull/383 which is built on top of this PR. This adds (or fixes) support for cert-based auth in NCS.

TLDR; I'd prefer not to do this if I would have better/alternative solution. Any ideas are welcome.

mniestroj commented 1 year ago

Actually the alternative approach would be moving from prj.conf to snippets, as in https://github.com/golioth/golioth-zephyr-sdk/pull/384. However, this is not supported yet in NCS (maybe in next release it will) and there are issues with applying samples.yaml configuration overrides, which would break twister builds/tests.

sam-golioth commented 1 year ago

Actually the alternative approach would be moving from prj.conf to snippets, as in #384. However, this is not supported yet in NCS (maybe in next release it will) and there are issues with applying samples.yaml configuration overrides, which would break twister builds/tests.

Why do snippets work for the changes in #384 but not the changes in this PR?

mniestroj commented 1 year ago

Actually the alternative approach would be moving from prj.conf to snippets, as in #384. However, this is not supported yet in NCS (maybe in next release it will) and there are issues with applying samples.yaml configuration overrides, which would break twister builds/tests.

Why do snippets work for the changes in #384 but not the changes in this PR?

Zephyr snippets is a new feature and there are issues, e.g. with twister, as seen in #384 github actions (they fail). But overall, snippets approach could be used in this PR as well. The thing is... it's not ready yet for using, so my proposal in this PR was to move to Kconfig.defconfig and once snippets get more adoption and some issues are fixed, we could move to snippets for better clarity (than with Kconfig defaults).