No CETL_NODISCARD for most of the public api (#359)

No `CETL_NODISCARD` for the most of public api (issue #350), but private (aka `detail`) stuff stays under strict `[[nodiscard]]` policy for all non-`void` returning functions. Also: - Less `NOSONAR`-s by switching some internal `void*`-s → `cetl::byte*`-s. - Eliminated `IMultiplexer` at CAN - no use of it; but will be at UDP. - More docs for public api. - Fixed Build Status and Sonar Cloud badges at `README.md`. --------- Co-authored-by: Sergei Shirokov <sshirokov@malwarebytes.com> Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
OpenCyphal-Garage · May 14, 2024 · abcfc14 · abcfc14
1 parent 29bd3db
commit abcfc14
Show file tree

Hide file tree

Showing 24 changed files with 204 additions and 219 deletions.
diff --git a/README.md b/README.md
@@ -1,15 +1,14 @@
 ![Cyphal](docs/images/html/opencyphal_logo.svg) Cyphal stack in C++
 ===================
 
-[![Build Status](https://badge.buildkite.com/af844974c06af6406e3b2192d98298b02b30f6ebebb5f8b16c.svg)](https://buildkite.com/uavcan/libcyphal-v1)
+[![Build Status](https://github.com/OpenCyphal-Garage/libcyphal/actions/workflows/tests.yml/badge.svg)](https://github.com/OpenCyphal-Garage/libcyphal)
 [![Forum](https://img.shields.io/discourse/https/forum.opencyphal.org/users.svg)](https://forum.opencyphal.org)
-[![Sonarqube Badge](https://sonarcloud.io/api/project_badges/measure?project=UAVCAN_libuavcan&metric=alert_status)](https://sonarcloud.io/dashboard?id=UAVCAN_libuavcan)
-[![Sonarqube Coverage](https://sonarcloud.io/api/project_badges/measure?project=UAVCAN_libuavcan&metric=coverage)](https://sonarcloud.io/dashboard?id=UAVCAN_libuavcan)
+[![Sonarqube Badge](https://sonarcloud.io/api/project_badges/measure?project=OpenCyphal-Garage_libcyphal&metric=alert_status)](https://sonarcloud.io/project/overview?id=OpenCyphal-Garage_libcyphal)
+[![Sonarqube Coverage](https://sonarcloud.io/api/project_badges/measure?project=OpenCyphal-Garage_libcyphal&metric=coverage)](https://sonarcloud.io/project/overview?id=OpenCyphal-Garage_libcyphal)
 [![Documentation](https://img.shields.io/badge/docs-passing-green.svg)](https://opencyphal.org/libcyphal/)
 
 > **WARNING** libcyphal v1 is not yet complete. This is a work-in-progress.
 
 Portable reference implementation of the [Cyphal protocol stack](https://opencyphal.org) in C++ for embedded systems, Linux, and POSIX-compliant RTOSs.
 
 Cyphal is a lightweight protocol designed for reliable communication in aerospace and robotic applications over robust vehicular networks.
-
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 "10fbb2b7b89473d68e73db7235848b0692169e5a")
+set(cetl_GIT_TAG "63a118441a80078ebfeabf509c342bc0480a50b2")
 
 FetchContent_Declare(
     cetl

diff --git a/include/libcyphal/transport/can/delegate.hpp b/include/libcyphal/transport/can/delegate.hpp
@@ -13,7 +13,6 @@
 
 #include <canard.h>
 #include <cetl/cetl.hpp>
-#include <cetl/pf17/attribute.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 #include <cetl/rtti.hpp>
 
@@ -50,16 +49,13 @@ class TransportDelegate
 public:
     /// @brief RAII class to manage memory allocated by Canard library.
     ///
-    /// NOSONAR cpp:S5008 for `void*` are unavoidable: integration with low-level C code of Canard memory management.
     /// NOSONAR cpp:S4963 for below `class CanardMemory` - we do directly handle resources here.
     ///
     class CanardMemory final  // NOSONAR cpp:S4963
         : public cetl::rtti_helper<CanardMemoryTypeIdType, ScatteredBuffer::IStorage>
     {
     public:
-        CanardMemory(TransportDelegate& delegate,
-                     void* const        buffer,  // NOSONAR cpp:S5008
-                     const std::size_t  payload_size)
+        CanardMemory(TransportDelegate& delegate, cetl::byte* const buffer, const std::size_t payload_size)
             : delegate_{delegate}
             , buffer_{buffer}
             , payload_size_{payload_size}
@@ -93,10 +89,8 @@ class TransportDelegate
             return payload_size_;
         }
 
-        // NOSONAR cpp:S5008 is unavoidable: integration with low-level Lizard memory access.
-        //
         CETL_NODISCARD std::size_t copy(const std::size_t offset_bytes,
-                                        void* const       destination,  // NOSONAR cpp:S5008
+                                        cetl::byte* const destination,
                                         const std::size_t length_bytes) const override
         {
             CETL_DEBUG_ASSERT((destination != nullptr) || (length_bytes == 0),
@@ -110,15 +104,15 @@ class TransportDelegate
             const std::size_t bytes_to_copy = std::min(length_bytes, payload_size_ - offset_bytes);
             // Next nolint is unavoidable: we need offset from the beginning of the buffer.
             // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
-            (void) std::memmove(destination, static_cast<cetl::byte*>(buffer_) + offset_bytes, bytes_to_copy);
+            (void) std::memmove(destination, buffer_ + offset_bytes, bytes_to_copy);
             return bytes_to_copy;
         }
 
     private:
         // MARK: Data members:
 
         TransportDelegate& delegate_;
-        void*              buffer_;  // NOSONAR cpp:S5008
+        cetl::byte*        buffer_;
         std::size_t        payload_size_;
 
     };  // CanardMemory

diff --git a/include/libcyphal/transport/can/media.hpp b/include/libcyphal/transport/can/media.hpp
@@ -10,7 +10,6 @@
 #include "libcyphal/types.hpp"
 
 #include <cetl/cetl.hpp>
-#include <cetl/pf17/attribute.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 #include <cetl/pf20/cetlpf.hpp>
 
@@ -68,20 +67,22 @@ class IMedia
     ///
     /// @return Returns `nullopt` on success; otherwise some `MediaError` in case of a low-level error.
     ///
-    CETL_NODISCARD virtual cetl::optional<MediaError> setFilters(const Filters filters) noexcept = 0;
+    virtual cetl::optional<MediaError> setFilters(const Filters filters) noexcept = 0;
 
     /// @brief Schedules the frame for transmission asynchronously and return immediately.
     ///
     /// @return Returns `true` if accepted or already timed out; `false` to try again later.
     ///
-    CETL_NODISCARD virtual Expected<bool, MediaError> push(const TimePoint                    deadline,
-                                                           const CanId                        can_id,
-                                                           const cetl::span<const cetl::byte> payload) noexcept = 0;
+    virtual Expected<bool, MediaError> push(const TimePoint                    deadline,
+                                            const CanId                        can_id,
+                                            const cetl::span<const cetl::byte> payload) noexcept = 0;
 
     /// @brief Takes the next payload fragment (aka CAN frame) from the reception queue unless it's empty.
     ///
     /// @param payload_buffer The payload of the frame will be written into the mutable `payload_buffer` (aka span).
     /// @return Description of a received fragment if available; otherwise an empty optional is returned immediately.
+    ///         `nodiscard` is used to prevent ignoring the return value, which contains not only possible media error,
+    ///         but also important metadata (like `payload_size` field for further parsing of the result payload).
     ///
     CETL_NODISCARD virtual Expected<cetl::optional<RxMetadata>, MediaError> pop(
         const cetl::span<cetl::byte> payload_buffer) noexcept = 0;

diff --git a/include/libcyphal/transport/can/msg_rx_session.hpp b/include/libcyphal/transport/can/msg_rx_session.hpp
@@ -15,7 +15,6 @@
 
 #include <canard.h>
 #include <cetl/cetl.hpp>
-#include <cetl/pf17/attribute.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 
 #include <chrono>
@@ -156,7 +155,9 @@ class MessageRxSession final : public IMessageRxSession, private IRxSessionDeleg
                 : cetl::make_optional<NodeId>(transfer.metadata.remote_node_id);
 
         const MessageTransferMetadata   meta{transfer_id, timestamp, priority, publisher_node_id};
-        TransportDelegate::CanardMemory canard_memory{delegate_, transfer.payload, transfer.payload_size};
+        TransportDelegate::CanardMemory canard_memory{delegate_,
+                                                      static_cast<cetl::byte*>(transfer.payload),
+                                                      transfer.payload_size};
 
         (void) last_rx_transfer_.emplace(MessageRxTransfer{meta, ScatteredBuffer{std::move(canard_memory)}});
     }

diff --git a/include/libcyphal/transport/can/msg_tx_session.hpp b/include/libcyphal/transport/can/msg_tx_session.hpp
@@ -14,7 +14,7 @@
 #include "libcyphal/types.hpp"
 
 #include <canard.h>
-#include <cetl/pf17/attribute.hpp>
+#include <cetl/cetl.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 
 namespace libcyphal

diff --git a/include/libcyphal/transport/can/svc_rx_sessions.hpp b/include/libcyphal/transport/can/svc_rx_sessions.hpp
@@ -15,7 +15,6 @@
 
 #include <canard.h>
 #include <cetl/cetl.hpp>
-#include <cetl/pf17/attribute.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 
 #include <chrono>
@@ -159,7 +158,9 @@ class SvcRxSession final : public Interface_, private IRxSessionDelegate  // NOS
         const auto timestamp      = TimePoint{std::chrono::microseconds{transfer.timestamp_usec}};
 
         const ServiceTransferMetadata   meta{transfer_id, timestamp, priority, remote_node_id};
-        TransportDelegate::CanardMemory canard_memory{delegate_, transfer.payload, transfer.payload_size};
+        TransportDelegate::CanardMemory canard_memory{delegate_,
+                                                      static_cast<cetl::byte*>(transfer.payload),
+                                                      transfer.payload_size};
 
         (void) last_rx_transfer_.emplace(ServiceRxTransfer{meta, ScatteredBuffer{std::move(canard_memory)}});
     }

diff --git a/include/libcyphal/transport/can/svc_tx_sessions.hpp b/include/libcyphal/transport/can/svc_tx_sessions.hpp
@@ -14,7 +14,7 @@
 #include "libcyphal/types.hpp"
 
 #include <canard.h>
-#include <cetl/pf17/attribute.hpp>
+#include <cetl/cetl.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 
 namespace libcyphal

diff --git a/include/libcyphal/transport/can/transport.hpp b/include/libcyphal/transport/can/transport.hpp
@@ -16,14 +16,13 @@
 #include "libcyphal/transport/contiguous_payload.hpp"
 #include "libcyphal/transport/errors.hpp"
 #include "libcyphal/transport/msg_sessions.hpp"
-#include "libcyphal/transport/multiplexer.hpp"
 #include "libcyphal/transport/svc_sessions.hpp"
 #include "libcyphal/transport/transport.hpp"
 #include "libcyphal/transport/types.hpp"
 #include "libcyphal/types.hpp"
 
 #include <canard.h>
-#include <cetl/pf17/attribute.hpp>
+#include <cetl/cetl.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 #include <cetl/pf20/cetlpf.hpp>
 
@@ -110,12 +109,9 @@ class TransportImpl final : public ICanTransport, private TransportDelegate  //
     using MediaArray = libcyphal::detail::VarArray<Media>;
 
 public:
-    CETL_NODISCARD static Expected<UniquePtr<ICanTransport>, FactoryError> make(
-        cetl::pmr::memory_resource&  memory,
-        IMultiplexer&                multiplexer,
-        const cetl::span<IMedia*>    media,
-        const std::size_t            tx_capacity,
-        const cetl::optional<NodeId> local_node_id)
+    CETL_NODISCARD static Expected<UniquePtr<ICanTransport>, FactoryError> make(cetl::pmr::memory_resource& memory,
+                                                                                const cetl::span<IMedia*>   media,
+                                                                                const std::size_t           tx_capacity)
     {
         // Verify input arguments:
         // - At least one media interface must be provided, but no more than the maximum allowed (255).
@@ -129,21 +125,14 @@ class TransportImpl final : public ICanTransport, private TransportDelegate  //
         {
             return ArgumentError{};
         }
-        if (local_node_id.has_value() && (local_node_id.value() > CANARD_NODE_ID_MAX))
-        {
-            return ArgumentError{};
-        }
 
         const MediaArray media_array{make_media_array(memory, media_count, media, tx_capacity)};
         if (media_array.size() != media_count)
         {
             return MemoryError{};
         }
 
-        const auto canard_node_id = static_cast<CanardNodeID>(local_node_id.value_or(CANARD_NODE_ID_UNSET));
-
-        auto transport =
-            libcyphal::detail::makeUniquePtr<Spec>(memory, Spec{}, memory, multiplexer, media_array, canard_node_id);
+        auto transport = libcyphal::detail::makeUniquePtr<Spec>(memory, Spec{}, memory, media_array);
         if (transport == nullptr)
         {
             return MemoryError{};
@@ -152,21 +141,13 @@ class TransportImpl final : public ICanTransport, private TransportDelegate  //
         return transport;
     }
 
-    TransportImpl(Spec,
-                  cetl::pmr::memory_resource& memory,
-                  IMultiplexer&               multiplexer,
-                  MediaArray                  media_array,
-                  const CanardNodeID          canard_node_id)
+    TransportImpl(Spec, cetl::pmr::memory_resource& memory, MediaArray media_array)
         : TransportDelegate{memory}
         , media_array_{std::move(media_array)}
         , should_reconfigure_filters_{false}
         , total_message_ports_{0}
         , total_service_ports_{0}
     {
-        // TODO: Use it!
-        (void) multiplexer;
-
-        canard_instance().node_id = canard_node_id;
     }
 
     TransportImpl(const TransportImpl&)                = delete;
@@ -654,14 +635,20 @@ class TransportImpl final : public ICanTransport, private TransportDelegate  //
 
 }  // namespace detail
 
-CETL_NODISCARD inline Expected<UniquePtr<ICanTransport>, FactoryError> makeTransport(
-    cetl::pmr::memory_resource&  memory,
-    IMultiplexer&                multiplexer,
-    const cetl::span<IMedia*>    media,
-    const std::size_t            tx_capacity,
-    const cetl::optional<NodeId> local_node_id)
+/// @brief Makes a new CAN transport instance.
+///
+/// NB! Lifetime of the transport instance must never outlive `memory` and `media` instances.
+///
+/// @param memory Reference to a polymorphic memory resource to use for all allocations.
+/// @param media Collection of redundant media interfaces to use.
+/// @param tx_capacity Total number of frames that can be queued for transmission per `IMedia` instance.
+/// @return Unique pointer to the new CAN transport instance or an error.
+///
+inline Expected<UniquePtr<ICanTransport>, FactoryError> makeTransport(cetl::pmr::memory_resource& memory,
+                                                                      const cetl::span<IMedia*>   media,
+                                                                      const std::size_t           tx_capacity)
 {
-    return detail::TransportImpl::make(memory, multiplexer, media, tx_capacity, local_node_id);
+    return detail::TransportImpl::make(memory, media, tx_capacity);
 }
 
 }  // namespace can

diff --git a/include/libcyphal/transport/contiguous_payload.hpp b/include/libcyphal/transport/contiguous_payload.hpp
@@ -8,7 +8,7 @@
 
 #include "types.hpp"
 
-#include <cetl/pf17/attribute.hpp>
+#include <cetl/cetl.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 #include <cetl/pf20/cetlpf.hpp>
 

diff --git a/include/libcyphal/transport/msg_sessions.hpp b/include/libcyphal/transport/msg_sessions.hpp
@@ -10,7 +10,6 @@
 #include "session.hpp"
 #include "types.hpp"
 
-#include <cetl/pf17/attribute.hpp>
 #include <cetl/pf17/cetlpf.hpp>
 
 #include <cstddef>
@@ -34,31 +33,31 @@ struct MessageTxParams final
 class IMessageRxSession : public IRxSession
 {
 public:
-    CETL_NODISCARD virtual MessageRxParams getParams() const noexcept = 0;
+    virtual MessageRxParams getParams() const noexcept = 0;
 
     /// @brief Receives a message from the transport layer.
     ///
     /// Method is not blocking, and will return immediately if no message is available.
     ///
     /// @return A message transfer if available; otherwise an empty optional.
     ///
-    CETL_NODISCARD virtual cetl::optional<MessageRxTransfer> receive() = 0;
+    virtual cetl::optional<MessageRxTransfer> receive() = 0;
 
 };  // IMessageRxSession
 
 class IMessageTxSession : public ITxSession
 {
 public:
-    CETL_NODISCARD virtual MessageTxParams getParams() const noexcept = 0;
+    virtual MessageTxParams getParams() const noexcept = 0;
 
     /// @brief Sends a message to the transport layer.
     ///
     /// @param metadata Additional metadata associated with the message.
     /// @param payload_fragments Segments of the message payload.
     /// @return `nullopt` in case of success; otherwise a transport error.
     ///
-    CETL_NODISCARD virtual cetl::optional<AnyError> send(const TransferMetadata& metadata,
-                                                         const PayloadFragments  payload_fragments) = 0;
+    virtual cetl::optional<AnyError> send(const TransferMetadata& metadata,
+                                          const PayloadFragments  payload_fragments) = 0;
 };  // IMessageTxSession
 
 }  // namespace transport