Clarify newkey logic with enum

[mkmer]

After some struggles understanding newkey logic, I had made these changes for my own sanity.

• add enum for newkey values
• use "good" name for last_keytimer
• Fix comments for 2 to 0 forcing.

[Allan-N]

While I support adding an enum I think we can do better with the enum value naming. What do the KEYUP_ALLOWED, NOKEYUP_ALLOWED, and OLD_KEYUP_ALLOWED names mean? And, at the very least, it would be good to add some comments in app_rpt.h about each value and when it should be used.

[mkmer]

I don't disagree. These were names that made sense to me with limited understanding as I was trying to understand the logic.
I will be happy to change to whatever the team would like.

[Allan-N]

I just noticed your enum value comments :-)

... and now that I know more about the variable/values "I" might suggest a few renames.

• KEYUP_ALLOWED : OK
• OLD_KEYUP_ALLOWED --> KEYUP_ALLOWED_OLD
• NOKEYUP_ALLOWED --> KEYUP_NOT_ALLOWED

and, I will add that I'm not wild about the "_OLD"

Also, the app_rpt.c handling of newkey == 1 is odd ... and because the handling differs from newkey == 2 that makes me question _OLD even more.

But, before making changes I think we should hear what @InterLinked1 and @KB4MDD think about the naming?

[InterLinked1]

I very much support this change, I was going to do it myself but not having a radio background I wasn't super confident about what the values actually meant, so thanks for doing this.

Mostly stylistic recommendations here.

[InterLinked1]

Also, please squash all your commits together into a single commit, and please rename the commit to be prefixed with the module name, e.g. app_rpt: Use enum for newkey variable.

[KB4MDD]

I agree with @Allan-N, I think the enums should setup with KEYUP. I don't know the purpose of _OLD value - it would be nice to give it a good name if we understand how it works.

[mkmer]

I agree with @Allan-N, I think the enums should setup with KEYUP. I don't know the purpose of _OLD value - it would be nice to give it a good name if we understand how it works.

I called it old key because of this code:

app_rpt/apps/app_rpt/rpt_channel.c

Lines 463 to 483 in 604328d

	void send_newkey(struct ast_channel *chan)
	{
	/* app_sendtext locks the channel before calling ast_sendtext,
	* do this to prevent simultaneous channel servicing which can cause an assertion. */
	ast_channel_lock(chan);
	if (ast_sendtext(chan, NEWKEY1STR)) {
	ast_log(LOG_WARNING, "Failed to send text %s on %s\n", ast_channel_name(chan), NEWKEY1STR);
	}
	ast_channel_unlock(chan);
	return;
	}
	void send_old_newkey(struct ast_channel *chan)
	{
	ast_channel_lock(chan);
	if (ast_sendtext(chan, NEWKEYSTR)) {
	ast_log(LOG_WARNING, "Failed to send text %s on %s\n", ast_channel_name(chan), NEWKEYSTR);
	}
	ast_channel_unlock(chan);
	return;
	}

I suppose we should rename this function too?

[mkmer]

Also, the app_rpt.c handling of newkey == 1 is odd ... and because the handling differs from newkey == 2 that makes me question _OLD even more.

Best I can tell, there was an "old" mechanism for !NEWKEY!, I don't see anywhere in the current code base where it is sent other than a response to someone sending it, so this is probably hanging around for compatibility which may no longer be needed (who really knows if there are super old versions out there for sure...). I could be missing it.

