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

Fix complex member printing for DynamicDataHelper #2957

Merged
merged 5 commits into from
Sep 22, 2022
Merged

Fix complex member printing for DynamicDataHelper #2957

merged 5 commits into from
Sep 22, 2022

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Sep 20, 2022 •
edited
Loading

Description

This is a bugfix for the DynamicDataHelper, needed for a dynamic types and type introspection reference implementation for ROS 2 as proposed in REP 2011.

The printing methods for the helper segfault on attempting to print sequences of nested structs. This is because the print_complex_member() function uses the wrong DynamicData*. With the changes that this PR introduces, the segfaults disappear.

Additionally, this PR also cleans up the printing and adds useful tags for complex types.

Also, the DynamicDataHelper still has other bugs that are not addressed by this PR. Namely printing of dynamic sequences, which is addressed in #2827 .

I've tested that PR, and it's ready to merge, despite being listed as DRAFT. The REP reference implementation would be expedited if that PR was also merged.

PS: There are no tests for DynamicDataHelper, so I'm not implementing any in this PR.

Suitable Backports

@Mergifyio backport (branch/2.7.x)
@Mergifyio backport (branch/2.6.x)
Maybe more...

Example

The following is an example printout of a triply nested DynamicData instance printed with this PR's fixes.

nested_field: <struct/bitset>
	nested_char_field: 
	nested_bool_field: false
	nested_bool_seq_static_array_field: [false, false, false, false, false]
	nested_str_seq_bounded_array_field: [false, false, false, false, false, false, false, false, false, false]
	double_nested_field: <struct/bitset>
		double_nested_char_field: 
		triple_nested_field: <struct/bitset>
			triple_nested_char_field: 
			triple_nested_bool_seq_static_array_field: [false, false, false, false, false]
	double_nested_static_array_field: 
		[0] = <struct/bitset>
			double_nested_char_field: 
			triple_nested_field: <struct/bitset>
				triple_nested_char_field: 
				triple_nested_bool_seq_static_array_field: [false, false, false, false, false]
		[1] = <struct/bitset>
			double_nested_char_field: 
			triple_nested_field: <struct/bitset>
				triple_nested_char_field: 
				triple_nested_bool_seq_static_array_field: [false, false, false, false, false]
		[2] = <struct/bitset>
			double_nested_char_field: 
			triple_nested_field: <struct/bitset>
				triple_nested_char_field: 
				triple_nested_bool_seq_static_array_field: [false, false, false, false, false]
non_nested_byte_field: 0

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
  • Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • Documentation builds and tests pass locally.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.

Reviewer Checklist

  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

willstott101 and others added 3 commits July 8, 2022 15:39
@willstott101
DynamicDataHelper::print can print Sequences as well as Arrays
7c04c8d 
Signed-off-by: Will Stott <willstott101@gmail.com>
@methylDragon
Fix complex member printing
c876f95 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Clean up prints
5c1bc2e 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

methylDragon commented Sep 20, 2022 •
edited
Loading

I'm not sure why the tests are failing, they're nowhere close to what this PR has edited

Probably flaky tests?

@MiguelCompany
Copy link
Member

@methylDragon

Probably flaky tests?

Yes, those tests fail from time to time and are not related to this PR.

Before we review, would you mind adding the changes from #2827, so we merge and backport both at the same time?

@methylDragon
Copy link
Contributor Author

@methylDragon

Probably flaky tests?

Yes, those tests fail from time to time and are not related to this PR.

Before we review, would you mind adding the changes from #2827, so we merge and backport both at the same time?

dab9c06

Merge conflicts fixed.

@MiguelCompany MiguelCompany added no-test Skip CI tests if PR marked with this label no-aarch Skip build & test for aarch64 labels Sep 21, 2022
@MiguelCompany
Copy link
Member

@richiprosima Please test this

@MiguelCompany
Copy link
Member

@methylDragon Would you mind fixing the warnings on windows?

@methylDragon
Fix FastDDS windows CI error
df44d8c 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Copy link
Contributor Author

Let's try this df44d8c

@jsan-rt
Copy link
Contributor

jsan-rt commented Sep 22, 2022

@richiprosima please test this

@MiguelCompany MiguelCompany mentioned this pull request Sep 22, 2022
Closed
13 tasks
MiguelCompany
MiguelCompany approved these changes Sep 22, 2022
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @methylDragon !

@methylDragon
Copy link
Contributor Author

methylDragon commented Sep 22, 2022 •
edited
Loading

CH3EERS! I can't merge because I don't have write access 😬

@MiguelCompany MiguelCompany merged commit 78473a0 into eProsima:master Sep 22, 2022
@methylDragon methylDragon deleted the ch3/hotfix-dynamic-data-helper branch September 22, 2022 08:49
@MiguelCompany
Copy link
Member

@Mergifyio backport 2.7.x 2.6.x

@mergify mergify bot mentioned this pull request Sep 22, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 22, 2022
@methylDragon @mergify
Fix complex member printing for DynamicDataHelper (#2957)
8e31af2 
* DynamicDataHelper::print can print Sequences as well as Arrays
Signed-off-by: Will Stott <willstott101@gmail.com>

* Fix complex member printing

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Clean up prints

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix FastDDS windows CI error

Signed-off-by: methylDragon <methylDragon@gmail.com>

Signed-off-by: methylDragon <methylDragon@gmail.com>
Co-authored-by: Will Stott <willstott101@gmail.com>
(cherry picked from commit 78473a0)
@mergify mergify bot mentioned this pull request Sep 22, 2022
Merged
mergify bot pushed a commit that referenced this pull request Sep 22, 2022
@methylDragon @mergify
Fix complex member printing for DynamicDataHelper (#2957)
8b5c600 
* DynamicDataHelper::print can print Sequences as well as Arrays
Signed-off-by: Will Stott <willstott101@gmail.com>

* Fix complex member printing

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Clean up prints

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix FastDDS windows CI error

Signed-off-by: methylDragon <methylDragon@gmail.com>

Signed-off-by: methylDragon <methylDragon@gmail.com>
Co-authored-by: Will Stott <willstott101@gmail.com>
(cherry picked from commit 78473a0)

# Conflicts:
#	src/cpp/dynamic-types/DynamicDataHelper.cpp
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2022

backport 2.7.x 2.6.x

✅ Backports have been created

MiguelCompany added a commit that referenced this pull request Sep 30, 2022
@mergify @methylDragon @MiguelCompany
Fix complex member printing for DynamicDataHelper ((#2965)
03fdb06 
* Fix complex member printing for DynamicDataHelper (#2957)

* DynamicDataHelper::print can print Sequences as well as Arrays
Signed-off-by: Will Stott <willstott101@gmail.com>

* Fix complex member printing

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Clean up prints

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix FastDDS windows CI error

Signed-off-by: methylDragon <methylDragon@gmail.com>

Signed-off-by: methylDragon <methylDragon@gmail.com>
Co-authored-by: Will Stott <willstott101@gmail.com>
(cherry picked from commit 78473a0)

# Conflicts:
#	src/cpp/dynamic-types/DynamicDataHelper.cpp

* Fix conflict

Co-authored-by: methylDragon <methylDragon@gmail.com>

* Fixing linters

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
Co-authored-by: methylDragon <methylDragon@gmail.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
no-aarch Skip build & test for aarch64 no-test Skip CI tests if PR marked with this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@methylDragon @MiguelCompany @jsan-rt @willstott101