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

Minor improvement of multi-node info #444

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

Conversation

zcsahok
Copy link
Member

@zcsahok zcsahok commented Oct 2, 2024

  • mark own node on frequency display (Alt-J)
    image

  • on :INFo panel add own node name if LAN is active. Added "Total" to the received stats to visually separate it from the per-node stats.
    image

I see no way to correlate the host names to node ids due to way it's set up.
It would be maybe better to have a configuration like this

NODE_A=biggun1
NODE_B=biggun2
NODE_C=rxonly
THISNODE=B

Sending frequency info only each 2 minutes is also somewhat not optimal IMO.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 9, 2024

Looks good to me.

I see no way to correlate the host names to node ids due to way it's set up. It would be maybe better to have a configuration like this

NODE_A=biggun1
NODE_B=biggun2
NODE_C=rxonly
THISNODE=B

Could be done. Needs change of keywords (ADDNODE dropped NODE_A, NODE_B..NODE_H added). How about

ADDNODE=biggun1,A

or nodeid first?

Other way would be to collect nodeid's from incoming messages (but maybe error prone)

Sending frequency info only each 2 minutes is also somewhat not optimal IMO.

Easy change. What would you suggest?

@zcsahok
Copy link
Member Author

zcsahok commented Oct 9, 2024

I have to still play around with ADDNODE as this might actually work: nodes are added in the order they appear in the file.

For distributing freq we could set a flag on freq change in gettxinfo() to trigger sending freq info. If no freq change happens then the current 2 min schedule would be followed.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 11, 2024

For distributing freq we could set a flag on freq change in gettxinfo() to trigger sending freq info. If no freq change happens then the current 2 min schedule would be followed.

Sounds reasonable. We should take care to coordinate between our two threads here (send_freq() runs on main thread, gettxinfo() on background one).

@zcsahok
Copy link
Member Author

zcsahok commented Oct 13, 2024

Specifying nodes with implicit naming (= first ADDNODE will be A, then comes B, and so on) does work, but I find a more explicit syntax (NODE_B or even ADDNODE_B) clearer and easier to follow.
I guess there are not too many users out there using a multi-node setup, so changing the syntax would be a big issue.

From the code point of view we have just disable talking to ourselves. The original concept avoided it by instructing the user not to add their own node, so this exclusion was not built-in.

@dl1jbe
Copy link
Member

dl1jbe commented Oct 15, 2024

Specifying nodes with implicit naming (= first ADDNODE will be A, then comes B, and so on) does work, but I find a more explicit syntax (NODE_B or even ADDNODE_B) clearer and easier to follow.

Besides you do not have to follow a special order (Node A, B, C and so on as with ADDNODE alone).
Downside: We have to add 8 keywords now and keep that in sync with the allowed number of nodes (any way to keep that in sync easily?)

I guess there are not too many users out there using a multi-node setup, so changing the syntax would be a big issue.

At least we should add a deprecation warning to the old keyword for now.

From the code point of view we have just disable talking to ourselves. The original concept avoided it by instructing the user not to add their own node, so this exclusion was not built-in.

Yes, allowing it would give a more complete conversation log ( ALT+I ). Seems useful.

@zcsahok
Copy link
Member Author

zcsahok commented Oct 15, 2024

Implementing NODE_x requires just adding one keyword that has a variable part captured by regex. (s. ALT_x)
Its function would then check x against the configured amount of nodes the same way as ADDNODE does. If fact NODE_x is just a slight modification of ADDNODE.

Keeping ADDNODE as deprecated can cause functional issues. To avoid it we shall then selectively disable sending to ourselves.

  1. only ADDNODEs used: show deprecation warning then send to all (current behavior)
  2. no ADDNODE used, only NODE_x: treat index into bc_hostaddress as the node id, disable sending to ourselves (new behavior)
  3. a mix is detected: exit with an error

@dl1jbe
Copy link
Member

dl1jbe commented Oct 18, 2024

Implementing NODE_x requires just adding one keyword that has a variable part captured by regex. (s. ALT_x) Its function would then check x against the configured amount of nodes the same way as ADDNODE does. If fact NODE_x is just a slight modification of ADDNODE.

Well, that would work like a charm.

Keeping ADDNODE as deprecated can cause functional issues. To avoid it we shall then selectively disable sending to ourselves.

Maybe we should not mark it as 'deprecated' but 'no longer supported'.

There may be the case now, that someone adds its own address with NODE_x. As most messages sent by us do not make sense for ourself (except for talk messages) we have to filter them out during send.

The echo of our own talk messages do not need to go via the net at all.

@zcsahok
Copy link
Member Author

zcsahok commented Oct 24, 2024

As the discussion has somewhat drifted to the use of NODE_x config I'd open an separate issue on this.
Then the changes in this PR could be merged.

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

Successfully merging this pull request may close these issues.

2 participants