Fix multicast address

[sjm42]

Fixes #7674

🤝 Attestations

• I have tested that my proposed changes behave as described.
• I have tested that my proposed changes do not cause any obvious regressions on the following devices:
  • Heltec (Lora32) V3
  • LilyGo T-Deck
  • LilyGo T-Beam
  • RAK WisBlock 4631
  • Seeed Studio T-1000E tracker card
  • Other (please specify below)

[CLAassistant]

All committers have signed the CLA.

[Jorropo]

It "works" but this is very break everything and see if anyone cares kind of approach.

I think a better solution is to instantiate two UdpMulticastHandler, one with the old IP and one with the new.
Add docs saying the old IP is deprecated and remove it in 2~3 versions.

Edit: should be <10 SLOC change (make the IP an argument to UdpMulticastHandler and create two of them rather than one).

[sjm42]

Would that solution mean just sending all the udp packets to two multicast addresses, the old and new one,
until the old one is finally deprecated? That should work, given that the packet rate is quite small even in the "worst" case with meshtastic. Any LAN should be easily able to handle that.

[Jorropo]

This is exactly what I have in mind 🙂

[sjm42]

Well after looking at the code including main.cc and because udpIpAddress is a private field of the class UdpMulticastHandler,
I would rather suggest not changing the caller side at all but just rename the current udpIpAddress
to something like udpIpAddressDeprecated and creating a new udpIpAddress with the more correct address
in the constructor. Also just add another udp.writeTo() line to handle both destinations.

How would that sound? I can make a PR suggestion of this approach.

Edit: I realized I did not check how the udp receiving side is arranged. With repeating the udp.listenMulticast() call we would get everything as doubled on the receiving side, sigh. What would be best approach for receiving? For now on, use the old udp address for receiving, and "halfway" into old address deprecation, move on and only listen for the new address? And then finally, remove the deprecated sender address also?

[Jorropo]

You will need a second listener too.

It seems weird to me to do it here but I doubt I care.
Arguably this is a good candidate to here or even lower (add a function that takes multiple IPs and send the packet to each IP) since Linux has more efficient APIs to send an identical packet to multiple destinations, altho, altho this run at kbps at most so doesn't matter.

[sjm42]

If the code starts to send multicast to addresses a+b and also the receiving side would be listening to a+b and both nodes are running the new firmware, every packet would be received as duplicated.

One stupid "workaround" might be to first listen for both addresses and after first packet is seen on either address,
mark the other address internally to be ignored in the future. I am not familiar enough with the internals to be able to just shut down the other unnecessary listener afterwards.

Another option would be making the multicast address configurable and optional. The current default would be the old address naturally, but could be changed. Probably that would be way too much hassle to introduce something new to be configured, idk.

[Jorropo]

and both nodes are running the new firmware, every packet would be received as duplicated

You can give duplicated packets to the router and it will handle them.
I remember this used to cause issues but I am pretty sure Jonathan fixed it.

One stupid "workaround" might be to first listen for both addresses and after first packet is seen on either address,
mark the other address internally to be ignored in the future.

We don't want that, it is not gonna help us transition since unless you restart all your nodes with the new code they are still all gonna use the old multicast address.
You can do some filtering on a per packet basis, keep a buffer of like 10 packets hashes and don't send packets to the router if we already seen them.

But turns out the router already implement this logic so ... I would try letting both listeners callback into the router with the same packets and letting it dedup.

[sjm42]

Okay, if it is not harmful to send duplicates (other than that statistics get distorted?),
this should be quite simple. Just listen to two multicast addresses and only use the existing onReceive() handler.

On the sending side, just send the packet once for each destination address.
Is it safe to call udp.listenMulticast() twice and both multicast addresses are then subscribed to?

If that is yes, then this should work?

develop...sjm42:meshtastic-firmware:multicast-fix-take2

edit: oh, and also I am assuming that the udp.writeTo() is not modifying the send buffer at all. Is this correct?
The code is just sending exactly the same contents from the same buffer into two different destination multicast addresses.

[Jorropo]

Is it safe to call udp.listenMulticast() twice and both multicast addresses are then subscribed to?
If that is yes, then this should work?

On linux I wrote the code so it should work.
For ESP32 idk try it, I hope so it's not like 2 is a big number.

edit: oh, and also I am assuming that the udp.writeTo() is not modifying the send buffer at all. Is this correct?
The code is just sending exactly the same contents from the same buffer into two different destination multicast addresses.

It would be beyond insane if writeTo mutate the passed-in buffer.
writeTo is inspired from POSIX semantics and they clearly state functions like write, send, writeTo cannot modify the input buffer.

[sjm42]

Well if my proposed changes make sense, I would make a new pull request or what? :)

[Jorropo]

@sjm42 usually you would force push this branch to avoid sending more notifications to people watching this repo but both work.