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

feature/implement-PID_PRODUCT_VERSION #2132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature/implement-PID_PRODUCT_VERSION #2132

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#
cmake_minimum_required(VERSION 3.16)
# C++ is only for Iceoryx plugin
project(CycloneDDS VERSION 0.11.0 LANGUAGES C CXX)
project(CycloneDDS VERSION 0.11.0.0 LANGUAGES C CXX)

# Set a default build type if none was specified
set(default_build_type "RelWithDebInfo")
Expand Down
1 change: 1 addition & 0 deletions src/core/ddsi/include/dds/ddsi/ddsi_plist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ typedef struct ddsi_plist {

ddsi_protocol_version_t protocol_version;
ddsi_vendorid_t vendorid;
ddsi_product_version_t product_version;
ddsi_locators_t unicast_locators;
ddsi_locators_t multicast_locators;
ddsi_locators_t default_unicast_locators;
Expand Down
7 changes: 7 additions & 0 deletions src/core/ddsi/include/dds/ddsi/ddsi_protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ typedef struct {
uint8_t id[2];
} ddsi_vendorid_t;

typedef struct {
uint8_t major;
uint8_t minor;
uint8_t release;
uint8_t revision;
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it is major, minor, "something" ("patch"?) and I don't know that having four fields won't make life more complicated. What I would find really useful is to also have a commit hash in there (or perhaps just the first few bytes to keep the size in check).

} ddsi_product_version_t;

typedef struct ddsi_protocolid {
uint8_t id[4];
} ddsi_protocolid_t;
Expand Down
1 change: 1 addition & 0 deletions src/core/ddsi/src/ddsi__plist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ struct ddsi_xmsg;
#define PP_CYCLONE_RECEIVE_BUFFER_SIZE ((uint64_t)1 << 38)
#define PP_CYCLONE_TOPIC_GUID ((uint64_t)1 << 39)
#define PP_CYCLONE_REQUESTS_KEYHASH ((uint64_t)1 << 40)
#define PP_PRODUCT_VERSION ((uint64_t)1 << 41)

/* Set for unrecognized parameters that are in the reserved space or
in our own vendor-specific space that have the
Expand Down
1 change: 1 addition & 0 deletions src/core/ddsi/src/ddsi__protocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ typedef union ddsi_rtps_submessage {
#define DDSI_PID_TRANSPORT_PRIORITY 0x49u
#define DDSI_PID_PROTOCOL_VERSION 0x15u
#define DDSI_PID_VENDORID 0x16u
#define DDSI_PID_PRODUCT_VERSION 0x8000u
Copy link
Contributor

Choose a reason for hiding this comment

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

The vendor-specific flags are defined below, e.g., line 349 where it defines DDSI_PID_ADLINK_PARTICIPANT_VERSION_INFO. 0x8000 is already defined in this file, inherited from OpenSplice but not used by Cyclone (see

cyclonedds/src/core/ddsi/src/ddsi_plist.c

Line 2110 in d8cef89

static const struct piddesc piddesc_eclipse[] = {
and the table below it: it doesn't occur in either). There are a lot of parameters there that it doesn't grok, come to think of it.

I suspect 0x8000 has never been used by Cyclone, so I think it can be reclaimed.

#define DDSI_PID_UNICAST_LOCATOR 0x2fu
#define DDSI_PID_MULTICAST_LOCATOR 0x30u
#define DDSI_PID_MULTICAST_IPADDRESS 0x11u
Expand Down
6 changes: 5 additions & 1 deletion src/core/ddsi/src/ddsi_discovery_endpoint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,14 @@ static int sedp_write_endpoint_impl
else
{
assert (xqos != NULL);
ps.present |= PP_PROTOCOL_VERSION | PP_VENDORID;
ps.present |= PP_PROTOCOL_VERSION | PP_VENDORID | PP_PRODUCT_VERSION;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add it to endpoint discovery: the version is constant for all endpoints in a participant, endpoint discovery only runs after participant discovery and participant discovery messages are periodic. So putting in participant discovery both ensures you eventually will find out, and avoids sending it for potentially thousands of endpoints.

ps.protocol_version.major = DDSI_RTPS_MAJOR;
ps.protocol_version.minor = DDSI_RTPS_MINOR;
ps.vendorid = DDSI_VENDORID_ECLIPSE;
ps.product_version.major = DDS_VERSION_MAJOR;
ps.product_version.minor = DDS_VERSION_MINOR;
ps.product_version.release = DDS_VERSION_PATCH;
ps.product_version.revision = DDS_VERSION_TWEAK;

assert (epcommon != NULL);

Expand Down
6 changes: 5 additions & 1 deletion src/core/ddsi/src/ddsi_discovery_spdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,17 @@ void ddsi_get_participant_builtin_topic_data (const struct ddsi_participant *pp,

ddsi_plist_init_empty (dst);
dst->present |= PP_PARTICIPANT_GUID | PP_BUILTIN_ENDPOINT_SET |
PP_PROTOCOL_VERSION | PP_VENDORID | PP_DOMAIN_ID;
PP_PROTOCOL_VERSION | PP_VENDORID | PP_DOMAIN_ID | PP_PRODUCT_VERSION;
dst->participant_guid = pp->e.guid;
dst->builtin_endpoint_set = pp->bes;
dst->protocol_version.major = DDSI_RTPS_MAJOR;
dst->protocol_version.minor = DDSI_RTPS_MINOR;
dst->vendorid = DDSI_VENDORID_ECLIPSE;
dst->domain_id = gv->config.extDomainId.value;
dst->product_version.major = DDS_VERSION_MAJOR;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look a bit further down in the function can also see that it already puts something called "adlink version info" in. I am not sure the format is ideal, nor that we should keep it at all, but the name is irrelevant for compatibility and we could clean it up a bit.

For example, we could also redefine struct ddsi_adlink_participant_version_info as

typedef struct ddsi_cyclonedds_version
{
  uint8_t major;
  uint8_t minor;
  uint8_t release;
  uint8_t revision;
  uint32_t flags;
  uint32_t unused[3];
  char *internals;
} ddsi_cyclonedds_version_t;

It has proven useful to have a bunch of flags ready for use. The string can probably be replaced by something more useful, but if we really replace it by some other type (and not just change the contents) then we would need to change the parameter id to avoid upsetting "old" Cyclone versions.

OpenSplice doesn't interpret this if it comes from Cyclone, so we really have some freedom.

dst->product_version.minor = DDS_VERSION_MINOR;
dst->product_version.release = DDS_VERSION_PATCH;
dst->product_version.revision = DDS_VERSION_TWEAK;
/* Be sure not to send a DOMAIN_TAG when it is the default (an empty)
string: it is an "incompatible-if-unrecognized" parameter, and so
implementations that don't understand the parameter will refuse to
Expand Down
7 changes: 6 additions & 1 deletion src/core/ddsi/src/ddsi_discovery_topic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <string.h>
#include <stdlib.h>

#include "dds/version.h"
#include "dds/ddsrt/heap.h"
#include "dds/ddsrt/log.h"
#include "dds/ddsi/ddsi_domaingv.h"
Expand Down Expand Up @@ -41,10 +42,14 @@ static int ddsi_sedp_write_topic_impl (struct ddsi_writer *wr, int alive, const
ps.topic_guid = *guid;

assert (xqos != NULL);
ps.present |= PP_PROTOCOL_VERSION | PP_VENDORID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Like with the endpoints, I wouldn't add it for the topics either. I think we should just clean up/improve version information in SPDP.

ps.present |= PP_PROTOCOL_VERSION | PP_VENDORID | PP_PRODUCT_VERSION;
ps.protocol_version.major = DDSI_RTPS_MAJOR;
ps.protocol_version.minor = DDSI_RTPS_MINOR;
ps.vendorid = DDSI_VENDORID_ECLIPSE;
ps.product_version.major = DDS_VERSION_MAJOR;
ps.product_version.minor = DDS_VERSION_MINOR;
ps.product_version.release = DDS_VERSION_PATCH;
ps.product_version.revision = DDS_VERSION_TWEAK;

uint64_t qosdiff = ddsi_xqos_delta (xqos, defqos, ~(uint64_t)0);
if (gv->config.explicitly_publish_qos_set_to_default)
Expand Down
1 change: 1 addition & 0 deletions src/core/ddsi/src/ddsi_plist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,7 @@ static const struct piddesc piddesc_omg[] = {
#endif
PP (PROTOCOL_VERSION, protocol_version, Xox2),
PP (VENDORID, vendorid, Xox2),
PP (PRODUCT_VERSION, product_version, Xox2),
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost right! Xox2 means o times 2, so two octets. You really want four, which means Xox4 but that one doesn't exist. It could be added, but it would be equivalent to Xox2, Xox2, or even to Xo, Xo, Xo, Xo. I probably would go for the latter (out of laziness? who knows ...)

This would be a vendor-specific code, and so it would need to be moved to the vendor-specific code tables, though. And as I remarked before, I think I would first see if it wouldn't make sense to clean up the adlink_participant_info in a (sufficiently) backwards compatible manner.

PP (EXPECTS_INLINE_QOS, expects_inline_qos, Xb),
PP (PARTICIPANT_MANUAL_LIVELINESS_COUNT, participant_manual_liveliness_count, Xi),
PP (PARTICIPANT_BUILTIN_ENDPOINTS, participant_builtin_endpoints, Xu),
Expand Down
1 change: 1 addition & 0 deletions src/security/core/src/dds_security_serialize.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#define PID_TRANSPORT_PRIORITY 0x49u
#define PID_PROTOCOL_VERSION 0x15u
#define PID_VENDORID 0x16u
#define PID_PRODUCT_VERSION 0x8000u
Copy link
Contributor

Choose a reason for hiding this comment

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

As the product version is only informational now, I don't think anything is to be gained by also burdening the serializer used for the DDS Security with it. The clear text SPDP message will be good enough for the purpose.

Note that we shouldn't even have a separate "DDS Security" serializer. It only exists because the in-memory representation in the spec'd plug-in interface is different, and the dice landed the wrong way when a choice had to be made between adding a second serializer or having only a single serializer and converting between in-memory representations. Given the security sensitivity of (in particular de-)serializers, we should get rid of the second one.

In short, I wouldn't add it here.

#define PID_UNICAST_LOCATOR 0x2fu
#define PID_MULTICAST_LOCATOR 0x30u
#define PID_MULTICAST_IPADDRESS 0x11u
Expand Down
Loading