chan_simpleusb: Use PortAudio for audio output#1029
Open
mkmer wants to merge 9 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughMigration of chan_simpleusb audio I/O from OSS sounddev to PortAudio library, including refactored audio threading, new USB device lookup API in res_usbradio, and updated build configuration to link PortAudio. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@channels/chan_simpleusb.c`:
- Around line 480-492: open_stream() currently logs errors when
input_params.device or output_params.device equals paNoDevice but proceeds to
call Pa_OpenStream(), causing confusing PortAudio errors; update open_stream()
to check those conditions and return a PortAudio error (e.g., set res =
paInternalError or another appropriate paError) and exit early if either
input_params.device == paNoDevice or output_params.device == paNoDevice,
avoiding calling Pa_OpenStream() and ensuring pvt->stream is not used/left
uninitialized and the caller receives the clear failure immediately.
- Around line 1995-2004: The hangup path unconditionally calls
pthread_join(o->audiothread, ...) which is UB if that thread was never started;
update simpleusb_hangup to check o->audiothread != AST_PTHREADT_NULL before
calling pthread_join and only then set o->audiothread = AST_PTHREADT_NULL after
joining; likewise ensure the o->hidthread guard uses the same null-check
pattern. Also update the error path in simpleusb_call (where the HID thread is
joined on failure) to set o->hidthread = AST_PTHREADT_NULL after joining so
subsequent simpleusb_hangup won't attempt to join it again; keep store_config’s
initial AST_PTHREADT_NULL semantics intact.
- Around line 2071-2076: The current simpleusb_read() emits ast_log(LOG_ERROR)
on every call which can flood logs; change this to a non-noisy diagnostic by
either downgrading to ast_debug and include the channel name (use c->name or
ast_channel_name(c)) or implement a one-shot error log (static/atomic flag) so
the message is logged only once; keep returning &ast_null_frame and reference
simpleusb_read, simpleusb_audio_thread, ast_queue_frame, and ast_null_frame in
your fix so callers are clear this path is not used.
- Around line 1157-1164: The return path after
ast_radio_hid_device_init(o->devstr) failing in init_audio_device currently
returns without releasing usb_dev_lock; before returning on that error (the
block where o->usb_dev is NULL), call ast_mutex_unlock(&usb_dev_lock) to match
the other error paths and ensure the lock is released, then return -1. Locate
init_audio_device and the check around ast_radio_hid_device_init(o->devstr) to
add the ast_mutex_unlock(&usb_dev_lock) call just prior to the ast_log/return to
prevent the deadlock.
- Around line 2277-2292: When queuing the ENDPAGE frame inside the loop, clear
o->waspager before the continue so the condition won't fire again on the next
iteration; specifically, in the block that checks "if (o->waspager &&
(!ispager))" set o->waspager = false (or 0) prior to checking o->owner and
calling ast_queue_frame(&wf) (or before the continue) so that the ENDPAGE_STR
frame is only queued once per transition; preserve the existing owner check and
ast_queue_frame call and keep o->waspager = ispager after the loop as intended.
- Around line 462-490: The device matching using strstr(dev->name,
pvt->hw_device) in the device-discovery loop can mis-match prefixes (e.g.,
"hw:1" matching "hw:10"); update the check in that loop (the block that sets
input_params.device and output_params.device using pvt->hw_device and dev->name)
to perform an anchored match instead of plain substring: find the substring
position and then ensure the character after the match (if any) is a valid
boundary (',', ')', or '\0') or perform a stricter parse of dev->name to compare
the exact hw:<N> token; apply the same fix for both input and output device
selection so only exact card-index matches are accepted.
- Around line 4281-4285: The module calls Pa_Initialize() on load (see
Pa_Initialize at line ~4281) but never calls Pa_Terminate() on unload; update
unload_module() to call Pa_Terminate() after you close and destroy all PortAudio
streams and clean up channel pvts (the same cleanup sequence where you close
streams and free channel state in unload_module), ensuring Pa_Terminate() is
invoked only if Pa_Initialize() succeeded so host API threads and handles are
released.
In `@res/res_usbradio.c`:
- Around line 970-979: The loop currently returns any matching usb device (using
usb_busses, bus->dirname, dev->filename, and returning dev when strcasecmp(cur,
target) == 0); change it to only return devices that are supported HID audio
devices by applying the same filter/logic as ast_radio_hid_device_init(): after
matching cur to target, call or inline the HID-support check (e.g., the helper
that validates vendor/product/class or the ast_radio_hid_device_init()
predicates) and only return dev when that check succeeds; otherwise continue the
loop so unsupported USB audio cards do not get treated as HID-capable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de0d2cc7-e9c5-4ed4-ab4a-9d2df459cdc6
📒 Files selected for processing (4)
channels/Makefile.diffchannels/chan_simpleusb.cinclude/asterisk/res_usbradio.hres/res_usbradio.c
mkmer
force-pushed
the
portaudio-threaded-read
branch
2 times, most recently
from
2a84b0d to
0b6fc4c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
4e8237c to
c618b52
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
ec574c0 to
ffd86c7
Compare
davidgsd
reviewed
Apr 30, 2026
Allan-N
reviewed
May 1, 2026
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
a5f429a to
af3ea39
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@res/res_usbradio.c`:
- Around line 909-915: In ast_radio_check_audio the consecutive-clipping run
counter seq_clips and last_clip handling are wrong: ensure seq_clips is
initialized to 1 when a new clip is detected (not 0) and reset to 1 whenever a
gap is detected (i.e., when last_clip < 0 or last_clip + 1 != i start a new
run), and increment seq_clips only when the current sample immediately follows
the previous clipped sample (last_clip + 1 == i); apply the identical correction
to the second occurrence of this logic (the block around the other
CLIP_SAMP_THRESH check) so both clipping-run counters behave symmetrically.
- Around line 500-505: The code assigns the result of ast_realloc directly to
usb_device_list which can drop the original pointer on failure; change to use a
temporary pointer (e.g., new_list) when calling ast_realloc, check if new_list
is NULL, and only assign it to usb_device_list on success; if allocation fails,
keep the original usb_device_list intact, release the usb_list_lock and return
-1 (ensuring usb_device_list_size remains correct) — update the allocation site
that references ast_realloc, usb_device_list, usb_device_list_size, and
usb_list_lock accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40ba8553-9a49-4060-a872-aa1afcd78424
📒 Files selected for processing (5)
channels/Makefile.diffchannels/chan_simpleusb.cchannels/chan_usbradio.cinclude/asterisk/res_usbradio.hres/res_usbradio.c
✅ Files skipped from review due to trivial changes (1)
- channels/Makefile.diff
🚧 Files skipped from review as they are similar to previous changes (1)
- channels/chan_simpleusb.c
Contributor
|
The locked up USB interface issue occurred again. I was listening to a couple nets over the past couple hours, not doing anything in the terminal or transmitting to the node. Then about 5 minutes ago the audio just stopped, and the status LED on the UCI is lit solid, not flashing. ASL and the AllScan dashboard web app are all still running fine. Logfile is quite large with the extra debug enabled and with it showing all the AMI log messages for the status data sent to the dashboard. LMK if you see anything, and next steps... |
Contributor
|
USB interface locked up again, this time after only about 10 minutes. This time I did not have the web dashboard running so there should be less clutter in the log. I do not see anything in the log that seems to indicate in any way that the USB comms stopped. |
Collaborator
Author
Well... here is where it "died" (~11:27.359): We can see that the HID thread stopped or stopped hearing the pttkick pipe.... It looks like you only captured the debug in this log file, do you have the error and warning logs for this time frame? Maybe just clip out this time window (ish)? |
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
3cc24a9 to
dc2ac15
Compare
Collaborator
Author
|
I found a situation where |
Contributor
|
Updated everything and the issue is still happening. Occurred after about 20 minutes. I think I definitely pulled in everything, Log file is below. You mentioned earlier about some other error logs but I don't know where that would be? messages.log is the only logfile in /var/log/asterisk/, and I don't see any errors in the system log. |
Collaborator
Author
|
It's strange that we don't see any of the hidthread messages in your log (like these): |
Allan-N
reviewed
May 6, 2026
Allan-N
reviewed
May 6, 2026
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
portaudio-threaded-read
branch
4 times, most recently
from
18eefe9 to
576124e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
576124e to
4433cde
Compare
mkmer
force-pushed
the
portaudio-threaded-read
branch
from
a19b9d8 to
355bb8c
Compare
Collaborator
Author
|
This should be "solid" at this point. Turns out something with the USB stream stops draining the TX hardware buffer, which then causes the TX output to remain on ( becuase the code is trying to empty the buffer before releasing the TX). |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
channels/chan_voter.c (1)
1230-1231:⚠️ Potential issue | 🔴 CriticalFix frame-size padding math to avoid tail overread.
audio_samples += audio_samples % FRAME_SIZEdoes not round up to aFRAME_SIZEboundary; it can still leave a partial tail before the fixed-size copy loop.Proposed fix
- audio_samples += audio_samples % FRAME_SIZE; + audio_samples += (FRAME_SIZE - (audio_samples % FRAME_SIZE)) % FRAME_SIZE;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@channels/chan_voter.c` around lines 1230 - 1231, The padding math is wrong: replace the line that does audio_samples += audio_samples % FRAME_SIZE with logic that rounds audio_samples up to the next multiple of FRAME_SIZE (compute the padding as (FRAME_SIZE - (audio_samples % FRAME_SIZE)) % FRAME_SIZE and add that), so audio_samples becomes a true multiple of FRAME_SIZE before the fixed-size copy loop; update the code around the audio_samples variable in channels/chan_voter.c accordingly to use the new padded value to avoid tail overread.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@channels/chan_simpleusb.c`:
- Around line 1156-1159: When ast_radio_usb_device_from_alsa_card(o->devicenum)
returns NULL, fail the audio-device initialization instead of just logging: in
the function that contains the o->usb_dev assignment (the init_audio_device /
simpleusb init routine that sets o->usb_dev and then continues), change the
branch so that if o->usb_dev is NULL you log an error (not just ast_debug) and
immediately return -1 to abort initialization; this ensures the channel isn’t
marked and mixer probing isn’t attempted when the HID-backed USB device cannot
be resolved.
---
Duplicate comments:
In `@channels/chan_voter.c`:
- Around line 1230-1231: The padding math is wrong: replace the line that does
audio_samples += audio_samples % FRAME_SIZE with logic that rounds audio_samples
up to the next multiple of FRAME_SIZE (compute the padding as (FRAME_SIZE -
(audio_samples % FRAME_SIZE)) % FRAME_SIZE and add that), so audio_samples
becomes a true multiple of FRAME_SIZE before the fixed-size copy loop; update
the code around the audio_samples variable in channels/chan_voter.c accordingly
to use the new padded value to avoid tail overread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fbe10a7-7392-475a-9bb8-9aae798d0e75
📒 Files selected for processing (6)
channels/Makefile.diffchannels/chan_simpleusb.cchannels/chan_usbradio.cchannels/chan_voter.cinclude/asterisk/res_usbradio.hres/res_usbradio.c
✅ Files skipped from review due to trivial changes (1)
- channels/chan_usbradio.c
🚧 Files skipped from review as they are similar to previous changes (1)
- channels/Makefile.diff
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Seperate functions for stream cleanup Only flush buffer 1 time per exit
This was
linked to
issues
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add PortAudio thread
Refactor startup of HID thread
Allow use of hw:xxx to configure usb
This allows ASL to work on any modern kernel by eliminating the obsolete
/dev/dspinterface.Add a USB buffer "monitor" to handle the mysterious condition were the audio buffer stops draining causing the TX output to remain active. When the buffer exceeds 200ms the stream is now reset.
Summary by CodeRabbit
New Features
Changes