-
Notifications
You must be signed in to change notification settings - Fork 44
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
sof-tplgreader: parse rates from topology file #1174
sof-tplgreader: parse rates from topology file #1174
Conversation
961605b
to
f388f07
Compare
tools/tplgtool.py
Outdated
if rate & (1 << 8) != 0: | ||
rates.append("64000") | ||
if rate & (1 << 9) != 0: | ||
rates.append("96000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of repetition; I think you don't need to "unroll" that loop:
bit_rates_dict = { 1 : 8000, 3 : 16000, ...}
for bit in rates_dict:
if rate & (1 << bit) != 0:
rates.append(bit_rates_dict[bit])
Or even shorter
rates = [ bit_rates_dict[bit] for bit in bit_rates_dict
if rate & (1 << bit) != 0 ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good comment, but I can't get it to work. I put a TODO above. There are a couple of places where the same thing is applicable. Can I make this improvement as a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the error? Later = never.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test this to help but the new functions are never called. Did you forget a part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test this to help but the new functions are never called. Did you forget a part?
OK I figured it out: the "interactive" output of the parser is incomplete.
I just tested this below together with https://github.com/thesofproject/sof/pull/9017/files and it works perfectly fine:
@staticmethod
def _to_rate_string(rate):
# Note: intentionally put 48000 first to make first in the list
bit_rates_dict = { 7 : 48000, 6 : 44100, }
return [ str(bit_rates_dict[bit]) for
bit in bit_rates_dict if rate & (1 << bit) != 0 ]
I forgot the str()
earlier, is that why it was failing for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, it works nicely. I just updated again. Let me check the test results before remove draft
I have further changes and currently testing with devices now. Found some errors, will try to update the PR in this afternoon |
f388f07
to
a7f9990
Compare
README.md
Outdated
@@ -148,7 +148,8 @@ apl | |||
<br> Dumps info from tplg binary file. | |||
|
|||
* tplgtool.py | |||
<br> Dumps info from tplg binary file. (deprecated) | |||
<br> Dumps info from tplg binary file. sof-tplgreader.py still use this but new features | |||
will go to tplgtool2.py. Will be deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain a little bit more which script uses which script and when?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way these 3 scripts interact with each other is really not intuitive and it took me an unreasonable amount time to figure out. Since you're adding a new feature to the deprecated script (while leaving the newer script behind?), please add a couple more sentences describing the situation better. Right now this part of the README.md is worse than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this 3 scripts interact with each other is really not intuitive.
I agree this part.
I don't understand rest of your comment. Why do you think tplgtool.py is deprecated? README was misleading. When tplgtool2.py was introduced, (if it wants to replace tplgtool.py) it has to take care of all the functions in tplgtool.py. But it didn't. More likely it added a new file and put a deprecated notice on the currently being used script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think tplgtool.py is deprecated?
Because it says so:
$ ./tools/tplgtool.py
warnings.warn("tplgtool.py is deprecated, use tplgtool2.py instead.", DeprecationWarning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad I'm not the only one confused!
3d8eeef
to
9179e7e
Compare
Related to sampling rate in caps, there are rates, rate_min and rate_max. Added support for the rates list from the topology file and use first in the list as supported rate. The sampling rate 48000 is intentionally placed first in the dictionary to ensure it appears first in the list. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
9179e7e
to
840d3cc
Compare
This has some side-effects to tests with PCMs that have lower than 48000 rates: |
@fredoh9 should we revert temporarily while you design a fix? |
No, it will break every case. I will take a look shortly |
I was able to test with thesofproject/sof#9017