Skip to content

[23571] Removing empty qos_list from original writer as indicated in RTPS standard 2.5#6001

Merged
MiguelCompany merged 3 commits intomasterfrom
hotfix/change-original-writer-to-standard
Sep 4, 2025
Merged

[23571] Removing empty qos_list from original writer as indicated in RTPS standard 2.5#6001
MiguelCompany merged 3 commits intomasterfrom
hotfix/change-original-writer-to-standard

Conversation

@emiliocuestaf
Copy link
Copy Markdown
Contributor

@emiliocuestaf emiliocuestaf commented Sep 3, 2025

Description

This PR removes the empty ParameterList that was added to the original_writer_info serialization to be compliant with RTPS v2.5 standard.

  • Empty ParameterList was removed from OriginalWriterInfo struct and serialization
  • Parameter size changes from 32 to 28 as we remove the parameter list end sentinel and the lenght (2+2)
  • Some comments indicating where the QoS should have been added are also removed

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • [N/A] Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • [N/A] Any new/modified methods have been properly documented using Doxygen.
  • [N/A] Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • [N/A] New feature has been added to the versions.md file (if applicable).
  • [N/A] New feature has been documented/Current behavior is correctly described in the documentation.
  • [N/A] Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • N/A If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com>
@emiliocuestaf emiliocuestaf added this to the v3.4.0 milestone Sep 3, 2025
@emiliocuestaf emiliocuestaf changed the title Removing empty qos_list from original writer as indicated in rtps standard 2.5 [23571] Removing empty qos_list from original writer as indicated in rtps standard 2.5 Sep 3, 2025
@github-actions github-actions Bot added the ci-pending PR which CI is running label Sep 4, 2025
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

We should also delete the note of OriginalWriterInfo.hpp

@cferreiragonz cferreiragonz changed the title [23571] Removing empty qos_list from original writer as indicated in rtps standard 2.5 [23571] Removing empty qos_list from original writer as indicated in RTPS standard 2.5 Sep 4, 2025
Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com>
cferreiragonz
cferreiragonz previously approved these changes Sep 4, 2025
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

LGTM with green CI. We might also apply @juanlofer-eprosima suggestion and replace original_writer_guid with original_writer_info in the versions.md

Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com>
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@MiguelCompany MiguelCompany merged commit 8ea77ad into master Sep 4, 2025
5 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/change-original-writer-to-standard branch September 4, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-pending PR which CI is running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants