diff --git a/include/libcyphal/transport/can/can_transport_impl.hpp b/include/libcyphal/transport/can/can_transport_impl.hpp index f54e8d01d..04cc276a6 100644 --- a/include/libcyphal/transport/can/can_transport_impl.hpp +++ b/include/libcyphal/transport/can/can_transport_impl.hpp @@ -601,9 +601,9 @@ class TransportImpl final : private TransportDelegate, public ICanTransport // Media has not accepted the frame, so we need return original payload back to the item, // so that in the future potential retry could try to push it again. const auto org_payload = payload.release(); - frame.payload.size = std::get<0>(org_payload); - frame.payload.data = std::get<1>(org_payload); - frame.payload.allocated_size = std::get<2>(org_payload); + frame.payload.size = org_payload.size; + frame.payload.data = org_payload.data; + frame.payload.allocated_size = org_payload.allocated_size; } // If needed schedule (recursively!) next frame to push. diff --git a/include/libcyphal/transport/media_payload.hpp b/include/libcyphal/transport/media_payload.hpp index e02619053..8dea6ba1f 100644 --- a/include/libcyphal/transport/media_payload.hpp +++ b/include/libcyphal/transport/media_payload.hpp @@ -10,7 +10,6 @@ #include #include -#include #include namespace libcyphal @@ -26,12 +25,42 @@ namespace transport class MediaPayload final { public: + /// Structure with the payload size, pointer to the payload data, and the allocated size. + /// + /// NB! This structure (in contrast to the parent `MediaPayload` type) is intended for raw (unmanaged) and + /// explicit transfer of payload ownership out of the `MediaPayload` instance (see `release` method). + /// It's the caller's responsibility to deallocate the buffer with the correct memory resource, + /// or move it somewhere else with the same guarantee (like f.e. back to a lizard TX queue item). + /// If you just need to access the payload data (without owning it), use `getSpan` method instead. + /// + struct Ownership + { + /// Size of the payload data in bytes. + /// + /// Could be less or equal to the allocated size. + /// `0` when the payload is moved. + /// + std::size_t size; + + /// Pointer to the payload buffer. + /// + /// `nullptr` when the payload is moved. + /// + cetl::byte* data; + + /// Size of the allocated buffer. + /// + /// Could be greater or equal to the payload size. + /// `0` when the payload is moved. + /// + std::size_t allocated_size; + + }; // Ownership + /// Constructs a new empty payload. /// MediaPayload() - : size_{0U} - , data_{nullptr} - , allocated_size_{0U} + : ownership_{0U, nullptr, 0U} , mr_{nullptr} { } @@ -47,14 +76,12 @@ class MediaPayload final cetl::byte* const data, const std::size_t allocated_size, cetl::pmr::memory_resource* const mr) - : size_{size} - , data_{data} - , allocated_size_{allocated_size} + : ownership_{size, data, allocated_size} , mr_{mr} { - CETL_DEBUG_ASSERT(size_ <= allocated_size_, ""); - CETL_DEBUG_ASSERT((data_ == nullptr) || (mr_ != nullptr), ""); - CETL_DEBUG_ASSERT((data_ != nullptr) || ((size_ == 0) && (allocated_size_ == 0)), ""); + CETL_DEBUG_ASSERT(size <= allocated_size, ""); + CETL_DEBUG_ASSERT((data == nullptr) || (mr_ != nullptr), ""); + CETL_DEBUG_ASSERT((data != nullptr) || ((size == 0) && (allocated_size == 0)), ""); } /// Moves another payload into this new payload instance. @@ -62,14 +89,13 @@ class MediaPayload final /// @param other The other payload to move into. /// MediaPayload(MediaPayload&& other) noexcept - : size_{std::exchange(other.size_, 0U)} - , data_{std::exchange(other.data_, nullptr)} - , allocated_size_{std::exchange(other.allocated_size_, 0U)} + : ownership_{std::exchange(other.ownership_, {0U, nullptr, 0U})} , mr_{std::exchange(other.mr_, nullptr)} { - CETL_DEBUG_ASSERT(size_ <= allocated_size_, ""); - CETL_DEBUG_ASSERT((data_ == nullptr) || (mr_ != nullptr), ""); - CETL_DEBUG_ASSERT((data_ != nullptr) || ((size_ == 0) && (allocated_size_ == 0)), ""); + CETL_DEBUG_ASSERT(ownership_.size <= ownership_.allocated_size, ""); + CETL_DEBUG_ASSERT((ownership_.data == nullptr) || (mr_ != nullptr), ""); + CETL_DEBUG_ASSERT((ownership_.data != nullptr) || ((ownership_.size == 0) && (ownership_.allocated_size == 0)), + ""); } /// @brief Assigns another payload by moving it into this one. @@ -80,10 +106,8 @@ class MediaPayload final { reset(); - size_ = std::exchange(other.size_, 0U); - data_ = std::exchange(other.data_, nullptr); - allocated_size_ = std::exchange(other.allocated_size_, 0U); - mr_ = std::exchange(other.mr_, nullptr); + ownership_ = std::exchange(other.ownership_, {0U, nullptr, 0U}); + mr_ = std::exchange(other.mr_, nullptr); return *this; } @@ -103,7 +127,7 @@ class MediaPayload final /// cetl::span getSpan() const noexcept { - return {data_, size_}; + return {ownership_.data, ownership_.size}; } /// Gets size (in bytes) of allocated payload buffer. @@ -112,24 +136,23 @@ class MediaPayload final /// std::size_t getAllocatedSize() const noexcept { - return allocated_size_; + return ownership_.allocated_size; } /// Releases ownership of the payload by returning its data pointer and sizes. /// /// In use to return the payload to the lizard C API when media is not ready yet to accept the payload. /// After this call, corresponding internal fields will be nullified. + /// If you just need to access the payload data (without owning it), use `getSpan` method instead. /// /// @return Tuple with the payload size, pointer to the payload data, and the allocated size. /// It's the caller's responsibility to deallocate the buffer with the correct memory resource, /// or move it somewhere else with the same guarantee (like f.e. back to a lizard TX queue item). /// - std::tuple release() noexcept + Ownership release() noexcept { mr_ = nullptr; - return std::make_tuple(std::exchange(size_, 0U), - std::exchange(data_, nullptr), - std::exchange(allocated_size_, 0U)); + return std::exchange(ownership_, {0U, nullptr, 0U}); } /// Resets the payload by de-allocating its data buffer. @@ -138,40 +161,20 @@ class MediaPayload final /// void reset() noexcept { - if (data_ != nullptr) + if (ownership_.data != nullptr) { CETL_DEBUG_ASSERT(mr_ != nullptr, "Memory resource should not be null."); // No Sonar `cpp:S5356` b/c we integrate here PMR. - mr_->deallocate(data_, allocated_size_); // NOSONAR cpp:S5356 + mr_->deallocate(ownership_.data, ownership_.allocated_size); // NOSONAR cpp:S5356 - mr_ = nullptr; - data_ = nullptr; - size_ = 0; - allocated_size_ = 0; + mr_ = nullptr; + ownership_ = {0U, nullptr, 0U}; } } private: - /// Size of the payload data in bytes. - /// - /// Could be less or equal to the allocated size. - /// `0` when the payload is moved. - /// - std::size_t size_; - - /// Pointer to the payload buffer. - /// - /// `nullptr` when the payload is moved. - /// - cetl::byte* data_; - - /// Size of the allocated buffer. - /// - /// Could be greater or equal to the payload size. - /// `0` when the payload is moved. - /// - std::size_t allocated_size_; + Ownership ownership_; /// Holds pointer to the PMR which was used to allocate the payload buffer. Will be used to deallocate it. /// diff --git a/test/unittest/transport/test_media_payload.cpp b/test/unittest/transport/test_media_payload.cpp index 8f7a8de16..bde1b5de1 100644 --- a/test/unittest/transport/test_media_payload.cpp +++ b/test/unittest/transport/test_media_payload.cpp @@ -52,10 +52,10 @@ TEST_F(TestMediaPayload, default_ctor) EXPECT_THAT(payload.getAllocatedSize(), 0); // It's fine to attempt to reset or release an empty payload. - const auto fields = payload.release(); - EXPECT_THAT(std::get<0>(fields), 0); - EXPECT_THAT(std::get<1>(fields), nullptr); - EXPECT_THAT(std::get<2>(fields), 0); + const auto ownership = payload.release(); + EXPECT_THAT(ownership.size, 0); + EXPECT_THAT(ownership.data, nullptr); + EXPECT_THAT(ownership.allocated_size, 0); payload.reset(); } @@ -111,16 +111,16 @@ TEST_F(TestMediaPayload, release) MediaPayload payload{payload_size, payload_data, payload_allocated_size, &mr_}; - auto fields = payload.release(); - EXPECT_THAT(std::get<0>(fields), payload_size); - EXPECT_THAT(std::get<1>(fields), payload_data); - EXPECT_THAT(std::get<2>(fields), payload_allocated_size); - mr_.deallocate(std::get<1>(fields), std::get<2>(fields)); + auto ownership = payload.release(); + EXPECT_THAT(ownership.size, payload_size); + EXPECT_THAT(ownership.data, payload_data); + EXPECT_THAT(ownership.allocated_size, payload_allocated_size); + mr_.deallocate(ownership.data, ownership.allocated_size); - fields = payload.release(); - EXPECT_THAT(std::get<0>(fields), 0); - EXPECT_THAT(std::get<1>(fields), nullptr); - EXPECT_THAT(std::get<2>(fields), 0); + ownership = payload.release(); + EXPECT_THAT(ownership.size, 0); + EXPECT_THAT(ownership.data, nullptr); + EXPECT_THAT(ownership.allocated_size, 0); } TEST_F(TestMediaPayload, reset)