Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

canardTxPeek now returns mutable item #236

Merged
merged 8 commits into from
Nov 27, 2024
Merged

Conversation

serges147
Copy link
Contributor

canardTxPeek now returns mutable item - needed for payload ownership transfer.

@serges147 serges147 self-assigned this Nov 26, 2024
@serges147 serges147 marked this pull request as ready for review November 26, 2024 17:41
/// The memory resource used by this queue for allocating the enqueued items (CAN frames).
/// There is exactly one allocation per enqueued item, each allocation contains both the CanardTxQueueItem
/// and its payload, hence the size is variable.
/// The memory resource used by this queue for allocating payload data (CAN frames).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the

(also check "with additional amount parameter" above)

Copy link
Contributor Author

@serges147 serges147 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the

Sorry, did not get exactly what is missing for "the" comment. Could you please help?

The amount will be fixed by -> size - thnx!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the "the" article is missing

/// There is exactly one allocation per enqueued item, each allocation contains both the CanardTxQueueItem
/// and its payload, hence the size is variable.
/// The memory resource used by this queue for allocating payload data (CAN frames).
/// There is exactly one allocation per enqueued item. Its size is equal to the MTU of the queue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first sentence is no longer applicable -- there are two allocations, not one. I would remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this sentence specifically about this memory resource of the CanardTxQueue - from it we allocate once per enqueued item (not considering item itself b/c it is allocated from the instance resource).

What about the following variant?

/// The memory resource used by this queue for allocating payload data (CAN frames).
/// There is exactly one allocation of payload buffer per enqueued item (not considering the item itself
/// b/c it is allocated from different memory resource - the instance one; see CanardInstance::memory).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

/// If the application knows its MTU, it can use block allocation to avoid extrinsic fragmentation,
/// as well as a dedicated memory pool specifically for the TX queue payload for transmission.
/// Dedicated memory resources could be useful also for systems with special memory requirements for payload data.
/// For example, such a memory resource could be integrated with a peripheral DMA controller. So that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// For example, such a memory resource could be integrated with a peripheral DMA controller. So that
/// For example, such a memory resource could be integrated with the CAN message RAM. So that

This is more representative of the actual state of affairs with CAN FD controllers. Usually you have a separate memory region that is accessible both to the core and to the CAN controller peripheral. You find a free slot (usually with the help of a separate register), write the message into it, then signal the controller when it's ready to go.

/// Dedicated memory resources could be useful also for systems with special memory requirements for payload data.
/// For example, such a memory resource could be integrated with a peripheral DMA controller. So that
/// memory is allocated directly in the peripheral's memory space. Then it will be filled with payload data by
/// the Canard, and finally it will be ready to be directly transmitted by DMA HW (avoiding the need for copying).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// the Canard, and finally it will be ready to be directly transmitted by DMA HW (avoiding the need for copying).
/// the Canard, and finally it will be ready to be directly transmitted by the HW (avoiding the need for copying).

@@ -539,28 +553,43 @@ int32_t canardTxPush(CanardTxQueue* const que,
///
/// If the queue is non-empty, the returned value is a pointer to its top element (i.e., the next frame to transmit).
/// The returned pointer points to an object allocated in the dynamic storage; it should be eventually freed by the
/// application by calling CanardInstance::memory_free(). The memory shall not be freed before the entry is removed
/// application by calling `udpardTxFree`. The memory shall not be freed before the entry is removed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely you mean canard

/// as well as the internal frame payload buffer (if any) associated with it (using TX queue memory).
/// If the item argument is NULL, the function has no effect. The time complexity is constant.
/// If the item frame payload is NULL then it is assumed that the payload buffer was already freed,
/// or moved to different ownership (f.e. to media layer).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// or moved to different ownership (f.e. to media layer).
/// or moved to a different owner (f.e. to media layer).

@serges147 serges147 merged commit 60f22e2 into v4 Nov 27, 2024
17 checks passed
@serges147 serges147 deleted the sshirokov/v4_zero_tx_copy branch November 27, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants