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

[#129] fix missing connections #134

Merged

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Feb 23, 2024

Notes for Reviewer

The UsedChunkList exploits that the publisher/zero copy connection combination is using a pool allocator with equi-distant offset (buckets of same size). Therefore, we introduced number of samples and sample size.
We translate the memory addresses into indices (based on the above information) and set atomic bools to indicate if a sample is in use or not.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Relates to #116
Closes #129 #130 #131 #132

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 87.26287% with 94 lines in your changes are missing coverage. Please review.

Project coverage is 77.61%. Comparing base (3d99f1d) to head (e35af3f).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   77.40%   77.61%   +0.20%     
==========================================
  Files         179      180       +1     
  Lines       19668    19943     +275     
==========================================
+ Hits        15224    15478     +254     
- Misses       4444     4465      +21     
Files Coverage Δ
iceoryx2-bb/elementary/src/allocator.rs 92.59% <ø> (+3.30%) ⬆️
iceoryx2-bb/elementary/src/math.rs 61.76% <100.00%> (+1.15%) ⬆️
iceoryx2-bb/memory/src/bump_allocator.rs 80.88% <100.00%> (-0.55%) ⬇️
iceoryx2-bb/memory/src/heap_allocator.rs 89.74% <100.00%> (-0.26%) ⬇️
iceoryx2-bb/memory/src/pool_allocator.rs 90.04% <100.00%> (-0.31%) ⬇️
iceoryx2-bb/posix/src/file.rs 66.50% <ø> (+0.15%) ⬆️
iceoryx2-cal/src/dynamic_storage/process_local.rs 95.51% <100.00%> (+0.32%) ⬆️
iceoryx2-cal/src/shared_memory/mod.rs 100.00% <ø> (ø)
iceoryx2-cal/src/shared_memory/posix.rs 88.14% <100.00%> (+0.15%) ⬆️
iceoryx2-cal/src/shared_memory_directory/mod.rs 90.36% <100.00%> (-0.06%) ⬇️
... and 29 more

... and 5 files with indirect coverage changes

@elfenpiff elfenpiff force-pushed the iox2-129-fix-missing-connections branch from c073654 to 7d9ce08 Compare February 23, 2024 10:08
@elfenpiff elfenpiff changed the title Iox2 129 fix missing connections [#129] fix missing connections Feb 25, 2024
@elfenpiff elfenpiff requested a review from elBoberido February 26, 2024 23:41
@elfenpiff elfenpiff force-pushed the iox2-129-fix-missing-connections branch 10 times, most recently from 850a2de to 280002c Compare March 3, 2024 23:48
.cirrus.yml Show resolved Hide resolved
@elfenpiff elfenpiff force-pushed the iox2-129-fix-missing-connections branch from 3a38661 to be5af47 Compare March 5, 2024 14:29
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

9 files to go. Looks good so far.

.cirrus.yml Outdated Show resolved Hide resolved
iceoryx2-bb/memory/src/bump_allocator.rs Outdated Show resolved Hide resolved
iceoryx2-bb/memory/tests/one_chunk_allocator_tests.rs Outdated Show resolved Hide resolved
};

if let Err(e) = new_self.populate_listener_channels() {
warn!(from new_self, "The new Notifier port is unable to connect to every Listener port, caused by {:?}.", e);
}

std::sync::atomic::compiler_fence(Ordering::SeqCst);
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. After reading the documentation, a few question marks remain. Especially since the example should be broken on a weakly ordered CPU

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Only the mpmc::Container left but thats something for tomorrow

…e start into start_address for allocators; rename receive/retrieve channel into submission/completion channel; code cleanup
@elfenpiff elfenpiff requested a review from elBoberido March 6, 2024 08:57
@elfenpiff elfenpiff requested a review from elBoberido March 6, 2024 22:44
…instead of acquiring it from the base address
@elfenpiff elfenpiff force-pushed the iox2-129-fix-missing-connections branch from 5b9d0cf to e35af3f Compare March 6, 2024 22:50
@elfenpiff elfenpiff merged commit 5c01563 into eclipse-iceoryx:main Mar 6, 2024
17 of 20 checks passed
@elfenpiff elfenpiff deleted the iox2-129-fix-missing-connections branch March 6, 2024 23:33
@elfenpiff elfenpiff mentioned this pull request Mar 7, 2024
17 tasks
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.

Publisher/Subscriber fail to connect
2 participants