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

sample/zbus: qemu_nios2 needs faster tick for zbus test with picolibc #53684

Closed

Conversation

keith-packard
Copy link
Collaborator

@keith-packard keith-packard commented Jan 10, 2023

Picolibc is enough faster than the minimal C library that the zbus benchmark will likely complete in well under 10ms on qemu_nios2. As this target cannot provide a clock at higher resolution than the system tick, we need to increase that rate to get a non-zero runtime for this benchmark.

This is a replacement for #53589 and a response to #53563, which didn't quite resolve the issue on this target.

Signed-off-by: Keith Packard [email protected]

Copy link
Contributor

@rodrigopex rodrigopex left a comment

Choose a reason for hiding this comment

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

I was considering finding a less verbose way to do that. However, in this case, you had to copy twice the same setup for adding only one line in each copied chunk. I could not find any way cleaner to do that. Any advice @nashif @galak @PerMac? I am citing you guys because of the twister's maintainers list.

@keith-packard
Copy link
Collaborator Author

I was considering finding a less verbose way to do that. However, in this case, you had to copy twice the same setup for adding only one line in each copied chunk. I could not find any way cleaner to do that. Any advice @nashif @galak @PerMac? I am citing you guys because of the twister's maintainers list.

Yeah, having some ability to have per-target extra config flags would be useful here. I did try changing the config globally but couldn't find a combination that worked on every target.

@nashif
Copy link
Member

nashif commented Jan 11, 2023

would something like this work?

--- a/samples/subsys/zbus/benchmark/sample.yaml
+++ b/samples/subsys/zbus/benchmark/sample.yaml
@@ -19,6 +19,7 @@ tests:
       - CONFIG_BM_ONE_TO=8
       - CONFIG_BM_MESSAGE_SIZE=256
       - CONFIG_BM_ASYNC=y
+      - arch:nios2:CONFIG_SYS_CLOCK_TICKS_PER_SEC=1000
   sample.zbus.benchmark_sync:

any other suggestions? This can also be

`platform::CONFIG_...

or we can go fancy and have

CONFIG_SYS_CLOCK_TICKS_PER_SEC=1000 if arch == nios2 but that is more complicated to implement.

@keith-packard
Copy link
Collaborator Author

would something like this work?

I'd say it's entirely up to you; obviously any of the suggestions you made would be fine. Take that as a feature request for some future release?

I'd suggest not waiting for that though; no reason to block fixing things while the tooling catches up with this kind of improvement.

@nashif
Copy link
Member

nashif commented Jan 11, 2023

I'd suggest not waiting for that though; no reason to block fixing things while the tooling catches up with this kind of improvement.

PR on the way :)

@nashif
Copy link
Member

nashif commented Jan 11, 2023

#53725

@keith-packard
Copy link
Collaborator Author

#53725

🥳 I'll give this a try and post some review there when I get a chance.

Copy link
Contributor

@rodrigopex rodrigopex left a comment

Choose a reason for hiding this comment

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

It would be better to use the new approach for conditional extra_config.

@keith-packard
Copy link
Collaborator Author

It would be better to use the new approach for conditional extra_config.

I was hoping to see fixes for west proposed so this patch wouldn't break existing usage, but sure, I'll push the updated version.

@stephanosio
Copy link
Member

@keith-packard Could you rebase?

@keith-packard
Copy link
Collaborator Author

Looks like this change was merged along with #53338 although it wasn't included in that PR. I discovered this when attempting to rebase and got 'noop' ...

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.

4 participants