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: Enumerate autopatch callmode #562

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Mar 16, 2025

Enumerate autopatch callmode
Fix miscellaneous formating.

@mkmer mkmer added the code quality Improvments around code quality without functional changes label Mar 16, 2025
@mkmer mkmer self-assigned this Mar 16, 2025
@mkmer mkmer force-pushed the enumerate_autopatch_modes branch from f3a0b28 to 9124a23 Compare March 16, 2025 12:45
@@ -202,17 +202,17 @@ static int rpt_do_stats(int fd, int argc, const char *const *argv)
ider_state = "CLEAN";

switch (myrpt->callmode) {
case 1:
case EXTEN_WAIT:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What information / criteria did you use when naming the patch_call_mode enums? Asking because you've got one set of enum names and the corresponding "patch_state" name strings here appear to be out-of-sync. Should the enum/names have been :

enum patch_call_mode { DOWN, DIALING, CONNECTING, UP, FAILED };

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 could go with those, I more or less went with what we are doing in the states:
In your example:
DIALING isn't really dialing -> It's waiting for DTMF digits to populate the "number' to call. (Hence EXTEN_WAIT - waiting on the extension to populate or timeout)
CONNECTING -> could work, but we are actually "dialing" / "waiting" for an answer in that state.
UP -> ACTIVE sure, that could work
FAILED -> CONGESTION What's the right "Asterisk" name for busy signal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Waiting for DTMF digits seems like we are "dialing" numbers until we have enough :) As for a failed call, I would think there are many reasons (busy, network congested, the [internet] path is down, etc).

Note: my comment was just asking the question about what we call it in the code vs. what text we are using to describe things. They don't need to agree but the differences can be a source of confusion.

Copy link
Collaborator Author

@mkmer mkmer Mar 16, 2025

Choose a reason for hiding this comment

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

The whole point is to make it "obvious" -> so if there are questions, it's probably NOT obvious :)
As usual, I picked something that made sense to me at the moment with the expectation of comments and improvements.
Let's see what the others think about name choices.

@mkmer mkmer requested review from KB4MDD and InterLinked1 March 16, 2025 15:10
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.

IBusy is BUSY in Asterisk, CONGESTION is something different.
However, what's the context? Not clear which one is appropriate but "busy" typically refers to an end user being busy while CONGESTION is more of a trunking thing.

I don't have any particular insights on names, but all of the enum values should have a common prefix to avoid conflicts.

@@ -271,6 +271,8 @@ enum rpt_dns_method {
LOOKUP_FILE
};

enum patch_call_mode { DOWN, DIALING, CONNECTING, UP, FAILED };
Copy link
Member

Choose a reason for hiding this comment

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

It should be formatted similar to the other enums, e.g.

enum patch_call_mode {
<TAB>DOWN,
<TAB>DIALING,
}

As for naming, how about using CALLMODE_ as the prefix, so we have CALLMODE_DOWN, CALLMODE_DIALING, etc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improvments around code quality without functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants