Skip to content

Conversation

@keqiaozhang
Copy link
Contributor

add a -f option to support audio format.

@keqiaozhang keqiaozhang requested a review from a team as a code owner March 10, 2023 14:14
dbaluta
dbaluta previously approved these changes Mar 10, 2023
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

This PR is hiding a backwards-incompatible change. I understand you did that to align with alsabat which is good but it will break existing users. This should at the very least be explained in a separate commit message, clearly visible in the git log.

  • commit 1: rename -f[requency] option to -F for alsabat consistency
  • commit 2: add new -f[format] option

With the NEW code, what happens when someone does check-alsabat.sh -f 821? Do they get a meaningful error message from alsabat? Add that error message in the first commit message.


OPT_NAME['r']='rate' OPT_DESC['r']='sample rate'
OPT_HAS_ARG['r']=1 OPT_VAL['r']=48000
OPT_HAS_ARG['r']=1 OPT_VAL['r']=48000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@keqiaozhang keqiaozhang force-pushed the alsa_format branch 2 times, most recently from 482481b to 3783bca Compare March 13, 2023 07:16
@keqiaozhang
Copy link
Contributor Author

Please help to review. If no objections, I will merge this PR myself, because I also need to update the parameters(-f => -F) in web side for alsabat related cases.

image

Comment on lines +36 to +40
OPT_NAME['f']='format' OPT_DESC['f']='target format'
OPT_HAS_ARG['f']=1 OPT_VAL['f']="S16_LE"

OPT_NAME['F']='frequency' OPT_DESC['F']='target frequency'
OPT_HAS_ARG['F']=1 OPT_VAL['F']=821
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the change of -f break the backward compatibility? For example, daily MTL test runs for different frequencies with -f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so we need to update the alsabat test config in web accordingly when merging this PR.

marc-hb
marc-hb previously approved these changes Mar 13, 2023
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 13, 2023

Very small conflict with #991 that I just merged, need to rebase sorry.

One model missing and some suspend/resume failures in https://sof-ci.01.org/softestpr/PR1009/build173/devicetest/index.html

https://sof-ci.01.org/softestpr/PR1009/build174/devicetest/index.html all green.

…tency

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
add a new option -f to support different formats in alsabat test.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
@keqiaozhang
Copy link
Contributor Author

@marc-hb , rebased. Please give a check again. if no problems, I will merge it today.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Please make sure the test results are OK-ish.

@keqiaozhang keqiaozhang merged commit f81a09f into thesofproject:main Mar 15, 2023
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