Service RX/TX Request/Response sessions (#356)

- Added Svc RX & TX sessions (both for Request & Response). - Now that we have more sessions, `ContiguousPayload` building moved to transport impl: - less code duplication - less to verify by unit tests - sessions now more like facade for RX/TX - they do necessary params validation/conversion, but ultimately delegate to the transport - switch to `cetl::unbounded_variant` (instead of former `cetl::any`) Also: - minor changes for previous PR requests - more docs for stable api; excluding `*::detail::*` from generated docs Codo coverage: <img width="1723" alt="Screenshot 2024-05-02 at 09 53 11" src="https://github.com/OpenCyphal-Garage/libcyphal/assets/2915466/56c4ab26-177c-4cd8-806e-173cf38eb84c"> --------- Co-authored-by: Sergei Shirokov <[email protected]>
OpenCyphal-Garage · May 3, 2024 · e271e12 · e271e12
1 parent ab7627e
commit e271e12
Show file tree

Hide file tree

Showing 27 changed files with 1,558 additions and 228 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -0,0 +1,31 @@
+Checks: >-
+  boost-*,
+  bugprone-*,
+  cert-*,
+  clang-analyzer-*,
+  cppcoreguidelines-*,
+  google-*,
+  hicpp-*,
+  llvm-*,
+  misc-*,
+  modernize-*,
+  performance-*,
+  portability-*,
+  readability-*,
+  -cppcoreguidelines-avoid-const-or-ref-data-members,
+  -google-readability-todo,
+  -readability-avoid-const-params-in-decls,
+  -readability-identifier-length,
+  -llvm-header-guard,
+  -llvm-include-order,
+  -*-use-trailing-return-type,
+  -*-named-parameter,
+CheckOptions:
+  - key:   readability-function-cognitive-complexity.Threshold
+    value: '90'
+  - key:   readability-magic-numbers.IgnoredIntegerValues
+    value: '1;2;3;4;5;8;10;16;20;32;60;64;100;128;256;500;512;1000'
+WarningsAsErrors: '*'
+HeaderFilterRegex: '.*\.hpp'
+AnalyzeTemporaryDtors: false
+FormatStyle: file
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -62,7 +62,7 @@ if (${CMAKE_CXX_PLATFORM_ID} STREQUAL "Linux")
 endif()
 
 # Don't normalize deviance: if CMAKE_TOOLCHAIN_FILE is not set then provide
-# an initalized default to display in the status thus avoiding a warning about
+# an initialized default to display in the status thus avoiding a warning about
 # using an uninitialized variable.
 if (NOT CMAKE_TOOLCHAIN_FILE)
     set(LOCAL_CMAKE_TOOLCHAIN_FILE "(not set)")

diff --git a/cmake/modules/Findcetl.cmake b/cmake/modules/Findcetl.cmake
@@ -6,7 +6,7 @@
 
 include(FetchContent)
 set(cetl_GIT_REPOSITORY "https://github.com/OpenCyphal/cetl.git")
-set(cetl_GIT_TAG "886a0d227a043511eed6b252ea0f788590c50e75")
+set(cetl_GIT_TAG "10fbb2b7b89473d68e73db7235848b0692169e5a")
 
 FetchContent_Declare(
     cetl

diff --git a/docs/doxygen.ini b/docs/doxygen.ini
@@ -1071,7 +1071,7 @@ EXCLUDE_PATTERNS       =
 # wildcard * is used, a substring. Examples: ANamespace, AClass,
 # ANamespace::AClass, ANamespace::*Test
 
-EXCLUDE_SYMBOLS        =
+EXCLUDE_SYMBOLS        = *::detail::*
 
 # The EXAMPLE_PATH tag can be used to specify one or more files or directories
 # that contain example code fragments that are included (see the \include

diff --git a/include/libcyphal/transport/can/delegate.hpp b/include/libcyphal/transport/can/delegate.hpp
@@ -6,6 +6,7 @@
 #ifndef LIBCYPHAL_TRANSPORT_CAN_DELEGATE_HPP_INCLUDED
 #define LIBCYPHAL_TRANSPORT_CAN_DELEGATE_HPP_INCLUDED
 
+#include <libcyphal/transport/types.hpp>
 #include <libcyphal/transport/errors.hpp>
 #include <libcyphal/transport/scattered_buffer.hpp>
 
@@ -29,7 +30,7 @@ namespace detail
 /// This internal transport delegate class serves the following purposes:
 /// 1. It provides memory management functions for the Canard library.
 /// 2. It provides a way to convert Canard error codes to `AnyError` type.
-/// 3. It provides interface to access the transport from various session classes.
+/// 3. It provides an interface to access the transport from various session classes.
 ///
 class TransportDelegate
 {
@@ -40,7 +41,7 @@ class TransportDelegate
 public:
     /// @brief RAII class to manage memory allocated by Canard library.
     ///
-    class CanardMemory final : public cetl::rtti_helper<CanardMemoryTypeIdType, ScatteredBuffer::Interface>
+    class CanardMemory final : public cetl::rtti_helper<CanardMemoryTypeIdType, ScatteredBuffer::IStorage>
     {
     public:
         CanardMemory(TransportDelegate& delegate, void* const buffer, const std::size_t payload_size)
@@ -70,7 +71,7 @@ class TransportDelegate
         CanardMemory& operator=(const CanardMemory&)     = delete;
         CanardMemory& operator=(CanardMemory&&) noexcept = delete;
 
-        // MARK: ScatteredBuffer::Interface
+        // MARK: ScatteredBuffer::IStorage
 
         CETL_NODISCARD std::size_t size() const noexcept final
         {
@@ -161,10 +162,9 @@ class TransportDelegate
     ///
     /// Internal method which is in use by TX session implementations to delegate actual sending to transport.
     ///
-    CETL_NODISCARD virtual cetl::optional<AnyError> sendTransfer(const CanardMicrosecond       deadline,
+    CETL_NODISCARD virtual cetl::optional<AnyError> sendTransfer(const TimePoint               deadline,
                                                                  const CanardTransferMetadata& metadata,
-                                                                 const void* const             payload,
-                                                                 const std::size_t             payload_size) = 0;
+                                                                 const PayloadFragments        payload_fragments) = 0;
 
 protected:
     ~TransportDelegate() = default;
@@ -230,26 +230,28 @@ class TransportDelegate
 
 };  // TransportDelegate
 
-/// This internal session delegate class serves the following purpose: it provides interface
-/// to access a session from transport (by casting canard's `user_reference` member to this class).
+// MARK: -
+
+/// This internal session delegate class serves the following purpose: it provides an interface (aka gateway)
+/// to access RX session from transport (by casting canard's `user_reference` member to this class).
 ///
-class SessionDelegate
+class IRxSessionDelegate
 {
 public:
-    SessionDelegate(const SessionDelegate&)                = delete;
-    SessionDelegate(SessionDelegate&&) noexcept            = delete;
-    SessionDelegate& operator=(const SessionDelegate&)     = delete;
-    SessionDelegate& operator=(SessionDelegate&&) noexcept = delete;
+    IRxSessionDelegate(const IRxSessionDelegate&)                = delete;
+    IRxSessionDelegate(IRxSessionDelegate&&) noexcept            = delete;
+    IRxSessionDelegate& operator=(const IRxSessionDelegate&)     = delete;
+    IRxSessionDelegate& operator=(IRxSessionDelegate&&) noexcept = delete;
 
     /// @brief Accepts a received transfer from the transport dedicated to this RX session.
     ///
     virtual void acceptRxTransfer(const CanardRxTransfer& transfer) = 0;
 
 protected:
-    SessionDelegate()  = default;
-    ~SessionDelegate() = default;
+    IRxSessionDelegate()  = default;
+    ~IRxSessionDelegate() = default;
 
-};  // SessionDelegate
+};  // IRxSessionDelegate
 
 }  // namespace detail
 }  // namespace can

diff --git a/include/libcyphal/transport/can/media.hpp b/include/libcyphal/transport/can/media.hpp
@@ -50,7 +50,7 @@ class IMedia
     /// This value may change arbitrarily at runtime. The transport implementation will query it before every
     /// transmission on the port. This value has no effect on the reception pipeline as it can accept arbitrary MTU.
     ///
-    CETL_NODISCARD virtual std::size_t getMtu() const noexcept = 0;
+    virtual std::size_t getMtu() const noexcept = 0;
 
     /// @brief Set the filters for the CAN bus.
     ///

diff --git a/include/libcyphal/transport/can/msg_rx_session.hpp b/include/libcyphal/transport/can/msg_rx_session.hpp
@@ -25,22 +25,33 @@ namespace can
 ///
 namespace detail
 {
-class MessageRxSession final : public IMessageRxSession, private SessionDelegate
+
+/// @brief A class to represent a message subscriber RX session.
+///
+class MessageRxSession final : public IMessageRxSession, private IRxSessionDelegate
 {
-    // In use to disable public construction.
-    // See https://seanmiddleditch.github.io/enabling-make-unique-with-private-constructors/
-    struct Tag
+    /// @brief Defines specification for making interface unique ptr.
+    ///
+    struct Spec
     {
-        explicit Tag()  = default;
         using Interface = IMessageRxSession;
         using Concrete  = MessageRxSession;
+
+        // In use to disable public construction.
+        // See https://seanmiddleditch.github.io/enabling-make-unique-with-private-constructors/
+        explicit Spec() = default;
     };
 
 public:
     CETL_NODISCARD static Expected<UniquePtr<IMessageRxSession>, AnyError> make(TransportDelegate&     delegate,
                                                                                 const MessageRxParams& params)
     {
-        auto session = libcyphal::detail::makeUniquePtr<Tag>(delegate.memory(), Tag{}, delegate, params);
+        if (params.subject_id > CANARD_SUBJECT_ID_MAX)
+        {
+            return ArgumentError{};
+        }
+
+        auto session = libcyphal::detail::makeUniquePtr<Spec>(delegate.memory(), Spec{}, delegate, params);
         if (session == nullptr)
         {
             return MemoryError{};
@@ -49,7 +60,7 @@ class MessageRxSession final : public IMessageRxSession, private SessionDelegate
         return session;
     }
 
-    MessageRxSession(Tag, TransportDelegate& delegate, const MessageRxParams& params)
+    MessageRxSession(Spec, TransportDelegate& delegate, const MessageRxParams& params)
         : delegate_{delegate}
         , params_{params}
         , subscription_{}
@@ -65,14 +76,17 @@ class MessageRxSession final : public IMessageRxSession, private SessionDelegate
         CETL_DEBUG_ASSERT(result >= 0, "There is no way currently to get an error here.");
         CETL_DEBUG_ASSERT(result > 0, "New subscription supposed to be made.");
 
-        subscription_.user_reference = static_cast<SessionDelegate*>(this);
+        subscription_.user_reference = static_cast<IRxSessionDelegate*>(this);
     }
 
     ~MessageRxSession() final
     {
-        ::canardRxUnsubscribe(&delegate_.canard_instance(),
-                              CanardTransferKindMessage,
-                              static_cast<CanardPortID>(params_.subject_id));
+        const int8_t result = ::canardRxUnsubscribe(&delegate_.canard_instance(),
+                                                    CanardTransferKindMessage,
+                                                    static_cast<CanardPortID>(params_.subject_id));
+        (void) result;
+        CETL_DEBUG_ASSERT(result >= 0, "There is no way currently to get an error here.");
+        CETL_DEBUG_ASSERT(result > 0, "Subscription supposed to be made at constructor.");
     }
 
 private:
@@ -108,7 +122,7 @@ class MessageRxSession final : public IMessageRxSession, private SessionDelegate
         // Nothing to do here currently.
     }
 
-    // MARK: SessionDelegate
+    // MARK: IRxSessionDelegate
 
     void acceptRxTransfer(const CanardRxTransfer& transfer) final
     {
@@ -129,9 +143,8 @@ class MessageRxSession final : public IMessageRxSession, private SessionDelegate
 
     // MARK: Data members:
 
-    TransportDelegate&    delegate_;
-    const MessageRxParams params_;
-
+    TransportDelegate&                delegate_;
+    const MessageRxParams             params_;
     CanardRxSubscription              subscription_;
     cetl::optional<MessageRxTransfer> last_rx_transfer_;
 

diff --git a/include/libcyphal/transport/can/msg_tx_session.hpp b/include/libcyphal/transport/can/msg_tx_session.hpp
@@ -26,22 +26,31 @@ namespace can
 ///
 namespace detail
 {
+
 class MessageTxSession final : public IMessageTxSession
 {
-    // In use to disable public construction.
-    // See https://seanmiddleditch.github.io/enabling-make-unique-with-private-constructors/
-    struct Tag
+    /// @brief Defines specification for making interface unique ptr.
+    ///
+    struct Spec
     {
-        explicit Tag()  = default;
         using Interface = IMessageTxSession;
         using Concrete  = MessageTxSession;
+
+        // In use to disable public construction.
+        // See https://seanmiddleditch.github.io/enabling-make-unique-with-private-constructors/
+        explicit Spec() = default;
     };
 
 public:
     CETL_NODISCARD static Expected<UniquePtr<IMessageTxSession>, AnyError> make(TransportDelegate&     delegate,
                                                                                 const MessageTxParams& params)
     {
-        auto session = libcyphal::detail::makeUniquePtr<Tag>(delegate.memory(), Tag{}, delegate, params);
+        if (params.subject_id > CANARD_SUBJECT_ID_MAX)
+        {
+            return ArgumentError{};
+        }
+
+        auto session = libcyphal::detail::makeUniquePtr<Spec>(delegate.memory(), Spec{}, delegate, params);
         if (session == nullptr)
         {
             return MemoryError{};
@@ -50,7 +59,7 @@ class MessageTxSession final : public IMessageTxSession
         return session;
     }
 
-    MessageTxSession(Tag, TransportDelegate& delegate, const MessageTxParams& params)
+    MessageTxSession(Spec, TransportDelegate& delegate, const MessageTxParams& params)
         : delegate_{delegate}
         , params_{params}
         , send_timeout_{std::chrono::seconds{1}}
@@ -75,29 +84,13 @@ class MessageTxSession final : public IMessageTxSession
     CETL_NODISCARD cetl::optional<AnyError> send(const TransferMetadata& metadata,
                                                  const PayloadFragments  payload_fragments) final
     {
-        // libcanard currently does not support fragmented payloads (at `canardTxPush`).
-        // so we need to concatenate them when there are more than one non-empty fragment.
-        // See https://github.com/OpenCyphal/libcanard/issues/223
-        //
-        const transport::detail::ContiguousPayload contiguous_payload{delegate_.memory(), payload_fragments};
-        if ((contiguous_payload.data() == nullptr) && (contiguous_payload.size() > 0))
-        {
-            return MemoryError{};
-        }
-
-        const TimePoint deadline = metadata.timestamp + send_timeout_;
-        const auto deadline_us   = std::chrono::duration_cast<std::chrono::microseconds>(deadline.time_since_epoch());
-
         const auto canard_metadata = CanardTransferMetadata{static_cast<CanardPriority>(metadata.priority),
                                                             CanardTransferKindMessage,
                                                             static_cast<CanardPortID>(params_.subject_id),
                                                             CANARD_NODE_ID_UNSET,
                                                             static_cast<CanardTransferID>(metadata.transfer_id)};
 
-        return delegate_.sendTransfer(static_cast<CanardMicrosecond>(deadline_us.count()),
-                                      canard_metadata,
-                                      contiguous_payload.data(),
-                                      contiguous_payload.size());
+        return delegate_.sendTransfer(metadata.timestamp + send_timeout_, canard_metadata, payload_fragments);
     }
 
     // MARK: IRunnable