Msg & Svc UDP RX pipelines are implemented. #368

serges147 · 2024-07-05T17:13:04Z

Also:

"Error" → "Failure" for variant errors.
~100% Coverage:

…verification #docs #sonar

…. #verification #docs #sonar

… #verification #docs #sonar

…Cyphal-Garage/libcyphal into sshirokov/344_fallible_run2

… #docs #sonar

…#verification #docs #sonar

pavel-kirienko

The UDP transport appears to be missing a detailed documentation of the socket construciton policy and the associted error handling approach. It has to be made very explicit that sockets are created ad-hoc.
The new error result types look overly convoluted, I recommend keeping the less verbose form -- see my comment in one of the affected places. I understand the desire to have a clearly named pair of types for the success and failure case but it doesn't seem to justify the existence of the extra entities.

include/libcyphal/transport/can/media.hpp

pavel-kirienko · 2024-07-09T09:52:32Z

include/libcyphal/types.hpp

+
+private:
+    // No Sonar `cpp:S5356` b/c we check here low level PMR alignment expectation.
+    static bool isAligned(cetl::byte* const raw_bytes_ptr, const std::size_t size)


Why aren't we getting an unused method error here in non-debug builds?

honestly IDK... what do you propose?

I am just surprised that it is accepted. Would it build if you removed the assertion check where this method is referenced?

I tried - it compiles fine.

serges147 · 2024-07-09T11:02:39Z

The UDP transport appears to be missing a detailed documentation of the socket construciton policy and the associted error handling approach. It has to be made very explicit that sockets are created ad-hoc.

Agree, I will add such documentation.

lydia-at-amazon · 2024-07-09T17:28:52Z

include/libcyphal/transport/udp/delegate.hpp

+        udpard_node_id_ = node_id;
+
+        UdpardUDPIPEndpoint endpoint{};
+        const std::int8_t   result = ::udpardRxRPCDispatcherStart(&rpc_dispatcher_, node_id, &endpoint);


Looks like udpardRxRPCDispatcherStart is going to get called even if the application only cares about broadcast messages. Is there a lot of overhead to udpardRxRPCDispatcherStart? Should we optimize the code to only call this function if the application registers interest in a service?

It's called only when node id is set.

From udpard.h header file documentation:

/// The time complexity is constant. This function does not invoke the dynamic memory manager.

I had discussed this with Pavel at initial stage of the PR - we (correct me please @pavel-kirienko if I'm wrong) agreed that calling this is fine even if there will be no later "interest in a service" as you said. Up to 3 TX sockets (per each redundant media) will be made only once for the whole transport instance, and these TX sockets are in use for both messages and services.

Sounds good, thanks!

include/libcyphal/transport/udp/udp_transport_impl.hpp

lydia-at-amazon · 2024-07-09T18:51:05Z

include/libcyphal/transport/udp/udp_transport_impl.hpp

+        auto media_failure = withMediaRxSocketsFor(  //
+            new_msg_node,
+            [](const Media&, const IRxSocket&, const UdpardRxSubscription&, const IRxSessionDelegate&)
+                -> cetl::optional<AnyFailure> { return cetl::nullopt; });


Noticing that we're using a lot of lambda functions in this code base. Not saying this is necessarily bad, but I think lambdas will be hard to certify at DAL-C and above. Probably fine to leave for now.

AFAIK, compilers should be pretty good to optimize them to a simple plain function (or even completely eliminate/inline it). In this case there is no even capturing of anything (see [] empty list), so...
Also, in autosar c++ 14, again afaik, there nothing really against usage of lambda-s in general, yes there are rules, but not really against its usage in general.

but I think lambdas will be hard to certify at DAL-C and above

Are they prohibited by high-integrity coding standards?

lydia-at-amazon · 2024-07-09T19:04:28Z

include/libcyphal/transport/udp/udp_transport_impl.hpp

@@ -717,13 +768,268 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport  //
        return cetl::nullopt;
    }

+    /// @brief Tries to call an action with a media and its RX socket.
+    ///
+    /// The RX socket is made on demand if necessary (but `endpoint` parameter should have a value).


Shouldn't we only need to make the RX socket once during initialization? Is this essentially creating a new RX socket every time we call receive?

These are made only when there is no one previously created. Such empty condition (namely UniquePtr is equal tonullptr) may happen in two scenarios:

the very first call when RX subscription/service is made (see makeMsgRxSession & makeSvcRxSession)

on transport run call - when for any reason the first (above) scenario had failed (but the transient error handler said "it's fine, continue!"), user have got a valid RX session, and now this session is part of transport's run life - hence transport tries to "recover" from this empty socket condition by attempting again. Implementation of IMedia may do different strategies in such case.

For example:
a. do nothing, let it be, and very quickly return error again - has relatively small impact
b. really try create a socket again and again on each run cycle until it succeeded eventually.
c. have some kind of "max attempts counter" and do the "b" several times, and then switch to "a"
d. or, make it time based - like occasionally, once in a while, do retry recreate such faulty media socket, but not on every attempt.

Note, that currently we don't have means to "mark" a media as permanently "broken", so yes, at transport level we do retry recreate socket on each run, but once it succeeded we of course stop doing it (b/c we finally got one).

Perfect, thank you for the description!

pavel-kirienko · 2024-07-10T10:03:13Z

looks good so far

serges147 · 2024-07-10T12:33:02Z

include/libcyphal/transport/udp/udp_transport_impl.hpp

-
-                // No Sonar `cpp:S909` b/c it make sense to use `continue` statement here - the corner case of
-                // "early" (by deadline) transfer drop. Using `if` would make the code less readable and more nested.
-                continue;  // NOSONAR cpp:S909


BTW, with this helper peekFirstValidUdpardTxItem method there is no need of NOSONAR anymore (for the continue).

serges147 · 2024-07-10T12:36:37Z

include/libcyphal/types.hpp

