diff --git a/.clang-tidy b/.clang-tidy index 99d9c05b2..5849475a7 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -12,6 +12,7 @@ Checks: >- performance-*, portability-*, readability-*, + -boost-use-ranges, -clang-analyzer-core.uninitialized.Assign, -cppcoreguidelines-avoid-const-or-ref-data-members, -cppcoreguidelines-use-default-member-init, @@ -22,7 +23,9 @@ Checks: >- -modernize-type-traits, -modernize-use-constraints, -modernize-use-default-member-init, + -modernize-use-designated-initializers, -modernize-use-nodiscard, + -modernize-use-ranges, -readability-avoid-const-params-in-decls, -readability-identifier-length, -*-use-trailing-return-type, diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 8fdcbfbe7..755d930ee 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -1,6 +1,6 @@ { "name": "linux development environment for libcyphal", - "image": "ghcr.io/opencyphal/toolshed:ts22.4.10", + "image": "ghcr.io/opencyphal/toolshed:ts24.4.3", "customizations": { "vscode": { "extensions": [ diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index d673ce8d1..530e1e1ce 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -15,14 +15,14 @@ jobs: contains(github.ref, '/issue/') || (github.event_name == 'pull_request') runs-on: ubuntu-latest - container: ghcr.io/opencyphal/toolshed:ts22.4.10 + container: ghcr.io/opencyphal/toolshed:ts24.4.3 steps: - uses: actions/checkout@v4 with: fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis - name: get nunavut - run: > - pip install git+https://github.com/OpenCyphal/nunavut.git@3.0.preview + run: | + pip install --break-system-packages git+https://github.com/OpenCyphal/nunavut.git@3.0.preview - name: Install sonar-scanner and build-wrapper uses: SonarSource/sonarcloud-github-c-cpp@v2 - name: Run tests diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 6f0423517..f3cf995a5 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,7 +16,7 @@ jobs: contains(github.ref, '/issue/') || (github.event_name == 'pull_request') runs-on: ubuntu-latest - container: ghcr.io/opencyphal/toolshed:ts22.4.10 + container: ghcr.io/opencyphal/toolshed:ts24.4.3 steps: - uses: actions/checkout@v4 - name: Cache ext modules @@ -29,8 +29,8 @@ jobs: key: ${{ runner.os }}-${{ hashFiles('cmake/modules/*.cmake') }} - name: get nunavut # TODO: setup a venv, cache, and distribute to the other jobs. - run: > - pip install git+https://github.com/OpenCyphal/nunavut.git@3.0.preview + run: | + pip install --break-system-packages git+https://github.com/OpenCyphal/nunavut.git@3.0.preview - name: configure run: > ./build-tools/bin/verify.py @@ -47,7 +47,7 @@ jobs: contains(github.ref, '/issue/') || (github.event_name == 'pull_request') runs-on: ubuntu-latest - container: ghcr.io/opencyphal/toolshed:ts22.4.10 + container: ghcr.io/opencyphal/toolshed:ts24.4.3 needs: [warmup] strategy: matrix: @@ -69,8 +69,8 @@ jobs: path: external key: ${{ runner.os }}-${{ hashFiles('cmake/modules/*.cmake') }} - name: get nunavut - run: > - pip install git+https://github.com/OpenCyphal/nunavut.git@3.0.preview + run: | + pip install --break-system-packages git+https://github.com/OpenCyphal/nunavut.git@3.0.preview - name: run tests env: GTEST_COLOR: yes @@ -101,7 +101,7 @@ jobs: contains(github.ref, '/issue/') || (github.event_name == 'pull_request') runs-on: ubuntu-latest - container: ghcr.io/opencyphal/toolshed:ts22.4.10 + container: ghcr.io/opencyphal/toolshed:ts24.4.3 needs: [warmup] steps: - uses: actions/checkout@v4 @@ -114,8 +114,8 @@ jobs: path: external key: ${{ runner.os }}-${{ hashFiles('cmake/modules/*.cmake') }} - name: get nunavut - run: > - pip install git+https://github.com/OpenCyphal/nunavut.git@3.0.preview + run: | + pip install --break-system-packages git+https://github.com/OpenCyphal/nunavut.git@3.0.preview - name: doc-gen run: > ./build-tools/bin/verify.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e7b28e9e7..db1332ca8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -147,10 +147,10 @@ and manually run it. ### TLDR ``` -docker pull ghcr.io/opencyphal/toolshed:ts22.4.10 +docker pull ghcr.io/opencyphal/toolshed:ts24.4.3 git clone {this repo} cd {this repo} -docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts22.4.10 +docker run --rm -it -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts24.4.3 mkdir build cd build cmake .. @@ -200,7 +200,7 @@ To ensure that the formatting matches the expectations of the CI suite, invoke Clang-Format of the correct version from the container (be sure to use the correct image tag): ``` -docker run --rm -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts22.4.10 ./build-tools/bin/verify.py build-danger-danger-repo-clang-format-in-place +docker run --rm -v ${PWD}:/repo ghcr.io/opencyphal/toolshed:ts24.4.3 ./build-tools/bin/verify.py build-danger-danger-repo-clang-format-in-place ``` ### `issue/*` and hashtag-based CI triggering diff --git a/docs/examples/1_presentation/example_1_presentation_0_hello_raw_pubsub_udp.cpp b/docs/examples/1_presentation/example_1_presentation_0_hello_raw_pubsub_udp.cpp index 0665f08d4..880382668 100644 --- a/docs/examples/1_presentation/example_1_presentation_0_hello_raw_pubsub_udp.cpp +++ b/docs/examples/1_presentation/example_1_presentation_0_hello_raw_pubsub_udp.cpp @@ -151,8 +151,8 @@ TEST_F(Example_1_Presentation_0_HelloRawPubSub_Udp, main) const TimePoint msg_deadline = arg.approx_now + 1s; constexpr cetl::span message{"Hello, World!"}; // - const std::array, 1> payload_fragments{ - cetl::span{reinterpret_cast(message.data()), // NOLINT + const std::array payload_fragments{ + PayloadFragment{reinterpret_cast(message.data()), // NOLINT message.size()}}; EXPECT_THAT(raw_publisher.publish(msg_deadline, payload_fragments), testing::Eq(cetl::nullopt)); }); diff --git a/docs/examples/1_presentation/example_1_presentation_2_heartbeat_getinfo_udp.cpp b/docs/examples/1_presentation/example_1_presentation_2_heartbeat_getinfo_udp.cpp index ca2134b76..e62c3ac8d 100644 --- a/docs/examples/1_presentation/example_1_presentation_2_heartbeat_getinfo_udp.cpp +++ b/docs/examples/1_presentation/example_1_presentation_2_heartbeat_getinfo_udp.cpp @@ -176,7 +176,7 @@ TEST_F(Example_1_Presentation_2_Heartbeat_GetInfo_Udp, main) // Bring up 'GetInfo' server. // - using GetInfo_1_0 = uavcan::node::GetInfo_1_0; + using uavcan::node::GetInfo_1_0; const std::string node_name{"org.opencyphal.Ex_1_Pres_2_HB_GetInfo_UDP"}; std::copy_n(node_name.begin(), std::min(node_name.size(), 50UL), std::back_inserter(state.get_info_response.name)); // diff --git a/docs/examples/1_presentation/example_1_presentation_3_hb_getinfo_ping_linux_can.cpp b/docs/examples/1_presentation/example_1_presentation_3_hb_getinfo_ping_linux_can.cpp index f7121e116..380870137 100644 --- a/docs/examples/1_presentation/example_1_presentation_3_hb_getinfo_ping_linux_can.cpp +++ b/docs/examples/1_presentation/example_1_presentation_3_hb_getinfo_ping_linux_can.cpp @@ -424,8 +424,8 @@ TEST_F(Example_1_Presentation_3_HB_GetInfo_Ping_Can, main) // 7. Bring up 'GetInfo' server. // - using GetInfo_1_0 = uavcan::node::GetInfo_1_0; - uavcan::node::GetInfo_1_0::Response get_info_response{{&mr_}}; + using uavcan::node::GetInfo_1_0; + GetInfo_1_0::Response get_info_response{{&mr_}}; get_info_response.protocol_version.major = 1; const std::string node_name{"org.opencyphal.Ex_1_Pres_3_HB_GetInfo_Ping_CAN"}; std::copy_n(node_name.begin(), std::min(node_name.size(), 50UL), std::back_inserter(get_info_response.name)); diff --git a/docs/examples/platform/node_helpers.hpp b/docs/examples/platform/node_helpers.hpp index 89eb5b946..70415f23d 100644 --- a/docs/examples/platform/node_helpers.hpp +++ b/docs/examples/platform/node_helpers.hpp @@ -70,13 +70,15 @@ struct NodeHelpers const TxMetadata& metadata) { using traits = typename T::_traits_; + using libcyphal::transport::PayloadFragment; + std::array buffer{}; const auto data_size = serialize(value, buffer).value(); // NOLINTNEXTLINE - const cetl::span fragment{reinterpret_cast(buffer.data()), data_size}; - const std::array, 1> payload{fragment}; + const PayloadFragment fragment{reinterpret_cast(buffer.data()), data_size}; + const std::array payload{fragment}; return tx_session.send(metadata, payload); } diff --git a/include/libcyphal/common/cavl/cavl.hpp b/include/libcyphal/common/cavl/cavl.hpp index 0a38736d1..dcce161db 100644 --- a/include/libcyphal/common/cavl/cavl.hpp +++ b/include/libcyphal/common/cavl/cavl.hpp @@ -81,12 +81,12 @@ class Node // NOSONAR cpp:S1448 using DerivedType = Derived; // Tree nodes cannot be copied for obvious reasons. - Node(const Node&) = delete; + Node(const Node&) = delete; // NOLINT bugprone-crtp-constructor-accessibility auto operator=(const Node&) -> Node& = delete; // Tree nodes can be moved. We update the pointers in the adjacent nodes to keep the tree valid, // as well as root node pointer if needed (see `moveFrom`). This operation is constant time. - Node(Node&& other) noexcept + Node(Node&& other) noexcept // NOLINT bugprone-crtp-constructor-accessibility { moveFrom(other); } @@ -102,7 +102,7 @@ class Node // NOSONAR cpp:S1448 } protected: - Node() = default; + Node() = default; // NOLINT bugprone-crtp-constructor-accessibility ~Node() = default; /// Accessors for advanced tree introspection. Not needed for typical usage. diff --git a/include/libcyphal/presentation/client.hpp b/include/libcyphal/presentation/client.hpp index 57617fdbb..cc13fd1aa 100644 --- a/include/libcyphal/presentation/client.hpp +++ b/include/libcyphal/presentation/client.hpp @@ -92,11 +92,13 @@ class ClientBase ClientBase& operator=(ClientBase&& other) noexcept { - CETL_DEBUG_ASSERT(shared_client_ != nullptr, "Not supposed to move construct to already moved `this`."); CETL_DEBUG_ASSERT(other.shared_client_ != nullptr, "Not supposed to move construct from already moved `other`."); - (void) shared_client_->release(); + if (nullptr != shared_client_) + { + (void) shared_client_->release(); + } shared_client_ = std::exchange(other.shared_client_, nullptr); priority_ = other.priority_; diff --git a/include/libcyphal/presentation/common_helpers.hpp b/include/libcyphal/presentation/common_helpers.hpp index df8aa3894..80332fed9 100644 --- a/include/libcyphal/presentation/common_helpers.hpp +++ b/include/libcyphal/presentation/common_helpers.hpp @@ -100,8 +100,8 @@ static auto tryPerformOnSerialized(const Message& message, return result_size.error(); } - const cetl::span data_span{buffer.data(), result_size.value()}; - const std::array, 1> fragments{data_span}; + const transport::PayloadFragment data_span{buffer.data(), result_size.value()}; + const std::array fragments{data_span}; return std::forward(action)(fragments); } @@ -135,8 +135,8 @@ static auto tryPerformOnSerialized(const Message& message, return result_size.error(); } - const cetl::span data_span{buffer.get(), result_size.value()}; - const std::array, 1> fragments{data_span}; + const transport::PayloadFragment data_span{buffer.get(), result_size.value()}; + const std::array fragments{data_span}; return std::forward(action)(fragments); } diff --git a/include/libcyphal/presentation/publisher.hpp b/include/libcyphal/presentation/publisher.hpp index 1e9b035ad..9ee7721e7 100644 --- a/include/libcyphal/presentation/publisher.hpp +++ b/include/libcyphal/presentation/publisher.hpp @@ -78,10 +78,12 @@ class PublisherBase PublisherBase& operator=(PublisherBase&& other) noexcept { - CETL_DEBUG_ASSERT(impl_ != nullptr, "Not supposed to move to already moved `this`."); CETL_DEBUG_ASSERT(other.impl_ != nullptr, "Not supposed to move from already moved `other`."); - (void) impl_->release(); + if (nullptr != impl_) + { + (void) impl_->release(); + } impl_ = std::exchange(other.impl_, nullptr); priority_ = other.priority_; diff --git a/include/libcyphal/presentation/subscriber.hpp b/include/libcyphal/presentation/subscriber.hpp index 758177f6d..5e4a16680 100644 --- a/include/libcyphal/presentation/subscriber.hpp +++ b/include/libcyphal/presentation/subscriber.hpp @@ -50,9 +50,12 @@ class SubscriberBase : public SubscriberImpl::CallbackNode SubscriberBase& operator=(SubscriberBase&& other) noexcept { - CETL_DEBUG_ASSERT(impl_ != nullptr, "Not supposed to move to already moved `this`."); + CETL_DEBUG_ASSERT(other.impl_ != nullptr, "Not supposed to move from already moved `other`."); - impl_->releaseCallbackNode(*this); + if (nullptr != impl_) + { + impl_->releaseCallbackNode(*this); + } impl_ = std::exchange(other.impl_, nullptr); diff --git a/include/libcyphal/transport/can/can_transport_impl.hpp b/include/libcyphal/transport/can/can_transport_impl.hpp index 59d448d6a..82a0f70cc 100644 --- a/include/libcyphal/transport/can/can_transport_impl.hpp +++ b/include/libcyphal/transport/can/can_transport_impl.hpp @@ -109,7 +109,7 @@ class TransportImpl final : private TransportDelegate, public ICanTransport private: CETL_NODISCARD static CanardMemoryResource makeTxMemoryResource(IMedia& media_interface) { - using LizardHelpers = libcyphal::transport::detail::LizardHelpers; + using transport::detail::LizardHelpers; // TX memory resource is used for raw bytes block allocations only. // So it has no alignment requirements. @@ -147,10 +147,8 @@ class TransportImpl final : private TransportDelegate, public ICanTransport return ArgumentError{}; } - // False positive of clang-tidy - we move `media_array` to the `transport` instance, so can't make it const. - // NOLINTNEXTLINE(misc-const-correctness) - MediaArray media_array = makeMediaArray(memory, media_count, media, tx_capacity); - if (media_array.size() != media_count) + MediaArray media_array{media_count, &memory}; + if (!makeMediaArray(media_array, media_count, media, tx_capacity)) { return MemoryError{}; } @@ -514,18 +512,23 @@ class TransportImpl final : private TransportDelegate, public ICanTransport return tryHandleTransientFailure(std::move(*failure), media.index(), canardInstance()); } - CETL_NODISCARD static MediaArray makeMediaArray(cetl::pmr::memory_resource& memory, - const std::size_t media_count, - const cetl::span media_interfaces, - const std::size_t tx_capacity) + CETL_NODISCARD static bool makeMediaArray(MediaArray& media_array, + const std::size_t media_count, + const cetl::span media_interfaces, + const std::size_t tx_capacity) { - MediaArray media_array{media_count, &memory}; - - // Reserve the space for the whole array (to avoid reallocations). - // Capacity will be less than requested in case of out of memory. - media_array.reserve(media_count); - if (media_array.capacity() >= media_count) +#if defined(__cpp_exceptions) + try { +#endif + // Reserve the space for the whole array (to avoid reallocations). + // Capacity will be less than requested in case of out of memory. + media_array.reserve(media_count); + if (media_array.capacity() < media_count) + { + return false; + } + std::size_t index = 0; for (IMedia* const media_interface : media_interfaces) { @@ -538,9 +541,14 @@ class TransportImpl final : private TransportDelegate, public ICanTransport } CETL_DEBUG_ASSERT(index == media_count, ""); CETL_DEBUG_ASSERT(media_array.size() == media_count, ""); - } + return true; - return media_array; +#if defined(__cpp_exceptions) + } catch (const std::bad_alloc&) + { + return false; + } +#endif } static void flushCanardTxQueue(CanardTxQueue& canard_tx_queue, const CanardInstance& canard_instance) diff --git a/include/libcyphal/transport/can/delegate.hpp b/include/libcyphal/transport/can/delegate.hpp index 635a8eedf..8e8f0fdd6 100644 --- a/include/libcyphal/transport/can/delegate.hpp +++ b/include/libcyphal/transport/can/delegate.hpp @@ -101,6 +101,14 @@ class CanardMemory final : public ScatteredBuffer::IStorage return bytes_to_copy; } + void forEachFragment(ScatteredBuffer::IFragmentsVisitor& visitor) const override + { + if ((buffer_ != nullptr) && (payload_size_ > 0)) + { + visitor.onNext({buffer_, payload_size_}); + } + } + private: // MARK: Data members: @@ -471,13 +479,23 @@ class TransportDelegate // Now we know that we have at least one active port, // so we need preallocate temp memory for the total number of active ports. // - filters.reserve(total_active_ports); - if (filters.capacity() < total_active_ports) +#if defined(__cpp_exceptions) + try + { +#endif + filters.reserve(total_active_ports); + if (filters.capacity() < total_active_ports) + { + // This is out of memory situation. + return false; + } + +#if defined(__cpp_exceptions) + } catch (const std::bad_alloc&) { - // This is out of memory situation. return false; } - +#endif // `ports_count` counting is just for the sake of debug verification. std::size_t ports_count = 0; diff --git a/include/libcyphal/transport/contiguous_payload.hpp b/include/libcyphal/transport/contiguous_payload.hpp index 444241d0a..a4809377e 100644 --- a/include/libcyphal/transport/contiguous_payload.hpp +++ b/include/libcyphal/transport/contiguous_payload.hpp @@ -45,13 +45,11 @@ class ContiguousPayload final , payload_size_{0} , allocated_buffer_{nullptr} { - using Fragment = cetl::span; - // Count fragments skipping empty ones. Also keep tracking of the total payload size // and pointer to the last non-empty fragment (which will be in use for the optimization). // const auto total_non_empty_fragments = - std::count_if(payload_fragments.begin(), payload_fragments.end(), [this](const Fragment frag) { + std::count_if(payload_fragments.begin(), payload_fragments.end(), [this](const PayloadFragment frag) { if (frag.empty()) { return false; @@ -68,9 +66,9 @@ class ContiguousPayload final if (cetl::byte* const buffer = allocated_buffer_) { std::size_t offset = 0; - for (const Fragment frag : payload_fragments) + for (const PayloadFragment frag : payload_fragments) { - // Next nolint is unavoidable: we need offset from the beginning of the buffer. + // Next nolint is unavoidable: we need to offset from the beginning of the buffer. // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) (void) std::memmove(&buffer[offset], frag.data(), frag.size()); offset += frag.size(); diff --git a/include/libcyphal/transport/media_payload.hpp b/include/libcyphal/transport/media_payload.hpp index 8dea6ba1f..f75d132c9 100644 --- a/include/libcyphal/transport/media_payload.hpp +++ b/include/libcyphal/transport/media_payload.hpp @@ -6,6 +6,8 @@ #ifndef LIBCYPHAL_TRANSPORT_MEDIA_PAYLOAD_HPP_INCLUDED #define LIBCYPHAL_TRANSPORT_MEDIA_PAYLOAD_HPP_INCLUDED +#include "types.hpp" + #include #include @@ -125,7 +127,7 @@ class MediaPayload final /// /// Returns an empty (`{nullptr, 0}`) span if the payload is moved, released or reset. /// - cetl::span getSpan() const noexcept + PayloadFragment getSpan() const noexcept { return {ownership_.data, ownership_.size}; } diff --git a/include/libcyphal/transport/msg_sessions.hpp b/include/libcyphal/transport/msg_sessions.hpp index 216673bc7..d4aea2e7b 100644 --- a/include/libcyphal/transport/msg_sessions.hpp +++ b/include/libcyphal/transport/msg_sessions.hpp @@ -7,6 +7,7 @@ #define LIBCYPHAL_TRANSPORT_MSG_SESSIONS_HPP_INCLUDED #include "errors.hpp" +#include "scattered_buffer.hpp" #include "session.hpp" #include "types.hpp" @@ -34,6 +35,18 @@ struct MessageTxParams final PortId subject_id{}; }; +struct MessageRxMetadata final +{ + TransferRxMetadata rx_meta{}; + cetl::optional publisher_node_id; +}; + +struct MessageRxTransfer final +{ + MessageRxMetadata metadata{}; + ScatteredBuffer payload; +}; + /// @brief Defines an abstract interface of a transport layer receive session for message subscription. /// /// Use transport's `makeMessageRxSession` factory function to create an instance of this interface. diff --git a/include/libcyphal/transport/scattered_buffer.hpp b/include/libcyphal/transport/scattered_buffer.hpp index 7019b305c..47ab33c8a 100644 --- a/include/libcyphal/transport/scattered_buffer.hpp +++ b/include/libcyphal/transport/scattered_buffer.hpp @@ -7,6 +7,7 @@ #define LIBCYPHAL_TRANSPORT_SCATTERED_BUFFER_HPP_INCLUDED #include "libcyphal/config.hpp" +#include "types.hpp" #include #include @@ -33,6 +34,28 @@ class ScatteredBuffer final /// static constexpr std::size_t StorageVariantFootprint = config::Transport::ScatteredBuffer_StorageVariantFootprint(); + /// @brief Defines interface for visiting internal fragments of the scattered buffer. + /// + class IFragmentsVisitor + { + public: + IFragmentsVisitor(const IFragmentsVisitor&) = delete; + IFragmentsVisitor& operator=(const IFragmentsVisitor&) = delete; + IFragmentsVisitor& operator=(IFragmentsVisitor&&) noexcept = delete; + IFragmentsVisitor(IFragmentsVisitor&&) noexcept = delete; + + /// @brief Notifies the visitor about the next fragment of the scattered buffer. + /// + /// See also `forEachFragment` method of the `ScatteredBuffer`. + /// + virtual void onNext(const PayloadFragment fragment) = 0; + + protected: + IFragmentsVisitor() = default; + ~IFragmentsVisitor() = default; + + }; // IFragmentsVisitor + /// @brief Defines storage interface for the scattered buffer. /// /// @see ScatteredBuffer::ScatteredBuffer(AnyStorage&& any_storage) @@ -72,6 +95,12 @@ class ScatteredBuffer final cetl::byte* const destination, const std::size_t length_bytes) const = 0; + /// @brief Reports the internal fragments of the storage to the specified visitor. + /// + /// @param visitor The visitor will be called (by `onNext` method) for each fragment of the storage. + /// + virtual void forEachFragment(IFragmentsVisitor& visitor) const = 0; + // MARK: RTTI static constexpr cetl::type_id _get_type_id_() noexcept @@ -201,6 +230,18 @@ class ScatteredBuffer final return storage_->copy(offset_bytes, static_cast(destination), length_bytes); } + /// @brief Reports the internal fragments of the storage to the specified visitor. + /// + /// @param visitor The visitor will be called (by `onNext` method) for each fragment of the storage. + /// + void forEachFragment(IFragmentsVisitor& visitor) const + { + if (const auto* const storage = storage_) + { + storage->forEachFragment(visitor); + } + } + private: cetl::unbounded_variant storage_variant_; const IStorage* storage_; diff --git a/include/libcyphal/transport/svc_sessions.hpp b/include/libcyphal/transport/svc_sessions.hpp index f4a601a0a..3cbea6a41 100644 --- a/include/libcyphal/transport/svc_sessions.hpp +++ b/include/libcyphal/transport/svc_sessions.hpp @@ -7,6 +7,7 @@ #define LIBCYPHAL_TRANSPORT_SVC_SESSION_HPP_INCLUDED #include "errors.hpp" +#include "scattered_buffer.hpp" #include "session.hpp" #include "types.hpp" @@ -46,6 +47,24 @@ struct ResponseTxParams final PortId service_id{}; }; +struct ServiceTxMetadata final +{ + TransferTxMetadata tx_meta{}; + NodeId remote_node_id{}; +}; + +struct ServiceRxMetadata final +{ + TransferRxMetadata rx_meta{}; + NodeId remote_node_id{}; +}; + +struct ServiceRxTransfer final +{ + ServiceRxMetadata metadata{}; + ScatteredBuffer payload; +}; + /// @brief Defines an abstract interface of a transport layer receive session for service. /// /// @see IRxSession, ISession diff --git a/include/libcyphal/transport/transport.hpp b/include/libcyphal/transport/transport.hpp index ed463bad1..25aa3ffcf 100644 --- a/include/libcyphal/transport/transport.hpp +++ b/include/libcyphal/transport/transport.hpp @@ -21,6 +21,13 @@ namespace libcyphal namespace transport { +struct ProtocolParams final +{ + TransferId transfer_id_modulo{}; + std::size_t mtu_bytes{}; + NodeId max_nodes{}; +}; + /// @brief Interface for a transport layer. /// class ITransport diff --git a/include/libcyphal/transport/types.hpp b/include/libcyphal/transport/types.hpp index 8a02f227e..e5d338d5a 100644 --- a/include/libcyphal/transport/types.hpp +++ b/include/libcyphal/transport/types.hpp @@ -6,8 +6,6 @@ #ifndef LIBCYPHAL_TRANSPORT_TYPES_HPP_INCLUDED #define LIBCYPHAL_TRANSPORT_TYPES_HPP_INCLUDED -#include "scattered_buffer.hpp" - #include "libcyphal/types.hpp" #include @@ -48,59 +46,32 @@ enum class Priority : std::uint8_t Optional = 7, }; -struct ProtocolParams final -{ - TransferId transfer_id_modulo{}; - std::size_t mtu_bytes{}; - NodeId max_nodes{}; -}; +/// @brief Defines immutable fragment of raw data (as span of const bytes). +/// +using PayloadFragment = cetl::span; + +/// @brief Defines a span of immutable raw data fragments. +/// +using PayloadFragments = cetl::span; struct TransferMetadata final { TransferId transfer_id{}; Priority priority{}; }; + struct TransferTxMetadata final { TransferMetadata base{}; TimePoint deadline; }; + struct TransferRxMetadata final { TransferMetadata base{}; TimePoint timestamp; }; -/// @brief Defines a span of immutable fragments of payload. -using PayloadFragments = cetl::span>; - -struct MessageRxMetadata final -{ - TransferRxMetadata rx_meta{}; - cetl::optional publisher_node_id; -}; -struct MessageRxTransfer final -{ - MessageRxMetadata metadata{}; - ScatteredBuffer payload; -}; - -struct ServiceTxMetadata final -{ - TransferTxMetadata tx_meta{}; - NodeId remote_node_id{}; -}; -struct ServiceRxMetadata final -{ - TransferRxMetadata rx_meta{}; - NodeId remote_node_id{}; -}; -struct ServiceRxTransfer final -{ - ServiceRxMetadata metadata{}; - ScatteredBuffer payload; -}; - } // namespace transport } // namespace libcyphal diff --git a/include/libcyphal/transport/udp/delegate.hpp b/include/libcyphal/transport/udp/delegate.hpp index 3a7149706..57496fc56 100644 --- a/include/libcyphal/transport/udp/delegate.hpp +++ b/include/libcyphal/transport/udp/delegate.hpp @@ -138,8 +138,6 @@ class UdpardMemory final : public ScatteredBuffer::IStorage cetl::byte* const destination, const std::size_t length_bytes) const override { - using FragSpan = const cetl::span; - // TODO: Use `udpardGather` function when it will be available with offset support. CETL_DEBUG_ASSERT((destination != nullptr) || (length_bytes == 0), @@ -152,8 +150,8 @@ class UdpardMemory final : public ScatteredBuffer::IStorage // Find first fragment to start from (according to source `offset_bytes`). // - std::size_t src_offset = 0; - const struct UdpardFragment* frag = &payload_; + std::size_t src_offset = 0; + const UdpardFragment* frag = &payload_; while ((nullptr != frag) && (offset_bytes >= (src_offset + frag->view.size))) { src_offset += frag->view.size; @@ -169,11 +167,13 @@ class UdpardMemory final : public ScatteredBuffer::IStorage while ((nullptr != frag) && (dst_offset < length_bytes)) { CETL_DEBUG_ASSERT(nullptr != frag->view.data, ""); - // Next nolint-s are unavoidable: we need offset from the beginning of the buffer. - // No Sonar `cpp:S5356` b/c we integrate here with libcanard raw C buffers. - // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) - FragSpan frag_span{static_cast(frag->view.data) + view_offset, // NOSONAR cpp:S5356 - std::min(frag->view.size - view_offset, length_bytes - dst_offset)}; + // Next nolint-s are unavoidable: we need to offset from the beginning of the buffer. + // No Sonar `cpp:S5356` & `cpp:S5357` b/c we integrate here with libcanard raw C buffers. + const auto* const offset_data = + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) + static_cast(frag->view.data) + view_offset; // NOSONAR cpp:S5356 cpp:S5357 + const PayloadFragment frag_span{offset_data, + std::min(frag->view.size - view_offset, length_bytes - dst_offset)}; CETL_DEBUG_ASSERT(frag_span.size() <= (frag->view.size - view_offset), ""); // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) @@ -190,6 +190,22 @@ class UdpardMemory final : public ScatteredBuffer::IStorage return total_bytes_copied; } + void forEachFragment(ScatteredBuffer::IFragmentsVisitor& visitor) const override + { + const UdpardFragment* fragment = &payload_; + while (nullptr != fragment) + { + const auto& frag_view = fragment->view; + if ((nullptr != frag_view.data) && (frag_view.size > 0)) + { + visitor.onNext({static_cast(frag_view.data), // NOSONAR cpp:S5356 cpp:S5357 + frag_view.size}); + } + + fragment = fragment->next; + } + } + private: // MARK: Data members: diff --git a/include/libcyphal/transport/udp/udp_transport_impl.hpp b/include/libcyphal/transport/udp/udp_transport_impl.hpp index 5fadaf9ca..15a1b9124 100644 --- a/include/libcyphal/transport/udp/udp_transport_impl.hpp +++ b/include/libcyphal/transport/udp/udp_transport_impl.hpp @@ -119,7 +119,7 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport private: CETL_NODISCARD static UdpardMemoryResource makeTxMemoryResource(IMedia& media_interface) { - using LizardHelpers = libcyphal::transport::detail::LizardHelpers; + using transport::detail::LizardHelpers; // TX memory resource is used for raw bytes block allocations only. // So it has no alignment requirements. @@ -164,10 +164,8 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport const UdpardNodeID unset_node_id = UDPARD_NODE_ID_UNSET; - // False positive of clang-tidy - we move `media_array` to the `transport` instance, so can't make it const. - // NOLINTNEXTLINE(misc-const-correctness) - MediaArray media_array = makeMediaArray(memory_resources, media_count, media, &unset_node_id, tx_capacity); - if (media_array.size() != media_count) + MediaArray media_array{media_count, &memory_resources.general}; + if (!makeMediaArray(memory_resources, media_array, media_count, media, &unset_node_id, tx_capacity)) { return MemoryError{}; } @@ -618,19 +616,25 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport return failure; } - CETL_NODISCARD static MediaArray makeMediaArray(const MemoryResources& memory, - const std::size_t media_count, - const cetl::span media_interfaces, - const UdpardNodeID* const local_node_id_, - const std::size_t tx_capacity) + CETL_NODISCARD static bool makeMediaArray(const MemoryResources& memory, + MediaArray& media_array, + const std::size_t media_count, + const cetl::span media_interfaces, + const UdpardNodeID* const local_node_id_, + const std::size_t tx_capacity) { - MediaArray media_array{media_count, &memory.general}; - - // Reserve the space for the whole array (to avoid reallocations). - // Capacity will be less than requested in case of out of memory. - media_array.reserve(media_count); - if (media_array.capacity() >= media_count) +#if defined(__cpp_exceptions) + try { +#endif + // Reserve the space for the whole array (to avoid reallocations). + // Capacity will be less than requested in case of out of memory (or `bad_alloc` if exceptions are enabled). + media_array.reserve(media_count); + if (media_array.capacity() < media_count) + { + return false; + } + std::size_t index = 0; for (IMedia* const media_interface : media_interfaces) { @@ -643,9 +647,14 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport } CETL_DEBUG_ASSERT(index == media_count, ""); CETL_DEBUG_ASSERT(media_array.size() == media_count, ""); - } + return true; - return media_array; +#if defined(__cpp_exceptions) + } catch (const std::bad_alloc&) + { + return false; + } +#endif } /// @brief Tries to run an action with media and its TX socket (the latter one is made on demand if necessary). @@ -704,8 +713,6 @@ class TransportImpl final : private TransportDelegate, public IUdpTransport /// void sendNextFrameToMediaTxSocket(Media& media, ITxSocket& tx_socket) { - using PayloadFragment = cetl::span; - TimePoint tx_deadline; while (UdpardTxItem* const tx_item = peekFirstValidTxItem(media.udpard_tx(), tx_deadline)) { diff --git a/test/unittest/application/node/test_get_info_provider.cpp b/test/unittest/application/node/test_get_info_provider.cpp index 89034c4f5..0176be191 100644 --- a/test/unittest/application/node/test_get_info_provider.cpp +++ b/test/unittest/application/node/test_get_info_provider.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include diff --git a/test/unittest/application/node/test_heartbeat_producer.cpp b/test/unittest/application/node/test_heartbeat_producer.cpp index 5792f0584..e4ae7eec7 100644 --- a/test/unittest/application/node/test_heartbeat_producer.cpp +++ b/test/unittest/application/node/test_heartbeat_producer.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include diff --git a/test/unittest/application/node/test_registry_provider.cpp b/test/unittest/application/node/test_registry_provider.cpp index 248f48e8a..2f5758cb5 100644 --- a/test/unittest/application/node/test_registry_provider.cpp +++ b/test/unittest/application/node/test_registry_provider.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include diff --git a/test/unittest/application/test_node.cpp b/test/unittest/application/test_node.cpp index 5cde5ad58..6b4140706 100644 --- a/test/unittest/application/test_node.cpp +++ b/test/unittest/application/test_node.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include diff --git a/test/unittest/common/cavl/.clang-tidy b/test/unittest/common/cavl/.clang-tidy index 7538dc5f0..577a25beb 100644 --- a/test/unittest/common/cavl/.clang-tidy +++ b/test/unittest/common/cavl/.clang-tidy @@ -11,6 +11,7 @@ Checks: >- misc-*, portability-*, readability-*, + -boost-use-ranges, -hicpp-function-size, -hicpp-no-array-decay, -google-readability-todo, diff --git a/test/unittest/platform/test_single_threaded_executor.cpp b/test/unittest/platform/test_single_threaded_executor.cpp index 946abb6c5..14cb504e7 100644 --- a/test/unittest/platform/test_single_threaded_executor.cpp +++ b/test/unittest/platform/test_single_threaded_executor.cpp @@ -27,10 +27,10 @@ namespace { -using Duration = libcyphal::Duration; -using TimePoint = libcyphal::TimePoint; -using Callback = libcyphal::IExecutor::Callback; -using Schedule = libcyphal::IExecutor::Callback::Schedule; +using libcyphal::Duration; +using libcyphal::TimePoint; +using Callback = libcyphal::IExecutor::Callback; +using Schedule = libcyphal::IExecutor::Callback::Schedule; using namespace libcyphal::platform; // NOLINT This our main concern here in the unit tests. using testing::Eq; diff --git a/test/unittest/presentation/test_client.cpp b/test/unittest/presentation/test_client.cpp index a5e5a2b06..e9417d5eb 100644 --- a/test/unittest/presentation/test_client.cpp +++ b/test/unittest/presentation/test_client.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include diff --git a/test/unittest/presentation/test_presentation.cpp b/test/unittest/presentation/test_presentation.cpp index 22bdbfdc1..0713d0aa8 100644 --- a/test/unittest/presentation/test_presentation.cpp +++ b/test/unittest/presentation/test_presentation.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -309,7 +310,7 @@ TEST_F(TestPresentation, makePublisher_with_failure) } // Emulate that there is no memory available for the `PublisherImpl`. { - using PublisherImpl = libcyphal::presentation::detail::PublisherImpl; + using libcyphal::presentation::detail::PublisherImpl; StrictMock msg_tx_session_mock; EXPECT_CALL(msg_tx_session_mock, deinit()).Times(1); @@ -430,7 +431,7 @@ TEST_F(TestPresentation, makeSubscriber_with_failure) } // Emulate that there is no memory available for the `SubscriberImpl`. { - using SubscriberImpl = libcyphal::presentation::detail::SubscriberImpl; + using libcyphal::presentation::detail::SubscriberImpl; StrictMock msg_rx_session_mock; EXPECT_CALL(msg_rx_session_mock, deinit()).Times(1); @@ -499,7 +500,7 @@ TEST_F(TestPresentation, makeServer_custom) Presentation presentation{mr_, scheduler_, transport_mock_}; - auto maybe_server = presentation.makeServer([](const auto&, auto) {}); + auto maybe_server = presentation.makeServer([](const auto&, const auto&) {}); ASSERT_THAT(maybe_server, VariantWith>(_)); EXPECT_CALL(req_rx_session_mock, deinit()).Times(1); @@ -526,7 +527,10 @@ TEST_F(TestPresentation, makeServer_raw) Presentation presentation{mr_, scheduler_, transport_mock_}; - auto maybe_server = presentation.makeServer(rx_params.service_id, rx_params.extent_bytes, [](const auto&, auto) {}); + auto maybe_server = presentation.makeServer( // + rx_params.service_id, + rx_params.extent_bytes, + [](const auto&, const auto&) {}); ASSERT_THAT(maybe_server, VariantWith(_)); EXPECT_CALL(req_rx_session_mock, deinit()).Times(1); diff --git a/test/unittest/transport/can/test_can_delegate.cpp b/test/unittest/transport/can/test_can_delegate.cpp index 37fda2ea3..3103310f4 100644 --- a/test/unittest/transport/can/test_can_delegate.cpp +++ b/test/unittest/transport/can/test_can_delegate.cpp @@ -3,8 +3,10 @@ /// Copyright Amazon.com Inc. or its affiliates. /// SPDX-License-Identifier: MIT +#include "cetl_gtest_helpers.hpp" // NOLINT(misc-include-cleaner) #include "memory_resource_mock.hpp" #include "tracking_memory_resource.hpp" +#include "transport/scattered_buffer_storage_mock.hpp" #include "verification_utilities.hpp" #include @@ -103,17 +105,13 @@ class TestCanDelegate : public testing::Test TEST_F(TestCanDelegate, CanardMemory_copy) { - TransportDelegateImpl delegate{mr_}; - auto& canard_instance = delegate.canardInstance(); - constexpr std::size_t payload_size = 4; constexpr std::size_t allocated_size = payload_size + 1; - auto* const payload = - static_cast(canard_instance.memory.allocate(static_cast(&delegate), allocated_size)); + auto* const payload = static_cast(mr_.allocate(allocated_size)); fillIotaBytes({payload, allocated_size}, b('0')); CanardMutablePayload canard_payload{payload_size, payload, allocated_size}; - const CanardMemory canard_memory{mr_, canard_payload}; + const CanardMemory canard_memory{mr_, canard_payload}; EXPECT_THAT(canard_memory.size(), payload_size); EXPECT_THAT(canard_payload.size, 0); EXPECT_THAT(canard_payload.data, nullptr); @@ -165,16 +163,12 @@ TEST_F(TestCanDelegate, CanardMemory_copy) TEST_F(TestCanDelegate, CanardMemory_copy_on_moved) { - TransportDelegateImpl delegate{mr_}; - auto& canard_instance = delegate.canardInstance(); - constexpr std::size_t payload_size = 4; - auto* const payload = - static_cast(canard_instance.memory.allocate(static_cast(&delegate), payload_size)); + auto* const payload = static_cast(mr_.allocate(payload_size)); fillIotaBytes({payload, payload_size}, b('0')); CanardMutablePayload canard_payload{payload_size, payload, payload_size}; - CanardMemory old_canard_memory{mr_, canard_payload}; + CanardMemory old_canard_memory{mr_, canard_payload}; EXPECT_THAT(old_canard_memory.size(), payload_size); EXPECT_THAT(canard_payload.size, 0); EXPECT_THAT(canard_payload.data, nullptr); @@ -201,6 +195,46 @@ TEST_F(TestCanDelegate, CanardMemory_copy_on_moved) } } +TEST_F(TestCanDelegate, CanardMemory_forEachFragment) +{ + // Valid payload + { + StrictMock visitor_mock; + + constexpr std::size_t payload_size = 4; + auto* const payload = static_cast(mr_.allocate(payload_size)); + fillIotaBytes({payload, payload_size}, b('0')); + + CanardMutablePayload canard_payload{payload_size, payload, payload_size}; + const CanardMemory canard_memory{mr_, canard_payload}; + + EXPECT_CALL(visitor_mock, onNext(ElementsAre(b('0'), b('1'), b('2'), b('3')))); + canard_memory.forEachFragment(visitor_mock); + } + + // Zero size + { + StrictMock visitor_mock; + + constexpr std::size_t payload_size = 0; + auto* const payload = static_cast(mr_.allocate(payload_size)); + fillIotaBytes({payload, payload_size}, b('0')); + + CanardMutablePayload canard_payload{payload_size, payload, payload_size}; + const CanardMemory canard_memory{mr_, canard_payload}; + canard_memory.forEachFragment(visitor_mock); + } + + // Null + { + StrictMock visitor_mock; + + CanardMutablePayload canard_payload{0, nullptr, 0}; + const CanardMemory canard_memory{mr_, canard_payload}; + canard_memory.forEachFragment(visitor_mock); + } +} + TEST_F(TestCanDelegate, optAnyFailureFromCanard) { EXPECT_THAT(TransportDelegate::optAnyFailureFromCanard(-CANARD_ERROR_OUT_OF_MEMORY), diff --git a/test/unittest/transport/can/test_can_msg_tx_session.cpp b/test/unittest/transport/can/test_can_msg_tx_session.cpp index fb176dfd4..c2360d7a5 100644 --- a/test/unittest/transport/can/test_can_msg_tx_session.cpp +++ b/test/unittest/transport/can/test_can_msg_tx_session.cpp @@ -190,7 +190,7 @@ TEST_F(TestCanMsgTxSession, send_empty_payload) return IMedia::PushResult::Success{false /* is_accepted */}; }); EXPECT_CALL(media_mock_, registerPushCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.deadline = now() + timeout; auto failure = session->send(metadata, empty_payload); diff --git a/test/unittest/transport/can/test_can_svc_tx_sessions.cpp b/test/unittest/transport/can/test_can_svc_tx_sessions.cpp index ff9cdde0a..ccccd5ccf 100644 --- a/test/unittest/transport/can/test_can_svc_tx_sessions.cpp +++ b/test/unittest/transport/can/test_can_svc_tx_sessions.cpp @@ -195,7 +195,7 @@ TEST_F(TestCanSvcTxSessions, send_request) return IMedia::PushResult::Success{true /* is_accepted */}; }); EXPECT_CALL(media_mock_, registerPushCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.deadline = now() + timeout; const auto failure = session->send(metadata, empty_payload); @@ -247,7 +247,7 @@ TEST_F(TestCanSvcTxSessions, send_request_with_argument_error) return IMedia::PushResult::Success{true /* is_accepted */}; }); EXPECT_CALL(media_mock_, registerPushCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.deadline = now() + timeout; const auto failure = session->send(metadata, empty_payload); diff --git a/test/unittest/transport/can/test_can_transport.cpp b/test/unittest/transport/can/test_can_transport.cpp index 842428b82..7f81cee5a 100644 --- a/test/unittest/transport/can/test_can_transport.cpp +++ b/test/unittest/transport/can/test_can_transport.cpp @@ -1066,7 +1066,7 @@ TEST_F(TestCanTransport, receive_svc_responses_with_fallible_oom_canard) EXPECT_CALL(mr_mock, do_reallocate(nullptr, 0, _, _)).WillRepeatedly(Return(nullptr)); #endif - // 1st run: canard RX has failed to accept frame and there is no transient error handler. + // 1st run: canard RX has failed to accept frame, and there is no transient error handler. scheduler_.scheduleAt(1s, [&](const auto&) { // scheduler_.scheduleNamedCallback("rx"); @@ -1075,7 +1075,7 @@ TEST_F(TestCanTransport, receive_svc_responses_with_fallible_oom_canard) EXPECT_THAT(session->receive(), Eq(cetl::nullopt)); }); }); - // 2nd run: canard RX has failed to accept frame and there is "failing" transient error handler. + // 2nd run: canard RX has failed to accept frame, and there is "failing" transient error handler. scheduler_.scheduleAt(2s, [&](const auto&) { // transport->setTransientErrorHandler(std::ref(handler_mock)); @@ -1086,7 +1086,7 @@ TEST_F(TestCanTransport, receive_svc_responses_with_fallible_oom_canard) EXPECT_THAT(session->receive(), Eq(cetl::nullopt)); }); }); - // 3rd run: canard RX has failed to accept frame and there is "success" transient error handler - + // 3rd run: canard RX has failed to accept frame, and there is "success" transient error handler - // the received frame should be just dropped, but overall `run` result should be success (aka ignore OOM). scheduler_.scheduleAt(3s, [&](const auto&) { // diff --git a/test/unittest/transport/scattered_buffer_storage_mock.hpp b/test/unittest/transport/scattered_buffer_storage_mock.hpp index 623bef7d4..7f29e3b92 100644 --- a/test/unittest/transport/scattered_buffer_storage_mock.hpp +++ b/test/unittest/transport/scattered_buffer_storage_mock.hpp @@ -64,6 +64,7 @@ class ScatteredBufferStorageMock : public ScatteredBuffer::IStorage { return (mock_ != nullptr) ? mock_->size() : 0; } + std::size_t copy(const std::size_t offset_bytes, cetl::byte* const destination, const std::size_t length_bytes) const override @@ -71,6 +72,14 @@ class ScatteredBufferStorageMock : public ScatteredBuffer::IStorage return (mock_ != nullptr) ? mock_->copy(offset_bytes, destination, length_bytes) : 0; } + void forEachFragment(ScatteredBuffer::IFragmentsVisitor& visitor) const override + { + if (nullptr != mock_) + { + mock_->forEachFragment(visitor); + } + } + private: ScatteredBufferStorageMock* mock_{nullptr}; @@ -100,9 +109,25 @@ class ScatteredBufferStorageMock : public ScatteredBuffer::IStorage MOCK_METHOD(std::size_t, size, (), (const, noexcept, override)); // NOLINT(bugprone-exception-escape) MOCK_METHOD(std::size_t, copy, (const std::size_t, cetl::byte* const, const std::size_t), (const, override)); + MOCK_METHOD(void, forEachFragment, (ScatteredBuffer::IFragmentsVisitor&), (const, override)); }; // ScatteredBufferStorageMock +class ScatteredBufferVisitorMock : public ScatteredBuffer::IFragmentsVisitor +{ +public: + ScatteredBufferVisitorMock() = default; + virtual ~ScatteredBufferVisitorMock() = default; + + ScatteredBufferVisitorMock(const ScatteredBufferVisitorMock&) = delete; + ScatteredBufferVisitorMock(ScatteredBufferVisitorMock&&) noexcept = delete; + ScatteredBufferVisitorMock& operator=(const ScatteredBufferVisitorMock&) = delete; + ScatteredBufferVisitorMock& operator=(ScatteredBufferVisitorMock&&) noexcept = delete; + + MOCK_METHOD(void, onNext, (const PayloadFragment fragment), (override)); + +}; // ScatteredBufferVisitorMock + } // namespace transport } // namespace libcyphal diff --git a/test/unittest/transport/udp/test_udp_delegate.cpp b/test/unittest/transport/udp/test_udp_delegate.cpp index 989440d16..40cb07c6a 100644 --- a/test/unittest/transport/udp/test_udp_delegate.cpp +++ b/test/unittest/transport/udp/test_udp_delegate.cpp @@ -3,8 +3,10 @@ /// Copyright Amazon.com Inc. or its affiliates. /// SPDX-License-Identifier: MIT +#include "cetl_gtest_helpers.hpp" // NOLINT(misc-include-cleaner) #include "memory_resource_mock.hpp" #include "tracking_memory_resource.hpp" +#include "transport/scattered_buffer_storage_mock.hpp" #include "verification_utilities.hpp" #include @@ -340,6 +342,87 @@ TEST_F(TestUdpDelegate, UdpardMemory_copy_empty) EXPECT_THAT(udpard_memory.copy(1, buffer.data(), 3), 0); } +TEST_F(TestUdpDelegate, UdpardMemory_forEachFragment) +{ + const TransportDelegateImpl delegate{general_mr_, &fragment_mr_, &payload_mr_}; + + // Valid single-fragment payload + { + auto* const payload = allocateNewUdpardPayload(4); + fillIotaBytes({payload, 4}, b('0')); + + constexpr std::size_t payload_size = 4; + UdpardRxTransfer rx_transfer{}; + rx_transfer.payload_size = payload_size; + rx_transfer.payload = UdpardFragment{nullptr, {payload_size, payload}, {payload_size, payload}}; + + const UdpardMemory udpard_memory{delegate.memoryResources(), rx_transfer}; + + StrictMock visitor_mock; + EXPECT_CALL(visitor_mock, onNext(ElementsAre(b('0'), b('1'), b('2'), b('3')))); + udpard_memory.forEachFragment(visitor_mock); + } + + // Valid multi-fragment payload + { + auto* const payload0 = allocateNewUdpardPayload(7); + + UdpardRxTransfer rx_transfer{}; + rx_transfer.payload = UdpardFragment{nullptr, {7, payload0}, {7, payload0}}; + rx_transfer.payload.next = allocateNewUdpardFragment(8); + rx_transfer.payload.next->next = allocateNewUdpardFragment(9); + + auto* const payload1 = static_cast(rx_transfer.payload.next->origin.data); + auto* const payload2 = static_cast(rx_transfer.payload.next->next->origin.data); + fillIotaBytes({payload0, 7}, b('0')); + fillIotaBytes({payload1, 8}, b('A')); + fillIotaBytes({payload2, 9}, b('a')); + + constexpr std::size_t payload_size = 3 + 4 + 2; + rx_transfer.payload_size = payload_size; + rx_transfer.payload.view = {3, payload0 + 2}; + rx_transfer.payload.next->view = {4, payload1 + 1}; + rx_transfer.payload.next->next->view = {2, payload2 + 3}; + + const UdpardMemory udpard_memory{delegate.memoryResources(), rx_transfer}; + + const testing::InSequence in_sequence; + StrictMock visitor_mock; + EXPECT_CALL(visitor_mock, onNext(ElementsAre(b('2'), b('3'), b('4')))); + EXPECT_CALL(visitor_mock, onNext(ElementsAre(b('B'), b('C'), b('D'), b('E')))); + EXPECT_CALL(visitor_mock, onNext(ElementsAre(b('d'), b('e')))); + udpard_memory.forEachFragment(visitor_mock); + } + + // Zero size + { + auto* const payload = allocateNewUdpardPayload(0); + fillIotaBytes({payload, 0}, b('0')); + + constexpr std::size_t payload_size = 0; + UdpardRxTransfer rx_transfer{}; + rx_transfer.payload_size = payload_size; + rx_transfer.payload = UdpardFragment{nullptr, {payload_size, payload}, {payload_size, payload}}; + + const UdpardMemory udpard_memory{delegate.memoryResources(), rx_transfer}; + + StrictMock visitor_mock; + udpard_memory.forEachFragment(visitor_mock); + } + + // Null + { + UdpardRxTransfer rx_transfer{}; + rx_transfer.payload_size = 0; + rx_transfer.payload = UdpardFragment{nullptr, {0, nullptr}, {0, nullptr}}; + + const UdpardMemory udpard_memory{delegate.memoryResources(), rx_transfer}; + + StrictMock visitor_mock; + udpard_memory.forEachFragment(visitor_mock); + } +} + TEST_F(TestUdpDelegate, optAnyFailureFromUdpard) { EXPECT_THAT(TransportDelegate::optAnyFailureFromUdpard(-UDPARD_ERROR_MEMORY), diff --git a/test/unittest/transport/udp/test_udp_msg_tx_session.cpp b/test/unittest/transport/udp/test_udp_msg_tx_session.cpp index 094efdc9f..bb7dfdf09 100644 --- a/test/unittest/transport/udp/test_udp_msg_tx_session.cpp +++ b/test/unittest/transport/udp/test_udp_msg_tx_session.cpp @@ -269,7 +269,7 @@ TEST_F(TestUdpMsgTxSession, send_empty_payload) EXPECT_CALL(tx_socket_mock_, send(_, _, _, _)) .WillOnce(Return(ITxSocket::SendResult::Success{false /* is_accepted */})); EXPECT_CALL(tx_socket_mock_, registerCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.deadline = now() + 1s; auto failure = session->send(metadata, empty_payload); diff --git a/test/unittest/transport/udp/test_udp_svc_tx_sessions.cpp b/test/unittest/transport/udp/test_udp_svc_tx_sessions.cpp index 57b92dd9c..1f693d495 100644 --- a/test/unittest/transport/udp/test_udp_svc_tx_sessions.cpp +++ b/test/unittest/transport/udp/test_udp_svc_tx_sessions.cpp @@ -286,7 +286,7 @@ TEST_F(TestUdpSvcTxSessions, send_empty_payload_request) EXPECT_CALL(tx_socket_mock_, send(_, _, _, _)) .WillOnce(Return(ITxSocket::SendResult::Success{false /* is_accepted */})); EXPECT_CALL(tx_socket_mock_, registerCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.deadline = now() + 1s; auto failure = session->send(metadata, empty_payload); @@ -349,7 +349,7 @@ TEST_F(TestUdpSvcTxSessions, send_empty_payload_responce) EXPECT_CALL(tx_socket_mock_, send(_, _, _, _)) .WillOnce(Return(ITxSocket::SendResult::Success{false /* is_accepted */})); EXPECT_CALL(tx_socket_mock_, registerCallback(_)) // - .WillOnce(Invoke([](auto) { return libcyphal::IExecutor::Callback::Any{}; })); + .WillOnce(Invoke([](const auto&) { return libcyphal::IExecutor::Callback::Any{}; })); metadata.tx_meta.deadline = now() + 1s; auto failure = session->send(metadata, empty_payload); diff --git a/test/unittest/verification_utilities.hpp b/test/unittest/verification_utilities.hpp index 2ae413dc9..bde910182 100644 --- a/test/unittest/verification_utilities.hpp +++ b/test/unittest/verification_utilities.hpp @@ -6,6 +6,8 @@ #ifndef LIBCYPHAL_VERIFICATION_UTILITIES_HPP_INCLUDED #define LIBCYPHAL_VERIFICATION_UTILITIES_HPP_INCLUDED +#include + #include #include @@ -45,20 +47,20 @@ std::array makeIotaArray(const cetl::byte init) } template -std::array, 1> makeSpansFrom(const std::array& payload) +std::array makeSpansFrom(const std::array& payload) { return {payload}; } template -std::array, 2> makeSpansFrom(const std::array& payload1, +std::array makeSpansFrom(const std::array& payload1, const std::array& payload2) { return {payload1, payload2}; } template -static bool tryDeserialize(T& obj, const cetl::span> fragments) +static bool tryDeserialize(T& obj, const transport::PayloadFragments fragments) { std::vector bytes; for (const auto& fragment : fragments)