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

Make the trigger of the DDS Pipe callbacks configurable #87

Merged
merged 12 commits into from
Nov 22, 2023

Conversation

Tempate
Copy link
Contributor

@Tempate Tempate commented Oct 16, 2023

In the previous version, the DDS Pipe's callbacks were only triggered by the discovery of readers, so the DDS-Record-Replay had to simulate the creation of a reader to trigger the creation of the Bridge. In this version, the entity type that triggers the callbacks is configurable. The DDS Recorder triggers them with the discovery of writers, and the DDS Replayer doesn't trigger them by discovery, and just starts replaying the data as soon as the program is launched.

Merge after:

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (4369964) 20.02% compared to head (c0cc609) 19.64%.

Files Patch % Lines
ddsrecorder/src/cpp/tool/DdsRecorder.cpp 40.74% 8 Missing and 8 partials ⚠️
ddsreplayer/src/cpp/tool/DdsReplayer.cpp 0.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   20.02%   19.64%   -0.39%     
==========================================
  Files          29       29              
  Lines        2696     2698       +2     
  Branches     1409     1412       +3     
==========================================
- Hits          540      530      -10     
- Misses       1699     1721      +22     
+ Partials      457      447      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tempate
Copy link
Contributor Author

Tempate commented Nov 14, 2023

Copy link
Contributor

@jepemi jepemi left a comment

Choose a reason for hiding this comment

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

Check that dynamic types work properly.

ddsreplayer/src/cpp/tool/DdsReplayer.cpp Show resolved Hide resolved
ddsreplayer/src/cpp/tool/DdsReplayer.cpp Outdated Show resolved Hide resolved
ddsrecorder/src/cpp/tool/DdsRecorder.cpp Show resolved Hide resolved
jepemi
jepemi previously approved these changes Nov 14, 2023
Copy link
Contributor

@jepemi jepemi left a comment

Choose a reason for hiding this comment

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

LGTM

ddsrecorder/src/cpp/tool/DdsRecorder.cpp Outdated Show resolved Hide resolved
ddsreplayer/src/cpp/tool/DdsReplayer.cpp Outdated Show resolved Hide resolved
ddsreplayer/src/cpp/tool/DdsReplayer.cpp Show resolved Hide resolved
ddsrecorder/src/cpp/tool/DdsRecorder.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

Please change the name and description of the PR (related PRs as well).

@Tempate Tempate changed the title Dynamically trigger the DDS Pipe's callbacks Make the trigger of the DDS Pipe callbacks configurable Nov 21, 2023
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

Piggyback: please fix the structure of recorder and replayer configuration sections (manual topics and others shouldn't be within topic filtering section). Also note max transmission rate in replayer config is not well placed.

Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

Please rebase

docs/rst/replaying/usage/configuration.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima left a comment

Choose a reason for hiding this comment

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

LGTM

@juanlofer-eprosima juanlofer-eprosima merged commit 77ebb97 into main Nov 22, 2023
34 checks passed
@juanlofer-eprosima juanlofer-eprosima deleted the feature/dynamic_entity_creation branch November 22, 2023 12:37
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.

4 participants