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 all 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
27 changes: 23 additions & 4 deletions tools/capture-test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,22 @@
"""

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)")
ap.add_argument("--capmap", type=str, default="01",
help="Capture channel map (as string, e.g. '23' to select 3rd/4th elems)")
ap.add_argument("--noise", default="noise.wav",
help="WAV file containing 'noise' for capture")
ap.add_argument("--duration", type=int, default=3, help="Capture duration (seconds)")
Expand All @@ -69,6 +73,7 @@ def parse_opts():
opts = ap.parse_args()
if not opts.capchan:
opts.capchan = opts.chan
opts.capmap = [int(x) for x in opts.capmap]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  assert len(opts.capmap) == opts.capchan

(never silently discard user input)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentional. The specific device that needs this has a 4-channel PDM DMIC device with only two connected microphones (on different inputs on different boards, even) and needs to be compared against 2-channel PCM stream. The default handling is two channels for all streams, which seems reasonable.

Copy link
Collaborator

@marc-hb marc-hb Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you missed my point, which was only about "input validation". What happens if the user screws up and opt.capchan and opt.capmap are inconsistent with each other? More specifically, what happens to spurious/extra digits in opts.capmap? Whatever happens, it should never silently discard (part of) the user input. At the very least a [log.]warning. It's also a useful statement for the code reader (it would have helped at least me for sure).

opts.base_test = not (opts.chirp_test or opts.echo_test)

class ALSA:
Expand Down Expand Up @@ -103,6 +108,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 +117,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 @@ -282,7 +298,9 @@ def cap_to_playback(buf):
# treat it, and it can plausibly create false positive chirp signals
# loud enough).
for i in range(0, len(buf), capsz):
frame = [scale * x for x in struct.unpack(capfmt, buf[i:i+capsz])[0:opts.chan]]
frame = struct.unpack(capfmt, buf[i:i+capsz]) # Decode
Copy link
Collaborator

@marc-hb marc-hb Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Re-using the same name frame over and over does not help. The comments save the day but a little bit more imagination would not hurt... At least a plural?

Suggested change
frame = struct.unpack(capfmt, buf[i:i+capsz]) # Decode
frames = struct.unpack(capfmt, buf[idx:idx+sample_bytes]) # Decode

It would help for capsz and i's names to reflect that they are byte numbers.

frame = [frame[n] for n in opts.capmap] # Select via channel map
frame = [scale * x for x in frame] # Convert to float
if last_frame:
delta_sum += sum(abs(last_frame[x] - frame[x]) for x in range(opts.chan))
last_frame = frame
Expand Down Expand Up @@ -352,7 +370,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
Loading