@@ -189,7 +189,7 @@ struct PmrRawBytesDeleter final

        if ((nullptr != memory_resource_) && (nullptr != raw_bytes_ptr))
        {
-            CETL_DEBUG_ASSERT(isAligned(raw_bytes_ptr, size_bytes_), "Unexpected alignment of the memory buffer.");
+            // CETL_DEBUG_ASSERT(isAligned(raw_bytes_ptr, size_bytes_), "Unexpected alignment of the memory buffer.");


Commenting usage of isAligned doesn't make it fail on CI... so, it seems that clang-tidy misses it for some reason

sonarqubecloud · 2024-07-10T12:47:02Z

Quality Gate failed

Failed conditions
5.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

serges147 added 30 commits May 15, 2024 17:33

merge

c709371

rule of 5 #verification #docs #sonar

c3a4ede

#verification #docs #sonar

0b62171

#verification #docs #sonar

d1b11bd

Make IRunnable::run as fallible. #verification #docs #sonar

7d39c7f

build fixes #verification #docs #sonar

a86f2a2

latest CETL #verification #docs #sonar

5ce13e7

#verification #docs #sonar

aea6adc

fix sonar issue #verification #docs #sonar

e718503

fix clang-tidy issues #verification #docs #sonar

d7a72c5

build fixes #verification #docs #sonar

ccc0e1e

pr fixes

3875779

100% coverage for runMediaFilters (including transient error handler).

b38c6de

Extended AnyErrorReport #verification #docs #sonar

856554c

more docs for setTransientErrorHandler

7d240e0

Drop problematic frame of runMediaTransmit.

792a292

Cover transport::can::TransportImpl::sendTransfer error handling. #…

e2ff3c3

…verification #docs #sonar

Cover transport::can::TransportImpl::runMediaTransmit error handling.

00c78dc

Cover transport::can::TransportImpl::runMediaReceive error handling…

ee82f35

…. #verification #docs #sonar

#verification #docs #sonar

b69232c

Address 2 "cpp:S134" Sonar issues ("should not be nested too deeply")

ddae37b

Address 2 "cpp:S134" Sonar issues ("should not be nested too deeply")…

5cd8c83

… #verification #docs #sonar

Merge branch 'sshirokov/344_fallible_run2' of https://github.com/Open…

04110ac

…Cyphal-Garage/libcyphal into sshirokov/344_fallible_run2

Use std::exchange at CanardMemory move constructor. #verification…

a3341c3

… #docs #sonar

First draft of UDP (dummy) entities. #verification #docs #sonar

13cd2ed

minor fixes #verification #docs #sonar

1691c2c

Merge branch 'main' into sshirokov/344_fallible_run2

96c43ab

Merge branch 'sshirokov/344_fallible_run2' into sshirokov/udp

19dc98d

minor Sonar fixes #verification #docs #sonar

ddbd62d

minor fixes #verification #docs #sonar

b9161f4

serges147 added 11 commits July 3, 2024 21:21

Activated TestUdpSvcRxSessions::run_and_receive_requests unit test …

5837153

…#verification #docs #sonar

minor sonar fixes #verification #docs #sonar

d5629b4

#verification #docs #sonar

2ec9d23

#verification #docs #sonar

e8bfddc

Merge branch 'main' into sshirokov/udp_rx2 #verification #docs #sonar

a25c5c3

#verification #docs #sonar

cd0b67c

#verification #docs #sonar

c20c369

#verification #docs #sonar

7001da8

#verification #docs #sonar

02e6ed1

#verification #docs #sonar

95051fd

#verification #docs #sonar

ae6ee1e

serges147 self-assigned this Jul 5, 2024

minor fix

437d9a7

serges147 requested review from pavel-kirienko and lydia-at-amazon July 5, 2024 17:30

serges147 marked this pull request as ready for review July 5, 2024 17:30

pavel-kirienko approved these changes Jul 9, 2024

View reviewed changes

lydia-at-amazon reviewed Jul 9, 2024

View reviewed changes

pavel-kirienko mentioned this pull request Jul 10, 2024

Make a better Expected #369

Open

serges147 added 2 commits July 10, 2024 14:03

PR fixes/typos #verification #docs #sonar

395119b

Fixed bug with unnecessary TX socket creation #verification #docs #sonar

5abaa29

serges147 commented Jul 10, 2024

View reviewed changes

minor addon

b4d31fc

lydia-at-amazon approved these changes Jul 10, 2024

View reviewed changes

serges147 merged commit 3a48e90 into main Jul 10, 2024
20 of 21 checks passed

serges147 deleted the sshirokov/udp_rx2 branch July 10, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Msg & Svc UDP RX pipelines are implemented. #368

Msg & Svc UDP RX pipelines are implemented. #368

serges147 commented Jul 5, 2024 •

edited

Loading

pavel-kirienko left a comment

pavel-kirienko Jul 9, 2024

serges147 Jul 9, 2024

pavel-kirienko Jul 9, 2024

serges147 Jul 9, 2024

serges147 Jul 10, 2024

serges147 commented Jul 9, 2024

lydia-at-amazon Jul 9, 2024

serges147 Jul 10, 2024 •

edited

Loading

lydia-at-amazon Jul 10, 2024

lydia-at-amazon Jul 9, 2024

serges147 Jul 10, 2024

pavel-kirienko Jul 10, 2024

lydia-at-amazon Jul 9, 2024

serges147 Jul 10, 2024 •

edited

Loading

lydia-at-amazon Jul 10, 2024

pavel-kirienko commented Jul 10, 2024

serges147 Jul 10, 2024

serges147 Jul 10, 2024

sonarqubecloud bot commented Jul 10, 2024

Msg & Svc UDP RX pipelines are implemented. #368

Msg & Svc UDP RX pipelines are implemented. #368

Conversation

serges147 commented Jul 5, 2024 • edited Loading

pavel-kirienko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 commented Jul 9, 2024

Choose a reason for hiding this comment

serges147 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-kirienko commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 10, 2024

Quality Gate failed

serges147 commented Jul 5, 2024 •

edited

Loading

serges147 Jul 10, 2024 •

edited

Loading

serges147 Jul 10, 2024 •

edited

Loading