Issue/352 single TX copy #407
Conversation
- Added new `IMedia::getTxMemoryResource` method. - Media implementations (at examples) now have their own TX memory resource. - Addopted recent v4 canard api changes. - each `canardTxInit` now initialized with media TX memory resource - addressed "// TODO: Remove this workaround when the issue is resolved." - canard memory management now has de-allocation amount parameter. - Extended all transport related unit tests to verify that TX pipeline uses its dedicated memory resource.
- Introduced `libcyphal::transport::MediaPayload` - in use to pass payload data between the transport layer and its media. - Use lates latest Canard v4 api
- latest canard v4 - latest udpard v2
| /// In use to pass payload data between the transport layer and its media. | ||
| /// It also manages memory ownership of the allocated payload buffer. | ||
| /// | ||
| class MediaPayload final // NOSONAR : cpp:S4963 - we do directly handle resources here. |
There was a problem hiding this comment.
https://next.sonarqube.com/sonarqube/coding_rules?open=cpp%3AS4963&rule_key=cpp%3AS4963 says this rule is deprecated. Should we be updating our scanner to use https://next.sonarqube.com/sonarqube/coding_rules?rule_key=cpp%3AS3624&selected=cpp%3AS3624 instead?
There was a problem hiding this comment.
I'll try. Thanks for noticing it!
There was a problem hiding this comment.
It worked, but if you don't mind I would like make these changes (in all places) in the next pr.
| public: | ||
| /// Constructs a new empty payload. | ||
| /// | ||
| MediaPayload() = default; |
There was a problem hiding this comment.
Violates AUTOSAR A12-1-2 since the move and copy constructor initialize data members.
| @@ -66,9 +67,16 @@ class IMedia | |||
|
|
|||
| /// @brief Schedules the frame for transmission asynchronously and return immediately. | |||
| /// | |||
| /// Concrete media implementation has multiple options with how to handle `payload` buffer: | |||
| /// - just copy the buffer data byts (using `payload.getSpan` const method) and return without changing the payload; | |||
There was a problem hiding this comment.
| /// - just copy the buffer data byts (using `payload.getSpan` const method) and return without changing the payload; | |
| /// - just copy the buffer data bytes (using `payload.getSpan` const method) and return without changing the payload; |
|
| /// 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 |
There was a problem hiding this comment.
Why a tuple and not a named structure? The user will have to remember which element is the allocated size instead of it being named.
There was a problem hiding this comment.
ok, I'll create separate nested Ownership type, like following:
class MediaPayload final
{
public:
/// Structure 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).
/// See also `release` method.
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
...
Ownership release() noexcept
{
...
These changes (along with necessary changes at lizards) make TX pipeline as "low copy" (see issue #352).
IMedia::getTxMemoryResource()memory resource.MediaPayloadpayloads, which are capable to transfer payload memory ownership (f.e. back to the media) - so now it's possible f.e. implement usage of CAN memory hardware for transmission.