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

Feature/280/output dds #329

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Feature/280/output dds #329

wants to merge 24 commits into from

Conversation

pparrilla
Copy link
Contributor

This pull request is coming from the issue #280

@pparrilla pparrilla requested a review from javmorcas June 10, 2020 15:37
@javmorcas javmorcas changed the base branch from master to feature/280/rework June 15, 2020 11:40
javmorcas
javmorcas requested changes Jun 15, 2020
View reviewed changes
Copy link
Member

@javmorcas javmorcas left a comment

Choose a reason for hiding this comment

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

Initial review for C++ Classic

perftest_qos_profiles.xml Outdated
Comment on lines 1102 to 1105
<resource_limits>
<max_samples>10</max_samples>
<initial_samples>1</initial_samples>
<max_samples_per_instance>1</max_samples_per_instance>
Copy link
Member

Choose a reason for hiding this comment

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

Fix indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcIdl/perftest.idl Outdated
Comment on lines 202 to 208
@optional unsigned long long min;
@optional unsigned long long max;
@optional unsigned long long h50;
@optional unsigned long long h90;
@optional unsigned long long h99;
@optional unsigned long long h9999;
@optional unsigned long long h999999;
Copy link
Member

Choose a reason for hiding this comment

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

Change this to unsigned long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcIdl/perftest.idl Outdated
Comment on lines 190 to 191
@optional unsigned long long packets;
@optional unsigned long long packetsS;
Copy link
Member

Choose a reason for hiding this comment

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

Change these to unsigned long

Copy link
Member

Choose a reason for hiding this comment

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

Lets call them:

packets
packetsPerSecond
packetsAverage
mbps
mbpsAverage
lostPackets
lostPacketsPercent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but in perftest_publisher.cpp, packets and packetsPerSecond are unsigned long long data types

srcIdl/perftest.idl Outdated
@optional perftestLatencyInfo pLatencyInfo;
@optional perftestThroughputInfo pThroughputInfo;
@optional double outputCpu;
};
Copy link
Member

Choose a reason for hiding this comment

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

Add a new line at the end of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcIdl/perftest.idl Outdated
@optional double packetsAve;
@optional double mbps;
@optional double mbpsAve;
@optional unsigned long long lost;
Copy link
Member

Choose a reason for hiding this comment

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

Unsigned long

srcCpp/perftest_publisher.cxx Outdated
@@ -1334,6 +1318,7 @@ class LatencyListener : public IMessagingCB
IMessagingWriter *_writer;
ParameterManager *_PM;
PerftestPrinter *_printer;
LatencyInfo _latInfo;
Copy link
Member

Choose a reason for hiding this comment

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

_latencyInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcCpp/perftest_publisher.cxx Outdated
@@ -1391,6 +1376,9 @@ class LatencyListener : public IMessagingCB
_PM = &PM;
_printer = printer;

_latInfo.dataLength = _printer->_dataLength;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use last_data_length instead of asking the _printer for the datalen.

Copy link
Member

Choose a reason for hiding this comment

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

It feels weird asking an structure for a member that you are going to later set in the same structure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, now it's _latencyInfo.dataLength = last_data_length

srcCpp/RTIDDSImpl.h Outdated
DDSDomainParticipant *participant;
DDSPublisher *publisher;
DDSTopic *topic;
perftestInfoDataWriter *ptInfoWriter;
Copy link
Member

Choose a reason for hiding this comment

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

infoDataWriter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcCpp/RTIDDSImpl.h Outdated
DDSPublisher *publisher;
DDSTopic *topic;
perftestInfoDataWriter *ptInfoWriter;
perftestInfo *ptInfo;
Copy link
Member

Choose a reason for hiding this comment

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

perftestInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

srcCpp/RTIDDSImpl.cxx
DDS_PublisherQos publisherQos;
DDS_DataWriterQos dwQos;

#ifndef RTI_MICRO
Copy link
Member

Choose a reason for hiding this comment

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

Can we check what is the behavior when using micro? We might need to make this differently.

Base automatically changed from feature/280/rework to master November 24, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javmorcas javmorcas javmorcas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@pparrilla @javmorcas