rpt_telemetry: Update PAGE command to use correct channel. #493

mkmer · 2025-02-16T18:42:42Z

Addressing issue #481
Looks like we are pointing to the wrong channel.

mkmer · 2025-02-16T18:45:00Z

debug.log
For history - debug log attached

mkmer · 2025-02-16T18:55:12Z

BTW: I suspect this is wrong as well, but have no way to test.

Lines 1171 to 1174 in b73bc4c

    
           res = mdc1200gen_start(myrpt->txchannel, mdcp->type, mdcp->UnitID, mdcp->DestID, mdcp->subcode); 
        
           ast_free(mdcp); 
        
           while (ast_channel_generatordata(myrpt->txchannel)) { 
        
           	if (ast_safe_sleep(myrpt->txchannel, 50)) {

KB4MDD · 2025-02-16T23:03:02Z

I have to ask what is the difference between mychannel and txchannel?

Was paging broken?

mkmer · 2025-02-16T23:55:44Z

COP 48 -> is paging.
Best I can tell, myrpt->txchannel is not the conferenced channel this function is supposed to play in. Every other instance in rpt_tele_thread is using mychannel. This change DOES fix the problem (maybe not correctly?)

mkmer · 2025-02-17T00:01:40Z

I'm not positive, but I don't think this is "right" - a bit out of my comfort zone here:

app_rpt/apps/app_rpt/rpt_telemetry.c

Line 1172 in c857a1a

ast_free(mdcp);

Is this a "hidden" malloc?

app_rpt/apps/app_rpt/rpt_telemetry.c

Line 1156 in c857a1a

mdcp = (struct mdcparams *) mytele->submode.p;

Same here:

app_rpt/apps/app_rpt/rpt_telemetry.c

Line 1149 in c857a1a

ast_free((char *) mytele->submode.p);

Allan-N · 2025-02-17T00:03:12Z

I gotta ask ... Without the proposed change, the PAGE code appears to be waiting on "mychannel" (an allocated pseudo-channel) and then playing the "page" to "myrpt->txchannel". What's the difference between the two channels? Is the "page" ONLY supposed to go to the "tx" channel? and could we be waiting on the wrong channel?

mkmer · 2025-02-17T00:08:57Z

The lock out is between rpt_tele_thread and app_rpt -> both trying to send "stuff" to the TX channel (I think anyway).
Since everything else in telemetry talks to the psudo channel, why wouldn't this? (As usual, I could easily be off mark)

Allan-N · 2025-02-17T00:15:12Z

Is this a "hidden" malloc?

No. The allocation is here :

app_rpt/apps/app_rpt/rpt_functions.c

Lines 1584 to 1595 in c857a1a

    
           cp = ast_malloc(j + 100); 
        
           if (!cp) { 
        
           	ast_log(LOG_WARNING, "cannot malloc"); 
        
           	return DC_ERROR; 
        
           } 
        
           memset(cp, 0, j + 100); 
        
           for (i = 1; i < argc; i++) { 
        
           	if (i != 1) 
        
           		strcat(cp, ","); 
        
           	strcat(cp, argv[i]); 
        
           } 
        
           rpt_telemetry(myrpt, PAGE, cp);

mkmer · 2025-02-17T00:20:36Z

Is this a "hidden" malloc?

No. The allocation is here :

app_rpt/apps/app_rpt/rpt_functions.c

Lines 1584 to 1595 in c857a1a

cp = ast_malloc(j + 100);

if (!cp) {

ast_log(LOG_WARNING, "cannot malloc");

return DC_ERROR;

}

memset(cp, 0, j + 100);

for (i = 1; i < argc; i++) {

if (i != 1)

strcat(cp, ",");

strcat(cp, argv[i]);

}

rpt_telemetry(myrpt, PAGE, cp);

Oh - so my proposed memory leak fix is hidden in the called function? That seems a bit odd/difficult to follow.
I think we should pull that out if this is true (go with my proposed free and remove the above ref one)
Thoughts?
I guess looking at it again - this is a treading thing?

Allan-N · 2025-02-17T00:28:07Z

so my proposed memory leak fix is hidden in the called function? That seems a bit odd/difficult to follow.
I think we should pull that out if this is true (go with my proposed free and remove the above ref one)
Thoughts?

If the intent is for the caller to say "here's everything you need to do what I want and I'm gonna move on" then it's essentially handing off the allocated memory and need not worry about it's disposal. The alternative would be to assume that the rpt_telemetry() call either operates synchronously or that it will make a copy of any data it needs to retain. I'd leave the code alone (having rpt_telemetry free any allocated/passed "data"). Of course, a few comments can't hurt 😄

KB4MDD · 2025-02-17T00:28:58Z

So is mychannel associated with a conference that would cause this to go out to all linked repeaters?

If so, that could be the reason it is directed to the txchannel.

Allan-N · 2025-02-17T00:33:24Z

So is mychannel associated with a conference that would cause this to go out to all linked repeaters?

It looks like the conference (as currently being setup) is NOT limited to the local tx conference.

app_rpt/apps/app_rpt/rpt_telemetry.c

Lines 1057 to 1060 in c857a1a

    
           /* make a conference for the tx */ 
        
           /* If the telemetry is only intended for a local audience, only connect the ID audio to the local tx conference so linked systems can't hear it */ 
        
           /* first put the channel on the conference in announce mode */ 
        
           if (rpt_conf_add(mychannel, myrpt, mytele->mode == ID1 || mytele->mode == PLAYBACK || mytele->mode == TEST_TONE || mytele->mode == STATS_GPS_LEGACY ? RPT_CONF : RPT_TELECONF, RPT_CONF_CONFANN)) {

mkmer · 2025-02-17T00:42:02Z

So - what's the right answer? Use mychannel and add it to the local conference or try to figure out the channel lock issue?

mkmer · 2025-02-17T13:06:15Z

So is mychannel associated with a conference that would cause this to go out to all linked repeaters?

It looks like the conference (as currently being setup) is NOT limited to the local tx conference.

app_rpt/apps/app_rpt/rpt_telemetry.c

Lines 1057 to 1060 in c857a1a

/* make a conference for the tx */

/* If the telemetry is only intended for a local audience, only connect the ID audio to the local tx conference so linked systems can't hear it */

/* first put the channel on the conference in announce mode */

if (rpt_conf_add(mychannel, myrpt, mytele->mode == ID1 || mytele->mode == PLAYBACK || mytele->mode == TEST_TONE || mytele->mode == STATS_GPS_LEGACY ? RPT_CONF : RPT_TELECONF, RPT_CONF_CONFANN)) {

This code is a bit interesting (as it's not doing what we think?)
I generated a test where rpt_conf_add(mychannel, myrpt, RPT_TELECONF, RPT_CONF_CONFANN) and one with rpt_conf_add(mychannel, myrpt, RPT_CONF , RPT_CONF_CONFANN) -> neither send telemetry out links.

Debug shows DAHDI joining different conferences but not hearing a difference on the audio route.

~~Looking through conference setup... I'm wondering if we should be using myrpt->txpchannel rather than myrpt->txchannel~~ txpchannel has the same blocking issue as txchannel :(

app_rpt/apps/app_rpt.c

Line 2830 in c857a1a

    
           if (rpt_conf_add(myrpt->txpchannel, myrpt, RPT_TXCONF, RPT_CONF_CONF | RPT_CONF_TALKER)) {

app_rpt/apps/app_rpt.c

Line 2777 in c857a1a

    
           if (rpt_conf_create(myrpt->telechannel, myrpt, RPT_TELECONF, RPT_CONF_CONF | RPT_CONF_TALKER | RPT_CONF_LISTENER)) {

mkmer · 2025-02-17T13:40:50Z

My interpretation/understanding at this point :

myrpt->txchannel -> the actual tx output channel. In my case it's simpleusb on one system and usrp on the other. Locked often by app_rpt()
myrpt->txpchannel -> the psuedo channel used for tx? Locked often by app_rpt().
mychannel is a psuedo channel created in rpt_telem_thread() and joined to a conference (Either RPT_CONF or RPT_TELECONF.

In the first two cases (using given pointers), cop 48 1,2,3,4,5,6... will generate "already blocked by thread" debug messages and unreliably transmit tones.

Using mychannel elminiates the "already block by thread" debug messages AND transmits tones reliably.

apps/app_rpt/rpt_functions.c

apps/app_rpt/rpt_telemetry.c

InterLinked1

I don't really have the expertise to comment here, but I think the most important thing though here is testing. It sounds like mkmer has already tested it, have multiple people validated the old way didn't work properly and the new way works on their systems?

Allan-N · 2025-02-20T20:23:34Z

Can you comment on myrpt->txchannel vs. mychannel? and RPT_CONF vs. RPT_TELECONF? and whether/how to control whether the transmitted tones are sent only locally vs. locally + downstream?

Allan-N · 2025-02-20T21:30:37Z

And here is what I believe to be the answer to one of my questions :

app_rpt/apps/app_rpt.c

Lines 2773 to 2777 in ce12077

    
           if (myrpt->p.duplex == 2 || myrpt->p.duplex == 4) { 
        
           	res = rpt_conf_create(myrpt->pchannel, myrpt, RPT_CONF, RPT_CONF_CONFANNMON); 
        
           } else { 
        
           	res = rpt_conf_create(myrpt->pchannel, myrpt, RPT_CONF, RPT_CONF_CONF | RPT_CONF_LISTENER | RPT_CONF_TALKER); 
        
           }

app_rpt/apps/app_rpt.c

Lines 2808 to 2809 in ce12077

    
           /* make a conference for the voice/tone telemetry */ 
        
           if (rpt_conf_create(myrpt->telechannel, myrpt, RPT_TELECONF, RPT_CONF_CONF | RPT_CONF_TALKER | RPT_CONF_LISTENER)) {

I do think we want to be using the RPT_TELECONF conference

Allan-N · 2025-02-20T22:02:20Z

apps/app_rpt/rpt_telemetry.c

@@ -1057,7 +1057,9 @@ void *rpt_tele_thread(void *this)
 	/* make a conference for the tx */
 	/* If the telemetry is only intended for a local audience, only connect the ID audio to the local tx conference so linked systems can't hear it */
 	/* first put the channel on the conference in announce mode */
-	if (rpt_conf_add(mychannel, myrpt, mytele->mode == ID1 || mytele->mode == PLAYBACK || mytele->mode == TEST_TONE || mytele->mode == STATS_GPS_LEGACY ? RPT_CONF : RPT_TELECONF, RPT_CONF_CONFANN)) {
+	if (rpt_conf_add(mychannel, myrpt, mytele->mode == MDC1200 || mytele->mode == PAGE ||


Not sure about adding MDC1200 and PAGE here. We want to be treated like LOCALPLAY, not PLAYBACK

Seems odd that we would send a TEST_TONE or ID1 out links... Is there a LOCALPLAY defined or is PLAYBACK actually rpt xxx localplay?

mkmer · 2025-02-21T14:40:04Z

Bit of off line conversation:
It's possible myrpt->txchannel was an incorrect "hack" to get MDC1200 and PAGE (cop 48) to go out the TX in any mode. Using RPT_TELECONF will (as it appears anyway) not send tones out if we are in duplex mode = 0...

apps/app_rpt/rpt_telemetry.c

Update MDC1200 command to use correct channel. Local conference for MDC1200 and PAGE to match previous audio path

KB4MDD · 2025-02-21T19:54:54Z

Does paging work with all duplex settings?

mkmer added the bug Something isn't working label Feb 16, 2025

mkmer self-assigned this Feb 16, 2025

mkmer linked an issue Feb 16, 2025 that may be closed by this pull request

cop,48 produces choppy and broken telemetry #481

Closed

mkmer force-pushed the issue_481 branch from 553c71b to 62a7411 Compare February 16, 2025 23:48

mkmer force-pushed the issue_481 branch from 62a7411 to 22bf196 Compare February 17, 2025 00:26

mkmer force-pushed the issue_481 branch 7 times, most recently from f7d5489 to b8bf237 Compare February 17, 2025 01:33

Allan-N reviewed Feb 17, 2025

View reviewed changes

apps/app_rpt/rpt_functions.c Outdated Show resolved Hide resolved

Allan-N reviewed Feb 17, 2025

View reviewed changes

apps/app_rpt/rpt_telemetry.c Outdated Show resolved Hide resolved

mkmer force-pushed the issue_481 branch from b8bf237 to 6e1743b Compare February 17, 2025 15:19

mkmer force-pushed the issue_481 branch 3 times, most recently from b4aa570 to 004d20a Compare February 17, 2025 15:54

mkmer requested a review from InterLinked1 February 20, 2025 14:18

InterLinked1 reviewed Feb 20, 2025

View reviewed changes

Allan-N requested review from KB4MDD, InterLinked1 and Allan-N February 20, 2025 21:32

Allan-N approved these changes Feb 20, 2025

View reviewed changes

Allan-N requested changes Feb 20, 2025

View reviewed changes

mkmer requested a review from Allan-N February 21, 2025 14:36

Allan-N approved these changes Feb 21, 2025

View reviewed changes

InterLinked1 requested changes Feb 21, 2025

View reviewed changes

apps/app_rpt/rpt_telemetry.c Outdated Show resolved Hide resolved

InterLinked1 approved these changes Feb 21, 2025

View reviewed changes

mkmer added 6 commits February 21, 2025 14:26

rpt_telemetry: Update PAGE command to use correct channel.

416101f

Update MDC1200 command to use correct channel. Local conference for MDC1200 and PAGE to match previous audio path

Send PAGE and MDC to RPT_TELECONF

567d33f

readability update

0b7d692

Change from mode to type

b401154

No mixed declarations

550cb37

Use ENUM rpt_conf_type

f4ce340

mkmer force-pushed the issue_481 branch from ee2d6cc to f4ce340 Compare February 21, 2025 19:26

Allan-N merged commit 01b8099 into AllStarLink:master Feb 21, 2025
2 checks passed

mkmer deleted the issue_481 branch February 21, 2025 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpt_telemetry: Update PAGE command to use correct channel. #493

rpt_telemetry: Update PAGE command to use correct channel. #493

mkmer commented Feb 16, 2025

mkmer commented Feb 16, 2025

mkmer commented Feb 16, 2025

KB4MDD commented Feb 16, 2025

mkmer commented Feb 16, 2025

mkmer commented Feb 17, 2025

Allan-N commented Feb 17, 2025

mkmer commented Feb 17, 2025

Allan-N commented Feb 17, 2025 •

edited

Loading

mkmer commented Feb 17, 2025 •

edited

Loading

Allan-N commented Feb 17, 2025 •

edited

Loading

KB4MDD commented Feb 17, 2025

Allan-N commented Feb 17, 2025

mkmer commented Feb 17, 2025

mkmer commented Feb 17, 2025 •

edited

Loading

mkmer commented Feb 17, 2025 •

edited

Loading

InterLinked1 left a comment

Allan-N commented Feb 20, 2025

Allan-N commented Feb 20, 2025

Allan-N Feb 20, 2025 •

edited

Loading

mkmer Feb 20, 2025

mkmer commented Feb 21, 2025

KB4MDD commented Feb 21, 2025

rpt_telemetry: Update PAGE command to use correct channel. #493

rpt_telemetry: Update PAGE command to use correct channel. #493

Conversation

mkmer commented Feb 16, 2025

mkmer commented Feb 16, 2025

mkmer commented Feb 16, 2025

KB4MDD commented Feb 16, 2025

mkmer commented Feb 16, 2025

mkmer commented Feb 17, 2025

Allan-N commented Feb 17, 2025

mkmer commented Feb 17, 2025

Allan-N commented Feb 17, 2025 • edited Loading

mkmer commented Feb 17, 2025 • edited Loading

Allan-N commented Feb 17, 2025 • edited Loading

KB4MDD commented Feb 17, 2025

Allan-N commented Feb 17, 2025

mkmer commented Feb 17, 2025

mkmer commented Feb 17, 2025 • edited Loading

mkmer commented Feb 17, 2025 • edited Loading

InterLinked1 left a comment

Choose a reason for hiding this comment

Allan-N commented Feb 20, 2025

Allan-N commented Feb 20, 2025

Allan-N Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

mkmer Feb 20, 2025

Choose a reason for hiding this comment

mkmer commented Feb 21, 2025

KB4MDD commented Feb 21, 2025

Allan-N commented Feb 17, 2025 •

edited

Loading

mkmer commented Feb 17, 2025 •

edited

Loading

Allan-N commented Feb 17, 2025 •

edited

Loading

mkmer commented Feb 17, 2025 •

edited

Loading

mkmer commented Feb 17, 2025 •

edited

Loading

Allan-N Feb 20, 2025 •

edited

Loading