To me, it looks like !NEWKEY1! mechanism keeps a remote from triggering TX until the channel receives the message, then we set l->newkey to 0 audio begins to flow. This was an incorrect statement based on comments. Actual code sets `newkey = 2 (or KEYUP_NOT_ALLOWED) when !NEWKEY1! is received.

First we set up the channel and flag l->newkey = KEYUP_NOT_ALLOWED (2)

app_rpt/apps/app_rpt.c

Lines 6686 to 6692 in 604328d

	l->gott = 0;
	l->rxlingertimer = ((l->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);
	l->newkeytimer = NEWKEYTIME;
	l->newkey = 0;
	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;

Then we send a couple of !NEWKEY1! messages:

app_rpt/apps/app_rpt.c

Lines 6740 to 6763 in 604328d

	if (ast_channel_state(chan) != AST_STATE_UP) {
	ast_answer(chan);
	if (l->name[0] > '9') {
	if (ast_safe_sleep(chan, 500) == -1) {
	return -1;
	}
	} else {
	if (!phone_mode) {
	send_newkey(chan);
	}
	}
	}
	rpt_mutex_unlock(&myrpt->blocklock);
	rpt_mutex_unlock(&myrpt->lock); /* Moved unlock to AFTER the if... answer block above, to prevent ast_waitfor_n assertion due to simultaneous channel access */
	rpt_update_links(myrpt);
	if (myrpt->p.archivedir) {
	donodelog_fmt(myrpt,"LINK%s,%s", l->phonemode ? "(P)" : "", l->name);
	}
	doconpgm(myrpt, l->name);
	if ((!phone_mode) && (l->name[0] <= '9')) {
	rpt_mutex_lock(&myrpt->blocklock);
	send_newkey(chan);
	rpt_mutex_unlock(&myrpt->blocklock);
	}

The other side of the link then responds with !NEWKEY1! for answering the call:

app_rpt/apps/app_rpt.c

Lines 4454 to 4468 in 604328d

	if (f->frametype == AST_FRAME_CONTROL) {
	if (f->subclass.integer == AST_CONTROL_ANSWER) {
	char lconnected = l->connected;
	__kickshort(myrpt);
	myrpt->rxlingertimer = ((myrpt->iaxkey) ? RX_LINGER_TIME_IAXKEY : RX_LINGER_TIME);
	l->connected = 1;
	l->hasconnected = 1;
	l->thisconnected = 1;
	l->elaptime = -1;
	if (!l->phonemode) {
	rpt_mutex_lock(&myrpt->blocklock);
	send_newkey(l->chan);
	rpt_mutex_unlock(&myrpt->blocklock);
	}

The key to control happening is here:

app_rpt/apps/app_rpt.c

Lines 4488 to 4499 in 604328d

	if ((f->subclass.integer == AST_CONTROL_RADIO_KEY) && (l->newkey < 2)) {
	rxkey_helper(myrpt, l);
	}
	/* if RX un-key */
	if (f->subclass.integer == AST_CONTROL_RADIO_UNKEY) {
	rxunkey_helper(myrpt, l);
	}
	if (f->subclass.integer == AST_CONTROL_HANGUP) {
	ast_frfree(f);
	remote_hangup_helper(myrpt, l);
	break;
	}

If it is (>=2) we ignore AST_CONTROL_RADIO_KEY - hence I called it KEYUP_NOT_ALLOWED

There are a few things in here that should also get #DEFINES to clear things up:

app_rpt/apps/app_rpt.c

Line 6759 in 604328d

if ((!phone_mode) && (l->name[0] <= '9')) {

l->name[0] <= '9' -> That's the first character of the node id (right?) What type are we really looking for?

Same thing here:

app_rpt/apps/app_rpt.c

Line 6742 in 604328d

if (l->name[0] > '9') {

l->name > '9'

I think that means it's not numeric - maybe a web receiver?

Then there is this != 0

app_rpt/apps/app_rpt.c

Line 6691 in 604328d

if ((!phone_mode) && (l->name[0] != '0') && strcasecmp(ast_channel_tech(chan)->type, "echolink") && strcasecmp(ast_channel_tech(chan)->type, "tlb")) {

What nodes have 0 for the first character?

This is a LOT! I feel like putting it in words may help me verify my thoughts and allow others to confirm I'm not out in the weeds.

[Allan-N]

Looking over app_rpt.c and all of the place where we check newkey == 1 you will comparisons to REDUNDANT_TX_TIME. All references to REDUNDANT_TX_TIME relate to newkey == 1. Given this, I would like to suggest :

• KEYUP_ALLOWED_OLD --> KEYUP_ALLOWED_REDUNDANT

and maybe :

• send_old_newkey() -> send_newkey_redundant()

[InterLinked1]

In the commit message, enum should be all lowercase. ENUM in Asterisk refers to something completely different :)

[InterLinked1]

More comments describing when a repeater is in each state might be nice :)

[mkmer]

I've added some comments to these - I think it's more clear.

[mkmer]

I think I've untangled this a bit. I went with 2 enums one for the repeater structure and one for the link structure. Hopefully that will help be a bit more clear which one we are using. If I've done it right, I think it will explain it self :)

[mkmer]

As far as I can tell all this newkey business is doing is deciding if we send ast_indicate(l->chan, AST_CONTROL_RADIO_KEY); down the channel. 0,1=YES 2=NO. When newkey==2, there is a different path to actually keying up the repeater. I changed the enum names to RADIO_KEY_xxx because it doesn't stop keyup of the repeater and I was feeling like KEYUP was confusing too.

As you dig around, let's see if it helps...

[Allan-N]

Since we (currently) only have one enum, maybe change enum rpt_newkey to enum newkey