Skip to content

Commit

Permalink
Added MediaPayload::Ownership nested struct #verification #sonar #docs
Browse files Browse the repository at this point in the history
  • Loading branch information
@serges147
serges147 committed Dec 9, 2024
1 parent 9a4424d commit bc19292
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 67 deletions.
6 changes: 3 additions & 3 deletions include/libcyphal/transport/can/can_transport_impl.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
105 changes: 54 additions & 51 deletions include/libcyphal/transport/media_payload.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include <cetl/pf20/cetlpf.hpp>

#include <cstddef>
#include <tuple>
#include <utility>

namespace libcyphal
Expand All @@ -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}
{
}
Expand All @@ -47,29 +76,26 @@ 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.
///
/// @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.
Expand All @@ -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;
}
Expand All @@ -103,7 +127,7 @@ class MediaPayload final
///
cetl::span<const cetl::byte> getSpan() const noexcept
{
return {data_, size_};
return {ownership_.data, ownership_.size};
}

/// Gets size (in bytes) of allocated payload buffer.
Expand All @@ -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<std::size_t, cetl::byte*, std::size_t> 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.
Expand All @@ -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.
///
Expand Down
26 changes: 13 additions & 13 deletions test/unittest/transport/test_media_payload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit bc19292

Please sign in to comment.