Add nodedb channel handling

[sbias]

Store channel of a node to nodedb and use it to send our packets to.

[github-actions]

🤖 Pull request artifacts

file	commit
pr2384-firmware-2.1.6.8bc42c3.zip	8bc42c3

[GUVWAF]

While the code looks good to me, the only thing that changes here is the initial NodeInfo (when you don’t know the node yet) and the function sendNetworkPing, which is sent on a double button press and regularly when the screen is currently displaying this node.
All the broadcast intervals you set only apply to the primary channel. If I’m right, the goal of this is to share NodeInfo and position only to specific nodes. With this, it works only for these cases I mentioned before. I’m not sure whether that solves the use case and if it’s not going to create more confusion, since you need to have different primary channels, but the one node needs the primary channel of the other as a secondary channel.
I'm wondering if it would be better to make the channel on which you sent position updates configurable, such that you can share a secondary channel only to the nodes that are allowed to receive them.

With this you can indeed also transmit DMs on a secondary channel, but you can still only receive DMs on your primary channel (because that is where you sent your NodeInfo on and will be saved as your channel by the other node). Meaning that still anyone that has the same primary channel can receive the DMs. If you want to make a real private conversation, you would have to do a 'handshake' telling which channel to use for a DM to a specific node.

[caveman99]

@GUVWAF the idea behind this PR was to lay the groundworks for sending DM not only on the primary channel. by remembering where you heard the node (and only responding to requests, not actively broadcasting info on a non-primary channel to you) - meaning you can use setups where you have several mesh channels on the same device and talk/dm even to nodes you don't receive on your primary channel (which may very well be THEIR primary channel. So broadcasting/announcing should only happen on the primary (for me) but i should respond to nodes not on the primary, with nodeinfo and positions requested. This way (in theory) we don't put unneccessary packets on the air, but building up a database to remeber how to reach a certain node by DM.

[garthvh]

@GUVWAF the idea behind this PR was to lay the groundworks for sending DM not only on the primary channel. by remembering where you heard the node (and only responding to requests, not actively broadcasting info on a non-primary channel to you) - meaning you can use setups where you have several mesh channels on the same device and talk/dm even to nodes you don't receive on your primary channel (which may very well be THEIR primary channel. So broadcasting/announcing should only happen on the primary (for me) but i should respond to nodes not on the primary, with nodeinfo and positions requested. This way (in theory) we don't put unneccessary packets on the air, but building up a database to remeber how to reach a certain node by DM.

Not actively broadcasting seems like the right choice here. This has zero client impact since we already get this data for every packet correct?

[sbias]

Well, that is right, both of it. The first goal was to implement direct communication between 2 devices where one has the primary channel as the secondary but don't share the primary.
As a next step it might be possible to make channels configurable if broadcasts should be sent on, but that means sending my nodeinfo 8 times for one broadcast if i have configured 8 channels for this. I'm not sure if this worth the airtime.
In this PR the nodeinfo is sent to any unknown nodes once heard, so, it might take some minutes but that info will propagate over time. And if a posInfo was sent by a tracker on his primary which was received on a secondary, that posInfo will be updated in nodeDb as well.

[GUVWAF]

I now see that for responses already the same channel as the request came in on was used, so position requests also work indeed:

void setReplyTo(meshtastic_MeshPacket *p, const meshtastic_MeshPacket &to)
{
    ...
    p->channel = to.channel; // Use the same channel that the request came in on

As a next step it might be possible to make channels configurable if broadcasts should be sent on, but that means sending my nodeinfo 8 times for one broadcast if i have configured 8 channels for this. I'm not sure if this worth the airtime.

I agree that you should not actively broadcast on every channel, I was more thinking of setting it to one specific channel (like it is the primary channel now), such that you only need one channel for that in a mesh. Now if you want to use separate channels for DMs, you need a different primary channel. If you then still want to receive the position/device telemetry broadcasts, you have to add all primary channels of the other nodes as secondary channels.

[GUVWAF]

Basically this means per node you can only have 1 private chat for DMs and only the node of that chat can also receive your position/device telemetry packets.

If you share your primary channel to multiple other nodes such that they can also receive your position/device telemetry, they all can receive DMs sent to you.

[caveman99]

there's no privacy concern i think. dm's can be read by the passing nodes already, the difference here is we send it out to another network we may have just as secondary. this is not to improve privacy, but to do more flexible setups, in line with QR-sharing only a subset or even one of your channels. imagine you having your private mesh but you can add your public city net as a secondary and reach people on it through DM.

[GUVWAF]

If you have a private mesh, you can indeed send a DM on a secondary channel to someone on a public net. But if the one on the public net wants to send a DM back, it needs to have the primary channel of the private mesh as a secondary channel (so it's not really private anymore for that node).

But if this is not to improve privacy, I'm fine with it :)

[caveman99]

If i want to answer the DM in your scenario, won't te sending node receive the DM through the same channel? Cause the answering node ideally also has on file on which channel it heard the target node, and sends it down that way. Or do we need to add DM receive (in contrast to sending) capability on secondary channels?

TL/DR: can a node receive DM on a channel index that is not zero/primary?

[GUVWAF]

Oh, I was wrong there, a node can receive a DM on a secondary channel, there's nothing we need to add for that, because it will just be able to decode it.

In this scenario, the one on the private net would receive the NodeInfo on the channel of the public net and send a response on that channel. Therefore, the one on the public net would just use its primary channel to send the DM. So, the one on the public net doesn't need to have the key of the private net as secondary channel.

It's just that the user of the private net doesn't know whether the channel of the private or public net will be used (if you don't know whether the other has the private net's channel as primary or not), so if you're using secondary channels, you can't assume that DMs can only be read by nodes on your primary channel. But, yeah, this PR is not about improving privacy.

TL/DR: This stuff is confusing, but it looks like it is going to work just fine with this PR :)

[thebentern]

Great work!
We'll have to have our alphanauts test this one thoroughly

[garthvh]

I am interested in thoughts from @andrekir he knows these details inside and out.

[andrekir]

not too familiar with the code, does this override the packet's channel? (ie. client sends packet to node_123 on channel 8 but this changes to NodeInfo.channel)

if so, it is not recommended and packet channel should always be honored. otherwise it can break things (like admin channels).

[sbias]

this is a good point. if a channel was requested it should be respected. but, any idea how to differ between not set and primary (0) ?

[caveman99]

the only packets this is intended for are DM packets, which are up to now on channel 0 - secondary channels will always be non-zero value. So if the channel (from the phone) is zero AND the destination is not a broadcast we can safely alter it. If not, leave it alone.

[sbias]

if we want to set this only for DM it might be an idea to not set it in this router but in the client itself? Or somewhere between?

[andrekir]

NodeInfo.channel set vs. unset is not the issue. overriding the channel will break the special admin/gpio channels and agree clients should pick NodeInfo.channel if that's the intended target. this is best left out.

[sbias]

my plan was to check p.channel if a channel was request to not override it :)

