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 subscription API functions #46

Merged
merged 6 commits into from
Aug 10, 2023
Merged

Implement subscription API functions #46

merged 6 commits into from
Aug 10, 2023

Conversation

pavel-kirienko
Copy link
Member

It is now possible to subscribe to subjects and receive data from them. The only missing bit is the RPC services API, all other functionality of the library is already implemented and can be used.

This changeset also brings udpardGather that, well, gathers a fragmented payload buffer into one contiguous chunk of memory that can be passed into Nunavut. Later we should amend Nunavut to support fragmented buffers directly to eliminate extra data copying and memory utilization.

After the RPC services API is done, I am going to add comprehensive end-to-end tests, for which there is a stub already. Aside from that we also have this (the e2e test mentioned there is different though): #35

This changeset also fixes an internal documentation error that provided an incorrect estimation of the worst-case recursion depth.

@pavel-kirienko pavel-kirienko self-assigned this Aug 9, 2023
@pavel-kirienko pavel-kirienko marked this pull request as ready for review August 9, 2023 17:10
@pavel-kirienko pavel-kirienko requested review from thirtytwobits and lydia-at-amazon and removed request for thirtytwobits August 9, 2023 17:10
libudpard/udpard.h Show resolved Hide resolved
std::array<std::uint8_t, 1024> mono{};

// Copy full size payload.
std::generate(mono.begin(), mono.end(), [] { return std::rand() % 256; });
Copy link
Member

Choose a reason for hiding this comment

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

To generate random test bits use std::independent_bits_engine and be sure to provide a stable seed so the tests are reproducible. I tend to use googletest, value-parameterized for this where I run the test using a few seeds to really get the psedo-random mojo going but I'm fine with just using unity here and a single seed value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I overlooked the seeding part. However, I think it would be unwise to use the C++ RNG utilities here because this is a C library and most of the test code is written in C, so in the interest of consistency and compatibility with future tests written in C, it is desirable to use rand(). The quality of the random numbers matters little in this case.

I implemented seeding in the last commit.

UDPARD_ASSERT(frag->view.data != NULL);
const size_t frag_size = smaller(frag->view.size, destination_size - offset);
// NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
(void) memmove(((byte_t*) destination) + offset, frag->view.data, frag_size);
Copy link
Member

Choose a reason for hiding this comment

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

Do we define byte_t as unsigned char? I forgot if this was a local typedef or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

libudpard/udpard.c Outdated Show resolved Hide resolved
… C RNG utilities because this is a C library and most of the test code is written in C.
@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 0 Code Smells

96.6% 96.6% 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 c03b3ee into main2 Aug 10, 2023
24 checks passed
@pavel-kirienko pavel-kirienko deleted the pavel branch August 10, 2023 14:23
Copy link
Collaborator

@lydia-at-amazon lydia-at-amazon left a comment

Choose a reason for hiding this comment

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

Sorry for the very late review, looks good! Excited to see the final API implemented and glad we added the udpardGather function. Can't approve anymore, can only give you two thumbs up: 👍 👍

tests/src/test_intrusive_rx.c Show resolved Hide resolved
tests/src/test_intrusive_rx.c Show resolved Hide resolved
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