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

Add support for a info message on the echolink connect page #432

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

Conversation

firealarmss
Copy link

Adds a feature to display a set message (from the config file) on the echolink app when connected.

@mkmer mkmer added the enhancement New feature or request label Feb 13, 2025
Copy link
Collaborator

@Allan-N Allan-N left a comment

Choose a reason for hiding this comment

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

I'm OK taking the current changes AS-IS and take care of the added changes/enhancements in a separate PR.

Optimize replace \n by relocation
Use S_OR to simplify string copy
Fix white space
@mkmer
Copy link
Collaborator

mkmer commented Feb 18, 2025

I was able to apply the suggested changes.

Restore message construction code
Reduce use of strlen() calls.
Add warning messages
Send truncated message (and keep warning log)
More optimization around +=
change i to offset for clarity.
eliminate string dup
@@ -217,6 +218,7 @@ do not use 127.0.0.1

#define DELIMCHR ','
#define QUOTECHR 34
#define EL_INIT_BUFFER = sizeof("oNDATA\rWelcome to Allstar Node \rEcholink Node %s\rNumber \r \rSystems Linked:\r") + 300 /* initial string buffer size for ast_str buffer */
Copy link
Member

Choose a reason for hiding this comment

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

ast_str will dynamically expand... we don't need to be super precise about how big it is, and this duplicates the phrasing from send_info. I think much of the tree will just start with 256 or something like that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want it to "make sense" why we started with this "size" or just magic number it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to pass an initial length. Why not pick a reasonable value.

Fix up comments
@mkmer
Copy link
Collaborator

mkmer commented Feb 18, 2025

I was searching this code base an i'll be... they used ast_str...

struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);

and
struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);

Thing is:

  • not checking for failure and should (right?),
  • not freeing the pointer on exit in either case?

If you agree this is "wrong", I'll create a PR to fix it.

@InterLinked1
Copy link
Member

I was searching this code base an i'll be... they used ast_str...

struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);

and

struct ast_str *cap_buf = ast_str_alloca(AST_FORMAT_CAP_NAMES_LEN);

Thing is:

  • not checking for failure and should (right?),
  • not freeing the pointer on exit in either case?

If you agree this is "wrong", I'll create a PR to fix it.

alloca allocates on the stack, not from the heap, so no need to free. It can be slightly more dangerous so usually you want to be more careful with the size. alloca should always succeed unless it crashes the program, you can't really handle it.

All references to "AllStar", not allStar, Allstar, etc
missing node numbers in string init
fix up more comments - strncat using incorrect length
EL_WELCOME_MESSAGE
no = in define :(
Multi line define :(
Copy link

@Murf Murf left a comment

Choose a reason for hiding this comment

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

Looking forward to using this in anger

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.

6 participants