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

test-case: manual: Simple probes functionality test #923

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsarha
Copy link
Contributor

@jsarha jsarha commented Jun 17, 2022

Signed-off-by: Jyri Sarha [email protected]

This is very simple and fast put together test for probe capture functionality.

Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @jsarha . I think this is really useful to document the test process.

Copy link
Collaborator

@fredoh9 fredoh9 left a comment

Choose a reason for hiding this comment

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

Overall looks good and thank you for the update.
Almost ready to merge, good clarity for manual test note. When you update it please make it normal PR.

@jsarha jsarha force-pushed the probe-capture-manual-test branch from c43759a to 8a791ed Compare June 20, 2022 10:19
@jsarha jsarha marked this pull request as ready for review June 20, 2022 12:31
@jsarha jsarha requested a review from a team as a code owner June 20, 2022 12:31
aiChaoSONG
aiChaoSONG previously approved these changes Jun 22, 2022
Copy link

@aiChaoSONG aiChaoSONG left a comment

Choose a reason for hiding this comment

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

@jsarha Are those configurations in this document all supported by thesofproject/sof and thesofproject/linux? So I can try manually.

@jsarha
Copy link
Contributor Author

jsarha commented Jun 22, 2022

@jsarha Are those configurations in this document all supported by thesofproject/sof and thesofproject/linux? So I can try manually.

Yes, they are. Also the both PRs fixing the problems related to this are now merged. I wrote the test descriptions based on how I got the feature working last week, so I think it should be good.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 22, 2022

The form looks great but I'm ignorant about the content. @jsarha could you get a couple more reviewers?

ranj063
ranj063 previously approved these changes Jun 22, 2022
@marc-hb marc-hb requested a review from a team June 23, 2022 20:55
@jsarha jsarha requested review from kv2019i and ujfalusi June 27, 2022 06:23
Copy link
Contributor

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

@jsarha, great to have something for the probes!
I have couple candidates for clarification, if you think they stand.

@jsarha jsarha dismissed stale reviews from ranj063 and aiChaoSONG via 9dcf2d6 June 30, 2022 09:31
@jsarha jsarha force-pushed the probe-capture-manual-test branch from 8a791ed to 9dcf2d6 Compare June 30, 2022 09:31
CONFIG_PROBE_POINTS_MAX=16
CONFIG_PROBE_DMA_MAX=4

In fact PROBE_DMA_MAX is ignored as inection is not currently supported
Copy link
Contributor

Choose a reason for hiding this comment

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

s/inection/injection
Why mention CONFIG_PROBE_DMA_MAX in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If CONFIG_PROBE is enabled through menuconfig the CONFIG_PROBE_DMA_MAX option appears with the default value of 4, so I think it causes less confusion to mention it than leaving it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it comes with 0 as default, but never checked the kconfig file to be honest..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you have put the zero there. If enabled for the first time (I think I had not touched the items before), the two numeric options appear as above.

test-case/manual/check-simple-probe-capture.md Outdated Show resolved Hide resolved
@jsarha jsarha force-pushed the probe-capture-manual-test branch from 9dcf2d6 to 256272a Compare June 30, 2022 16:00
@kv2019i
Copy link
Contributor

kv2019i commented Mar 7, 2023

@jsarha @ujfalusi Would be nice to get a version of this merged. I had to look this up to re-learn how to use probes ... and I mean, I thought this was merged, so it took some time before I realized and I need to look it up here in open PRs....

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.

7 participants