Implementation of UDP TX #363
Conversation
…verification #docs #sonar
…. #verification #docs #sonar
… #verification #docs #sonar
…Cyphal-Garage/libcyphal into sshirokov/344_fallible_run2
serges147
requested review from
pavel-kirienko,
lydia-at-amazon and
thirtytwobits
|
| struct Publish | ||
| { | ||
| UdpardMicrosecond deadline_us; | ||
| UdpardPriority priority; | ||
| UdpardPortID subject_id; | ||
| UdpardTransferID transfer_id; | ||
| }; | ||
| struct Request | ||
| { | ||
| UdpardMicrosecond deadline_us; | ||
| UdpardPriority priority; | ||
| UdpardPortID service_id; | ||
| UdpardNodeID server_node_id; | ||
| UdpardTransferID transfer_id; | ||
| }; | ||
| struct Respond | ||
| { | ||
| UdpardMicrosecond deadline_us; | ||
| UdpardPriority priority; | ||
| UdpardPortID service_id; | ||
| UdpardNodeID client_node_id; | ||
| UdpardTransferID transfer_id; | ||
| }; |
There was a problem hiding this comment.
Should we not extract the common fields into a new type and use composition with that?
There was a problem hiding this comment.
Am I right that you are proposing to extract deadline_us, priority & transfer_id to some BaseMetadata?
In general I saw and was aware of this duplication, but decided that a bit of duplication in detail won't hurt but instead will give clear independence/separation of the cases (which are instantiated in 3 different places, and in the end consumed by 3 different operator()-s)...
There was a problem hiding this comment.
@pavel-kirienko ? (see above question)
| static cetl::optional<AnyError> optAnyErrorFromUdpard(const std::int32_t result) | ||
| { | ||
| // Udpard error results are negative, so we need to negate them to get the error code. | ||
| const std::int32_t canard_error = -result; |
| struct IpEndpoint final | ||
| { | ||
| std::uint32_t ip_address; | ||
| std::uint16_t udp_port; | ||
| }; |
There was a problem hiding this comment.
I think it would be wiser to alias UdpardUDPIPEndpoint so that you don't have to convert there and back in your code. Also I think it's more accurate to call it a UDP/IP endpoint, not just an IP endpoint.
There was a problem hiding this comment.
My thinking was that we are under udp:: namespace, so the type is kinda already has UDP, like udp::IpEndpoint...
|
@thirtytwobits Scott, how is PR overall? Aproveable? ;) |
TransportDelegate&TransportImp.udp::IMedia,udp::ITxSocket&upd::IRxSocketprotocols.libcyphal::detail::UniquePtrSpechelper).std::exchange.Here is code coverage. Note that low (red) coverage at RX is currently expected - will be addressed by near future next PRs.
