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>
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>
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>
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>
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 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>
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>
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>
The current situation for performance measurements and checking network
behaviour is rather unsatisfactory, as the only tools available are
``pubsub`` and the ``roundtrip`` and ``throughput`` examples. The first
can do many things thanks to its thousand-and-one options, but its
purpose really is to be able to read/write arbitrary data with arbitrary
QoS -- though the arbitrary data bit was lost in the hacked conversion
from the original code. The latter two have a terrible user interface,
don't perform any verification that the measurement was successful and
do not provide the results in a convenient form.
Furthermore, the abuse of the two examples as the primary means for
measuring performance has resulted in a reduction of their value as an
example, e.g., they can do waitset- or listener-based reading (and the
throughput one also polling-based), but that kind of complication does
not help a new user understand what is going on. Especially not given
that these features were simply hacked in.
Hence the need for a new tool, one that integrates the common
measurements and can be used to verify that the results make sense. It
is not quite done yet, in particular it is lacking in a number of
aspects:
* no measurement of CPU- and network load, memory usage and context
switches yet;
* very limited statistics (min/max/average, if you're lucky; no
interesting things such as jitter on a throughput test yet);
* it can't yet gather the data from all participants in the network
using DDS;
* it doesn't output the data in a convenient file format yet;
* it doesn't allow specifying boundaries within which the results
must fall for the run to be successful.
What it does verify is that all the endpoint matches that should exist
given the discovered participant do in fact come into existence,
reporting an error (and exiting with an exit status code of 1) if they
don't, as well as checking the number of participants. With the way the
DDSI protocol works, this is a pretty decent network connectivity check.
The raw measurements needed for the desired statistics (apart from
system-level measurements) are pretty much made, so the main thing that
still needs to be done is exploit them and output them. It can already
replace the examples for most benchmarks (only the 50%/90%/99%
percentiles are still missing for a complete replacement).
Signed-off-by: Erik Boasson <eb@ilities.com>
Most of the places where the status flags were reset, this happened
without holding m_observer_lock protecting these status flags. For most
of these statuses, they are only ever set/reset while also holding the
entity lock, but this is not true for all of them (DATA_AVAILABLE for
example), and thus there are some cases where retrieving the status
could lead to losing the raising of a (at least a DATA_AVAILABLE)
status.
The problem was introduced in ba46cb1140.
Signed-off-by: Erik Boasson <eb@ilities.com>
The DATA_AVAILABLE status was reset by read and take while holding the
upper-layer reader lock, but after completing the read/take operation on
the RHC. As data can be written into the RHC without holding the
upper-layer reader lock, new data could arrive in between the
reading/taking and the resetting of the DATA_AVAILABLE status, leading
to a missed detection. Resetting DATA_AVAILABLE prior to accessing the
RHC solves this.
Signed-off-by: Erik Boasson <eb@ilities.com>
Changes the semantics of dds_qget_{user,group,topic}data to always
append a 0 byte to any non-empty value without counting it in the size.
(An empty value is always represented by a null pointer and a size of
0). The advantage is that any code treating the data as the octet
sequence it formally is will do exactly the same, but any code written
with the knowledge that it should be a string can safely interpret it as
one.
Signed-off-by: Erik Boasson <eb@ilities.com>
As was the plan with the introduction of ddsrt; this includes renaming
the identifiers to match the capitalization style and removes old junk.
Signed-off-by: Erik Boasson <eb@ilities.com>
Extend the endpoint built-in topic data with the participant instance
handle (the GUID was already present). Having the instance handle
available makes it trivial to look up the participant, whereas a lookup
of the GUID is rather impractical.
Signed-off-by: Erik Boasson <eb@ilities.com>
Adds a new "ignorelocal" QoS to the readers/writers to ignore local
matching readers/writers, with three settings:
* DDS_IGNORELOCAL_NONE: default
* DDS_IGNORELOCAL_PARTICIPANT: ignores readers/writers in the same
participant
* DDS_IGNORELOCAL_PROCESS: ignores readers/writers in the same process
These can be set/got using dds_qset_ignorelocal and
dds_qget_ignorelocal.
If a matching reader or writer is ignored because of this setting, it is
as-if that reader or writer doesn't exist. No traffic will be generated
or data retained on its behalf.
There are no consequences for interoperability as this is (by
definition) a local affair.
Signed-off-by: Erik Boasson <eb@ilities.com>
The DDSI reader/writer pointers are now returned as out parameters
instead of as a return value, so that the upper-layer reference is set
before any listener can be invoked.
Signed-off-by: Erik Boasson <eb@ilities.com>
It was called strangely early in the deleting of the reader, even before
the DDSI reader was no longer being accessed by other threads. The
immediate and obvious problem is that it resets the pointer to the
upper-layer entity even though this can still be dereferenced in
invoking a listener, resulting in a crash.
Secondly it blocks until there are no listener calls any more (and the
resetting of that pointer will prevent any further listener
invocations), but a similar piece of logic is already in generic entity
code that resets the mask and then waits for all listener invocations to
complete. Having both is a problem.
Signed-off-by: Erik Boasson <eb@ilities.com>
If a (proxy) writer delivers data to a reader that has a data_available
listener calling read/take while that reader is being deleted, blocked
in set_listener waiting for the listeners to complete, then a deadlock
can occur:
* listener calling read/take then attempt to lock reader;
* deleting the reader locks the reader, then waits for the listeners to
complete while holding the lock
This commits unlocks the reader before waiting for the listeners to
complete.
Signed-off-by: Erik Boasson <eb@ilities.com>
Remove all the "if asleep then awake ..." stuff from the code by making
awake/asleep calls nestable, whereas before it "awake ; awake" really
meant a transition through "asleep". This self-evidently necessitates
fixing those places where the old behaviour was relied on upon, but
fortunately those are few.
Signed-off-by: Erik Boasson <eb@ilities.com>
The old entity handle mechanism suffered from a number of problems, the
most terrible one being that it would only ever allocate 1000 handles
(not even have at most 1000 in use at the same time). Secondarily, it
was protected by a single mutex that actually does show up as a limiting
factor in, say, a polling-based throughput test with small messages.
Thirdly, it tried to provide for various use cases that don't exist in
practice but add complexity and overhead.
This commit totally rewrites the mechanism, by replacing the old array
with a hash table and allowing a near-arbitrary number of handles as
well as reuse of handles. It also removes the entity "kind" bits in the
most significant bits of the handles, because they only resulted in
incorrect checking of argument validity. All that is taken out, but
there is still more cleaning up to be done. It furthermore removes an
indirection in the handle-to-entity lookup by embedding the
"dds_handle_link" structure in the entity.
Handle allocation is randomized to avoid the have a high probability of
quickly finding an available handle (the total number of handles is
limited to a number much smaller than the domain from which they are
allocated). The likelihood of handle reuse is still dependent on the
number of allocated handles -- the fewer handles there are, the longer
the expected time to reuse. Non-randomized handles would give a few
guarantees more, though.
It moreover moves the code from the "util" to the "core/ddsc" component,
because it really is only used for entities, and besides the new
implementation relies on the deferred freeing (a.k.a. garbage collection
mechanism) implemented in the core.
The actual handle management has two variants, selectable with a macro:
the preferred embodiment uses a concurrent hash table, the actually used
one performs all operations inside a single mutex and uses a
non-concurrent version of the hash table. The reason the
less-predeferred embodiment is used is that the concurrent version
requires the freeing of entity objects to be deferred (much like the
GUID-to-entity hash tables in DDSI function, or indeed the key value to
instance handle mapping). That is a fair bit of work, and the
non-concurrent version is a reasonable intermediate step.
Signed-off-by: Erik Boasson <eb@ilities.com>
The introduction of properly functioning query conditions adds some
overhead, this commit removes some of that cost by avoiding some calls
to update_conditions when there are no query conditions.
It also removes the has_changed field from the instance, instead using a
local boolean to track whether DATA_AVAILABLE should be raised or not.
Signed-off-by: Erik Boasson <eb@ilities.com>
Deleting a writer causes unregisters (and possibly disposes) in the rest
of the network, and these updates to the instances should trigger
DATA_AVAILABLE.
Signed-off-by: Erik Boasson <eb@ilities.com>
The rtps_init function used to initialize all data structures and start
all threads used by the protocol stack, allowing discovery of remote
entities before the built-in topic data structures were initialized.
(Very) early discovery of a remote participant thus led to a crash.
This commit splits the initialisation, providing a separate function for
starting, in particular, the threads receiving data from the network.
In terms of threads created, it matches exactly with the rtps_stop /
rtps_fini split that already existed to address the exact same problem
on termination.
Signed-off-by: Erik Boasson <eb@ilities.com>
This adds an Internal/EnableExpensiveChecks setting for enabling some or all expensive run-time checks to avoid a massive slowdown when assertions are enabled at compile-time. Currently these cover only the writer and reader-history cache checking.
Signed-off-by: Erik Boasson <eb@ilities.com>
- Replace os_result by dds_retcode_t and move DDS return code defines down.
Eliminates the need to convert between different return code types.
- Move dds_time_t down and remove os_time.
Eliminates the need to convert between different time representations and
reduces code duplication.
- Remove use of Microsoft source-code annotation language (SAL).
SAL annotations are Microsoft specific and not very well documented. This
makes it very difficult for contributers to write.
- Rearrange the abstraction layer to be feature-based. The previous layout
falsely assumed that the operating system dictates which implementation is
best suited. For general purpose operating systems this is mostly true, but
embedded targets require a slightly different approach and may not even offer
all features. The new layout makes it possible to mix-and-match feature
implementations and allows for features to not be implemented at all.
- Replace the os prefix by ddsrt to avoid name collisions.
- Remove various portions of unused and unwanted code.
- Export thread names on all supported platforms.
- Return native thread identifier on POSIX compatible platforms.
- Add timed wait for condition variables that takes an absolute time.
- Remove system abstraction for errno. The os_getErrno and os_setErrno were
incorrect. Functions that might fail now simply return a DDS return code
instead.
- Remove thread-specific memory abstraction. os_threadMemGet and accompanying
functions were a mess and their use has been eliminated by other changes in
this commit.
- Replace attribute (re)defines by ddsrt_ prefixed equivalents to avoid name
collisions and problems with faulty __nonnull__ attributes.
Signed-off-by: Jeroen Koekkoek <jeroen@koekkoek.nl>
The "rhc" test runs a random sequence of operations (writes, reads, &c.)
through an RHC with conditions attached to it. All possible state masks
are used, and query conditions are tried with a condition that only
tests the key value, and one that tests attribute values. It depends on
the internal checking logic of the RHC, which is currently enabled only
in Debug builds because of the associated run-time overhead.
Signed-off-by: Erik Boasson <eb@ilities.com>
Various details:
- index replaced throughout by bitmask
- caching a single sample in RHC for deserialising samples when query conditions are used
- short-circuiting the trivial case of an instance matched neither before nor after a change to its state
- combining inc/dec counters into a single delta in condition evaluation
- major speed-up of rhc_check_counts by not checking the condition bit masks every single time
Signed-off-by: Erik Boasson <eb@ilities.com>
The specification says the data in an invalid sample must not be inspected, but that means retrieving the key values from a disposed/unregistered instance after a take is impossible. So, the elegant thing to do is to always provide the key values in the data. The remaining question is then whether the non-key fields should be left in whatever state they happened to be in, or be set to zero. The latter is more consistent in the interface, and that is almost invariably a good thing.
Signed-off-by: Erik Boasson <eb@ilities.com>
