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

Update repository for Fast DDS 3.0.0 compatibility #141

Closed
wants to merge 7 commits into from
Closed

Update repository for Fast DDS 3.0.0 compatibility #141

wants to merge 7 commits into from

Conversation

LuciaEchevarria99
Copy link
Contributor

@LuciaEchevarria99 LuciaEchevarria99 commented Apr 27, 2024 •
edited by juanlofer-eprosima
Loading

@LuciaEchevarria99 LuciaEchevarria99 changed the title Update repository for FastDDS 3.0.0 compatibility Update repository for Fast-DDS 3.0.0 compatibility May 16, 2024
@LuciaEchevarria99 LuciaEchevarria99 changed the title Update repository for Fast-DDS 3.0.0 compatibility Update repository for Fast DDS 3.0.0 compatibility May 16, 2024
@LuciaEchevarria99
Apply changes for Dynamic Types
846189a 
Signed-off-by: Lucia Echevarria <[email protected]>
LuciaEchevarria99
LuciaEchevarria99 commented Jun 11, 2024
View reviewed changes
ddsrecorder/test/blackbox/mcap/McapFileCreationTest.cpp Outdated
#include <fastrtps/types/DynamicDataFactory.h>
#include <fastrtps/types/TypeObjectFactory.h>
#include <fastdds/dds/xtypes/dynamic_types/DynamicData.hpp>
#include <fastdds/dds/xtypes/dynamic_types/DynamicType.hpp>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests need to be fixed

LuciaEchevarria99
LuciaEchevarria99 commented Jun 11, 2024
View reviewed changes
ddsreplayer/src/cpp/tool/DdsReplayer.cpp Outdated
// Set as server in TypeLookup service
pqos.wire_protocol().builtin.typelookup_config.use_client = false;
pqos.wire_protocol().builtin.typelookup_config.use_server = true;

// Participant creation via factory
Copy link
Contributor Author

@LuciaEchevarria99 LuciaEchevarria99 Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Comment for the reviewer: The dyn_participant_ has been removed from the Replayer. Its sole purpose was to manage the DynamicTypes, but now this responsibility has been transferred to the participants within the DDSPipe.

Tempate
Tempate suggested changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@Tempate Tempate left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for the hard work. Everything looks much cleaner now.

ddsrecorder/CMakeLists.txt
else()
set(DDS_TYPES_VERSION "v2")
endif()

file(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe manually selecting the SOURCE_FILES is no longer required. We can do it as it was being done before b388f09. Take a look at https://github.com/eProsima/DDS-Record-Replay/blob/11fcf5c89dbad7e5ad8c9f1ee37e9577abe5ae00/ddsrecorder/CMakeLists.txt.

ddsrecorder/src/cpp/command_receiver/CommandReceiver.hpp
#include <cpp_utils/macros/custom_enumeration.hpp>

#include <ddspipe_participants/configuration/SimpleParticipantConfiguration.hpp>
#include <string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should include <memory> as well, to support shared_ptr.

ddsreplayer/src/cpp/tool/DdsReplayer.cpp
@@ -498,19 +368,28 @@ fastrtps::types::TypeIdentifier DdsReplayer::deserialize_type_identifier_(
fastcdr::FastBuffer fastbuffer((char*)payload.data, parameter_length);

// Read data
fastrtps::rtps::CDRMessage::readData(cdr_message, payload.data, parameter_length);
if (cdr_message != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid nested ifs. Use &s.

ddsreplayer/src/cpp/tool/DdsReplayer.cpp
@@ -540,19 +419,28 @@ fastrtps::types::TypeObject DdsReplayer::deserialize_type_object_(
fastcdr::FastBuffer fastbuffer((char*)payload.data, parameter_length);

// Read data
fastrtps::rtps::CDRMessage::readData(cdr_message, payload.data, parameter_length);
if (cdr_message != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use &s here as well.

ddsreplayer/src/cpp/tool/DdsReplayer.cpp
"Failed to create {" << topic->m_topic_name << ";" << topic->type_name <<
"} DDS topic, aborting dynamic writer creation...");
return;
registered_types_.insert({dynamic_type.type_name(), type_identifiers});
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually add data to a map like:

registered_types[dynamic_type.type_name()] = type_identifiers;

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp
const uint32_t size_octet = sizeof(value);
if (!(cdr_message && (cdr_message->pos + size_octet <= cdr_message->max_size)))
{
// TODO Warning
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code would also be more readable if you continue at the end of the if to avoid the else.

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp

// Add data
bool valid = fastrtps::rtps::CDRMessage::addData(cdr_message, payload.data, payload.length);
if (!(cdr_message && (cdr_message->pos + payload.length <= cdr_message->max_size))|| (payload.length > 0 && !payload.data))
Copy link
Contributor

Choose a reason for hiding this comment

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

The same goes for this expression. It would be more readable if you untie it:

!cdr_message || cdr_message->pos + payload.length > cdr_message->max_size || (payload.length > 0 && !payload.data)

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp

// Add data
bool valid = fastrtps::rtps::CDRMessage::addData(cdr_message, payload.data, payload.length);
if (!(cdr_message && (cdr_message->pos + payload.length <= cdr_message->max_size))|| (payload.length > 0 && !payload.data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, I'm not an expert on Dynamic Types, but this looks like two different expressions for which you may want to throw two different warnings.

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp
#endif // if __BIG_ENDIAN__

// Create CDR message with payload
fastrtps::rtps::CDRMessage_t* cdr_message = new fastrtps::rtps::CDRMessage_t(payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be const?

ddsrecorder_participants/src/cpp/recorder/mcap/McapHandler.cpp

// Add data
bool valid = fastrtps::rtps::CDRMessage::addData(cdr_message, payload.data, payload.length);
if (!(cdr_message && (cdr_message->pos + payload.length <= cdr_message->max_size))|| (payload.length > 0 && !payload.data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please apply as well my comments below.

@LuciaEchevarria99
FIx tests
d9d0731 
Signed-off-by: Lucia Echevarria <[email protected]>
@LuciaEchevarria99
Regenerate types
f546358 
Signed-off-by: Lucia Echevarria <[email protected]>
@juanlofer-eprosima
Copy link
Contributor

Closed in favor of #148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tempate Tempate Tempate requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LuciaEchevarria99 @juanlofer-eprosima @Tempate