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.

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

Multiple: Remove RPT_TELECONF and Use ast_audiohook_volume* for ducking #563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Mar 16, 2025

Use ast_audiohook_volume*
Remove telemchannel and btelemchannel.
Adjusts volume for ALL telemetry messages.

@mkmer mkmer added the enhancement New feature or request label Mar 16, 2025
@mkmer mkmer self-assigned this Mar 16, 2025
Copy link
Member

@InterLinked1 InterLinked1 left a comment

Choose a reason for hiding this comment

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

First of all, I'm really excited about changes like this! The more code we can remove, the better! (without changing functionality, of course)

I think the commit message needs much more detail. It should explain why the old way was unnecessary and we can use audiohooks instead. For example, I thought these channels had something to do with telemetry, not adjusting the volume. Was that a misnomer? Or something else?

Also, I think for a change of this magnitude (removing channels), we should insist on tests. I would like to see a relevant test that passes prior to this change, and continues to pass afterwards. Once #533 merges, the tests should all be run by the CI.

@mkmer
Copy link
Collaborator Author

mkmer commented Mar 16, 2025

All that happens between telechannel and btelechannel is the ducking/volume adjustment code... That's it.
Every time we spin up a telemetry thread, we create a new DAHDI pseudo channel and link it either to the RPT_TELECONF or the RPT_CONF. RPT_CONF is connected to all audio moving around the system.

Also in the RPT_TELECONF is the telechannel. We simply copy telechannel to bchannel and in the middle we apply one of the two volume adjustments. Finally, the bchannel is connected to the RPT_TXCONF (which is local audio out)

So - all this does is move local audio connection in the telemetry thread FROM RPT_TELECONF to RPT_TXCONF.

Additionally, telemetry audio that was NOT connect to RPT_TELECONF was not receiving the p.telemduckgain or p.telemnomgain) adjustments.

@InterLinked1
Copy link
Member

All that happens between telechannel and btelechannel is the ducking/volume adjustment code... That's it. Every time we spin up a telemetry thread, we create a new DAHDI pseudo channel and link it either to the RPT_TELECONF or the RPT_CONF. RPT_CONF is connected to all audio moving around the system.

Also in the RPT_TELECONF is the telechannel. We simply copy telechannel to bchannel and in the middle we apply one of the two volume adjustments. Finally, the bchannel is connected to the RPT_TXCONF (which is local audio out)

So - all this does is move local audio connection in the telemetry thread FROM RPT_TELECONF to RPT_TXCONF.

Additionally, telemetry audio that was NOT connect to RPT_TELECONF was not receiving the p.telemduckgain or p.telemnomgain) adjustments.

Thanks for the explanation! If you can capture the gist of that in the commit message for posterity, I think that would be good.

I've seen some strange things in this codebase, but yeah, that sounds awfully inefficient. My guess, without confirming, is that audiohooks didn't exist in Asterisk when Jim wrote the code initially, and he may have deemed that an easy way to do that. But now that there are a lot of tools in the Asterisk core at our disposal, it makes sense to leverage those as much as we can.

Remove telemchannel and btelemchannel.
Adjusts volume for ALL telemetry messages.

Turns out this is a VERY complicated method of changing the volume on
telemetry messages.
Original design: telechannel is created and
joined RPT_TELECONF.  As telemetry channels are created the are joined to
RPT_TELECONF for local, or RPT_CONF for everywhere.
Audio on teh RPT_TELECONF is pulled out via telechannel, scaled via
p.telemnomgain when not keyed and p.telemduckgain when keyed, then copied
to btelechannel.  Finally, btelechannel is in the RPT_TXCONF delivering
the audio to the repeater.

With this PR, we remove RPT_TELECONF, telechannel, btelechannel and associated
code.  We now link new telemetry pseudo channels to RPT_TXCONF in place of RPT_TELECONF
and adjust the volume using ast_audiohook_volume_adjust().
@@ -272,7 +272,7 @@ enum rpt_dns_method {
};

#define DEFAULT_NODE_LOOKUP_METHOD LOOKUP_BOTH
#define DEFAULT_TELEMDUCKDB "-9"
#define DEFAULT_TELEMDUCKDB -9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our rpt.conf file has :

telemduckdb = -15                   ; Telemetry Ducking in dB when local or link voice tx in progress

It's OK for the two values to differ but it would be good for rpt.conf to note the default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-15 is a LONG way down... but I don't see why the shouldn't match


val = ast_variable_retrieve(cfg, cat, "telemduckdb");
rpt_vars[n].p.telemduckgain = pow(10.0, atof(S_OR(val, DEFAULT_TELEMDUCKDB)) / 20.0);
RPT_CONFIG_VAR_INT_DEFAULT(telemnomgain, "telemnomdb", 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing a configuration file "float" value to an integer value?

... and our current rpt.conf file has :

telemnomdb = -3                     ; Telemetry Nominal Amplitude reference in dB

Should this default also be the same?
and, it's OK for the two values to differ but it would be good for rpt.conf to note the default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see why not

val = ast_variable_retrieve(cfg, cat, "telemduckdb");
rpt_vars[n].p.telemduckgain = pow(10.0, atof(S_OR(val, DEFAULT_TELEMDUCKDB)) / 20.0);
RPT_CONFIG_VAR_INT_DEFAULT(telemnomgain, "telemnomdb", 1);
RPT_CONFIG_VAR_INT_DEFAULT(telemduckgain, "telemduckgain", DEFAULT_TELEMDUCKDB);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the configuration file "telemduckdb" variable to "telemduckgain" :-(

Is there a way to retain the current variable and convert it to the other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Allegedly, the asterisk volume want's dB, so we don't need to convert to the decimal value, just use dB. Of course if it's NOT dB on the volume, then we need to do something different.

@@ -677,8 +677,8 @@ struct rpt {
const char *locallist[16];
int nlocallist;
char ctgroup[16];
float telemnomgain; /*!< \brief nominal gain adjust for telemetry */
float telemduckgain; /*!< \brief duck on busy gain adjust for telemetry */
int telemnomgain; /*!< \brief nominal gain adjust for telemetry */
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are changing the variable types from "float" to "int" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't really need a float here... all the examples are int's and what kind of resolution do we really need on the gain for telemetry? (I get radio gains having decimal values to get things "perfect", but telemetry?)

@@ -3659,7 +3613,7 @@ static inline int rxchannel_read(struct rpt *myrpt, const int lasttx)
myrpt->linkactivitytimer = 0;
myrpt->keyed = 1;
time(&myrpt->lastkeyedtime);
myrpt->keyposttimer = KEYPOSTSHORTTIME;
myrpt->keyposttimer = KEYPOSTSHORTTIME;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we gained a trailing space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants