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

Iox #27 create smart chunk #1081

Merged
merged 13 commits into from
Feb 14, 2022

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Feb 9, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-#123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. Assign PR to reviewer

Notes for Reviewer

Creates SmartChunk, Request and Response according to: https://github.com/eclipse-iceoryx/iceoryx/blob/master/doc/design/request_response_communication.md

In the unit tests only the sample is tested since the Request/ResponseInterface are not yet available. When they become available the typed unit tests will be extended to support their types as well.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff elfenpiff self-assigned this Feb 9, 2022
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #1081 (6efba45) into master (947a850) will increase coverage by 0.86%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
+ Coverage   76.10%   76.96%   +0.86%     
==========================================
  Files         344      346       +2     
  Lines       13030    13172     +142     
  Branches     1869     1885      +16     
==========================================
+ Hits         9916    10138     +222     
+ Misses       2496     2417      -79     
+ Partials      618      617       -1     
Flag Coverage Δ
unittests 76.20% <100.00%> (+0.87%) ⬆️
unittests_timing 12.38% <0.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...eoryx_posh/include/iceoryx_posh/popo/publisher.hpp 100.00% <ø> (ø)
...posh/include/iceoryx_posh/internal/popo/sample.inl 100.00% <100.00%> (+17.14%) ⬆️
...include/iceoryx_posh/internal/popo/smart_chunk.hpp 100.00% <100.00%> (ø)
...include/iceoryx_posh/internal/popo/smart_chunk.inl 100.00% <100.00%> (ø)
iceoryx_posh/source/popo/server_options.cpp 100.00% <0.00%> (ø)
...ceoryx_binding_c/source/cpp2c_enum_translation.cpp 100.00% <0.00%> (ø)
...oofs/include/iceoryx_hoofs/posix_wrapper/timer.hpp 100.00% <0.00%> (ø)
...include/iceoryx_posh/capro/service_description.hpp 100.00% <0.00%> (ø)
...osh/internal/popo/building_blocks/chunk_sender.hpp 100.00% <0.00%> (ø)
...oryx_posh/internal/popo/ports/server_port_user.inl 70.83% <0.00%> (ø)
... and 14 more

Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
elfenpiff and others added 4 commits February 10, 2022 11:53
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <me@elchris.org>
Signed-off-by: Christian Eltzschig <christian.eltzschig@apex.ai>
@elfenpiff elfenpiff linked an issue Feb 11, 2022 that may be closed by this pull request
22 tasks
Signed-off-by: Christian Eltzschig <christian.eltzschig@apex.ai>
@elfenpiff elfenpiff marked this pull request as ready for review February 11, 2022 10:38
@elBoberido elBoberido mentioned this pull request Feb 11, 2022
20 tasks
iceoryx_posh/include/iceoryx_posh/popo/response.hpp Outdated Show resolved Hide resolved
iceoryx_posh/test/moduletests/test_popo_smart_chunk.cpp Outdated Show resolved Hide resolved
template <typename T1>
void setUnderlyingData(const T1& sut, const uint32_t dataValue, const uint64_t headerValue)
{
const_cast<uint32_t&>(sut.chunk.sample()->val) = dataValue;
Copy link
Member

Choose a reason for hiding this comment

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

And there they are, the sins of the past. I should have used data instead of sample for the chunk mock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be easy fixable. Should I rename it from sample to data in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

If you like you can do. I'm not sure if it affects open PRs though.


iox::cxx::optional<iox::Error> detectedError;
auto errorHandlerGuard = iox::ErrorHandler::setTemporaryErrorHandler(
[&detectedError](const iox::Error error, const std::function<void()>&, const iox::ErrorLevel errorLevel) {
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick. I guess this could be simplified to [&detectedError](const auto error, const auto&, const auto errorLevel) {

Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional to use auto only for ErrorLevel?

…n docu, minor code refactoring

Signed-off-by: Christian Eltzschig <christian.eltzschig@apex.ai>
elBoberido
elBoberido previously approved these changes Feb 11, 2022
Copy link
Contributor

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

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

Please add unique ids to the tests.

iceoryx_posh/include/iceoryx_posh/popo/response.hpp Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/popo/request.hpp Outdated Show resolved Hide resolved
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff elfenpiff merged commit ed8c3ad into eclipse-iceoryx:master Feb 14, 2022
@elfenpiff elfenpiff deleted the iox-#27-create-smart-chunk branch February 14, 2022 15:13
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.

Request/Response communication with iceoryx
3 participants