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.

Sign up for GitHub

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

Jump to bottom

Dds #1174 ensure compatibility with iceoryx 2.0 #1175

Merged
merged 9 commits into from
Mar 11, 2022
Merged

Dds #1174 ensure compatibility with iceoryx 2.0 #1175

merged 9 commits into from
Mar 11, 2022

Conversation

MatthiasKillat
Copy link
Contributor

No description provided.

@MatthiasKillat
Copy link
Contributor Author

MatthiasKillat commented Mar 3, 2022 •
edited
Loading

@sumanth-nirmal @eboasson We need to discuss how to communicate certain non-fatal errors which may be surprising to the user but should not lead to a fatal error.

In this particular case a failure to receive an iceoryx chunk (fast enough) should be logged/communicated to the user in some kind of error log. Before it could lead to the user missing data without understanding why.

I also need to try these changes with CycloneDDS C++, but will do so next. It will likely not be fully functional due to (hopefully) minor issues.

sumanth-nirmal
sumanth-nirmal reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@sumanth-nirmal sumanth-nirmal left a comment

Choose a reason for hiding this comment

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

The changes overall looks good

src/core/ddsc/src/shm_monitor.c Outdated Show resolved Hide resolved
src/core/ddsi/include/dds/ddsi/ddsi_shm_transport.h Show resolved Hide resolved
@sumanth-nirmal
Copy link
Contributor

We need to update the iceroyx version in the CI here

cyclonedds/.azure/templates/build-test.yml

Line 89 in 8154088

--branch "${ICEORYX_BRANCH:-v1.0.1}" \
, correct?

@dkroenke Does iceoryx-v1.90.0 have all the latest fixes needed, or should we need a new tag?

@sumanth-nirmal sumanth-nirmal mentioned this pull request Mar 4, 2022
Merged
@MatthiasKillat MatthiasKillat force-pushed the dds-#1174-ensure-compatibility-with-iceoryx-2.0 branch 3 times, most recently from 6e0e2c7 to 9e3f5ac Compare March 4, 2022 19:07
@MatthiasKillat MatthiasKillat marked this pull request as ready for review March 4, 2022 21:58
@MatthiasKillat
Copy link
Contributor Author

@eboasson The mac failure appears to be unrelated (some timeout?)

@MatthiasKillat MatthiasKillat force-pushed the dds-#1174-ensure-compatibility-with-iceoryx-2.0 branch from 4619976 to 15d8a9b Compare March 7, 2022 11:29
@MatthiasKillat MatthiasKillat force-pushed the dds-#1174-ensure-compatibility-with-iceoryx-2.0 branch from 15d8a9b to f7bdc3c Compare March 7, 2022 19:37
@sumanth-nirmal sumanth-nirmal mentioned this pull request Mar 8, 2022
Open
sumanth-nirmal and others added 9 commits March 10, 2022 13:38
@sumanth-nirmal @MatthiasKillat
Update to use the iceoryx_binding_c storage on heap and also use atta…
cba77ba 
…ch event calls with context data

Signed-off-by: Sumanth Nirmal <sumanth.724@gmail.com>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Introduce iox_sub_context_t
129050b 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Check iox-listener limits
a8cb94b 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Use iceoryx 2.0 subscriber queue limit
880e606 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Log if we hold too many icoeryx chunks
8c3f254 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Depend on iceoryx master
ba482d5 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Fix subcriber context
85c73cd 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Remove storage for iox structs
8b19309 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat
dds-eclipse-cyclonedds#1174 Refactoring due to iceoryx 2.0 API usage
9345acc 
Signed-off-by: Matthias Killat <matthias.killat@apex.ai>
@MatthiasKillat MatthiasKillat force-pushed the dds-#1174-ensure-compatibility-with-iceoryx-2.0 branch from 473c8e6 to 9345acc Compare March 10, 2022 12:45
@MatthiasKillat
Copy link
Contributor Author

@eboasson Fixed review findings, should be ready for another review.

@eboasson eboasson mentioned this pull request Mar 11, 2022
Closed
eboasson
eboasson approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

With -Wstrict-prototypes I get warnings that are solved by eclipse-iceoryx/iceoryx#1281. Once that one is merged, I'll merge this one.

@MatthiasKillat
Copy link
Contributor Author

@eboasson eclipse-iceoryx/iceoryx#1281 is merged

@eboasson eboasson merged commit d01cd02 into eclipse-cyclonedds:master Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sumanth-nirmal sumanth-nirmal sumanth-nirmal left review comments

@eboasson eboasson eboasson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatthiasKillat @sumanth-nirmal @eboasson