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

Clarify newkey logic with enum #453

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

Clarify newkey logic with enum #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
99 changes: 55 additions & 44 deletions apps/app_rpt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1675,16 +1675,16 @@ static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *s
return;
}
if (!strcmp(tmp, NEWKEYSTR)) {
if ((!mylink->newkey) || mylink->newkeytimer) {
if ((!mylink->link_newkey) || mylink->newkeytimer) {
mylink->newkeytimer = 0;
mylink->newkey = 1;
send_old_newkey(mylink->chan);
mylink->link_newkey = RADIO_KEY_ALLOWED_REDUNDANT;
send_newkey_redundant(mylink->chan);
}
return;
}
if (!strcmp(tmp, NEWKEY1STR)) {
mylink->newkeytimer = 0;
mylink->newkey = 2;
mylink->link_newkey = RADIO_KEY_NOT_ALLOWED;
return;
}
if (!strncmp(tmp, IAXKEYSTR, strlen(IAXKEYSTR))) {
Expand Down Expand Up @@ -2142,14 +2142,14 @@ static int handle_remote_data(struct rpt *myrpt, char *str)
if (!strcmp(tmp, DISCSTR))
return 0;
if (!strcmp(tmp, NEWKEYSTR)) {
if (!myrpt->newkey) {
send_old_newkey(myrpt->rxchannel);
myrpt->newkey = 1;
if (!myrpt->rpt_newkey) {
send_newkey_redundant(myrpt->rxchannel);
myrpt->rpt_newkey = RADIO_KEY_ALLOWED_REDUNDANT;
}
return 0;
}
if (!strcmp(tmp, NEWKEY1STR)) {
myrpt->newkey = 2;
myrpt->rpt_newkey = RADIO_KEY_NOT_ALLOWED;
return 0;
}
if (!strncmp(tmp, IAXKEYSTR, strlen(IAXKEYSTR))) {
Expand Down Expand Up @@ -2272,7 +2272,7 @@ static int attempt_reconnect(struct rpt *myrpt, struct rpt_link *l)
l->connecttime = 0;
l->thisconnected = 0;
l->iaxkey = 0;
l->newkey = 0;
l->link_newkey = RADIO_KEY_ALLOWED;

cap = ast_format_cap_alloc(AST_FORMAT_CAP_FLAG_DEFAULT);
if (!cap) {
Expand All @@ -2289,7 +2289,7 @@ static int attempt_reconnect(struct rpt *myrpt, struct rpt_link *l)
l->lastrealrx = 0;
l->rxlingertimer = ((l->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);
l->newkeytimer = NEWKEYTIME;
l->newkey = 2;
l->link_newkey = RADIO_KEY_NOT_ALLOWED;
while ((f1 = AST_LIST_REMOVE_HEAD(&l->textq, frame_list)))
ast_frfree(f1);
if (l->chan) {
Expand Down Expand Up @@ -2878,6 +2878,7 @@ static inline void dump_rpt(struct rpt *myrpt, const int lasttx, const int laste
ast_debug(2, "myrpt->sleepreq = %d\n", (int) myrpt->sleepreq);
ast_debug(2, "myrpt->p.parrotmode = %d\n", (int) myrpt->p.parrotmode);
ast_debug(2, "myrpt->parrotonce = %d\n", (int) myrpt->parrotonce);
ast_debug(2, "myrpt->rpt_newkey =%d\n", myrpt->rpt_newkey);

zl = myrpt->links.next;
while (zl != &myrpt->links) {
Expand All @@ -2893,7 +2894,7 @@ static inline void dump_rpt(struct rpt *myrpt, const int lasttx, const int laste
ast_debug(2, " link->retrytimer %ld\n", zl->retrytimer);
ast_debug(2, " link->retries = %d\n", zl->retries);
ast_debug(2, " link->reconnects = %d\n", zl->reconnects);
ast_debug(2, " link->newkey = %d\n", zl->newkey);
ast_debug(2, " link->link_newkey = %d\n", zl->link_newkey);
zl = zl->next;
}

Expand Down Expand Up @@ -3026,7 +3027,7 @@ static inline void rxunkey_helper(struct rpt *myrpt, struct rpt_link *l)
static inline void periodic_process_links(struct rpt *myrpt, const int elap)
{
struct ast_frame *f;
int x;
int newkeytimer_last;
struct rpt_link *l = myrpt->links.next;
while (l != &myrpt->links) {
int myrx, mymaxct;
Expand All @@ -3043,17 +3044,18 @@ static inline void periodic_process_links(struct rpt *myrpt, const int elap)
l->rxlingertimer = 0;

/* Update the timer, checking if it expired just now. */
x = l->newkeytimer;
newkeytimer_last = l->newkeytimer;
if (l->newkeytimer)
l->newkeytimer -= elap;
if (l->newkeytimer < 0)
l->newkeytimer = 0;

/* Some reverse-engineering comments here from NA debugging issue #46 (inbound calls being keyed when they shouldn't be)
* This if statement executes if the timer just expired.
* This does NOT include cases like in handle_link_data where we set newkeytimer = 0 explicitly + set newkey to 1 or 2 (because then x == 0 here)
* This if statement executes if the newkeytimer just expired.
* This does NOT include cases like in handle_link_data where we set newkeytimer = 0 explicitly + set newkey
* to RADIO_KEY_ALLOWED_REDUNDANT or RADIO_KEY_NOT_ALLOWED (because then newkeytimer_last == 0 here)
*/
if (x > 0 && !l->newkeytimer) {
if (newkeytimer_last > 0 && !l->newkeytimer) { /* Translation: We were timing, and it just expired */
/* Issue #46 background:
*
* There is a kind of "handshake" that happens when setting up the IAX2 trunk between two nodes,
Expand All @@ -3078,7 +3080,9 @@ static inline void periodic_process_links(struct rpt *myrpt, const int elap)
* Note that the above depiction separates the TX and RX, but there are only 3 text frames involved.
* In issue #46, 3 text frames are sent, but only 2 are really "received".
* And it so happens that the text frame that B doesn't get from A is exactly the text frame
* that is responsible for setting newkeytimer=0 and newkey=2, i.e. if this doesn't happen, then we'll hit the WARNING case in the below if statement.
* that is responsible for setting newkeytimer=0 and newkey=RADIO_KEY_NOT_ALLOWED, i.e. if this doesn't happen,
* then we'll hit the WARNING case in the below if statement. Because this code sets l->link_newkey to RADIO_KEY_ALLOWED,
* There is an unintended radio keyup.
*
* Note that all of these comments are from spending hours debugging this issue and reverse-engineering, but at this point I'm pretty confident
* about these parts of the code, even though I'm not Jim Dixon and he didn't comment any of this code originally.
Expand All @@ -3091,19 +3095,23 @@ static inline void periodic_process_links(struct rpt *myrpt, const int elap)
if (l->thisconnected) {
/* We're connected, but haven't received a NEWKEY1STR text frame yet...
* The newkeytimer expired on a connected (~answered?) node, i.e. handle_link_data hasn't yet gotten called
* to set newkeytimer = 0 and newkey to non-zero, i.e. we haven't received a text frame with NEWKEY1STR over the IAX2 channel yet.
* to set newkeytimer = 0 and newkey to RADIO_KEY_NOT_ALLOWED, i.e. we haven't received a text frame with NEWKEY1STR over the IAX2 channel yet.
*/
if (l->newkey == 2) {
if (l->link_newkey == RADIO_KEY_NOT_ALLOWED) {
/* This can ripple to have consequences down the line, namely we might start writing voice frames
* across the IAX2 link because of this, basically causing us to be transmitting (keyed).
* If this happens, this indicates a problem upstream, and we should emit a warning here
* since undesired behavior will likely ensue.
* We probably SHOULD just hangup the line right here if we are connected and did not receive the ~!NEWKEY1!
* message as something is wrong with the text messaging part of the connection.
*/
ast_log(LOG_WARNING, "%p newkeytimer expired on connected node, setting newkey from 2 to 0.\n", l);
l->newkey = 0;
ast_log(LOG_WARNING, "%p newkeytimer expired on connected node, setting newkey from RADIO_KEY_NOT_ALLOWED to RADIO_KEY_ALLOWED.\n", l);
l->link_newkey = RADIO_KEY_ALLOWED;
}
} else {
/* If not connected yet (maybe a slow link connection?), wait another NEWKEYTIME ms */
/* If not connected yet (maybe a slow link connection?), wait another NEWKEYTIME ms (forever! - probably should limit
* the number of retries here)
*/
l->newkeytimer = NEWKEYTIME;
}
}
Expand All @@ -3112,7 +3120,7 @@ static inline void periodic_process_links(struct rpt *myrpt, const int elap)
if (l->linkmode < 1)
l->linkmode = 1;
}
if ((l->newkey == 2) && l->lastrealrx && (!l->rxlingertimer)) {
if ((l->link_newkey == RADIO_KEY_NOT_ALLOWED) && l->lastrealrx && (!l->rxlingertimer)) {
rxunkey_helper(myrpt, l);
}

Expand Down Expand Up @@ -3167,7 +3175,7 @@ static inline void periodic_process_links(struct rpt *myrpt, const int elap)
ast_debug(7, "@@@@ node %s sent node string %s to node %s\n", myrpt->name, lstr, l->name);
}
}
if (l->newkey == 1) {
if (l->link_newkey == RADIO_KEY_ALLOWED_REDUNDANT) {
if ((l->retxtimer += elap) >= REDUNDANT_TX_TIME) {
l->retxtimer = 0;
if (l->chan && l->phonemode == 0) {
Expand Down Expand Up @@ -4294,7 +4302,7 @@ static inline int process_link_channels(struct rpt *myrpt, struct ast_channel *w
*totx = 0;
if (l->phonemode == 0 && l->chan && (l->lasttx != *totx)) {
if (*totx && !l->voterlink) {
if (l->newkey < 2)
if (l->link_newkey != RADIO_KEY_NOT_ALLOWED)
ast_indicate(l->chan, AST_CONTROL_RADIO_KEY);
} else {
ast_indicate(l->chan, AST_CONTROL_RADIO_UNKEY);
Expand Down Expand Up @@ -4333,7 +4341,7 @@ static inline int process_link_channels(struct rpt *myrpt, struct ast_channel *w

l->rxlingertimer = ((l->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);

if ((l->newkey == 2) && (!l->lastrealrx)) {
if ((l->link_newkey == RADIO_KEY_NOT_ALLOWED) && (!l->lastrealrx)) {
rxkey_helper(myrpt, l);
}
if (((l->phonemode) && (l->phonevox)) || (!strcasecmp(ast_channel_tech(l->chan)->type, "echolink"))
Expand Down Expand Up @@ -4485,7 +4493,7 @@ static inline int process_link_channels(struct rpt *myrpt, struct ast_channel *w
l->reconnects++;
}
/* if RX key */
if ((f->subclass.integer == AST_CONTROL_RADIO_KEY) && (l->newkey < 2)) {
if ((f->subclass.integer == AST_CONTROL_RADIO_KEY) && (l->link_newkey != RADIO_KEY_NOT_ALLOWED)) {
rxkey_helper(myrpt, l);
}
/* if RX un-key */
Expand Down Expand Up @@ -4522,12 +4530,15 @@ static inline int process_link_channels(struct rpt *myrpt, struct ast_channel *w
fac_frame(f, fac);
}
/* foop */
if (l->chan && (l->lastrx || (!altlink(myrpt, l))) && ((l->newkey < 2) || l->lasttx || strcasecmp(ast_channel_tech(l->chan)->type, "IAX2"))) {
if (l->chan && (l->lastrx || (!altlink(myrpt, l))) && ((l->link_newkey != RADIO_KEY_NOT_ALLOWED) || l->lasttx || strcasecmp(ast_channel_tech(l->chan)->type, "IAX2"))) {
/* Reverse-engineering comments from NA debugging issue #46:
* We may be receiving frames from channel drivers but we discard them and don't pass them on if newkey hasn't been set to 2 yet.
* Of course if handle_link_data is never called to set newkey to 2 and stop newkeytimer, then at some point, we'll
* set newkey = 0 forcibly (see comments in that part of the code for more info), which will cause us to start passing on the voice frames here.
* If this happens, then we're passing voice frames so we're keyed up and transmitting, essentially.
* We may be receiving frames from channel drivers but we discard them and don't pass them on if newkey is set to != RADIO_KEY_NOT_ALLOWED yet.
* This happens when the reset code forces it to RADIO_ALLOWED.
* Of course if handle_link_data is never called to set newkey to RADIO_KEY_NOT_ALLOWED and stop newkeytimer, then at some point, we'll
* set newkey = RADIO_KEY_ALLOWED forcibly (see comments in that part of the code for more info),
* If this happens, we're passing voice frames and now sending AST_READIO_KEY messages
* so we're keyed up and transmitting, essentially, which we don't want to happen.
*
*/
ast_write(l->chan, f);
}
Expand Down Expand Up @@ -4583,7 +4594,7 @@ static inline int monchannel_read(struct rpt *myrpt)
/* go thru all the links */
while (l != &myrpt->links) {
/* foop */
if (l->chan && altlink(myrpt, l) && (!l->lastrx) && ((l->newkey < 2) || l->lasttx || strcasecmp(ast_channel_tech(l->chan)->type, "IAX2"))) {
if (l->chan && altlink(myrpt, l) && (!l->lastrx) && ((l->link_newkey != RADIO_KEY_NOT_ALLOWED) || l->lasttx || strcasecmp(ast_channel_tech(l->chan)->type, "IAX2"))) {
if (l->chan && (!strcasecmp(ast_channel_tech(l->chan)->type, "irlp"))) {
ast_write(l->chan, fs);
} else {
Expand Down Expand Up @@ -5857,7 +5868,7 @@ static inline int exec_chan_read(struct rpt *myrpt, struct ast_channel *chan, ch
if (f->frametype == AST_FRAME_VOICE) {
struct ast_frame *f1;
int ismuted;
if (myrpt->newkey == 2) {
if (myrpt->rpt_newkey == RADIO_KEY_NOT_ALLOWED){
myrpt->rxlingertimer = ((myrpt->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);
if (!*keyed) {
*keyed = 1;
Expand Down Expand Up @@ -5959,7 +5970,7 @@ static inline int exec_chan_read(struct rpt *myrpt, struct ast_channel *chan, ch
return -1;
}
/* if RX key */
if ((f->subclass.integer == AST_CONTROL_RADIO_KEY) && (myrpt->newkey < 2)) {
if ((f->subclass.integer == AST_CONTROL_RADIO_KEY) && (myrpt->rpt_newkey != RADIO_KEY_NOT_ALLOWED)) {
ast_debug(7, "@@@@ rx key\n");
*keyed = 1;
myrpt->rerxtimer = 0;
Expand Down Expand Up @@ -6029,7 +6040,7 @@ static inline int exec_pchannel_read(struct rpt *myrpt, struct ast_channel *chan
return -1;
}
if (f->frametype == AST_FRAME_VOICE) {
if ((myrpt->newkey < 2) || myrpt->remoterx || strcasecmp(ast_channel_tech(chan)->type, "IAX2")) {
if ((myrpt->rpt_newkey != RADIO_KEY_NOT_ALLOWED) || myrpt->remoterx || strcasecmp(ast_channel_tech(chan)->type, "IAX2")) {
ast_write(chan, f);
}
}
Expand Down Expand Up @@ -6686,12 +6697,12 @@ static int rpt_exec(struct ast_channel *chan, const char *data)
l->gott = 0;
l->rxlingertimer = ((l->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);
l->newkeytimer = NEWKEYTIME;
l->newkey = 0;
l->link_newkey = RADIO_KEY_ALLOWED;
l->iaxkey = 0;
if ((!phone_mode) && (l->name[0] != '0') && strcasecmp(ast_channel_tech(chan)->type, "echolink") && strcasecmp(ast_channel_tech(chan)->type, "tlb")) {
l->newkey = 2;
l->link_newkey = RADIO_KEY_NOT_ALLOWED;
}
ast_debug(7, "newkey: %d\n", l->newkey);
ast_debug(7, "newkey: %d\n", l->link_newkey);
if (l->name[0] > '9') {
l->newkeytimer = 0;
}
Expand Down Expand Up @@ -7005,7 +7016,7 @@ static int rpt_exec(struct ast_channel *chan, const char *data)
myrpt->reload = 0;
myrpt->tele.next = &myrpt->tele;
myrpt->tele.prev = &myrpt->tele;
myrpt->newkey = 0;
myrpt->rpt_newkey = RADIO_KEY_ALLOWED;
myrpt->iaxkey = 0;
myrpt->lastitx = !myrpt->lastitx;
myrpt->tunerequest = 0;
Expand Down Expand Up @@ -7069,7 +7080,7 @@ static int rpt_exec(struct ast_channel *chan, const char *data)
}
/* if is a webtransceiver */
if (myrpt->remote_webtransceiver)
myrpt->newkey = 2;
myrpt->rpt_newkey = RADIO_KEY_NOT_ALLOWED;
myrpt->loginuser[0] = 0;
myrpt->loginlevel[0] = 0;
myrpt->authtelltimer = 0;
Expand Down Expand Up @@ -7200,7 +7211,7 @@ static int rpt_exec(struct ast_channel *chan, const char *data)
}
keyed = myrx;
update_rxlingertimer(myrpt, elap);
if ((myrpt->newkey == 2) && keyed && (!myrpt->rxlingertimer)) {
if ((myrpt->rpt_newkey == RADIO_KEY_NOT_ALLOWED) && keyed && (!myrpt->rxlingertimer)) {
myrpt->rerxtimer = 0;
keyed = 0;
}
Expand Down Expand Up @@ -7239,7 +7250,7 @@ static int rpt_exec(struct ast_channel *chan, const char *data)

if (rem_rx && !myrpt->remoterx) {
myrpt->remoterx = 1;
if (myrpt->newkey < 2)
if (myrpt->rpt_newkey != RADIO_KEY_NOT_ALLOWED)
ast_indicate(chan, AST_CONTROL_RADIO_KEY);
}
if (!rem_rx && myrpt->remoterx) {
Expand All @@ -7256,7 +7267,7 @@ static int rpt_exec(struct ast_channel *chan, const char *data)
break; /* if not logged in, hang up after a time */
}
}
if (myrpt->newkey == 1) {
if (myrpt->rpt_newkey == RADIO_KEY_ALLOWED_REDUNDANT) {
if ((myrpt->retxtimer += elap) >= REDUNDANT_TX_TIME) {
myrpt->retxtimer = 0;
if ((myrpt->remoterx) && (!myrpt->remotetx))
Expand Down
12 changes: 10 additions & 2 deletions apps/app_rpt/app_rpt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,14 @@ struct rpt_chan_stat {
#define NEWKEY1STR "!NEWKEY1!"
#define IAXKEYSTR "!IAXKEY!"


/*! \brief Repeater link connection newkey handshake state */
mkmer marked this conversation as resolved.
Show resolved Hide resolved
enum newkey {
RADIO_KEY_ALLOWED, /*!< AST_CONTROL_RADIO_KEY is allowed on repeater channel */
RADIO_KEY_ALLOWED_REDUNDANT, /*!< "!NEWKEY!" - AST_CONTROL_RADIO_KEY allowed on the repeater channel */
RADIO_KEY_NOT_ALLOWED /*!< "!NEWKEY1!" message - AST_CONTROL_RADIO_KEY are not allowed on the repeater channel */
};

struct vox {
float speech_energy;
float noise_energy;
Expand Down Expand Up @@ -418,7 +426,7 @@ struct rpt_link {
char wasvox;
int voxtotimer;
char voxtostate;
char newkey;
enum newkey link_newkey;
char iaxkey;
int linkmode;
int newkeytimer;
Expand Down Expand Up @@ -840,7 +848,7 @@ struct rpt {
int linkposttimer;
int keyposttimer;
int lastkeytimer;
char newkey;
enum newkey rpt_newkey;
char iaxkey;
char inpadtest;
long rxlingertimer;
Expand Down
2 changes: 1 addition & 1 deletion apps/app_rpt/rpt_channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ void send_newkey(struct ast_channel *chan)
return;
}

void send_old_newkey(struct ast_channel *chan)
void send_newkey_redundant(struct ast_channel *chan)
{
ast_channel_lock(chan);
if (ast_sendtext(chan, NEWKEYSTR)) {
Expand Down
2 changes: 1 addition & 1 deletion apps/app_rpt/rpt_channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@ int send_link_pl(struct rpt *myrpt, char *txt);
/*! \brief send newkey request */
void send_newkey(struct ast_channel *chan);

void send_old_newkey(struct ast_channel *chan);
void send_newkey_redundant(struct ast_channel *chan);
4 changes: 2 additions & 2 deletions apps/app_rpt/rpt_link.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,10 +678,10 @@ int connect_link(struct rpt *myrpt, char *node, int mode, int perma)
l->hasconnected = l->perma = perma;
l->newkeytimer = NEWKEYTIME;
l->iaxkey = 0;
l->newkey = 2;
l->link_newkey = RADIO_KEY_NOT_ALLOWED;
l->voterlink = voterlink;
if (strncasecmp(s1, "echolink/", 9) == 0) {
l->newkey = 0;
l->link_newkey = RADIO_KEY_ALLOWED;
}
if (!strncasecmp(s1, "iax2/", 5) || !strncasecmp(s1, "echolink/", 9) || !strncasecmp(s1, "tlb/", 4)
#ifdef ALLOW_LOCAL_CHANNELS
Expand Down