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

Support of disable_heartbeat_piggyback through XML and DDS API [12533] #2216

Merged
merged 7 commits into from
Dec 20, 2021

Conversation

richiware
Copy link
Member

@richiware richiware commented Sep 21, 2021

No description provided.

@richiware richiware changed the title Support of disable_heartbeat_piggyback through XML and DDS API Support of disable_heartbeat_piggyback through XML and DDS API [12533] Sep 21, 2021
Copy link
Contributor

@IkerLuengo IkerLuengo left a comment

Choose a reason for hiding this comment

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

Add the new field to the XMLElementParserTests to check that the new field is correctly parsed.

It should be added to the test_xml_profiles.xml and test_xml_profiles_rooted.xml in the test directory too.

src/cpp/rtps/xmlparser/XMLElementParser.cpp Outdated Show resolved Hide resolved
@MiguelCompany
Copy link
Member

@richiware This has conflicts, please rebase

@EduPonz EduPonz added this to the v2.5.0 milestone Dec 10, 2021
@richiware richiware force-pushed the feature/disable_heartbeat_piggyback branch from b33e032 to 349efca Compare December 13, 2021 09:33
@richiware
Copy link
Member Author

@MiguelCompany I've rebased and added a new blackbox test.

JLBuenoLopez
JLBuenoLopez previously approved these changes Dec 15, 2021
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM. Only a suggestion although it is not really required. I will leave it to you

Comment on lines 84 to 85
std::list<KeyedHelloWorld> uniquekey_keyedhelloworld_data_generator(
size_t max)
{
uint16_t index = 0;
size_t maximum = max ? max : 10;
std::list<KeyedHelloWorld> returnedValue(maximum);

std::generate(returnedValue.begin(), returnedValue.end(), [&index]
{
KeyedHelloWorld hello;
hello.index(index);
hello.key(index + 1);
std::stringstream ss;
ss << "HelloWorld " << index;
hello.message(ss.str());
++index;
return hello;
});

return returnedValue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, instead of creating the new generator where the only change with respect to the already existing generator is use a different key in each sample, the previous generator can be used giving as parameter flag in case that the key must be unique.

std::list<KeyedHelloWorld> default_keyedhelloworld_data_generator(
        size_t max,
        bool unique_key)

This is a mere suggestion and I am fine leaving the code as it is.

JLBuenoLopez
JLBuenoLopez previously approved these changes Dec 16, 2021
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM, though Windows CI has not finished so I am going to rerun it

@JLBuenoLopez
Copy link
Contributor

@richiprosima Please test Windows

@JLBuenoLopez
Copy link
Contributor

@richiprosima please test Windows

@JLBuenoLopez JLBuenoLopez added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Dec 16, 2021
Signed-off-by: Ricardo González <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
…d API.

Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

LGTM

@EduPonz
Copy link

EduPonz commented Dec 17, 2021

@richiprosima please test this

@MiguelCompany
Copy link
Member

@richiprosima Please test windows

@MiguelCompany MiguelCompany merged commit b107676 into master Dec 20, 2021
@MiguelCompany MiguelCompany deleted the feature/disable_heartbeat_piggyback branch December 20, 2021 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants