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.

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

[SKIP SOF-TEST] capture-test: Add buffer size control & verbose logging #1207

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[SKIP SOF-TEST] capture-test: Add buffer size control & verbose logging #1207

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4191f3
tools/capture-test.py: pylint cleanups
andyross Jun 27, 2024
e09defe
tools/capture-test.py: Fix ALSA parameter
andyross Jun 27, 2024
1a864ad
capture-test: Add buffer size control & verbose logging
andyross Jun 18, 2024
f717aa3
capture-test.py: Add channel map feature
andyross Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions tools/capture-test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,17 @@
"""

def parse_opts():
global opts
global opts # pylint: disable=global-statement
ap = argparse.ArgumentParser(description=HELP_TEXT,
formatter_class=argparse.RawDescriptionHelpFormatter)
ap.add_argument("-v", "--verbose", action="store_true", help="Verbose output")
ap.add_argument("--disable-rtnr", action="store_true", help="Disable RTNR noise reduction")
ap.add_argument("-c", "--card", type=int, default=0, help="ALSA card index")
ap.add_argument("--pcm", type=int, default=16, help="Output ALSA PCM index")
ap.add_argument("--cap", type=int, default=18, help="Capture ALSA PCM index")
ap.add_argument("--rate", type=int, default=48000, help="Sample rate")
ap.add_argument("--chan", type=int, default=2, help="Output channel count")
ap.add_argument("--bufsz", type=int, default=2048, help="Buffer size in frames")
ap.add_argument("--capchan", type=int,
help="Capture channel count (if different from output)")
ap.add_argument("--capbits", type=int, default=16, help="Capture sample bits (16 or 32)")
Expand Down Expand Up @@ -103,6 +105,7 @@ def err_wrap(ret):
return ret
def alloc(self, typ):
return (C.c_byte * getattr(self.lib, f"snd_{typ}_sizeof")())()
# pylint: disable=too-few-public-methods
class pcm_channel_area_t(C.Structure):
_fields_ = [("addr", C.c_ulong), ("first", C.c_int), ("step", C.c_int)]

Expand All @@ -111,8 +114,18 @@ def pcm_init_stream(pcm, rate, chans, fmt, access):
alsa.snd_pcm_hw_params_any(pcm, hwp)
alsa.snd_pcm_hw_params_set_format(pcm, hwp, fmt)
alsa.snd_pcm_hw_params_set_channels(pcm, hwp, chans)
alsa.snd_pcm_hw_params_set_rate(pcm, hwp, rate, alsa.PCM_STREAM_PLAYBACK)
alsa.snd_pcm_hw_params_set_rate(pcm, hwp, rate, 0)
marc-hb marked this conversation as resolved.
Show resolved Hide resolved
alsa.snd_pcm_hw_params_set_access(pcm, hwp, access)
alsa.snd_pcm_hw_params_set_buffer_size(pcm, hwp, opts.bufsz)
if opts.verbose:
print("Set hw_params:")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you will likely reply "overkill!" but this really smells like a missed import logging opportunity. People often overstate the complexity of Python's built-in logging, for a counter example see how little code logging required in recent commit 5419b42

Also, something resistant to change something :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logging puts ugly prefix nonsense on the output lines, which IMHO is exactly what you don't want for human readable --verbose output. For a running system with multiple levels and the expectation of an engineer pouring through logs to intuit state? Sure. Here, for a --verbose flag? That's just not the way unix tools are expected to work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Logging puts ugly prefix nonsense on the output lines

No, it does not. As seen in 5419b42

ipython3

import logging
logging.basicConfig(level=logging.INFO, format="%(message)s")
logging.info("foo")

foo # no prefix!

And that's all it takes.

Here, for a --verbose flag?

Python is a high level language. This means complicated things are possible, but simple things are easy: it's not because logging has 5 levels that you have to use them. You can just use two: verbose and not. Same for prefixes, multiple loggers and what not: they can be completely ignored.

What logging gives even when not using any complex/fancy feature:

  • the code is not littered with if opt.verbose conditionals and indentation
  • best flexibility to change or add extra levels temporarily for debugging purposes with single-line changes and no wrangling with if opt.verbose conditionals and indentation.

Don't get me wrong: I do NOT think this PR should be blocked on switching to logging. I only want to thank you for confirming that logging's complexity is generally overstated and misunderstood :-)

Maybe its documentation is missing the example above...

out = C.c_ulong(0)
alsa.snd_output_buffer_open(C.byref(out))
alsa.snd_pcm_hw_params_dump(hwp, out)
buf = C.c_ulong(0)
alsa.snd_output_buffer_string(out, C.byref(buf))
print(C.string_at(buf.value).decode("ascii", errors="ignore"))

alsa.snd_pcm_hw_params(pcm, hwp)

def ctl_disable_rtnr():
Expand Down Expand Up @@ -352,7 +365,8 @@ def echo_test():
# Just slurps in the wav file and chops off the header, assuming
# the user got the format and sampling rate correct.
WAV_HDR_LEN = 44
buf = open(opts.noise, "rb").read()[WAV_HDR_LEN:]
with open(opts.noise, "rb") as f:
buf = f.read()[WAV_HDR_LEN:]

(rfd, wfd) = os.pipe()
pid = os.fork()
Expand Down