Skip to content

sys/ztimer: rework Kconfig#17165

Merged
MrKevinWeiss merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/sys/ztimer_kconfig_entry
Nov 11, 2021
Merged

sys/ztimer: rework Kconfig#17165
MrKevinWeiss merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/sys/ztimer_kconfig_entry

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Nov 9, 2021

Contribution description

This changes the entry point for the ztimer modules in Kconfig, trying to fix:

  1. the xtimer compatibility modules selection issue (driver/at30tse75x: port to ztimer_usec #17137)
  2. the dependency loop happening in the native migration of RTC to ztimer (cpu/native: migrate periph_rtc to ztimer #17125)

Testing procedure

Issues/PRs references

Alternative to #17160

@github-actions github-actions bot added Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Nov 9, 2021
@leandrolanzieri leandrolanzieri mentioned this pull request Nov 9, 2021
Merged
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Nov 10, 2021

I'm fine with this change, I would maybe like the commit message to be more descriptive mentioning that this is a special case. I'm wondering if when tackling the network stack we will to do this kinds of things as well...

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Nov 10, 2021

I'm wondering if when tackling the network stack we will to do this kinds of things as well...

IMO we should try hard to avoid this. The main reason to switch the entry point of ztimer module selection was the (special) case of native RTC, which we still seem to want to work out of the box. In a more conservative point of view native actually does not provide HAS_PERIPH_RTC unless MODULE_ZTIMER_MSEC is active (that would be a depends on). That would also prevent the dependency loop, but adds more steps to select that periph.

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 10, 2021
kfessel
kfessel reviewed Nov 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

i tested: #17137 with this PR, it does no longer need the the patch to tests/driver_io1_xplained/app.config.test ✔️ -> i like it. (it would have saved me a lot of time.
reviewing the KConfig this looks good to me (but I haven't much experience with Kconfig): this seems to create a intemediate config value which is then used to determine whether the MODULE_XTIMER_ON_ZTIMER is need (sound good to me).
Lets have murdock take a look at this.

kfessel
kfessel approved these changes Nov 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@kfessel kfessel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is good to go -maybe you want to ask for a second opinion from someone with more KConfig experience.

My Test: ✔️ , Read: ✔️ , Murdock: ✔️

If merging is done with the current commits (messages) please provide a very descriptive Merge message.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Nov 11, 2021

maybe you want to ask for a second opinion from someone with more KConfig experience.

I spent some time looking into this in detail and helping come up with a solution and agree that it is sufficient.

@MrKevinWeiss MrKevinWeiss merged commit da60ba5 into RIOT-OS:master Nov 11, 2021
@kfessel kfessel mentioned this pull request Nov 11, 2021
Merged
@leandrolanzieri leandrolanzieri mentioned this pull request Nov 11, 2021
Closed
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Nov 11, 2021

Hmm I was not fast enough to address @fjmolinas comment before this got merged

@leandrolanzieri leandrolanzieri deleted the pr/sys/ztimer_kconfig_entry branch November 11, 2021 12:26
@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Nov 11, 2021 •
edited
Loading

Hmm I was not fast enough to address @fjmolinas comment before this got merged

I think the merge message that @MrKevinWeiss wrote is very good and solves the "commit message not being descriptive" issue without doing another Murdock cycle. Only the word "special case" maybe missing.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Nov 11, 2021

Hmm I was not fast enough to address @fjmolinas comment before this got merged

I think the merge message that @MrKevinWeiss wrote is very good and solves the "commit message not being descriptive" issue without doing another Murdock cycle. Only the word "special case" maybe missing.

Ahh cool, missed that. Thanks for the review BTW!

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Nov 11, 2021

This was the first time I wrote something in the merge commit :) (I also just wanted to prevent another CI cycle)

@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
@MrKevinWeiss MrKevinWeiss mentioned this pull request May 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfessel kfessel kfessel approved these changes

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

Assignees

No one assigned

Labels

Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2022.01

Development

Successfully merging this pull request may close these issues.

4 participants

@leandrolanzieri @fjmolinas @MrKevinWeiss @kfessel