Add new constructors and members to bind host endpoint

[wep21]

Add bind function with argument to fix source port in sender application as below.

udp_driver_->sender()->bind({asio::ip::udp::v4(), udp_port});

[wep21]

@JWhitleyWork I'm sorry, but could you also review this?

[wep21]

@JWhitleyWork friendly ping

[JWhitleyWork]

@wep21 Sorry for the delay on this. I'm not sure I understand why this is necessary. The UdpSocket manages it's own endpoint internally - why is it necessary to use an externally-managed endpoint?

[wep21]

I think it is necessary in case the receiver application specifies the source port which is different from internal endpoint. (Currently the source port is random.)

ros2 run udp_driver udp_sender_node_exe --ros-args -p ip:=127.0.0.1 -p port:=30000

09:37:57.970937 IP localhost.39950 > localhost.30000: UDP, length 24

[JWhitleyWork]

@wep21 I have seen this before also. However, to address it, I would much rather have multiple constructors for UdpSocket and still allow that class to manage the endpoint. e.g.:

UdpSocket::UdpSocket(
  const IoContext & ctx,
  const std::string & host_ip,
  uint16_t host_port)
...

UdpSocket::UdpSocket(
  const ioContext & ctx,
  const std::string & remote_ip,
  uint16_t remote_port)
...

UdpSocket::UdpSocket(
  const ioContext & ctx,
  const std::string & host_ip,
  uint16_t host_port,
  const std::string & remote_ip,
  uint16_t remote_port)
...

And then have additional parameters for the remote values. Would you be OK with this?

[wep21]

@JWhitleyWork I think these constructor suits for the use case I suggested. How do you think about it?

UdpSocket::UdpSocket(
  const IoContext & ctx,
  const std::string & remote_ip,
  const uint16_t remote_port,
  const std::string & host_ip,
  const uint16_t host_port,
)
: m_ctx(ctx),
  m_udp_socket(ctx.ios()),
  m_remote_endpoint(address::from_string(remote_ip), remort_port),
  m_host_endpoint(address::from_string(host_ip), host_port)

UdpSocket::UdpSocket(
  const IoContext & ctx,
  const std::string & remote_ip,
  const uint16_t remote_port,
  const uint16_t host_port,
)
: m_ctx(ctx),
  m_udp_socket(ctx.ios()),
  m_remote_endpoint(address::from_string(remote_ip), remort_port),
  m_host_endpoint(udp::v4(), host_port)

UdpSocket::UdpSocket(
  const IoContext & ctx,
  const std::string & remote_ip,
  const uint16_t remote_port,
)
: m_ctx(ctx),
  m_udp_socket(ctx.ios()),
  m_remote_endpoint(address::from_string(remote_ip), remort_port),
  m_host_endpoint()

void UdpSocket::bind()
{
  m_udp_socket.bind(m_host_endpoint);
}

[JWhitleyWork]

Yes, your solution looks good to me.

[wep21]

@JWhitleyWork I addressed your review at https://github.com/ros-drivers/transport_drivers/pull/65/commits/c75e749c6e3a7a9d747d4b2ea7a70ccba63d9b01.

[wep21]

@JWhitleyWork friendly ping.

[JWhitleyWork]

@wep21 Sorry for the delay. I think your solution is mostly fine. However, the remaining issue is that this changes the function of the "default" constructor (std::string, uint16_t). The previous behavior was that these represented the local ip and port and the new behavior is that these represent the remote ip and port. While I think that the remote ip and port are more likely to be what people want, changing the default behavior is problematic for Autoware.Auto and other projects which use this library. I think we need to make this constructor continue to be for the local ip and port and we can have the new constructors stay the same.

[wep21]

@JWhitleyWork I'm sorry, I misunderstood, but I think the issue is that argument of send_to is the remote endpoint, while the argument of async_receive_from is the local endpoint as below. host ip: 192.168.2.100, remote ip: 192.168.2.10

ros2 run udp_driver udp_sender_node_exe --ros-args -p ip:=192.168.2.10 -p port:=30000
15:49:39.905817 IP l92.168.2.100.44285 > sng-5f-sgw001.mx-fw1.l.sng.tier4.jp.30000: UDP, length 24

ros2 run udp_driver udp_receiver_node_exe --ros-args -p ip:=192.168.2.100 -p port:=30000
16:02:20.276325 IP sng-5f-sgw001.mx-fw1.l.sng.tier4.jp.59635 > dpc1806001.30000: UDP, length 14

Do you have any idea to handle this?

[JWhitleyWork]

@wep21 I have merged #73 which is going to require a new major release anyway. I will approve this as-is and make it part of the 2.0.0 release.

[JWhitleyWork]

Do you have any idea to handle this?

Does this mean that your PR needs changes as well or does this only apply to the current main?

[wep21]

@JWhitleyWork friendly ping