Code review: pubsub
General
-
(internal note: see our internal trac for more info) -
Rework the "real-time" API -
It should be transparent for the end-user ( resolutionMicroSeconds
should not appear in the API) -
(!672 (merged)) Is the creation of a dedicated thread SOPC_RT_Publisher_ThreadHeartBeatCallback
the best solution we have when we don't have an IRQ (I'm thinking abouttimer_create
withSIGEV_THREAD
that would be hopefully more regular than our currentsleep
-based wait) ? -
Put a configuration flag in the cmake configuration -
(!672 (merged)) If possible, remove SOPC_PUBSCHEDULER_HEARTBEAT_FROM_IRQ
and all#if
that uses it. -
(!672 (merged)) Design structures to make them aware whether they are compiled with the RT API or not, and prevent runtime ABI incompatibilities (prevent structures from changing sizes or make sure they are internal so that user can malloc them)
-
-
Use the S2OPC log system instead of printf -
At least define a printf_deb
instead of#ifdef DEBUG \n printf(...) \n #endif
-
Remove bangs and smileys and windows carriage returns \r
-
-
Check if volatile
qualifier is necessary in the pubsub implementation -
Remove all uses of __atomic_
which are not C99 (use either theAtomic
module orMutex
es) -
Remove all __attribute__
which are not C99 nor portable (if the feature is required, use a#define
that changes depending on the compiler to minimize compilation issues) -
Remove all code that implicitly re-invent the struct
with manual computation of offsets, such as in:-
(!672 (merged)) SOPC_DoubleBuffer #841 (closed) -
(!672 (merged)) SOPC_MsgBox and its inline events stored in a memory buffer in a double buffer that seemingly rely on an incorrectly implemented side effect of the double buffer to run correctly (does it really run correctly?) -
(!672 (merged)) SOPC_InterruptTimer same remarks
-
-
Remove calls to functions with side effects in if
statements, such as:-
SOPC_PubScheduler_Start
-
- Rename some functions that do more than their current name imply
-
SOPC_PubScheduler_Connection_Get_Transport
also creates sockets
RT API
-
(!672 (merged)) ptrCallbackStart
/Stop
/Send
fromrt_publisher
are the same types assopc_irq_timer_cb_start
/_stop
/_period_elapsed
frominterrupttimer
, they should be merged -
(!672 (merged)) ptrCallbackStart
is used in place ofStop
inSOPC_RT_Publisher_ConfigureMessage
-
(!672 (merged)) Break SOPC_RT_Publisher_VarMonitoringCallback
into subfunctions to reduce its length and indentation level -
(!672 (merged)) Refactor msgbox
andinterrupttimer
modules which seem to have lots of duplicate code (e.g.InterruptTimer_Instance_SetPeriod
which reserves a write buffer in its double buffer to update a field of its structs) -
Handle EINTR
inSOPC_RealTime_SleepUntil
and document it -
SOPC_RealTime_SleepUntil
is not defined insopc_time.h
but only inlinux/p_time.h
. It should be part of the interfaces to be ported with each OS. Moreover, the return value is not documented and seem to be returning true in case of error and false in case of success, which is not the usual convention.
RT side-effects
-
SOPC_PubSheduler_GetVariableRequestContext
is defined insopc_pub_source_variable.h
but is only used in examples; it is suspected that it is used in other examples, but if it is the case, it should be documented properly. -
each subscriber has its own sequence number that is initialized in SOPC_SubScheduler_Reader_Ctx_Create
but the publishers share the same global variablepubSchedulerCtx.sequenceNumber
(which is anonymously typed, and which size changes depending on the configuration because of#define
s)
Configuration
-
overall, the configuration structures replicate the OPC UA structures defined in the Part 14 of its specification, but the specification does not state that these structures are static or known in advance (some of the information in them, such as the PublisherIds, may not be known in advance to all subscribers, making our configuration somewhat difficult) ; I agree that it is good to find some of the key concepts of the PubSub in our configuration, but not all, as they make it overwhelmingly difficult to use... -
array lengths in sopc_pubsub_conf.c
should besize_t
, as they define limits in memory. We discussed many times whether lengths should be represented insize_t
(because it's the C type), or inu/int16/32_t
(because of the serialization of the structs in OPC UA), and maybe a better middle ground that would avoid cast is to have defines like#define size_t_at_most16
which gets the signedness of the serialized type but also represent the maximalsize_t
on that platform -
SOPC_PublishedDataItemsData
seems unused -
SOPC_SubscribedDataSetType
does not correspond to a type described in the Part 14 § 6.2.8 so it should be documented
Decoder/Encoder
-
sum up the current limitations (such as "only one DataSetMessage per NetworkMessage") -
remove silent failures (such as if (0 != dataSmessage_type)
inSOPC_UADP_NetworkMessage_Decode
, orif (DATASET_LL_DSM_ENCODING_TYPE != field_encoding)
): at least log, maybe also assert false -
the subscriber should not stop when SOPC_UADP_NetworkMessage_Decode
failed when unable to decode a message, because it could simply mean that a non-OPC UA message is received on this public multicast address. The code should be able to sort out whether there is an internal fatal error in decoding the message, an unknown security key, a signature problem, or simply a message that should not be parsed by the subscriber because it is not in the OPC UA format. The architecture of the decoding layers should take these requirements into account.
Tests
-
Rewrite RT tests to use libcheck
-
Remove goto
s, cascadedelse if
,printf
s,\r
s,#ifdef
s, and useck_assert*
-
Factorize functions if possible
Miscellaneous
-
SOPC_RT_Publisher_StartPubMsgCallback, // Not used
callingSOPC_RT_Publisher_Initializer_AddMessage
. Remove this argument if not used -
Remove empty trailing comments to let clang-format
format the code automatically -
Find a thread safe alternative to all calls to non-thread safe strerror
or change theTODO
s towarning
s ornote
s (!672 (comment 606644822)) -
on_mqtt_message_received
does not handleelse
cases (both on size of status) nor the return status ofon_message_received
-
Same remarks for on_udp_message_received
andon_message_received
-
Move the SOPC_PubSubState
structure away fromsopc_sub_scheduler.h
or rename itSOPC_SubState
(or another alternative) -
Rewrite init_sub_scheduler_ctx
to reduce maximum indentation level which violates our current coding rules (and to reduce its length)
Edited by Jérémie Chabod