Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

use queue size to ensure history depth #31

Merged
merged 1 commit into from
May 19, 2015
Merged

use queue size to ensure history depth #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions rmw_opensplice_cpp/src/functions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,16 +180,23 @@ rmw_create_publisher(
return nullptr;
}

DDS::DataWriterQos default_datawriter_qos;
status = dds_publisher->get_default_datawriter_qos(default_datawriter_qos);
DDS::DataWriterQos datawriter_qos;
status = dds_publisher->get_default_datawriter_qos(datawriter_qos);
if (status != DDS::RETCODE_OK) {
rmw_set_error_string("failed to get default datawriter qos");
// printf("get_default_datawriter_qos() failed. Status = %d\n", status);
return nullptr;
}

// ensure the history depth is at least the requested queue size
if (datawriter_qos.history.kind == DDS::KEEP_LAST_HISTORY_QOS &&
Copy link
Member

Choose a reason for hiding this comment

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

Why check if the history kind is correct? Why not always set the history.kind to that and always set the history.depth to queue_size?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the user has already configured a "better" history (e.g. keep all or a larger number) then the code should not reduce the history depth.

Copy link
Member

Choose a reason for hiding this comment

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

When would the user have had the chance to do that? The publisher was just created in this function. At best they could do it after the publisher is returned from this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is always possible to configure QoS parameters externally, e.g. via XML files.

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that if the default changes history kind then the queue size is not even set. Also, if I pass a queue size into this create function I would expect the queue is set to exactly what I asked, regardless of the default history settings.

Copy link
Member

Choose a reason for hiding this comment

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

If we want to be able to fall back to system defaults, then we need a "create default publisher" option. I think it is the wrong behavior for the function to receive a queue_size and then potentially not use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Always setting the history kind and depth here would prevent using vendor specific configuration options completely.

The history kind only support ALL and DEPTH N. If the default has been set to ALL the function doesn't reduce the history. If it is set to a specific DEPTH it ensures that it has at least the requested queue size. But if the default has been changed to a larger value it is not reduced.

Imo if the user changes the default configuration he is doing that for a specific reason and I would not overwrite that choice in our code.

Copy link
Member

Choose a reason for hiding this comment

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

The ability to do that is an implementation detail, this function must behave consistently based on the input. I think that if I design my node to explicitly need a queue size of 1 and you set the global to keep all that will not work. If you want to use the defaults then we need a way to specify "use default", which is how it is modeled in DDS. We cannot ask users to always specify a queue_size and then potentially ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have created ros2/rmw#13 to possibly extend the rmw interface to support both cases. Until then we will keep the behavior implemented in this PR.

datawriter_qos.history.depth < queue_size
) {
datawriter_qos.history.depth = queue_size;
}

DDS::DataWriter * topic_writer = dds_publisher->create_datawriter(
topic, default_datawriter_qos, NULL, DDS::STATUS_MASK_NONE);
topic, datawriter_qos, NULL, DDS::STATUS_MASK_NONE);
if (!topic_writer) {
rmw_set_error_string("failed to create datawriter");
return nullptr;
Expand Down Expand Up @@ -299,16 +306,23 @@ rmw_create_subscription(
return nullptr;
}

DDS::DataReaderQos default_datareader_qos;
status = dds_subscriber->get_default_datareader_qos(default_datareader_qos);
DDS::DataReaderQos datareader_qos;
status = dds_subscriber->get_default_datareader_qos(datareader_qos);
if (status != DDS::RETCODE_OK) {
rmw_set_error_string("failed to get default datareader qos");
// printf("get_default_datareader_qos() failed. Status = %d\n", status);
return nullptr;
}

// ensure the history depth is at least the requested queue size
if (datareader_qos.history.kind == DDS::KEEP_LAST_HISTORY_QOS &&
datareader_qos.history.depth < queue_size
) {
datareader_qos.history.depth = queue_size;
}

DDS::DataReader * topic_reader = dds_subscriber->create_datareader(
topic, default_datareader_qos, NULL, DDS::STATUS_MASK_NONE);
topic, datareader_qos, NULL, DDS::STATUS_MASK_NONE);

OpenSpliceStaticSubscriberInfo * subscriber_info = \
static_cast<OpenSpliceStaticSubscriberInfo *>(rmw_allocate(
Expand Down