search

vapourismo / knx-go

KNX clients and protocol implementation in Go
MIT License
91 stars 59 forks source link

Added MulticastLoopback + fixed WaitTime #53

Closed mobilarte closed 2 years ago

mobilarte commented 2 years ago

The purpose of this PR is twofold:

  1. Added MulticastLoopback to enable a sender and a receiver to co-exist on the same machine. For test purposes I wanted to have a sender and a listener on the same machine. Multicast loopback is disabled when using net.ListenMulticastUDP("udp4", ifi, addr), therefore we use net.ListenUDP("udp4", addr) and join the multicast group. A new option which is false in the DefaultRouterConfighas been added. In addition, when address is empty, the default KNX address is used.
  2. Fixed somewhat the send Timer (according to spec, a sender must wait for at least 5 ms between telegram sends or to avoid sending too many telegrams, wait for 20 ms to ensure that max 50 datagrams are sent per second). I choose to wait for 20 ms (should this be configurable?). On receiving ROUTING_BUSY, the listener shall wait for TIME_WAIT + a random time [0, 1]*50ms when Control is 0x00, otherwise not specified, in that case set random time = 0.

Plus a few dots and capitalizations (error strings should not be capitalized (ST1005) that's what go-staticcheck tells me).

Thank you.

vapourismo commented 2 years ago

It is fine to use go-staticcheck. I welcome the fixes. It would be great if you could integrate that into CI so everybody can follow it's advice. However, while it isn't integrated, please don't intersperse these type of fixes with other stuff - it polutes the set of changes with unrelated stuff and that is a bit annoying to review.

mobilarte commented 2 years ago

I completely understand your point, will not change any cosmetic stuff unless it is in a separate PR over the entire code. How can we integrate it into CI?

vapourismo commented 2 years ago

How can we integrate it into CI?

You would have to add a section in check.yaml. Please do it in a separate PR if you want to add it.

mobilarte commented 2 years ago

Why do you actually need multicast loopback support? It seems not very well fit for what you're trying to do, unless I misunderstand, because everybody receives their own transmissions - there is no separation of router and router client.

I would really like to see a test that enshrines what you're trying to do to make sure we keep that property.

If you have two programs running on the same machine, one being a sender only and one being a monitor only (to update knx device states), then the monitor will not receive the messages being sent out by the sender, unless it enables MulticastLoopback, but that is currently not possible because of ListenMulticastUDP.

vapourismo commented 2 years ago

Hey, @mobilarte! Do you have time to address the remaining review items?

vapourismo commented 2 years ago

I merged this with the addition of a5260507cf35d299a5b426b062d8288881a6abe6 to make the post-send pause configurable.