Skip to content

[21768] DDS data publication implementation#5

Merged
rsanchez15 merged 28 commits into
mainfrom
feature/publish-data
Jun 17, 2025
Merged

[21768] DDS data publication implementation#5
rsanchez15 merged 28 commits into
mainfrom
feature/publish-data

Conversation

@juanlofer-eprosima

Copy link
Copy Markdown
Contributor

juanlofer-eprosima and others added 8 commits May 12, 2025 11:09
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>
Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>
Signed-off-by: Eugenio Collado <eugeniocollado@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@EugenioCollado
EugenioCollado self-requested a review June 2, 2025 14:41
@juanlofer-eprosima
juanlofer-eprosima force-pushed the feature/publish-data branch 4 times, most recently from ae71ddf to b7b9464 Compare June 4, 2025 11:53
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
juanlofer-eprosima force-pushed the feature/publish-data branch 2 times, most recently from 29d5812 to 915cb76 Compare June 5, 2025 05:48
@EugenioCollado
EugenioCollado requested a review from Copilot June 6, 2025 09:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Implements DDS data publication by extending the enabler API, refactoring callbacks into a CallbackSet, and updating tests and examples to cover topic notifications and sample publishing.

  • Introduce CallbackSet and update create_dds_enabler signatures to accept it
  • Add DDSEnabler::publish method and wire up publication in runner and examples
  • Update tests to assert on received topics and comment out a large block of optional typed tests

Reviewed Changes

Copilot reviewed 91 out of 91 changed files in this pull request and generated no comments.

Show a summary per file
File Description
CMakeLists.txt (typed tests) Comment out many optional ddsenabler_send_samples_* entries
ReloadConfig.cpp Refactor callbacks into CallbackSet, fix formatting
DdsEnablerTest.cpp Add get_received_topics() assertions alongside types/data
DDSEnablerTester.hpp Introduce received_topics_, get_received_topics()
dds_enabler_runner.cpp Overload create_dds_enabler, drop old callbacks API
DDSEnabler.cpp / DDSEnabler.hpp Inject CallbackSet, add publish and set_internal_callbacks_
CallbackSet.hpp New struct defining DDS and log callbacks
project_settings.cmake Bump to C++20
examples and CLIParser Wire up persistence, file I/O helpers, CLI options
GitHub workflows Enable examples, repeat-until-fail on CI tests
Comments suppressed due to low confidence (4)

ddsenabler/test/ddsEnablerTests/ReloadConfig.cpp:161

  • In write_yaml_file there is no buffer declared, leading to an undefined identifier. You need to declare and populate a buffer (similar to write_json_file) or write Template directly.
outFile << buffer;

ddsenabler/test/ddsEnablerTests/ReloadConfig.cpp:139

  • [nitpick] The variable name Template conflicts with the C++ keyword and general naming conventions. Consider renaming it to yamlTemplate or similar.
std::string Template =

ddsenabler/include/ddsenabler/DDSEnabler.hpp:102

  • The new publish() API is not covered by any unit tests—consider adding tests to validate JSON publishing and error paths.
bool publish(

ddsenabler/test/ddsEnablerTests/ReloadConfig.cpp:122

  • [nitpick] Brace placement style changed here (opening brace on new line) but other sites in this file use the same-line style. Align with the project's coding style for consistency.
if (outFile.is_open())

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Comment thread ddsenabler_participants/include/ddsenabler_participants/CBWriter.hpp Outdated
Comment thread .github/workflows/docker-reusable-workflow.yml Outdated
Comment thread ddsenabler/examples/main.cpp
Comment thread ddsenabler/examples/utils.hpp
Comment thread ddsenabler/src/cpp/dds_enabler_runner.cpp
Comment thread docs/context_broker_interface.rst
Comment thread ddsenabler_yaml/src/cpp/EnablerConfiguration.cpp Outdated
Comment thread ddsenabler_test/compose/test_cases/publish/discovered_type/compose.yml Outdated
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>

@EugenioCollado EugenioCollado left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@rsanchez15
rsanchez15 merged commit 533f3ea into main Jun 17, 2025
35 of 39 checks passed
@rsanchez15
rsanchez15 deleted the feature/publish-data branch June 17, 2025 14:31
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