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

Implement the RX pipeline #43

Merged
merged 38 commits into from
Aug 9, 2023
Merged

Implement the RX pipeline #43

merged 38 commits into from
Aug 9, 2023

Conversation

pavel-kirienko
Copy link
Member

@pavel-kirienko pavel-kirienko commented Aug 2, 2023

  • Implement the common parts of the RX pipeline. At this point, about 90% of the work is done. All that is left is to add the basic public API wrappers for subscriptions and RPC-services. For details about the implementation of the RX pipeline refer to the line that says "Internally, the RX pipeline is arranged as follows".

  • Slightly refactor the code added earlier to uphold orthogonality and avoid unnecessary duplication.

  • Trivial renamings to keep names shorter, e.g.:

    • UdpardConstPayload -> UdpardPayload (const implied by default; there's also UdpardMutablePayload)
    • UdpardPayloadFragmentHandle -> UdpardFragment
  • Establish an internal convention that functions named "Destroy" deallocate the passed memory while "Free" only deallocate the memory of the associated resources. This is not API-visible.

  • Remove the port_id field from UdpardRxPort because it is not needed.

After this is merged, the remaining task is to simply implement the public RX pipeline functions on top of:

  • rxPortInit
  • rxPortAcceptFrame
  • rxPortFree

The test coverage is supposed to be full but llvm-cov slightly misreports it.

@pavel-kirienko pavel-kirienko self-assigned this Aug 2, 2023
@pavel-kirienko pavel-kirienko marked this pull request as ready for review August 2, 2023 16:05
@thirtytwobits
Copy link
Member

👀

Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

Nothing staggering in my review but a few points I'd like to discuss.

libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
/// required context to compute the size, or if the memory resource implementation does not require deallocation size.
///
/// The head of the fragment list is passed by value so it is not freed. This is in line with the UdpardRxTransfer
/// design, where the head is stored by value to reduce indirection in small transfers. We call it Scott's Head.
Copy link
Member

Choose a reason for hiding this comment

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

7v4wsn

libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
Copy link
Member

@thirtytwobits thirtytwobits left a comment

Choose a reason for hiding this comment

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

Only style feedback at this point.

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

92.2% 92.2% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.17) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@pavel-kirienko pavel-kirienko merged commit c92f8df into main2 Aug 9, 2023
24 checks passed
@pavel-kirienko pavel-kirienko deleted the pavel branch August 9, 2023 13:20
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.

None yet

2 participants