diff --git a/include/libcyphal/presentation/client_impl.hpp b/include/libcyphal/presentation/client_impl.hpp index 7330a75bf..07c5cb473 100644 --- a/include/libcyphal/presentation/client_impl.hpp +++ b/include/libcyphal/presentation/client_impl.hpp @@ -252,7 +252,7 @@ class SharedClient : public common::cavl::Node, if (auto* const transfer_id_map = delegate_.getTransferIdMap()) { const SessionSpec session_spec{response_rx_params_.service_id, response_rx_params_.server_node_id}; - transfer_id_map->setIdFor(next_transfer_id_, session_spec); + transfer_id_map->setIdFor(session_spec, next_transfer_id_); } delegate_.forgetSharedClient(*this); diff --git a/include/libcyphal/presentation/publisher_impl.hpp b/include/libcyphal/presentation/publisher_impl.hpp index 113b62533..e22717751 100644 --- a/include/libcyphal/presentation/publisher_impl.hpp +++ b/include/libcyphal/presentation/publisher_impl.hpp @@ -106,7 +106,7 @@ class PublisherImpl final : public common::cavl::Node, public Sha if (const auto local_node_id = delegate_.getLocalNodeId()) { const SessionSpec session_spec{subject_id_, local_node_id.value()}; - transfer_id_map->setIdFor(next_transfer_id_, session_spec); + transfer_id_map->setIdFor(session_spec, next_transfer_id_); } } diff --git a/include/libcyphal/transport/transfer_id_map.hpp b/include/libcyphal/transport/transfer_id_map.hpp index 05ff91ad5..29638503b 100644 --- a/include/libcyphal/transport/transfer_id_map.hpp +++ b/include/libcyphal/transport/transfer_id_map.hpp @@ -63,7 +63,7 @@ class ITransferIdMap /// Depending on the implementation, the previously set transfer ids may be stored in memory /// or also persisted to a file (but probably on exit to fulfill the above performance expectations). /// - virtual void setIdFor(const TransferId transfer_id, const SessionSpec& session_spec) noexcept = 0; + virtual void setIdFor(const SessionSpec& session_spec, const TransferId transfer_id) noexcept = 0; protected: ITransferIdMap() = default; @@ -108,36 +108,6 @@ class ITransferIdStorage }; // ITransferIdStorage -/// Default implementation of the transfer ID storage. -/// -/// It just stores given transfer id in memory. -/// In use by the Presentation layer entities as a default fallback behavior. -/// -class DefaultTransferIdStorage final : public ITransferIdStorage -{ -public: - explicit DefaultTransferIdStorage(const TransferId initial_transfer_id = 0) noexcept - : transfer_id_{initial_transfer_id} - { - } - - // ITransferIdMap - - TransferId load() const noexcept override - { - return transfer_id_; - } - - void save(const TransferId transfer_id) noexcept override - { - transfer_id_ = transfer_id; - } - -private: - TransferId transfer_id_; - -}; // DefaultTransferIdStorage - /// @brief Defines a trivial transfer ID generator. /// /// The generator is trivial in the sense that it simply increments the transfer ID. diff --git a/test/unittest/presentation/test_client.cpp b/test/unittest/presentation/test_client.cpp index 558516417..a5e5a2b06 100644 --- a/test/unittest/presentation/test_client.cpp +++ b/test/unittest/presentation/test_client.cpp @@ -11,6 +11,7 @@ #include "tracking_memory_resource.hpp" #include "transport/scattered_buffer_storage_mock.hpp" #include "transport/svc_sessions_mock.hpp" +#include "transport/transfer_id_map_mock.hpp" #include "transport/transport_gtest_helpers.hpp" #include "transport/transport_mock.hpp" #include "virtual_time_scheduler.hpp" @@ -23,6 +24,7 @@ #include #include #include +#include #include #include @@ -789,7 +791,7 @@ TEST_F(TestClient, raw_request_response_failures) State state{mr_, transport_mock_, rx_params}; // Emulate that transport supports only 2 concurrent transfers by having module equal to 2^1. - // This will make the client fail to make more than 2 request. + // This will make the client fail to make more than 2 requests. EXPECT_CALL(transport_mock_, getProtocolParams()).WillRepeatedly(Return(ProtocolParams{2, 0, 0})); Presentation presentation{mr_, scheduler_, transport_mock_}; @@ -829,6 +831,84 @@ TEST_F(TestClient, raw_request_response_failures) scheduler_.spinFor(10s); } +TEST_F(TestClient, multiple_clients_with_transfer_id_map) +{ + using SvcResPromise = ResponsePromise; + using SessionSpec = ITransferIdMap::SessionSpec; + + StrictMock transfer_id_map_mock; + + constexpr ResponseRxParams rx_params_n31{4, 147, 0x31}; + constexpr ResponseRxParams rx_params_n32{4, 147, 0x32}; + + State state_31{mr_, transport_mock_, rx_params_n31}; + State state_32{mr_, transport_mock_, rx_params_n32}; + + Presentation presentation{mr_, scheduler_, transport_mock_}; + presentation.setTransferIdMap(&transfer_id_map_mock); + + EXPECT_CALL(transfer_id_map_mock, getIdFor(SessionSpec{rx_params_n31.service_id, rx_params_n31.server_node_id})) + .WillOnce(Return(0x310)); + EXPECT_CALL(transfer_id_map_mock, getIdFor(SessionSpec{rx_params_n32.service_id, rx_params_n32.server_node_id})) + .WillOnce(Return(0x320)); + + auto maybe_client_n31 = presentation.makeClient( // + rx_params_n31.server_node_id, + rx_params_n31.service_id, + rx_params_n31.extent_bytes); + ASSERT_THAT(maybe_client_n31, VariantWith(_)); + cetl::optional client_n31 = cetl::get(std::move(maybe_client_n31)); + + auto maybe_client_n32 = presentation.makeClient( // + rx_params_n32.server_node_id, + rx_params_n32.service_id, + rx_params_n32.extent_bytes); + ASSERT_THAT(maybe_client_n32, VariantWith(_)); + cetl::optional client_n32 = cetl::get(std::move(maybe_client_n32)); + + scheduler_.scheduleAt(1s, [&](const auto&) { + // + EXPECT_CALL(state_31.req_tx_session_mock_, send(_, _)) // + .WillOnce(Invoke([now = now()](const auto& metadata, const auto) { + // + EXPECT_THAT(metadata.base.transfer_id, 0x310); + return cetl::nullopt; + })); + + const auto maybe_promise = client_n31->request(now() + 100ms, {}, now() + 2s); + EXPECT_THAT(maybe_promise, VariantWith(_)); + }); + scheduler_.scheduleAt(2s, [&](const auto&) { + // + EXPECT_CALL(state_32.req_tx_session_mock_, send(_, _)) // + .WillOnce(Invoke([now = now()](const auto& metadata, const auto) { + // + EXPECT_THAT(metadata.base.transfer_id, 0x320); + return cetl::nullopt; + })); + + const auto maybe_promise = client_n32->request(now() + 100ms, {}, now() + 2s); + EXPECT_THAT(maybe_promise, VariantWith(_)); + }); + scheduler_.scheduleAt(8s, [&](const auto&) { + // + EXPECT_CALL(transfer_id_map_mock, + setIdFor(SessionSpec{rx_params_n32.service_id, rx_params_n32.server_node_id}, 0x320 + 1)) + .WillOnce(Return()); + + client_n32.reset(); + }); + scheduler_.scheduleAt(9s, [&](const auto&) { + // + EXPECT_CALL(transfer_id_map_mock, + setIdFor(SessionSpec{rx_params_n31.service_id, rx_params_n31.server_node_id}, 0x310 + 1)) + .WillOnce(Return()); + + client_n31.reset(); + }); + scheduler_.spinFor(10s); +} + // NOLINTEND(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers, *-function-cognitive-complexity) } // namespace diff --git a/test/unittest/transport/test_transfer_id_map.cpp b/test/unittest/transport/test_transfer_id_map.cpp index 5da5fe164..ca0a6b5c6 100644 --- a/test/unittest/transport/test_transfer_id_map.cpp +++ b/test/unittest/transport/test_transfer_id_map.cpp @@ -27,7 +27,34 @@ using testing::StrictMock; // NOLINTBEGIN(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers) class TestTransferIdMapAndGens : public testing::Test -{}; +{ +protected: + class TransferIdStorage final : public detail::ITransferIdStorage + { + public: + explicit TransferIdStorage(const TransferId initial_transfer_id = 0) noexcept + : transfer_id_{initial_transfer_id} + { + } + + // detail::ITransferIdStorage + + TransferId load() const noexcept override + { + return transfer_id_; + } + + void save(const TransferId transfer_id) noexcept override + { + transfer_id_ = transfer_id; + } + + private: + TransferId transfer_id_; + + }; // TransferIdStorage + +}; // TestTransferIdMapAndGens // MARK: - Tests: @@ -77,11 +104,11 @@ TEST_F(TestTransferIdMapAndGens, trivial_max_tf_id) TEST_F(TestTransferIdMapAndGens, small_range_with_default_map) { - detail::DefaultTransferIdStorage default_transfer_id_storage{9}; + TransferIdStorage transfer_id_storage{9}; // on purpose bigger than modulo 4 - detail::SmallRangeTransferIdGenerator<8> tf_id_gen{4, default_transfer_id_storage}; + detail::SmallRangeTransferIdGenerator<8> tf_id_gen{4, transfer_id_storage}; - EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(1)); + EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(1)); // 9 % 4 -> 1 EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(2)); EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(3)); EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(0)); diff --git a/test/unittest/transport/transfer_id_map_mock.hpp b/test/unittest/transport/transfer_id_map_mock.hpp new file mode 100644 index 000000000..71e3ba490 --- /dev/null +++ b/test/unittest/transport/transfer_id_map_mock.hpp @@ -0,0 +1,37 @@ +/// @copyright +/// Copyright (C) OpenCyphal Development Team +/// Copyright Amazon.com Inc. or its affiliates. +/// SPDX-License-Identifier: MIT + +#ifndef LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED +#define LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED + +#include + +#include + +namespace libcyphal +{ +namespace transport +{ + +class TransferIdMapMock : public ITransferIdMap +{ +public: + TransferIdMapMock() = default; + virtual ~TransferIdMapMock() = default; + + TransferIdMapMock(const TransferIdMapMock&) = delete; + TransferIdMapMock(TransferIdMapMock&&) noexcept = delete; + TransferIdMapMock& operator=(const TransferIdMapMock&) = delete; + TransferIdMapMock& operator=(TransferIdMapMock&&) noexcept = delete; + + MOCK_METHOD(TransferId, getIdFor, (const SessionSpec& session_spec), (const, noexcept, override)); + MOCK_METHOD(void, setIdFor, (const SessionSpec& session_spec, const TransferId transfer_id), (noexcept, override)); + +}; // TransferIdMapMock + +} // namespace transport +} // namespace libcyphal + +#endif // LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED