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

Broadcast neighbor info #2535

Closed
wants to merge 19 commits into from
Closed

Broadcast neighbor info #2535

wants to merge 19 commits into from

Conversation

caveman99
Copy link
Sponsor Member

@caveman99 caveman99 commented May 31, 2023

also update trunk

also update trunk
@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2023

🤖 Pull request artifacts

file commit
pr2535-firmware-2.1.21.2e7c95a.zip 2e7c95a

thebentern added a commit to meshtastic/artifacts that referenced this pull request May 31, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request May 31, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 1, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 1, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 2, 2023
@GUVWAF GUVWAF self-requested a review June 6, 2023 19:16
@GUVWAF
Copy link
Member

GUVWAF commented Jun 6, 2023

This is not yet working correctly; now it regards everyone from whom it received a packet as neighbor. The last_sent_by_id needs to be updated in the FloodingRouter. Will look into this more later.

thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 6, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 7, 2023
@GUVWAF
Copy link
Member

GUVWAF commented Jun 8, 2023

Logic seems okay now. (Node 0 has NodeNum 16, Node 1 has 17, Node 2 has 18).

image

Still needs some more work, e.g. handling the AdminMessages for configuring the module and sending only the n most relevant edges.

@ajmcquilkin There is a MAX_NEIGHBOR_AGE of 10 minutes defined, but neighbors are not yet removed. You mentioned "Clients will need to individually set the timeout duration", but that would require another setting. I suggest we set this to 2x the broadcast interval (which is configurable), do you agree?

thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 8, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 10, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 13, 2023
@ajmcquilkin
Copy link
Member

@GUVWAF I think 2x the MAX_NEIGHBOR_AGE would be perfect, although as long as this is a client-side duration all we would need to do is document this expectation somewhere. I'm assuming that this would need to be 2x the broadcast interval of the node that the client is connected to.

This also makes me wonder whether 2x should be the default for the client, but where the user can override this default duration in the application settings. I'm envisioning an application override being necessary since the user may know that the broadcast interval of the connected node is much slower or faster than most nodes on the network.

A random idea would be for nodes to somehow advertise their timeout when they enter the network, but 1) that sounds out of scope of this PR and 2) there still wouldn't be a guarantee that all nodes would receive this advertisement.

@GUVWAF
Copy link
Member

GUVWAF commented Jun 21, 2023

@ajmcquilkin If you want it to be configurable in the client, we can also just not remove any neighbors in the firmware (like it is now, so easiest to implement) except when one does the NodeDB reset. Either way, I think then we still need to store the last Rx time and send this to the client. Then you can decide in the client whether a link should be removed. You could also more easily warn a user not to set it lower than the broadcast interval of any node, because then you might remove it too soon.

I know I voted against transmitting the Rx time over the mesh and I think that's also not necessary, but storing it will be needed if you want to remove any links after a certain time.

Unfortunately I can't attend the meeting on Thursday, but maybe you can discuss this there.

thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 25, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jun 27, 2023
@D4rk4
Copy link
Contributor

D4rk4 commented Jul 5, 2023

Any chance to get this feature in next release?

thebentern added a commit to meshtastic/artifacts that referenced this pull request Jul 22, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jul 24, 2023
thebentern added a commit to meshtastic/artifacts that referenced this pull request Jul 25, 2023
@caveman99
Copy link
Sponsor Member Author

This will go into 2.2.x - rebasing

@caveman99 caveman99 closed this Jul 26, 2023
@caveman99 caveman99 deleted the neighborinfo branch July 26, 2023 11:07
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.

5 participants