The CMake files now add "-Werror"/"/WX" if the "WERROR" CMake variable
is true. By default it is not; the CI builds set this.
Signed-off-by: Erik Boasson <eb@ilities.com>
Missing prototypes for exported functions cause a really huge issue on
Windows. Enabling the "missing prototypes" warning makes it much easier
to catch this problem. Naturally, any warnings caused by this have been
fixed.
Signed-off-by: Erik Boasson <eb@ilities.com>
This commit adds support for changing all mutable QoS except those that
affect reader/writer matching (i.e., deadline, latency budget and
partition). This is simply because the recalculation of the matches
hasn't been implemented yet, it is not a fundamental limitation.
Implementing this basically forced fixing up a bunch of inconsistencies
in handling QoS in entity creation. A silly multi-process ping-pong
test built on changing the value of user data has been added.
Signed-off-by: Erik Boasson <eb@ilities.com>
These topics are generated internally and never sent over the wire.
Performing full discovery for these is therefore a significant waste of
effort.
Signed-off-by: Erik Boasson <eb@ilities.com>
The DDSI spec version 2.1 forbade the use of ACKNACK/GAP messages with
an empty bitset, but most vendors used these forms anyway. The DDSI
stack of Cyclone had code to avoid generating these (though with a bug
where it could generate an invalid GAP), but for no real benefit.
Because the other vendors used them anyway, the stack has always been
perfectly capable of handling them.
DDSI spec version 2.3 allows these forms, and so there's no value in
maintaining the old complications. This also eliminates the invalid GAP
messages that could be generated at times.
Signed-off-by: Erik Boasson <eb@ilities.com>
The Compatibility/ArrivalOfDataAssertsPpAndEpLiveliness option was a
rather strange option: receipt of a message is proof of the existence of
the sender, so having an option to not treat it as such only adds
complexity without any benefit.
Signed-off-by: Erik Boasson <eb@ilities.com>
Padding used to not be cleared in this code base, but that has the
downside of valgrind reporting nuisance warnings (which could be fixed
using valgrind's programmatic interface) but also of potentially leaking
information. The cost of clearing the padding appears to be
insignificant compared to the cost of doing the real work, and so it is
probably best to just clear it.
Signed-off-by: Erik Boasson <eb@ilities.com>
The old parameter list parsing was a mess of custom code with tons of
duplicated checks, even though parameter list parsing really is a fairly
straightforward affair. This commit changes it to a mostly table-driven
implementation, where the vast majority of the settings are handled by a
generic deserializer and the irregular ones (like reliability, locators)
are handled by custom functions. The crazy ones (IPv4 address and port
rely on additional state and are completely special-cased).
Given these tables, the serialization, finalisation, validation,
merging, unalias'ing can all be handled by a very small amount of custom
code and an appropriately defined generic function for the common cases.
This also makes it possible to have all QoS validation in place, and so
removes the need for the specialized implementations for the various
entity kinds in the upper layer.
QoS inapplicable to an entity were previously ignored, allowing one to
have invalid values set in a QoS object when creating an entity,
provided that the invalid values are irrelevant to that entity. Whether
this is a good thing or not is debatable, but certainly it is a good
thing to avoid copying in inapplicable QoS settings. That in turn means
the behaviour of the API can remain the same.
It does turn out that the code used to return "inconsistent QoS" also
for invalid values. That has now been rectified, and it returns
"inconsistent QoS" or "bad parameter" as appropriate. Tests have been
updated accordingly.
Signed-off-by: Erik Boasson <eb@ilities.com>
There are some cases where "int" or "unsigend" actually makes sense, but
in a large number of cases it is really supposed to be either a 32-bit
integer, or, in some cases, at least a 32-bit integer. It is much to be
preferred to be clear about this.
Another reason is that at least some embedded platforms define, e.g.,
int32_t as "long" instead of "int". For the ones I am aware of the
"int" and "long" are actually the same 32-bit integer, but that
distinction can cause trouble with printf format specifications. So
again a good reason to be consistent in avoiding the
implementation-defined ones.
Signed-off-by: Erik Boasson <eb@ilities.com>
The functions did not touch the callback pointer if a null pointer was
passed in for the listener. That means one would have to initialize the
out parameter before the call or manually check the listener pointer to
know whether the callback point has a defined value following the call.
That's asking for trouble.
Thus, the decision to return a callback of 0 when no listener object is
passed in.
Signed-off-by: Erik Boasson <eb@ilities.com>
All this duplication was rather useless: the values are standardized
anyway and the conversion was a simple type cast without any check.
This commit unifies the definitions.
* DDSI now uses the definitions of the various QoS "kind" values from
the header file;
* The durations in the QoS objects are no longer in wire-format
representation, the conversions now happen only in conversion to/from
wire format;
* The core DDSI stack no longer uses DDSI time representations for time
stamps, instead using the "native" one;
* QoS policy ids duplication has been eliminated, again using the IDs
visible in the API -- the actual values are meaningless to the DDSI
stack anyway.
Signed-off-by: Erik Boasson <eb@ilities.com>
Code formatting was quite a mess (different indentation, completely
different ideas on where opening braces should go, spacing in various
places, early out versus single return or goto-based error handling,
&c.). This commit cleans it up.
A few doxygen comment fixes allowed turning on Clang's warnings for
doxygen comments, so those are no enabled by default as least on
Xcode-based builds.
Signed-off-by: Erik Boasson <eb@ilities.com>
* Remove dds_return_t / dds_retcode_t distinction (now there is only
dds_return_t and all error codes are always negative)
* Remove Q_ERR_... error codes and replace them by DDS_RETCODE_...
ones so that there is only one set of error codes
* Replace a whole bunch "int" return types that were used to return
Q_ERR_... codes by "dds_return_t" return types
Signed-off-by: Erik Boasson <eb@ilities.com>
A long-standing bug of Cyclone is that a sample written immediately
after a publication-matched event may never arrive at the reader that
was just matched. This happened because the reader need not have
completed discovery of the writer by the time the writer discovers the
reader, at which point the reader ignores the sample because it either
doesn't know the writer at all, or it hasn't yet seen a Heartbeat from
it.
That Heartbeat arrives shortly after, but by then it is too late: the
reader slaves decides to accept the next sample to be written by the
writer. (It has no choice, really: either you risk losing some data, or
you will be requesting all historical data, which is empathically not
what a volatile reader is about ...)
A related issue is the handling of historical data for transient-local
readers: it used to deliver this out-of-order, but that is firstly
against the specification, and secondly, against reasonable expectations
of those who use DDS as a mere publish-subscribe messaging system. To
add insult to injury, it didn't completely handle some reordering issues
with disposes ...
This commit changes the way writers respond to a request for
retransmission from volatile proxy readers and the way the
in-sync/out-of-sync setting of a reader with respect to a proxy-writer
is used. The first makes it safe for a Cyclone reader to ask a Cyclone
writer for all data (all these details not being covered in the specs it
errs on the reasonable side for other vendors, but that may cause the
data loss mentioned above): the writer simply send a Gap message to the
reader for all the sequence numbers prior to the matching.
The second changes the rule for switching from out-of-sync to in-sync:
that transition is now simply once the next sequence number to be
delivered to the reader equals the next sequence number that will be
delivered directly from the proxy writer object to all readers. (I.e.,
a much more intuitive notion than reaching some seemingly arbitrary
sequence number.)
To avoid duplicates the rule for delivery straight from a proxy writer
has changed: where samples were delivered from the proxy writer to all
matching readers, they are now delivered only to the matching readers
that are in-sync. To avoid ordering problems, the idea that historical
data can be delivered through the asynchronous delivery path even when
the regular data goes through the synchronous delivery path has been
abandoned. All data now always follows the same path.
As these same mechanisms are used for getting historical data into
transient-local readers, the ordering problem for the historical data
also disappeared.
The test stuff in src/core/xtests/initsampledeliv covers a lot of the
interesting cases: data published before the existene of a reader, after
it, mixes of volatile and transient-local. Running them takes quite a
bit of time, and they are not yet integrated in the CI builds (if ever,
because of that time).
Note: the "conservative built-in startup" option has been removed,
because it really makes no sense to keep a vague compatibility option
added a decade ago "just in case" that has never been used ...
Note: the workaround in the src/mpt/tests/basic/procs/hello.c (use
transient-local to ensure delivery of data) has been removed, as has
been its workaround for the already-fixed #146.
Signed-off-by: Erik Boasson <eb@ilities.com>
If a firewall blocks traffic over one network but not another, and the
one happens to be the default pick of Cyclone, then the MPT basic tests
won't work properly.
With the recent changes to the configuration handling for supporting
multiple sources of configuration, it makes far more sense to remove the
hardcoded network interface selection in the test configurations and to
append the test configuration file to the environment list rather than
to replace it.
Signed-off-by: Erik Boasson <eb@ilities.com>
The multi-process test driver program waits until all its children have
stopped (or timeout) by calling ddsrt_proc_waitpids in a loop. The way
the loop is written, it can try once more when all children have already
been waited for, in which case it reports a spurious error.
This fixes that problem by checking whether the error code was to be
expected.
Signed-off-by: Erik Boasson <eb@ilities.com>
With the introduction of FreeRTOS support, the requirement that the
underlying platform supports multiple processes became untenable, and
therefore the code for dealing with multiple processes is now
automatically detected via some CMake code and then effectuated in the
sources via a macro (DDSRT_HAVE_MULTI_PROCESS). Unfortunately, a typo
resulted in all platforms being treated as if without support for
multiple processes. Fortunately, at this stage the only consequence was
the disabling of the first few multi-process tests.
Signed-off-by: Erik Boasson <eb@ilities.com>
Sometimes it can be useful to force all data transmissions to go out via
multicasts, instead of using a unicast when a single unicast suffices.
This commit adds a General/PreferMulticast setting that, when set to
true, implements this behaviour.
It is "PreferMulticast" because readers that only advertise unicast
addresses will still receive the data via unicast.
The default behaviour is unchanged.
Signed-off-by: Erik Boasson <eb@ilities.com>
Two bits of the DDSI encoding "options" field are used by the XTypes
spec to indicate the amount of padding that had to be added at the end
to reach the nearest 4-byte boundary as required by the DDSI message
format.
These bits are now set in according with the spec, and for received
samples, the padding is subtracted from the inferred size of the data so
that, e.g., a struct T { octet x; } will never deserialise as a struct S
{ octet x, y; }.
Signed-off-by: Erik Boasson <eb@ilities.com>
Since macOS 10.12, the POSIX clock_gettime interface with various
different clocks is supported on macOS, so use those when building for
10.12 or newer.
Signed-off-by: Erik Boasson <eb@ilities.com>
The discovery data is sent asynchronously (via the "tev" thread, i.e.,
via the "xeventq"), but any messages generated but not yet sent upon
destruction of the event queue were discarded rather than sent out.
Deleting the last participant triggers the shutdown of Cyclone, and the
participant discovery message informing the world of its disappearance
always got discarded. Consequently, all other nodes would not become
aware of the disappearnce until the lease expired.
This commit changes the behaviour to send out those last few messages
and absent packet loss, all peers are now properly informed.
Signed-off-by: Erik Boasson <eb@ilities.com>
The CDR deserializer failed to check it was staying within the bounds of
the received data, and it turns out it also was inconsistent in its
interpretation of the (undocumented) serializer instructions. This
commit adds some information on the instruction format obtained by
reverse engineering the code and studying the output of the IDL
preprocessor, and furthermore changes a lot of the types used in the
(de)serializer code to have some more compiler support. The IDL
preprocessor is untouched and the generated instructinos do exactly the
same thing (except where change was needed).
The bulk of this commit replaces the implementation of the
(de)serializer. It is still rather ugly, but at least the very long
functions with several levels of nested conditions and switch statements
have been split out into multiple functions. Most of these have single
call-sites, so the compiler hopefully inlines them nicely.
The other important thing is that it adds a "normalize" function that
validates the structure of the CDR and performs byteswapping if
necessary. This means the deserializer can now assume a well-formed
input in native byte-order. Checks and conditional byteswaps have been
removed accordingly.
It changes some types to make a compile-time distinction between
read-only, native-endianness input, a native-endianness output, and a
big-endian output for dealing with key hashes. This should reduce the
risk of accidentally mixing endianness or modifying an input stream.
The preprocessor has been modified to indicate the presence of unions in
a topic type in the descriptor flags. If a union is present, any
memory allocated in a sample is freed first and the sample is zero'd out
prior to deserializing the new value. This is to prevent reading
garbage pointers for strings and sequences when switching union cases.
The test tool has been included in the commit but it does not get run by
itself. Firstly, it requires the presence of OpenSplice DDS as an
alternative implementation to check the CDR processing against.
Secondly, it takes quite a while to run and is of no interest unless one
changes something in the (de)serialization.
Finally, I have no idea why there was a "CDR stream" interface among the
public functions. The existing interfaces are fundamentally broken by
the removal of arbitrary-endianness streams, and the interfaces were
already incapable of proper error notification. So, they have been
removed.
Signed-off-by: Erik Boasson <eb@ilities.com>
If at startup the requested interface cannot be found (or no suitable
interface exists), the code failed to release all memory allocated in
the process.
Signed-off-by: Erik Boasson <eb@ilities.com>
In all cases where read/take allocates memory for storing samples but
the result turns out to be an empty set, the (observable) state of the
system should end up unchanged.
It turns out several cases were/are considered:
* application supplies buffers (i.e., buf[0] != NULL): no memory
allocated, so no issue.
* reader has no cached set ("m_loan" in the current code): read/take
allocated memory, cached the address and marked it as in use
("m_loan_out"), and modified buf[0] (and subsequent entries).
To undo this on returning an empty set, it now: resets the
"m_loan_out" flag to allow the cached buffer to be reused, and sets
buf[0] back to NULL.
* reader has a cached set, but it is not marked in use: read/take
marked it as in use and modified buf[0] (and subsequent entries).
To undo this, it now resets "m_loan_out" to indicate the cached buffer
is not in use, and sets buf[0] back to NULL.
* reader has a cached set that is currently in use: read/take allocated
memory and updated buf[0] (and subsequent entries) but left the cached
state alone.
To undo this, it now frees the memory and sets buf[0] back to NULL.
With this, in any path where the application lets dds_read/dds_take
allocate memory for the samples:
* it can still safely call dds_return_loan with buf[0] and the actual
return value of read/take (even if an error code), and whatever memory
was allocated will not be leaked;
* but it no longer has to do so when the result was empty (or error).
Signed-off-by: Erik Boasson <eb@ilities.com>
This commit changes a few things in the config handling:
* When reading the configuration from multiple sources, a source can now
override settings already set by a preceding source for settings that
are not lists. Previously, trying to change the value of a setting in a
subsequence file would be considered an error, just like trying to set
the value of a particular setting multiple times in a single
configuration file.
* A configuration fragment in CYCLONEDDS_URI now no longer requires the
CycloneDDS top-level tag to be specified. If it is missing it will be
assumed. This is only true for configuration fragments contained in
CYCLONEDDS_URI, not for data read from a file.
* A configuration fragment in CYCLONEDDS_URI no longer requires that all
elements are properly closed: a missing close tag is treated as-if it
is the end of the fragment and any elements are implicitly closed.
Again this does not apply to files.
* The configuration dump now lists explicitly which sources affected
each setting, with a default value indicated by an empty set.
The result of the latter two is that one can almost pretend that it is a
sane format instead of XML. For example, if one would like to override
tracing settings, one could just write:
CYCLONEDDS_URI="$CYCLONEDDS_URI,<Tracing><Verbosity>finest"
Signed-off-by: Erik Boasson <eb@ilities.com>
* As a simple matter of code hygiene, in particular to aid in checking for
leaks, ddsperf should free all memory it allocates on exit.
* Remove spurious mutex unlock in ddsperf
* Removing a participant means removing one or two entries from the "pong
writers" array ("pong wr"), and there it read 1 element beyond the end
of the array while moving the remaining elements forward.
* Constant-rate pinging was broken because of two reasons, one worse than
the other:
* setting the rate had a mismatch in variables (publication rate and
command-line argument) resulting in a completely wrong ping interval;
the code now has a bit more clear variable naming ...
* the timing of the pings was relative to the current time, but the
wakeup a little delayed, resulting in a lower rate than requested.
It now simply adds the ping interval to the scheduled ping time, rather
than the time at which the ping is being sent. To guard against really
late wakeups, rates that are too high, suspending the machine, &c. it
will in extremis delay the next ping.
Signed-off-by: Erik Boasson <eb@ilities.com>
This API consists of only header files which are grabbed from spec IDL. The dynamically loaded plugins should implement API functions.
Built-in plugins will be added to the repository later. Third party plugins will also be able to be integrated.
Signed-off-by: Kurtulus Oksuztepe <kurtulus.oksuztepe@adlinktech.com>
The primary reason is that this allows the implementator of the sertopic
to freely select an allocation strategy, instead of being forced to
allocate the sertopic itself and the names it contains in the common
header with ddsrt_malloc. The secondary reason is that it brings it in
line with the serdata.
Signed-off-by: Erik Boasson <eb@ilities.com>
The name parameter and the name in the sertopic parameter had to match
because it used the one as a key in a lookup checking whether the topic
exists already, and the other as key for the nodes in that index. As
the name is (currently) included in the sertopic, it shouldn't be passed
in separately as well.
Signed-off-by: Erik Boasson <eb@ilities.com>
The thread state management used by the GC and the liveliness monitoring
lazily creates entries for application threads that call (certain)
Cyclone API functions. It arranges for the entry allocated to such a
thread to be cleared once the thread terminates.
This means that if such a thread still exists once the last participant
is deleted (and Cyclone deinitializes), the corresponding thread entry
still exists as well. The assertion that all (known) threads must have
stopped on shutting down Cyclone is therefore incorrect.
This commit introduces a special state for thread entries that were
created lazily. It does monitor that they do not stay in the "awake"
state for too long, but otherwise doesn't care much about them.
Signed-off-by: Erik Boasson <eb@ilities.com>
The Cyclone DDS configuration is in principle an XML document, but it is
possible to write configuration fragments directly in the CYCLONEDDS_URI
environment variable. In that case, it is quite annoying to have to
enter the full closing tags all the time, and so it now allows closing
elements with a simple </> when not reading them from a file.
While it would be trivial to also allow this when reading the
configuration from a file, it seems that promulgating invalid XML would
be bad form ... and besides, in that case editors can help keep
everything in order.
Signed-off-by: Erik Boasson <eb@ilities.com>
The XML parser has two modes: it can parse a file or parse a
caller-owned string. In the former case, it owns its buffer and shifts
the contents to make room for more data read in from the file. This
shifting may not happen when parsing a string.
Signed-off-by: Erik Boasson <eb@ilities.com>
The entirely historical "DDSI2E" element within the CycloneDDS
configuration element is herewith eliminated. All settings contained in
that element (such as General, Discovery, Tracing) are now subelements
of the CycloneDDS top-level element. Old configurations continue to
work but will print a deprecation warning:
//CycloneDDS/DDSI2E: settings moved to //CycloneDDS
Any warnings/errors related for an element //CycloneDDS/DDSI2E/x will be
reported as errors for the new location, that is, for //CycloneDDS/x.
As the "settings moved" warning always precedes any other such warning,
confusion will hopefully be avoided.
Signed-off-by: Erik Boasson <eb@ilities.com>
The Internal/SuppressSPDPMulticast setting was one of several ways to
prevent the sending of participant discovery multicast messages while
still allowing multicast to be used for data communications. That
functionality has long since been subsumed by the AllowMulticast setting
(AllowMulticast = spdp,amc & Internal/SuppressSPDPMulticast is
equivalent to AllowMulticast = amc).
Signed-off-by: Erik Boasson <eb@ilities.com>
These settings all constitute settings from the long history of the DDSI
stack predating Eclipse Cyclone DDS and can reasonably be presumed never
to have been used in Cyclone. Their removal is therefore not expected
to break backwards compatibility (which would be anyway be limited to
Cyclone complaining about undefined settings at startup):
* Tracing/Timestamps[@absolute]: has always been ignored
* Tracing/Timestamps: has always been ignored
* General/EnableLoopback: ignored for quite some time, before that
changing it from the default resulted in crashes.
* General/StartupModeDuration: it did what it advertised (retain data in
the history caches of volatile writers as-if they were transient-local
with a durability history setting of keep-last 1 for the first few
seconds after startup of the DDSI stack) but had no purpose other than
complicating things as the volatile readers ignored the data anyway.
* General/StartupModeCoversTransient: see previous -- and besides,
transient data is not supported yet in Cyclone.
* Compatibility/RespondToRtiInitZeroAckWithInvalidHeartbeat: arguably a
good setting given that DDSI < 2.3 explicitly requires that all
HEARTBEAT messages sent by a writer advertise the existence of at least
1 sample, but this has been fixed in DDSI 2.3. As this requirement was
never respected by most DDSI implementations, there is no point in
retaining the setting, while it does remove a rather tricky problem
immediately after writer startup involving the conjuring up of a
sample that was annihilated immediately before it could have been
observed.
That conjuring up (as it turns out) can cause a malformed message to go
out (one that is harmless in itself). Fixing the generation of that
malformed message while the entire point of the trick is moot in DDSI
2.3 is a bit silly.
Note that full DDSI 2.3 compliance needs a bit more work, so not
bumping the DDSI protocol version number yet.
* Compatibility/AckNackNumbitsEmptySet: changing it from 0 breaks
compatibility with (at least) RTI Connext, and its reason for
existence disappers with a fix in DDSI 2.3.
* Internal/AggressiveKeepLastWhc: changing the setting from the default
made no sense whatsoever in Cyclone -- it would only add flow-control
and potentially block a keep-last writer where the spec forbids that.
* Internal/LegacyFragmentation: a left-over from almost a decade ago when
it was discovered that the specification was inconsistent in the use
of the message header flags for fragmented data, and this stack for a
while used the non-common interpretation. There is no reasonable way of
making the two modes compatible, and this setting merely existed to
deal with the compatibility issue with some ancient OpenSplice DDS
version.
* Durability/Encoding: historical junk.
* WatchDog and Lease: never had any function in Cyclone.
Signed-off-by: Erik Boasson <eb@ilities.com>
High sample rates require rather high rates of allocating and freeing
WHC nodes, serialised samples (serdata), and RTPS message fragments
(xmsg). A bunch of dedicated parallel allocators help take some
pressure off the regular malloc/free calls. However, these used to
gobble up memory like crazy, in part because of rather generous limits,
and in part because there was no restriction on the size of the samples
that would be cached, and it could end up caching large numbers of
multi-MB samples. It should be noted that there is no benefit to
caching large samples anyway, because the sample rate will be that much
lower.
This commit reduces the maximum number of entries for all three cases,
it furthermore limits the maximum size of a serdata or xmsg that can be
cached, and finally instead of instantiating a separate allocator for
WHC nodes per WHC, it now shares one across all WHCs. Total memory use
should now be limited to a couple of MB.
The caching can be disabled by setting ``FREELIST_TYPE`` to
``FREELIST_NONE`` in ``q_freelist.h``.
Signed-off-by: Erik Boasson <eb@ilities.com>
Multiplying time-in-ns since previous output line by 1e9 instead of
dividing it by 1e9 resulted in bit rate showing up as 0Mb/s.
Signed-off-by: Erik Boasson <eb@ilities.com>
nn_bitset_one sets the specified number of bits by first memset'ing the
words, then clearing bits set in a final partial word. It mishandled
the case where the number of bits is a multiple of 32, clearing the
entire word following the last one it was to touch.
Signed-off-by: Erik Boasson <eb@ilities.com>
Generally one doesn't need to include any internal header files in an
application, but the (unstable) interface for application-defined sample
representation and serialization does require including some. It turns
out a keyword clash had to be resolved (typename => type_name) and that
a whole bunch of them were missing the #ifdef __cplusplus / extern "C"
bit.
It further turned out that one had to pull in nearly all of the type
definitions, including some typedefs that are illegal in C++, e.g.,
typedef struct os_sockWaitset *os_sockWaitset;
C++ is right to forbid this, but Cyclone's header files were wrong to
force inclusion of so much irrelevant stuff. This commit leaves these
typedefs in place, but eliminates a few header file inclusions to avoid
the problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