[andrekir]

consider saving NodeInfo.channel for NodeInfo(?) packets only, so that we only tag the remote nodes primary channel.

otherwise it will save different channel values every time packets are received in any shared secondary channels.

[sbias]

This is an improvement of cause, but it does not resolve channel-hopping entirely. what if 2 nodes are on 2 shared channels and a 3. node requests nodeinfo on a secondary. that will change the channel too since it hears a nodeinfo on a secondary channel. i was thinking about a channel change if a node is heard on a primary channel, or channel-index is higher in NodeDb than new packet. But there is my main problem again, how to differ between, heard on primary and unset.

[caveman99]

every node has only one primary channel. So if you trigger the update only on received nodeinfo packets you will always have that node's primary channel from your perspective. If this value is zero, you share a common primary channel. If the local value here is already non-zero, don't update it, cause you obviously hear the node across several paths. This assumes a fairly static setup.

[sbias]

sorry, but i think that might not work if we send our nodeInfo on a secondary channel to reach out new unknown nodes while a third node is listening on that channel as well. I added that here: 85b18a4

[andrekir]

one option when receiving NodeInfo on a secondary channel --> sendOurNodeInfo with channel = our secondary channel (or any value for the matter, non-0 to indicate this is not our primary channel).

• nodes receiving a NodeInfo packet with a tagged NodeInfo.channel should consider lower priority
• if we already seen that node on our primary don't replace (unless 2-3x node_info_broadcast_secs ago?)

thoughts?

PS: node_info_broadcast_secs is our setting, so not ideal.

[caveman99]

That's a good point - sholdn't we consider nodeinfo / nodedb stale at some point and clean it up? That's another issue/PR but it would take care of this situation. My suggestion to prioritize reception on a secondary channel over the primary was that it is easier to do in code. Here again 0 can mean 2 things...

[andrekir]

added a few remarks but overall really like the proposal. 👍

This has zero client impact since we already get this data for every packet correct?

the advantage here is clients get NodeInfo.channel upfront in wantConfig, so we don't need to wait for a packet from the node to know their primary channel and can just point DMs to use NodeInfo.channel.

[caveman99]

regarding the channel 0 vs. unset problem: The field is new, so nobody uses it yet. Also there were requests in the past to allow more than 8 channels. i propose using a magic value of 31 (for reasons) to signal 'channel zero/primary'. this is never sent over the air, but only stored on a node.

[sbias]

Ok, an idea: I fix that toxic behavior of my change of ignoring requested channels @andrekir mentioned here (#2384 (comment)) by if-ing channel > 0 .

I add a check for updating channel in NodeDB to do this only if it is not a well known channel.

If done, i don't see any problems caused by this pr, so we can test it.
What do you think?

edit:
we can set channel-id to 255 in node-db as default and store only a first-seen channel. that should prevent channel-hoping. there is no problem of using channel 31 for default, but, in my opinion this should be changed everywhere than. translating ids in code might cause some confusion later on.

[andrekir]

edit: we can set channel-id to 255 in node-db as default and store only a first-seen channel. that should prevent channel-hoping. there is no problem of using channel 31 for default, but, in my opinion this should be changed everywhere than. translating ids in code might cause some confusion later on.

if-ing channel > 0 is not viable, so I believe using any non-0 value is not necessary. yet. 😃

[GUVWAF]

Shouldn't this be if (!p->channel) then?

[sbias]

yes, of cause, you are right, i will fix that

[geeksville]

This pull request has been mentioned on Meshtastic. There might be relevant details there:

https://meshtastic.discourse.group/t/question-about-private-messages-with-multiple-channels/11092/4

[meshtastic-bot]

This pull request has been mentioned on Meshtastic. There might be relevant details there:

https://meshtastic.discourse.group/t/meshtastic-channel-support-confirmation-of-understanding/13281/2