pkg/cryptoauthlib: model in kconfig#18011
Conversation
3eba7c8 to
6d03149
Compare
Einhornhool
left a comment
There was a problem hiding this comment.
In general this works, but I added some change suggestions
pkg/cryptoauthlib/Kconfig
Outdated
| select MODULE_ZTIMER | ||
| select MODULE_ZTIMER_USEC | ||
| select MODULE_ZTIMER_PERIPH_TIMER |
There was a problem hiding this comment.
| select MODULE_ZTIMER | |
| select MODULE_ZTIMER_USEC | |
| select MODULE_ZTIMER_PERIPH_TIMER | |
| select ZTIMER_USEC |
Here you can just use this
There was a problem hiding this comment.
What about this suggestion?
There was a problem hiding this comment.
That doesn't work (I tried): there were missing ztimer dependencies on some platform (comparing the output of info-modules with and without Kconfig).
There was a problem hiding this comment.
Ah, it's ZTIMER_USEC, not MODULE_ZTIMER_USEC. I think I tried the latter.
There was a problem hiding this comment.
Yeah, it is a workaround to avoid a circular dependency related to xtimer and ztimer. Hopefully will go away once we switch over
There was a problem hiding this comment.
Ok, I pushed that suggestion as well. And squashed.
pkg/cryptoauthlib/Kconfig
Outdated
| config PACKAGE_CRYPTOAUTHLIB | ||
| bool "Microchip CryptoAuthentication Library package" | ||
| depends on TEST_KCONFIG | ||
| select MODULE_AUTO_INIT_SECURITY | ||
| select MODULE_CRYPTOAUTHLIB_CONTRIB | ||
|
|
||
| config MODULE_CRYPTOAUTHLIB_CONTRIB | ||
| bool | ||
| depends on TEST_KCONFIG | ||
| select MODULE_ZTIMER | ||
| select MODULE_ZTIMER_USEC | ||
| select MODULE_ZTIMER_PERIPH_TIMER | ||
| select MODULE_PERIPH_I2C | ||
| select MODULE_PERIPH_I2C_RECONFIGURE if HAS_PERIPH_I2C_RECONFIGURE | ||
|
|
||
| config MODULE_CRYPTOAUTHLIB_TEST | ||
| bool "Module for cryptoauthlib tests" | ||
| depends on TEST_KCONFIG | ||
| select MODULE_CRYPTOAUTHLIB_TEST_JWT | ||
| select MODULE_CRYPTOAUTHLIB_TEST_TNG | ||
| select MODULE_CRYPTOAUTHLIB_TEST_ATCACERT |
There was a problem hiding this comment.
| config PACKAGE_CRYPTOAUTHLIB | |
| bool "Microchip CryptoAuthentication Library package" | |
| depends on TEST_KCONFIG | |
| select MODULE_AUTO_INIT_SECURITY | |
| select MODULE_CRYPTOAUTHLIB_CONTRIB | |
| config MODULE_CRYPTOAUTHLIB_CONTRIB | |
| bool | |
| depends on TEST_KCONFIG | |
| select MODULE_ZTIMER | |
| select MODULE_ZTIMER_USEC | |
| select MODULE_ZTIMER_PERIPH_TIMER | |
| select MODULE_PERIPH_I2C | |
| select MODULE_PERIPH_I2C_RECONFIGURE if HAS_PERIPH_I2C_RECONFIGURE | |
| config MODULE_CRYPTOAUTHLIB_TEST | |
| bool "Module for cryptoauthlib tests" | |
| depends on TEST_KCONFIG | |
| select MODULE_CRYPTOAUTHLIB_TEST_JWT | |
| select MODULE_CRYPTOAUTHLIB_TEST_TNG | |
| select MODULE_CRYPTOAUTHLIB_TEST_ATCACERT | |
| menuconfig PACKAGE_CRYPTOAUTHLIB | |
| bool "Microchip CryptoAuthentication Library package" | |
| depends on TEST_KCONFIG | |
| select MODULE_AUTO_INIT_SECURITY | |
| select MODULE_CRYPTOAUTHLIB_CONTRIB | |
| if PACKAGE_CRYPTOAUTHLIB | |
| config MODULE_CRYPTOAUTHLIB_TEST | |
| bool "Module for cryptoauthlib tests" | |
| depends on TEST_KCONFIG | |
| select MODULE_CRYPTOAUTHLIB_TEST_JWT | |
| select MODULE_CRYPTOAUTHLIB_TEST_TNG | |
| select MODULE_CRYPTOAUTHLIB_TEST_ATCACERT | |
| endif # PACKAGE_CRYPTOAUTHLIB | |
| config MODULE_CRYPTOAUTHLIB_CONTRIB | |
| bool | |
| depends on TEST_KCONFIG | |
| select MODULE_ZTIMER | |
| select MODULE_ZTIMER_USEC | |
| select MODULE_ZTIMER_PERIPH_TIMER | |
| select MODULE_PERIPH_I2C | |
| select MODULE_PERIPH_I2C_RECONFIGURE if HAS_PERIPH_I2C_RECONFIGURE |
If you do it like this, the test option is hidden from the menu until you choose the package.
There was a problem hiding this comment.
What is the benefit of having it hidden ?
There was a problem hiding this comment.
I pushed an alternative without the menu (because I don't see the point in having a menu for configuring modules).
There was a problem hiding this comment.
It is more intuitive to have modules that depend on the package inside the menuconfig. That is the way we have been doing it so far, instead of having a huge flat list that keeps changing every time you enable/disable a module. It does not affect .config users, and helps menuconfig users.
5e11790 to
348c251
Compare
|
Are we ok to squash this one ? |
0bf43aa to
8699fd4
Compare
8699fd4 to
47bf7e2
Compare
47bf7e2 to
ae530a9
Compare
MrKevinWeiss
left a comment
There was a problem hiding this comment.
ACK! Looks good in menuconfig and makes more sense that what I recommended... 👍
Contribution description
As the title says. The changes are pretty straight forward.
Something I don't quite understand is why I have to explicitly select
MODULE_ZTIMER_PERIPH_TIMERas ztimer backend. I thought the backend would be selected automatically (apparently there's another use of choice involved...)Testing procedure
Green Murdock
Issues/PRs references
None