Skip to content

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

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.

Sign up for GitHub

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

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

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

jsarha wants to merge 1 commit into from

Conversation

@jsarha
Copy link
Contributor

@jsarha jsarha commented Jun 17, 2022 •
edited
Loading

Signed-off-by: Jyri Sarha jyri.sarha@intel.com

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

kv2019i
kv2019i reviewed Jun 17, 2022
View reviewed changes
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.

fredoh9
fredoh9 reviewed Jun 17, 2022
View reviewed changes
fredoh9
fredoh9 reviewed Jun 17, 2022
View reviewed changes
Copy link
Contributor

@fredoh9 fredoh9 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.

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
View reviewed changes
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
View reviewed changes
@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
ujfalusi
ujfalusi reviewed Jun 27, 2022
View reviewed changes
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
ujfalusi
ujfalusi suggested changes Jun 30, 2022
View reviewed changes
test-case/manual/check-simple-probe-capture.md
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

@ujfalusi ujfalusi Jun 30, 2022

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

@jsarha jsarha Jun 30, 2022

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

@ujfalusi ujfalusi Jun 30, 2022

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

@jsarha jsarha Jul 1, 2022

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.

@jsarha
test-case: manual: Simple probes functionality test
256272a 
Signed-off-by: Jyri Sarha <jyri.sarha@intel.com>
@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

Reviewers

@fredoh9 fredoh9 fredoh9 left review comments

@marc-hb marc-hb marc-hb left review comments

@ranj063 ranj063 ranj063 left review comments

@kv2019i kv2019i Awaiting requested review from kv2019i

+2 more reviewers

@ujfalusi ujfalusi ujfalusi requested changes

@aiChaoSONG aiChaoSONG aiChaoSONG left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jsarha @marc-hb @kv2019i @ranj063 @ujfalusi @fredoh9 @aiChaoSONG