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

Bump minimum NodeInfo send to 5 minutes #3423

Merged
merged 4 commits into from
Mar 17, 2024
Merged

Bump minimum NodeInfo send to 5 minutes #3423

merged 4 commits into from
Mar 17, 2024

Conversation

thebentern
Copy link
Contributor

What do we think about this change to combat some of the excessive chattiness of NodeInfo on larger meshes?

@thebentern thebentern requested review from caveman99 and GUVWAF March 16, 2024 12:39
Copy link
Contributor

🤖 Pull request artifacts

empty string

file commit
firmware-2.3.1.b04beaa.zip b04beaa

thebentern added a commit to meshtastic/artifacts that referenced this pull request Mar 16, 2024
@andrekir
Copy link
Member

considering the flood expire time of 10 minutes, 3 min is very reasonable. maybe could even bump it up to 5.

@garthvh
Copy link
Member

garthvh commented Mar 16, 2024

Why not the whole 10?

@GUVWAF
Copy link
Member

GUVWAF commented Mar 16, 2024

I don't think we should set it too high, it's a bit annoying for first-time users if you have to wait a while before a node shows up in your node list if you start up one after the first sent out its initial NodeInfo.

@thebentern
Copy link
Contributor Author

Survey time? 😄
Thumbs up this comment for 3 minutes
Thumbs down this comment for 5 minutes

@andrekir
Copy link
Member

andrekir commented Mar 16, 2024

I'm fine with either 3 or 5 minutes. no real preference.

LOG_INFO("Heard a node on channel %d we don't know, sending NodeInfo and asking for a response.\n", mp->channel);
nodeInfoModule->sendOurNodeInfo(mp->from, true, mp->channel);

something else we should consider is always sending NodeInfo to NODENUM_BROADCAST, instead of DMs.

@garthvh
Copy link
Member

garthvh commented Mar 16, 2024

I'm fine with either 3 or 5 minutes. no real preference.

LOG_INFO("Heard a node on channel %d we don't know, sending NodeInfo and asking for a response.\n", mp->channel);
nodeInfoModule->sendOurNodeInfo(mp->from, true, mp->channel);

something else we should consider is always sending NodeInfo to NODENUM_BROADCAST, instead of DMs.

Yeah I fixed this the other day because it was making it seem like some nodes were on the wrong channel

@GUVWAF
Copy link
Member

GUVWAF commented Mar 16, 2024

I'm also fine with either 3 or 5 min.

In the specific case Andre linked I think it makes sense to send the NodeInfo as DM, because we request a response from the unknown node, we don't want everyone to respond.

@andrekir
Copy link
Member

In the specific case Andre linked I think it makes sense to send the NodeInfo as DM, because we request a response from the unknown node, we don't want everyone to respond.

for sure, we would send to NODENUM_BROADCAST and wantReplies false. unless we really need a response. otherwise as a DM we would only reply to one single node every 3-5 minutes.

@GUVWAF
Copy link
Member

GUVWAF commented Mar 16, 2024

Yeah, makes sense to only set lastSentToMesh when it's a broadcast.

@thebentern thebentern changed the title Bump minimum NodeInfo send to 3 minutes Bump minimum NodeInfo send to 5 minutes Mar 16, 2024
@thebentern
Copy link
Contributor Author

Could we make NodeInfo DMs (or maybe they already are and I missed it) read promiscuously as long as we can decode? That way, if I send a DM to interrogate NodeInfo only for a specific destination node that I don't know yet, any observers still would also get the benefit of observing that (but not rebroadcasting).

@garthvh
Copy link
Member

garthvh commented Mar 17, 2024

Could we make NodeInfo DMs (or maybe they already are and I missed it) read promiscuously as long as we can decode? That way, if I send a DM to interrogate NodeInfo only for a specific destination node that I don't know yet, any observers still would also get the benefit of observing that (but not rebroadcasting).

Yeah I was already able to decode them, I just was not expecting a dm

@thebentern thebentern merged commit e27f029 into master Mar 17, 2024
70 checks passed
@thebentern thebentern deleted the three-minute-min branch March 17, 2024 00:56
@andrekir
Copy link
Member

Could we make NodeInfo DMs (or maybe they already are and I missed it) read promiscuously as long as we can decode?

I like that. we could finally use sniffReceived().

/**
* Every (non duplicate) packet this node receives will be passed through this method. This allows subclasses to
* update routing tables etc... based on what we overhear (even for messages not destined to our node)
*/
void Router::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Routing *c)
{
// FIXME, update nodedb here for any packet that passes through us
}

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.

4 participants