Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughReplace DAHDI/pseudo channels with Local/pseudo and ast_bridge-based mixing; refactor pseudo-channel and conference APIs to use Local channels and bridge primitives; convert tone playback to string-based indications; update CDR/unreal handling; enable bridge_softmix and chan_bridge_media modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/app_rpt/rpt_manager.c (1)
303-339: Fix potential unlock of the wrong (or NULL) channel pointer; also update stale “DAHDI pseudo” wording.
You lockrxchannel(Line 328) but unlockrpt_vars[i].rxchannel(Line 335). Ifrpt_vars[i].rxchannelchanges during module unload, this can become an invalid unlock / NULL deref. Also the comment at Lines 310-313 still references “DAHDI pseudo”.Suggested minimal fix:
- ast_channel_unlock(rpt_vars[i].rxchannel); + ast_channel_unlock(rxchannel);And update the comment to refer to Local/pseudo (or “pseudo channel”) instead of DAHDI.
apps/app_rpt/app_rpt.h (1)
1099-1106: UpdateIS_PSEUDO*to match the new “Local/pseudo” reality (or support both).
IS_PSEUDO/IS_PSEUDO_NAMEstill check"DAHDI/pseudo"but other changes now treat"Local/pseudo"as the pseudo marker. This will silently break any remaining pseudo checks.Suggested approach (backward-compatible):
-#define IS_PSEUDO(c) (!strncasecmp(ast_channel_name(c), "DAHDI/pseudo", 12)) -#define IS_PSEUDO_NAME(c) (!strncasecmp(c, "DAHDI/pseudo", 12)) +#define IS_PSEUDO(c) ( \ + !strncasecmp(ast_channel_name(c), "Local/pseudo", 11) || \ + !strncasecmp(ast_channel_name(c), "DAHDI/pseudo", 12) \ +) +#define IS_PSEUDO_NAME(c) ( \ + !strncasecmp(c, "Local/pseudo", 11) || \ + !strncasecmp(c, "DAHDI/pseudo", 12) \ +)(Adjust prefix lengths if you want exact-match instead of prefix-match.)
apps/app_rpt/rpt_telemetry.c (1)
1659-1664: Verifyrpt_conf_add()moves channels between conferences or implement explicit removal.At line 1313, the channel joins its initial conference type (RPT_TXCONF for UNKEY/LOCUNKEY modes). At line 1660, the same channel is added to RPT_CONF. However,
rpt_conf_add()only callsast_unreal_channel_push_to_bridge(), which adds to a bridge without removing from the previous one. This leaves the channel in both conferences or in an inconsistent state. Either use Asterisk'sast_bridge_move()API for proper channel movement, or explicitly remove the channel from the old bridge before adding to the new one.apps/app_rpt/rpt_link.c (1)
759-772: Fix critical leak:l->pchanisn't cleaned up on failure.When
rpt_conf_add()fails at line 768, you clean upl->chanand free the link, butl->pchan(created viaast_request()at line 747) is left unreleased.rpt_link_free()does not clean it up. The same issue occurs in the error block at line 759.if (rpt_conf_add(l->pchan, myrpt, RPT_CONF)) { + ast_hangup(l->pchan); + l->pchan = NULL; ast_hangup(l->chan); rpt_link_free(l); return -1; }Apply the same fix to the error block at line 759-764.
apps/app_rpt.c (1)
2833-2933: Channel setup: deprecation message typo + ensurelocaltxchannelis never NULL before conf add.
- Line 2849: “depreciated” → “deprecated”.
- Since
rpt_conf_add(myrpt->localtxchannel, ...)assumes non-NULL, anast_assert(myrpt->localtxchannel)(or explicit NULL check) right before Line 2877 would make failures clearer.- ast_log(LOG_WARNING, "Using DAHDI channel %s is depreciated. Update your rpt.conf to use Local.\n", myrpt->rxchanname); + ast_log(LOG_WARNING, "Using DAHDI channel %s is deprecated. Update your rpt.conf to use Local.\n", myrpt->rxchanname);apps/app_rpt/rpt_bridging.c (2)
271-314:__rpt_request_pseudo: missing explicit slin read/write format setup (unlikerpt_request_pseudo_chan).Even if the cap negotiates slin, setting formats here matches the behavior in
rpt_request_pseudo_chan()and reduces surprises.@@ chan = ast_request("Local", cap, NULL, NULL, exten, NULL); @@ + ast_set_read_format(chan, ast_format_slin); + ast_set_write_format(chan, ast_format_slin); rpt_disable_cdr(chan);
176-247:__rpt_request: Local channel autoservice is never stopped (resource leak);localrxchanneltech check is inverted relative to its usage.
- Line 225 calls
ast_autoservice_start(p->chan)for Local channels but there is no correspondingast_autoservice_stopanywhere in the codebase, causing a resource leak.- Line 232 sets
localrxchannel = chanwhentech == "Local", but throughoutrpt_radio.c,localrxchannelis used exclusively with DAHDI ioctls (DAHDI_RADIO_SETPARAM,DAHDI_RADIO_GETPARAM). The check should betech == "DAHDI"to matchlocaltxchannellogic (lines 237/239) and its actual usage. The current assignment ensureslocalrxchannelis NULL when it should be set, and vice versa.
🧹 Nitpick comments (4)
apps/app_rpt/rpt_serial.c (1)
491-494: Clarify intent in “not a DAHDI channel” comment vs Local migration.
The comment (Line 491) says “if not a DAHDI channel…”, but the check is nowrxchannel != localrxchannel. If the goal is “pciradio requires DAHDI-tech backing”, consider checkingCHAN_TECH(myrpt->localrxchannel, "DAHDI")explicitly (and/or updating the comment).apps/app_rpt/rpt_radio.c (2)
58-69: Usechanconsistently inrpt_radio_set_remcommand_data(fd + safety).
The function acceptschanbut usesmyrpt->localrxchannelforast_channel_fd(). That’s fragile if callers ever pass something else.- if (ioctl(ast_channel_fd(myrpt->localrxchannel, 0), DAHDI_RADIO_SETPARAM, &r) == -1) { + if (ioctl(ast_channel_fd(chan, 0), DAHDI_RADIO_SETPARAM, &r) == -1) { ast_log(LOG_WARNING, "Cannot send RBI command for channel %s: %s\n", ast_channel_name(chan), strerror(errno)); return -1; }
135-140: Avoid long-term commented-out offhook code.
If offhook handling is intentionally removed, prefer deleting the block and adding a brief note in commit message / changelog. If it’s temporarily disabled, add a TODO with an issue reference.apps/app_rpt.c (1)
4612-4624: Parrot/monitor stream write gating changed—please sanity-check behavior in duplex modes.This is likely correct, but it’s easy to regress recording behavior when the TX/keying model changes (especially duplex=3 / simplex patch paths). A couple of targeted functional tests (simplex + autopatch active, full duplex, parrot once) would de-risk this.
Also applies to: 3866-3873
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/app_rpt.c(46 hunks)apps/app_rpt/app_rpt.h(4 hunks)apps/app_rpt/rpt_bridging.c(12 hunks)apps/app_rpt/rpt_bridging.h(5 hunks)apps/app_rpt/rpt_call.c(1 hunks)apps/app_rpt/rpt_cli.c(1 hunks)apps/app_rpt/rpt_functions.c(1 hunks)apps/app_rpt/rpt_link.c(2 hunks)apps/app_rpt/rpt_manager.c(1 hunks)apps/app_rpt/rpt_radio.c(5 hunks)apps/app_rpt/rpt_serial.c(2 hunks)apps/app_rpt/rpt_telemetry.c(3 hunks)configs/rpt/modules.conf(1 hunks)configs/rpt/rpt.conf(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/app_rpt/rpt_link.c (1)
apps/app_rpt/rpt_bridging.c (1)
__rpt_request_pseudo(271-314)
apps/app_rpt/rpt_serial.c (1)
apps/app_rpt/rpt_radio.c (2)
rpt_radio_set_param(51-56)rpt_radio_set_remcommand_data(58-69)
apps/app_rpt.c (2)
apps/app_rpt/rpt_bridging.c (1)
rpt_request_pseudo_chan(249-269)apps/app_rpt/rpt_radio.c (2)
dahdi_radio_set_ctcss_encode(46-49)rpt_radio_set_param(51-56)
apps/app_rpt/rpt_telemetry.c (1)
apps/app_rpt/rpt_bridging.c (1)
rpt_request_pseudo_chan(249-269)
apps/app_rpt/rpt_bridging.c (1)
apps/app_rpt/rpt_call.c (2)
rpt_make_call(76-83)rpt_disable_cdr(30-57)
apps/app_rpt/rpt_bridging.h (1)
apps/app_rpt/rpt_bridging.c (6)
rpt_request_pseudo_chan(249-269)__rpt_request_pseudo(271-314)__rpt_conf_create(316-344)__rpt_conf_add(346-366)rpt_play_tone(379-397)rpt_conf_get_muted(368-372)
🔇 Additional comments (22)
configs/rpt/modules.conf (1)
138-138: LGTM - Required for bridge-based conferencing.Enabling
bridge_softmix.sounconditionally is correct for the new Local channel architecture. Consider updating the comment block (lines 128-131) to clarify that this module is now required for core repeater functionality, not just autopatch/SIP configurations.configs/rpt/rpt.conf (1)
68-74: LGTM - Correct migration from DAHDI to Local channels.The default
rxchannelconfiguration correctly migrates fromdahdi/pseudotoLocal/pseudofor hub nodes. This aligns with the PR's objective of removing DAHDI dependency and using Local channels for pseudo/hub configurations.apps/app_rpt/rpt_bridging.h (5)
2-9: LGTM - Enum updated for Local channel migration.The addition of
RPT_LOCALTXCHANand removal of DAHDI-specific channel types (RPT_DAHDITXCHAN,RPT_PARROTCHAN,RPT_VOXCHANper AI summary) correctly reflects the architectural shift to Local channels.
50-62: LGTM - Pseudo channel API extended for Local channel support.The addition of the
extenparameter torpt_request_pseudo_chanand__rpt_request_pseudois necessary for Local channel requests which require an extension specification. The implementation inrpt_bridging.c(from relevant snippets) correctly uses this withast_request("Local", cap, NULL, NULL, exten, NULL).
64-69: Good addition of debug traceability.Adding
fileandlineparameters to conference create/add functions improves debugging for bridge-related issues. The macros correctly pass__FILE__and__LINE__automatically.
71-90: LGTM - Clean tone API using Asterisk indications.The new
rpt_play_toneAPI leveragingast_get_indication_toneis a good approach that uses standard Asterisk tone zone infrastructure instead of DAHDI-specific tone generation. The convenience macros for common tones (rpt_play_congestion,rpt_play_dialtone) improve readability.
114-114: Reconsider talking threshold framing—value is duration, not audio energy.The
DEFAULT_TALKING_THRESHOLDof 160 is a time duration in milliseconds, not an audio energy level. It specifies how long the DSP must detect voice activity before marking a channel as talking. This is Asterisk's standard default for talk detection hangover and is appropriate for preventing false triggers from brief noise. If field testing reveals the repeater experiences missed detections or unwanted voicing of background noise, the duration may need adjustment, but the existing 160 ms default aligns with established VoIP/radio practice.apps/app_rpt/rpt_functions.c (1)
791-793: LGTM - Correct migration of channel type check.The change from
IS_PSEUDOtoIS_LOCALcorrectly adapts the tune request handling for the new channel architecture. The logic appropriately skips sendingAST_CONTROL_RADIO_UNKEYto Local channels, which don't require this indication, maintaining equivalent behavior to the previous DAHDI/pseudo implementation.apps/app_rpt/rpt_cli.c (1)
1167-1168: LGTM - CLI output updated for new channel structure.The channel listing correctly reflects the architectural changes:
localrxchannelandlocaltxchannelreplace the removed DAHDI-specific channels- Removed channels (
parrotchannel,dahdirxchannel,dahditxchannel,voxchannel) are no longer part of the rpt structureThis ensures operators can properly debug the new Local channel-based configuration.
apps/app_rpt/rpt_serial.c (1)
417-425: Guard RBI ioctl path against NULL / wrong-techlocalrxchannel.
rbi_out()now unconditionally usesmyrpt->localrxchannel. If that pointer can be NULL (or not backed by a DAHDI fd), this becomes a crash or hard failure.Consider a defensive check + clearer log:
static void rbi_out(struct rpt *myrpt, unsigned char *data) { + if (!myrpt->localrxchannel) { + ast_log(LOG_WARNING, "No localrxchannel available for RBI on node %s\n", myrpt->name); + return; + } if (rpt_radio_set_param(myrpt->localrxchannel, myrpt, RPT_RADPAR_REMMODE, RPT_RADPAR_REM_RBI1)) { /* if setparam ioctl fails, its probably not a pciradio card */ rbi_out_parallel(myrpt, data); return; } rpt_radio_set_remcommand_data(myrpt->localrxchannel, myrpt, data, 5); }apps/app_rpt/app_rpt.h (2)
738-742: Bridge pointer ownership/refcounting needs to be explicit.
struct rpt_confnow storesstruct ast_bridge *conf/*txconf. Please ensure the create/destroy paths consistentlyao2_ref()/ao2_cleanup()(or equivalent) those bridges and that concurrent access is synchronized.
937-943: Channel pointer reshuffle looks fine—verify invariants for “local” pointers.*
Given later code usesmyrpt->localrxchannelfor DAHDI ioctls and other low-level operations, please ensure the new channel layout still guaranteeslocalrxchannel/localtxchannelare initialized before any such use (and cleared safely on teardown).apps/app_rpt/rpt_telemetry.c (1)
1250-1317: Verify buffer size forextenarray and confirm unused status; preferast_copy_string()if retained.The
char exten[10]buffer withstrncpy()poses two risks: (1) ifRPT_CONF_NAMEorRPT_TXCONF_NAMEexceed 9 characters, the string will be truncated without NUL termination; (2) ifextenis not used after assignment, it generates-Wunused-variablewarnings.Recommended approach:
- If
extenis actually passed torpt_request_pseudo_chan()or other functions, verify the actual lengths of theRPT_*CONF_NAMEconstants and increase the buffer size accordingly (typically at least 32 bytes for Asterisk extension names).- Replace
strncpy()withast_copy_string()to guarantee NUL termination:ast_copy_string(exten, RPT_CONF_NAME, sizeof(exten));- If
extenis truly unused after the assignments shown, remove it entirely.Verify in your dialplan whether
"Telem"is the correct extension for Local channel requests in therpt_request_pseudo_chan()call.apps/app_rpt.c (6)
292-294: Bridge/Unreal includes look reasonable for the new Local/bridge path.Also applies to: 310-310
1799-1802: Dial-timer reset tweaks look consistent (fixes “canmatch” path keeping the digit timer alive).Also applies to: 2612-2613
5497-5504: Cleanup guards aroundmonchannel/localtxchannelhangup look good.
5331-5336: This pattern is correct for handling Local channel wrappers. WhenIS_LOCAL(myrpt->rxchannel)is true, usingmyrpt->localrxchannel(which is initialized tomyrpt->pchannel) for DAHDI-specific operations is the proper approach. The code correctly branches on channel technology: Local channels usedahdi_radio_set_ctcss_encode()on the underlying physical channel, while "radio" and "simpleusb" channels useast_sendtext(). No issue here.
1265-1292: Update documentation comment—no_hangup_chanparameter no longer applies.The function comment at line 1262 references "no_hangup_chan = 1 arg", which is outdated. The current code uses
ast_pbx_run()directly, and the channel is properly managed: the thread is joinable, correctly joined at line 1583, and error paths explicitly hang up the channel. No behavioral issues found.
1293-1305: Talker callback setup has NULL-deref vulnerability and ignores hook install failure at lines 1363-1368.Code directly dereferences
bridge_chanandbridge_chan->featureswithout NULL checks after callingast_channel_internal_bridge_channel(p->chan), and ignores the return value ofast_bridge_talk_detector_hook().Guard the calls and check the return value:
bridge_chan = ast_channel_internal_bridge_channel(p->chan); + if (!bridge_chan || !bridge_chan->features) { + ast_log(LOG_WARNING, "No bridge channel/features for %s; talk detector not installed\n", ast_channel_name(mychannel)); + } else { bridge_chan->tech_args.talking_threshold = DEFAULT_TALKING_THRESHOLD; bridge_chan->tech_args.silence_threshold = VOX_OFF_DEBOUNCE_COUNT * 20; - ast_bridge_talk_detector_hook(bridge_chan->features, rpt_handle_talker_cb, myrpt, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL); + if (ast_bridge_talk_detector_hook(bridge_chan->features, rpt_handle_talker_cb, myrpt, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL)) { + ast_log(LOG_WARNING, "Failed to install talk detector hook for %s\n", ast_channel_name(mychannel)); + } + }Likely an incorrect or invalid review comment.
apps/app_rpt/rpt_bridging.c (3)
29-35: Local channel type plumbing is consistent across helpers (type/name/channel/app/app_data).Also applies to: 50-52, 88-90, 150-154, 167-171
368-372:rpt_conf_get_mutednow hardcoded to “not muted”—verify this won’t break link-mute/user-mute features.
405-421: Tone-zone swap with channel lock/unref looks fine.
mkmer
force-pushed
the
local-no-dialplan
branch
from
9649075 to
6564d35
Compare
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
mkmer
force-pushed
the
local-no-dialplan
branch
from
0c1f6fd to
5eb2df6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/app_rpt/rpt_link.c (1)
757-772: Failure cleanup leak: ifrpt_conf_add(l->pchan, ...)fails,l->pchanshould be hung up.
On the failure path (Lines 768-772), you hang upl->chanand freel, but don’t hang upl->pchan. That can leak a Local channel.Proposed change:
if (rpt_conf_add(l->pchan, myrpt, RPT_CONF)) { + ast_hangup(l->pchan); ast_hangup(l->chan); rpt_link_free(l); return -1; }apps/app_rpt/rpt_radio.c (1)
58-69: Add NULL and DAHDI type checks before allmyrpt->localrxchannelioctl operations.Multiple functions use
myrpt->localrxchannelfor DAHDI ioctls without validation:
rpt_radio_set_remcommand_data()(line 64)rpt_pciradio_serial_remote_io()(lines 77, 82, 104, 113, 125)dahdi_set_radpar()viarpt_radio_set_param()calls (lines 88, 92, 132, 141, 144)Dereferencing a NULL
localrxchannelinast_channel_fd()will crash. Lack of DAHDI type validation can cause ioctls to fail silently or with misleading errors on non-DAHDI channels.Add guards before these operations:
if (!myrpt->localrxchannel) { ast_log(LOG_WARNING, "No localrxchannel available for DAHDI radio ioctls\n"); return -1; } if (!CHAN_TECH(myrpt->localrxchannel, "DAHDI")) { ast_log(LOG_WARNING, "localrxchannel %s is not DAHDI; cannot perform DAHDI ioctls\n", ast_channel_name(myrpt->localrxchannel)); return -1; }Also,
rpt_radio_set_remcommand_data(struct ast_channel *chan, ...)logschanbut performs the ioctl onmyrpt->localrxchannel. Either usechanfor the ioctl or remove the unused parameter and update callers accordingly, depending on the intended design.apps/app_rpt.c (3)
2833-2933: Return failure when TXPCHAN fails to join TXCONF (currently continues in broken state).
Ifrpt_conf_add(myrpt->txpchannel, myrpt, RPT_TXCONF)fails,rpt_setup_channels()still returns0. This can leave the node “started” with missing bridge wiring.--- a/apps/app_rpt.c +++ b/apps/app_rpt.c @@ if (rpt_conf_add(myrpt->txpchannel, myrpt, RPT_TXCONF)) { rpt_hangup_rx_tx(myrpt); rpt_hangup(myrpt, RPT_TXPCHAN); rpt_hangup(myrpt, RPT_PCHAN); rpt_hangup(myrpt, RPT_MONCHAN); + rpt_hangup(myrpt, RPT_LOCALTXCHAN); + return -1; } return 0;
1293-1370: Guardast_channel_internal_bridge_channel()result andppointer against NULL before dereferencing; prevent hard crash during autopatch bringup.At lines 1361–1368,
p(fromast_channel_tech_pvt()) and the result ofast_channel_internal_bridge_channel(p->chan)are dereferenced unconditionally. Per Asterisk API contracts,ast_channel_internal_bridge_channel()returns NULL when the channel is not yet in a bridge, and the unreal pvt pointerpmay also be NULL. Current code will crash if either is NULL.--- a/apps/app_rpt.c +++ b/apps/app_rpt.c @@ -1360,13 +1360,21 @@ void *rpt_call(void *this) /* add talker callback */ myrpt->patch_talking = 0; /* Initialize patch_talking flag */ p = ast_channel_tech_pvt(mychannel); + if (!p || !p->chan) { + ast_log(LOG_ERROR, "Autopatch: missing Local channel unreal pvt (%p) or peer channel\n", p); + /* Decide: fail autopatch, or continue without talker detection */ + } else { ast_debug(1, "Adding talker callback to channel %s, private data %p\n", ast_channel_name(mychannel), p); bridge_chan = ast_channel_internal_bridge_channel(p->chan); - bridge_chan->tech_args.talking_threshold = DEFAULT_TALKING_THRESHOLD; - bridge_chan->tech_args.silence_threshold = VOX_OFF_DEBOUNCE_COUNT * 20; /* VOX is in 20ms count */ + if (!bridge_chan || !bridge_chan->features) { + ast_log(LOG_WARNING, "Autopatch: no bridge channel/features yet for %s\n", ast_channel_name(mychannel)); + } else { + bridge_chan->tech_args.talking_threshold = DEFAULT_TALKING_THRESHOLD; + bridge_chan->tech_args.silence_threshold = VOX_OFF_DEBOUNCE_COUNT * 20; /* VOX is in 20ms count */ - ast_debug(1, "Got Bridge channel %p\n", bridge_chan); - ast_bridge_talk_detector_hook(bridge_chan->features, rpt_handle_talker_cb, myrpt, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL); + ast_debug(1, "Got Bridge channel %p\n", bridge_chan); + ast_bridge_talk_detector_hook(bridge_chan->features, rpt_handle_talker_cb, myrpt, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL); + } + }
1261-1292: Fix autopatch PBX thread creation error handling: wrong return check enables undefined behavior and resource leak.
ast_pthread_create()returns 0 on success and a non-zero errno value on failure. The current checkif (res < 0)is incorrect—a positive error code would pass through as success, leavingthreadiduninitialized andpatch_thread_dataleaked. Change toif (res)and addast_free(patch_thread_data)to the error path.--- a/apps/app_rpt.c +++ b/apps/app_rpt.c @@ -1515,7 +1515,8 @@ rpt_call(void *data) patch_thread_data->myrpt = myrpt; patch_thread_data->mychannel = mychannel; res = ast_pthread_create(&threadid, NULL, rpt_pbx_autopatch_run, patch_thread_data); - if (res < 0) { + if (res) { + ast_log(LOG_ERROR, "Unable to start PBX (%d): %s\n", res, strerror(res)); - ast_log(LOG_ERROR, "Unable to start PBX!\n"); rpt_mutex_lock(&myrpt->lock); myrpt->callmode = CALLMODE_DOWN; rpt_mutex_unlock(&myrpt->lock); ast_hangup(mychannel); ast_hangup(genchannel); + ast_free(patch_thread_data); pthread_exit(NULL); }apps/app_rpt/rpt_telemetry.c (1)
1250-1302: Unusedextenvariable - dead code.The
extenvariable is populated in the switch statement (lines 1296, 1300) but is never used. The call torpt_request_pseudo_chanon line 1304 passes the hardcoded string"Telem"instead of usingexten. Either remove the unused variable or use it in the function call.- char exten[10]; ... - switch (mytele->mode) { - case ID1: - case PLAYBACK: - case TEST_TONE: - case STATS_GPS_LEGACY: - type = RPT_CONF; - strncpy(exten, RPT_CONF_NAME, sizeof(exten)); - break; - default: - type = RPT_TXCONF; - strncpy(exten, RPT_TXCONF_NAME, sizeof(exten)); - break; - } + switch (mytele->mode) { + case ID1: + case PLAYBACK: + case TEST_TONE: + case STATS_GPS_LEGACY: + type = RPT_CONF; + break; + default: + type = RPT_TXCONF; + break; + }apps/app_rpt/rpt_bridging.c (1)
271-314: Missing NULL check for unreal pvt - same issue asrpt_request_pseudo_chan.Similar to the previous function,
pis used without checking ifast_channel_tech_pvtreturned a valid pointer.rpt_disable_cdr(chan); p = ast_channel_tech_pvt(chan); + if (!p || !p->owner || !p->chan) { + ast_log(LOG_ERROR, "Local channel %s missing unreal pvt or endpoints\n", ast_channel_name(chan)); + ast_hangup(chan); + return -1; + } ast_answer(p->owner); ast_answer(p->chan);
♻️ Duplicate comments (4)
apps/app_rpt/rpt_radio.c (1)
38-44: Potential NULL-deref:rpt_radio_rx_set_ctcss_decode()passesmyrpt->localrxchannelwithout validating it.
This is the same core risk raised previously (the guard checksmyrpt->rxchanneltech, but the ioctl target ismyrpt->localrxchannel).apps/app_rpt/app_rpt.h (1)
744-748: Header correctness: add a forward declaration forstruct ast_bridge.
This header now usesstruct ast_bridge *but doesn’t show a forward-declare; relying on transitive includes is brittle and can break compilation.Proposed minimal fix near the top of the header (before first use):
+struct ast_bridge;apps/app_rpt/rpt_call.c (1)
25-60: Propagaterpt_disable_cdr()failures fromrpt_setup_call().The function at line 68 ignores the return value of
rpt_disable_cdr(), which can fail and return-1. Despite logging the failure,rpt_setup_call()unconditionally returns 0 at line 77, suppressing the error from callers likerpt_make_call(). This silently hides CDR disabling failures, degrading accounting behavior without notification.Handle the error:
int rpt_setup_call(struct ast_channel *chan, const char *addr, int timeout, const char *driver, const char *data, const char *desc, const char *callerid) { ast_debug(1, "Requested channel %s\n", ast_channel_name(chan)); ast_set_read_format(chan, ast_format_slin); ast_set_write_format(chan, ast_format_slin); - rpt_disable_cdr(chan); + if (rpt_disable_cdr(chan)) { + ast_log(AST_LOG_WARNING, "Failed to disable CDR for requested channel %s\n", ast_channel_name(chan)); + return -1; + } ast_channel_appl_set(chan, "Rpt");apps/app_rpt.c (1)
3649-3663: Parrot filename overflow fix looks good; minor consistency nit on NULLing stream.
Thesnprintf(PARROTFILE ".wav", ...)change addresses the priorstrcatoverflow risk. Consider settingmyrpt->parrotstream = NULL(not0) for consistency with nearby code.- myrpt->parrotstream = 0; + myrpt->parrotstream = NULL;
🧹 Nitpick comments (8)
apps/app_rpt/rpt_bridging.h (3)
50-63: Pseudo-channel request API: document/validateextenexpectations.
rpt_request_pseudo_chan(..., const char *exten)/__rpt_request_pseudo(..., const char *exten)now makeextenpart of the public contract; please clarify whether this is a dialplan extension, a label-only string, and whether NULL/empty is allowed (and enforce it consistently).
64-70:__rpt_conf_create/__rpt_conf_add: iffile/linearen’t used, suppress unused warnings or use in logs.
Right now the header strongly suggests these are for diagnostics; if the implementation doesn’t use them, add(void)file; (void)line;(or actually include them inast_log/ast_debug).
92-99:rpt_conf_get_muted()doc vs behavior +DEFAULT_TALKING_THRESHOLDvisibility.
- If
rpt_conf_get_muted()is still a stub (always 0), the header doc is misleading—either implement or mark as deprecated/unused.DEFAULT_TALKING_THRESHOLDis a public macro now; ensure it’s not duplicating another threshold constant elsewhere (and consider config-driven override).Also applies to: 114-114
apps/app_rpt/app_rpt.h (1)
943-945:IS_LOCAL()uses channel name prefix; consider using tech type check instead.
Name-prefix checks can be spoofed/unexpected if naming changes;CHAN_TECH(c, "Local")is a tighter predicate (assumingcis non-NULL). Not blocking, but worth considering since Local channels are now core to the architecture.Also applies to: 1110-1112, 1022-1022
apps/app_rpt.c (2)
4855-4855: Avoidast_verb(1, ...)debug spam in hot paths.
“I've made it to the loop” and “Parrot attached …” will be very noisy on production systems. Preferast_debug()(or higher verbosity level) unless intentionally user-facing.- ast_verb(1, "I've made it to the loop"); + ast_debug(1, "Entered main rpt loop\n"); @@ - ast_verb(1, "Parrot attached to %s\n", ast_channel_name(myrpt->pchannel)); + ast_debug(1, "Parrot attached to %s\n", ast_channel_name(myrpt->pchannel));Also applies to: 5277-5281
4641-4651: Unreachable/commented code after return.
txpchannel_readreturns immediately; the following commented block will never be revisited by the compiler or readers. Suggest removing the dead comments or restructuring.apps/app_rpt/rpt_bridging.c (2)
316-343: Previous issues addressed; consider guarding against re-initialization.The buffer size and debug log timing issues from previous reviews have been fixed. Consider adding a check if
*confptris already non-NULL to prevent potential bridge leaks on re-initialization, or document that this function should only be called once per conference type.+ if (*confptr) { + ast_debug(1, "Conference '%s' bridge already exists, skipping creation\n", conference_name); + return 0; + } conf = ast_bridge_base_new(AST_BRIDGE_CAPABILITY_MULTIMIX,
345-369: Buffer size inconsistency between__rpt_conf_createand__rpt_conf_add.
conference_nameis 64 bytes in__rpt_conf_createbut only 10 bytes here. For consistency and to avoid potential truncation issues, use the same buffer size.- char conference_name[10] = "\0"; + char conference_name[64] = "";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/app_rpt.c(46 hunks)apps/app_rpt/app_rpt.h(4 hunks)apps/app_rpt/rpt_bridging.c(12 hunks)apps/app_rpt/rpt_bridging.h(5 hunks)apps/app_rpt/rpt_call.c(1 hunks)apps/app_rpt/rpt_cli.c(1 hunks)apps/app_rpt/rpt_functions.c(1 hunks)apps/app_rpt/rpt_link.c(2 hunks)apps/app_rpt/rpt_manager.c(1 hunks)apps/app_rpt/rpt_radio.c(5 hunks)apps/app_rpt/rpt_serial.c(2 hunks)apps/app_rpt/rpt_telemetry.c(3 hunks)configs/rpt/modules.conf(1 hunks)configs/rpt/rpt.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/app_rpt/rpt_manager.c
- apps/app_rpt/rpt_cli.c
- configs/rpt/rpt.conf
- configs/rpt/modules.conf
- apps/app_rpt/rpt_functions.c
- apps/app_rpt/rpt_serial.c
🧰 Additional context used
🧬 Code graph analysis (4)
apps/app_rpt/rpt_link.c (1)
apps/app_rpt/rpt_bridging.c (1)
__rpt_request_pseudo(271-314)
apps/app_rpt.c (2)
apps/app_rpt/rpt_bridging.c (3)
rpt_request_pseudo_chan(249-269)rpt_hangup(109-140)__rpt_request_pseudo(271-314)apps/app_rpt/rpt_radio.c (2)
dahdi_radio_set_ctcss_encode(46-49)rpt_radio_set_param(51-56)
apps/app_rpt/rpt_bridging.c (1)
apps/app_rpt/rpt_call.c (2)
rpt_make_call(80-87)rpt_disable_cdr(30-61)
apps/app_rpt/rpt_bridging.h (1)
apps/app_rpt/rpt_bridging.c (6)
rpt_request_pseudo_chan(249-269)__rpt_request_pseudo(271-314)__rpt_conf_create(316-343)__rpt_conf_add(345-369)rpt_play_tone(382-400)rpt_conf_get_muted(371-375)
🪛 GitHub Actions: CI
apps/app_rpt/rpt_bridging.c
[error] 391-391: Compiler warning promoted to error: the comparison will always evaluate as 'true' for the address of 'zone->country' when checking for a missing tone. (address) -Werror
🔇 Additional comments (7)
apps/app_rpt/rpt_bridging.h (2)
71-90: Tone API macro names: OK, but consider consistency with Asterisk naming.
rpt_play_dialtone()uses"dial"(not"dialtone"), which is fine if it matches the zone indication name you expect everywhere. If mixed tone-zones are expected, it may be worth centralizing the canonical strings (or validating existence).
2-9: The concern is not substantiated.RPT_LOCALTXCHANis properly handled throughout the codebase: it has a corresponding struct field (myrpt->localtxchannel), is explicitly handled inrpt_chan_channel()(line 88–89), is assigned during request setup (lines 237–239), and is included in the type string function and debug output. The grouped handling inrpt_chan_name(),rpt_chan_app(), andrpt_chan_app_data()returningNULLis intentional—these functions are designed to returnNULLfor internal channel types (PCHAN,LOCALTXCHAN,MONCHAN,TXPCHAN). No resource leaks or missing logic detected.apps/app_rpt/rpt_link.c (1)
759-764: The documented locking requirement for__rpt_request_pseudo()does not apply to this call. WhenRPT_LINK_CHANflag is set (as here), the function receives arpt_linkpointer asdataandmyrptis NULL. The implementation only accessesmyrptfields within a guard block that explicitly checks!(flags & RPT_LINK_CHAN), so no lock is needed. The call is correct.For the second concern: Verify that
"IAX-Link"is a validLocal/extension in your Asterisk dialplan configuration, as this depends on runtime dialplan setup which cannot be verified from the codebase.apps/app_rpt/rpt_telemetry.c (2)
1313-1317: LGTM!Good addition of error handling for conference join failure with appropriate logging and cleanup via the abort path.
1660-1663: LGTM!The updated
rpt_conf_addcall with explicit conference type is consistent with the new bridge-based approach.apps/app_rpt/rpt_bridging.c (2)
402-424: LGTM!The tone zone management correctly handles reference counting and uses proper channel locking during the zone change.
50-51: LGTM!The renaming from
RPT_DAHDITXCHANtoRPT_LOCALTXCHANis consistent throughout the file, aligning with the DAHDI-to-Local migration.Also applies to: 88-89
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/app_rpt.c (4)
4611-4626: Samecheck_parrot()per-frame syscall concern onmonchannel_read().This will repeat the same close/unlink checks at frame rate; please gate cleanup to state transitions (or at least rate-limit).
1319-1368: Autopatch talker hook setup lacks NULL pointer guards and directly accesses internal structs.The code at lines 1355-1363 performs several unchecked pointer dereferences:
p = ast_channel_tech_pvt(mychannel)is unchecked, thenp->chanis dereferenced at line 1357bridge_chan = ast_channel_internal_bridge_channel(p->chan)is unchecked, thenbridge_chan->tech_argsis accessed at lines 1358-1359bridge_chan->featuresis passed toast_bridge_talk_detector_hook()at line 1363 without NULL verificationAdd NULL checks for
p,p->chan,bridge_chan, andbridge_chan->features. Fail the autopatch setup cleanly (hangup channels and setCALLMODE_DOWN) if any check fails.
292-310: Add Asterisk version guard or document minimum supported version forcore_unreal.husage.The
ast_unreal_pvtstructure accessed in the code is part of Asterisk's internal, non-public API and changes frequently across releases. Without a compile-time version guard (e.g.,#ifdef ASTERISK_VERSION_CHECKwith minimum version) or explicit documentation of the minimum supported Asterisk version, this code risks building and crashing against incompatible Asterisk releases. Either add a version guard or clearly document the supported Asterisk version range in the build system and README.
1264-1304: Fix return value for invalid hook_pvt and reduce debug noise.The callback returns
1whenmyrptis NULL, but Asterisk hook callbacks should return-1to request hook removal (not continue with invalid state). Return0to continue or-1to stop the hook—returning1does not match the documented convention and may cause undefined behavior.Additionally, the
ast_debug(1, ...)fires on every talking transition and will be verbose in production if debug logging is enabled; consider raising the debug level to4or higher unless this level of detail is essential for normal operation.apps/app_rpt/rpt_bridging.c (2)
176-247: Add NULL checks before dereferencing Local channel endpoints in__rpt_request.When
tech == "Local",p->chanandp->ownerare dereferenced without validation. Apply the guard pattern used elsewhere: check(!p || !p->owner || !p->chan)andast_hangup(chan)on failure, consistent with patterns inrpt_call.cand other Local channel handling in this file.
275-318:__rpt_request_pseudo: missing NULL checks before dereferencingast_unreal_pvtand endpoints (crash risk).Lines 288–289 dereference
p,p->owner, andp->chanwithout validation. Addif (!p || !p->owner || !p->chan)check afterp = ast_channel_tech_pvt(chan);and log error/return, matching the pattern used inrpt_call.cfor the same scenario.
♻️ Duplicate comments (5)
apps/app_rpt.c (3)
2832-2931:rpt_setup_channels: deprecation log text is incorrect + current behavior disables node on PSEUDO.Line 2847-2853 logs “DAHDI/Pseudo … is depreciated” while testing
IS_PSEUDO_NAME(...)(typo + wrong term), and it immediately returns-1(disables the repeater) when PSEUDO is configured. If that’s intentional, consider making it a hard error message (not “deprecated”), and fix the wording.
3648-3676:check_parrot()overflow fix looks good; but calling it per-frame will spamunlink()syscalls.Good change switching to
snprintf(..., PARROTFILE ".wav", ...)(fixes the earlierstrcatrisk). However,check_parrot()is invoked on everyrxchannel_read()(andmonchannel_read()), and it will attemptunlink()continuously when parrot is off. Consider gating so cleanup runs only on state transitions (e.g., when parrot just turned off, or whenparrotstreamwas non-NULL /parrotstate != IDLE).
5358-5368: Channel wait list:localtxchannelcan be NULL; decide between explicit assert vs safe skip.You conditionally add
monchannelnow, but still unconditionally appendmyrpt->localtxchannelwhen it differs fromtxchannel. If the design is fail-fast, consider anast_assert(myrpt->localtxchannel)right beforecs[n++] = ...to make the contract explicit (and avoid UB insideast_waitfor_nif it ever becomes NULL due to future refactors).apps/app_rpt/rpt_bridging.c (2)
320-347:__rpt_conf_create: free/destroy existing bridge on re-init to avoid leaks.If
*confptris already set, destroy it before overwriting. Also consider checkingsnprintftruncation even with the larger buffer.
349-373:__rpt_conf_add: conference name buffer is still too small (truncation/ambiguity).
char conference_name[10]can truncateRPT_CONF_NAME/RPT_TXCONF_NAME, making logs misleading and harming diagnostics. Use at least the same size as__rpt_conf_create(e.g., 64).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/app_rpt.c(46 hunks)apps/app_rpt/rpt_bridging.c(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/app_rpt/rpt_bridging.c (1)
apps/app_rpt/rpt_call.c (2)
rpt_make_call(80-87)rpt_disable_cdr(30-61)
apps/app_rpt.c (2)
apps/app_rpt/rpt_bridging.c (3)
rpt_request_pseudo_chan(249-273)rpt_hangup(109-140)__rpt_request_pseudo(275-318)apps/app_rpt/rpt_radio.c (2)
dahdi_radio_set_ctcss_encode(46-49)rpt_radio_set_param(51-56)
🔇 Additional comments (3)
apps/app_rpt/rpt_bridging.c (2)
386-404: Tone-indication migration looks correct (includingzone->country[0]fix).
29-35: These headers are standard public Asterisk APIs and stable in Asterisk 20 (the project's target version per README.md). No version checks needed.apps/app_rpt.c (1)
5329-5335: Review comment cannot be verified — the code referenced does not exist in the current codebase.The review mentions specific code at lines 5329-5335 in
apps/app_rpt.c, including calls toIS_LOCAL(),dahdi_radio_set_ctcss_encode(), and references tomyrpt->localrxchannel, but none of these identifiers exist in the file. The functions, macros, and struct members cited in the review are not found through any search of the repository.Either this review is based on a different code version or branch, or the references are incorrect. Please clarify the exact location and version of the code being reviewed.
InterLinked1
left a comment
InterLinked1
left a comment
There was a problem hiding this comment.
Overall, this looks really solid! Misc. comments throughout.
This is going to need a lot of testing to ensure that everything that used to work still works. I'm hoping @tsawyer might be able to help us test this. I'll also test once I get my node up here in the next week or so.
I am 99% sure I broke something - I can only test local USB node, a HUB, and a voter (RTCM mix mode) configuration. |
There was a problem hiding this comment.
-- Channel Local/Telem@default-00000084;2 joined 'softmix' base-bridge <fd100a41-1404-4bb2-85a9-58ae55e15f03>
[2025-12-13 15:26:25.122] WARNING[544546]: app_rpt/rpt_telemetry.c:2865 rpt_tele_thread: Attempting to clear active_telem (nil) when telem is 0x7fff00044ec0
-- Channel Local/Telem@default-00000084;2 left 'softmix' base-bridge <fd100a41-1404-4bb2-85a9-58ae55e15f03>
I'm seeeing this repeatedly on 3 nodes of various configs and both architectures. All nodes work otherwise so far. It seems synchronous with each time a connection (made outbound) being received unkeys. Connection mode doesn't matter.
All nodes are running asterisk 22.5.2+asl3-3.6.3-1.deb13.
Yes, me too. I think my "fix" for the log message is generating a "normal" condition warning OR it's actually bad and been happening for a while. #852 is where I moved things a bit. |
mkmer
force-pushed
the
local-no-dialplan
branch
from
89c96c7 to
82173c8
Compare
mkmer
force-pushed
the
local-no-dialplan
branch
from
acd35e0 to
45c5891
Compare
| [pstn-out] | ||
| exten => _NXXNXXXXXX,1,playback(ss-noservice) | ||
| same => n,Congestion | ||
| exten => _NXXNXXXXXX,1,Wait(1) |
There was a problem hiding this comment.
Just asking ... Is this a change that needs to be made when upgrading a system to the new code? or will upgraded systems with the older extensions.conf be OK?
There was a problem hiding this comment.
It should be fine in older systems, in the new version we truncate the (ss-noservice) message without it.
There was a problem hiding this comment.
Asking again ... Is this a change that needs to be made when upgrading a system to the new code (i.e. do we need a configuration file upgrader)?
There was a problem hiding this comment.
Ah, it is necessary to prevent truncation of the message. So "yes" we should upgrade the file.
It will still "function" (aka no crash or other "bad" behavior), just truncates the message.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/app_rpt/rpt_bridging.h (1)
50-60: Update doc comment: no longer “pseudo channel.”Line 51–55 still says “pseudo channel” but the API now requests Local/Announcer/Recorder channels.
✏️ Doc fix
- * \brief Request a pseudo channel + * \brief Request a Local/bridge channelapps/app_rpt/rpt_radio.c (1)
71-144: Add NULL check forlocalrxchannelbefore repeated ioctl/setparam calls.The equality check in the caller (
if (myrpt->rxchannel != myrpt->localrxchannel)) does not validate thatlocalrxchannelis non-NULL. Sincelocalrxchannelcan be NULL when the RX channel is not local, the function risks NULL dereference whenast_channel_fd(myrpt->localrxchannel, 0)is called repeatedly starting at line 76. Add an early guard to fail fast with a diagnostic log.✅ Defensive guard
int rpt_pciradio_serial_remote_io(struct rpt *myrpt, unsigned char *txbuf, int txbytes, unsigned char *rxbuf, int rxmaxbytes, int asciiflag) { int i, index, oldmode, olddata; struct dahdi_radio_param prm; + + if (!myrpt->localrxchannel) { + ast_log(LOG_WARNING, "No localrxchannel for remote IO on node %s\n", myrpt->name); + return -1; + } prm.radpar = DAHDI_RADPAR_UIOMODE;apps/app_rpt.c (1)
1270-1278: The autopatch function does not preserve no_hangup_chan semantics as documented.The comment states the function creates a thread "with
no_hangup_chan = 1 arg", but the code callsast_pbx_run(autopatch->mychannel), which is merely a wrapper forast_pbx_run_args(chan, NULL). This default behavior setsno_hangup_chanto 0 (false), causing Asterisk to hang up the channel automatically when PBX execution completes—contradicting the documented intent.To preserve the required no-hangup semantics, use
ast_pbx_run_args()with a properly initializedstruct ast_pbx_argswhereno_hangup_chan = 1.
🧹 Nitpick comments (3)
apps/app_rpt/rpt_bridging.c (2)
310-347: Consider setting read/write formats here for parity.Line 321–329 requests Local/Recorder/Announcer channels but doesn’t set formats like
__rpt_request_local_chan. If these channels carry audio, aligning formats can prevent unexpected transcoding.♻️ Suggested patch
chan = ast_request(rpt_chan_type_name(chantype, flags), cap, NULL, NULL, exten, NULL); if (!chan) { ast_log(LOG_ERROR, "Failed to request local channel\n"); return -1; } +ast_set_read_format(chan, ast_format_slin); +ast_set_write_format(chan, ast_format_slin); + rpt_disable_cdr(chan);
415-419: TODO stub: confirm mute semantics.Line 417 leaves mute detection unimplemented. If mute state is still needed, I can help wire it in or remove the TODO to avoid ambiguity.
apps/app_rpt/rpt_manager.c (1)
282-293: Stale comment references DAHDI after migration to Local channels.The code at Line 282 correctly checks for
"Local/pseudo", but the comment at Lines 290-291 still references "DAHDI pseudo channel". This should be updated for consistency.📝 Suggested fix
/* rxchan might've disappeared in the meantime. Verify it still exists before we try to lock it, - * at least unless it's a DAHDI pseudo channel. + * at least unless it's a Local pseudo channel. * XXX This was added to address assertions due to bad locking, but app_rpt should probably
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/app_rpt/rpt_bridging.c`:
- Around line 229-247: Remove the IS_LOCAL_NAME gate around rpt_make_call and
the subsequent state check so that rpt_make_call(chan, device,
RPT_DIAL_DURATION, tech, rpt_chan_app(chantype, flags),
rpt_chan_app_data(chantype), myrpt->name) is invoked for Local/... tech as well
and ast_channel_state(chan) is validated against AST_STATE_UP for all techs
(using ast_log and ast_hangup/return -1 on failure). Keep the channel pointer
assignment via chanptr = rpt_chan_channel(myrpt, NULL, chantype); *chanptr =
chan; and preserve setting myrpt->localrxchannel and myrpt->localtxchannel to
chan when IS_LOCAL_NAME(tech) (otherwise NULL) in the existing switch; this
ensures Local channels initialize and still marks them as local.
In `@apps/app_rpt/rpt_serial.c`:
- Around line 417-424: rbi_out currently dereferences myrpt->localrxchannel
before calling rpt_radio_set_param and rpt_radio_set_remcommand_data; guard
against a NULL localrxchannel at the start of rbi_out and take the parallel
fallback when it's NULL. Specifically, in rbi_out check if myrpt->localrxchannel
is NULL and if so call rbi_out_parallel(myrpt, data) and return; otherwise
proceed with rpt_radio_set_param(...) and rpt_radio_set_remcommand_data(...).
Ensure the same fallback logic used when rpt_radio_set_param fails remains
unchanged.
🧹 Nitpick comments (2)
apps/app_rpt.c (2)
3727-3741: Consider gatingcheck_parrot()cleanup to avoid per-frame unlink calls.
This runs inside audio paths; a simple “cleanup-done” flag could reduce syscalls under load.
4392-4406: Avoid holding the rpt lock duringast_readin the disconnect path.
Even though the channel is ready, keeping the lock held here is inconsistent with the other read paths and can still elongate the main loop if anything stalls.♻️ Suggested adjustment
- if (who == l->pchan) { - struct ast_frame *f; - f = ast_read(l->pchan); - if (!f) { - ast_debug(1, "@@@@ rpt:Hung Up\n"); - rpt_mutex_unlock(&myrpt->lock); - return -1; - } - ast_frfree(f); - rpt_mutex_unlock(&myrpt->lock); - return 0; - } else { + if (who == l->pchan) { + struct ast_frame *f; + rpt_mutex_unlock(&myrpt->lock); + f = ast_read(l->pchan); + if (!f) { + ast_debug(1, "@@@@ rpt:Hung Up\n"); + return -1; + } + ast_frfree(f); + return 0; + } else { continue; }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
To simplify channel driver setup we allow the "devstr=" value to be empty/blank indicating that we can/should match the first available interface. This works (and will continue to work) well as long as the "devstr=" value in the configuration file remains empty/blank. But, if the value is ever provided then we only match interfaces with the specified string. Moving the interface (accidentally or intentionally) to a different "port" will result in not finding/matching the interface. To minimize conflicts, we want to avoid having the tune menu write out the specific "devstr=" value to the configuration file unless needed. Now, we check if the current "devstr=" value is empty/blank and that there is only a single audio interface connected to the system. If so, we leave the value empty/blank. In addition, included (and reworked) the changes from unmerged PR #394 to add support for matching interface on the device "serial" number.
Use ast_str for stats url Add failure path for `ast_str_create()` Use RPT_AST_STR_INIT_SIZE eliminating magic numbers Simplify statpost string
mkmer
force-pushed
the
local-no-dialplan
branch
from
a861ff5 to
43f5b6a
Compare
5 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Configuration Changes
✏️ Tip: You can customize this high-level summary in your review settings.