Follow-up from "Port S2OPC to FreeRTOS"
The following discussions from !392 (merged) should be addressed:
-
( ➡ !461 (merged)) @brameret started a discussion:This function can be simplified and enhance for readability as follows:
-
const int trueInt
declaration can be moved to the single block were it is used - an early return can reduce the indentation level
- declaring and initializing
setOptStatus
later may simplify the control flow - why is
setOptStatus
sometimes compared to-1
and other times< 0
? The manual page states that either0
or-1
is returned
-
-
( ➡ !461 (merged)) @brameret started a discussion:return true
is ok.Is it the targeted behavior?
Same comment for
SOPC_Socket_Network_Clear
. -
( ➡ code changed, but comments forSOPC_Socket_CreateNew
should be applied here) (➡ !461 (merged)) @brameret started a discussion:return SOPC_STATUS_INVALID_PARAMETERS;
is ok.Is it the targeted behavior?
-
( ➡ !461 (merged)) @brameret started a discussion:SOMAXCONN
is the name of the reserved linux constant that does not exist under freertos. Maybe it would be less ambiguous to change it.This seems to be the maximum number of pending connections, could this macro be named
SOPC_MAX_PENDING_CONN
? -
( ➡ !461 (merged)) @brameret started a discussion:I can't find whether this behavior is relevant under lwIP.
@nottin: lwip api implement POSIX behaviour.
-
( ➡ code changed) @brameret started a discussion:return SOPC_STATUS_INVALID_PARAMETERS;
is ok.Is it the targeted behavior?
-
( ➡ unchanged) @brameret started a discussion:Are these errors relevant under freertos?
-
( ➡ unchanged) @brameret started a discussion:Are these errors relevant under freertos?
-
( ➡ !461 (merged)) @brameret started a discussion:else
invalid parameters ? Or just NOK ? Can this be merged with the followingstatus = NOK
for all cases<= -1
or even< 0
? -
( ➡ !461 (merged)) @brameret started a discussion:This call has a side effect and cannot be used in an
if
instruction.Is this legit to not reset the
sock
whenclose
failed? -
( ➡ !461 (merged)) @brameret started a discussion:The call to
getsockopt
shall be made outside of theif
instruction, as it has side effects.The function can be greatly simplified with the use of an early return.
-
( ➡ Fixed) @brameret started a discussion:The allocated stack will be more than 128 bytes.
-
( ➡ Fixed) @brameret started a discussion:Day of month is padded with a space in
__DATE__
when it is< 10
. May this break the parsing? As the format is always "Mmm DD YYYY", I think it is possible to usebuffer[4]
. And maybe also somebuffer[3] = '\0';
... -
( ➡ Fixed) @brameret started a discussion:Single statement block must be surrounded by curly braces.
-
( ➡ Obsolete) @brameret started a discussion:Can this be broke similarly by the presence of a left-padding space in
ptrDay
? -
( ➡ Fixed) @brameret started a discussion:atoi
returns anint
. -
( ➡ Obsolete) @brameret started a discussion:May this overflow ?
-
( ➡ Fixed) @brameret started a discussion:I understand that this function computes a timestamp compatible with the usual Linux timestamp (which EPOCH is 01/01/1970 00:00:00 UTC) to reuse the functions
SOPC_Time_FromTimeT
.I think that this is a good idea.
However,
mktime
and others are available on this platform (are they always available, or is it justnewlib-semihost
?). Moreover, newlib's EPOCH is the same as Linux's EPOCH. You can hence simplify this function greatly. -
( ➡ Fixed) @brameret started a discussion:gBuffer
is solely used inP_TIME_vSetInitialDateToBuild
, it may be moved there. -
( ➡ Fixed) @brameret started a discussion:It may be interesting to have mutable buffers to store the compilation time and date, so that unit tests with reference dates are possible.
-
( ➡ Fixed) @brameret started a discussion:CanThe public API isint64_t dt
be re-typed/namedSOPC_DateTime currentUTC
, as this is the result of this function ?int64_t SOPC_Time_GetCurrentTimeUTC(void);
whereas this function returns aSOPC_DateTime
. -
( ➡ Fixed) @brameret started a discussion:SOPC_Time_FromTimeT
requires atime_t
. The C specification does not specify a size fortime_t
. This variable should be atime_t
. -
( ➡ Fixed) @brameret started a discussion:We should check that
wTimeInTicks/configTICK_RATE_HZ
is less thanINT64_MAX
before casting it. However, as said in a previous comment, we are in fact converting to atime_t
. AsTIME_T_MAX
or similar flags don't exist, I don't see proper options yet to check that overflow. We could check forsizeof(time_t)
and assume that it corresponds to a signed value of the same size, then useINTxx_MAX
to check for the overflow. -
( ➡ Fixed) @brameret started a discussion:SOPC_Time_FromTimeT
should be called outside of theif
.Maybe it would be clearer if
currentTimeIn100NS
is called something else, as I only understood on linedt += currentTimeIn100NS
what was its purpose, but I can't think of something better yet :)Moreover,
currentTimeIn100NS
cannot overflow (10,000,000 <INT64_MAX
). -
( ➡ Fixed) @brameret started a discussion:You can use two arrays which store the length of each month to simplify this
switch
.Or add
break
statements for eachcase
, which is required by our coding rules. -
( ➡ Fixed) @brameret started a discussion:I am not sure that this includes all leap seconds.
mktime(0)
has an interesting result to compensate for this. -
( ➡ Fixed) @brameret started a discussion:These 6 lines are not checked against errors.
-
( ➡ !461 (merged)) @v.monfort started a discussion: (+1 comment)Is the license compatible with our Apache 2.0 license ?
@nottin: ok, both Systerel and Apache licenses will be added later.
yes this license is compatible with our Apache 2.0 license. The two headers are kept.
-
( ➡ Obsolete) @v.monfort started a discussion:Add explanation on why we let it here for now ? e.g.: S2OPC declare this structure (and not a pointer of this structure) therefore its size shall be known
@nottin: comments deleted in the last source code. Todo: structure definition shall be moved later to C file, but currently S2OPC api needs to know its size. obsolete comment
-
( ➡ Obsolete) @v.monfort started a discussion:same remark @nottin: ;) same remark obsolete comment
-
( ➡ Fixed) @brameret started a discussion:We have to discuss the presence of these
#ifdef
s that reduce readability. We have three suggestions for now:- use a more generic
DEBUG
macro (instead ofFOLLOW_ALLOC
), - use an
DEBUG_incrementCpt()
that is either a function when needed or the empty macro (this configuration is made outside of this function, so it gets rid of the current#ifdef FOLLOW_ALLOC
) - use unit tests and remove this debug counter.
This comment applies to each mention to
FOLLOW_ALLOC
.Option 2 is chosen for now, as it is a trade-off between debug mode and readability.
- use a more generic
-
( ➡ follow-up follow-up #669 (closed)) @brameret started a discussion: (+1 comment)The
p_utils
defines a doubly linked list supported by an array of constant sizeMAX_P_UTILS_LIST
. This should be documented, for instance inP_UTILS_LIST_Init
.The reverse link is not used (yet ?) and may be deleted to simplify the code.
In the future, an issue should be created to use
SOPC_Array
, if it is possible to do so and keep the thread safety properties of these functions. -
( ➡ follow-up follow-up #669 (closed)) @brameret started a discussion:Fields of this structure may be renamed for clarity.
value
is always calledtaskHandle
outside this module.pContext
is always ahTread
and could be retyped?infosField1/2
are always signals. It seems thatpContext
andinfoFields
are never used in the same time. -
( ➡ !461 (merged)) @brameret started a discussion:The documentation of
xSemaphoreTakeRecursive
states that recursive semaphores must be created withxSemaphoreCreateRecursiveMutex
. Is this a problem? Okay, no, as it is a macro pointing to CreateMutex. However, it does protects against faulty configurations. @nottin : FreeRTOS context is necessary. xQueueCreateMutex with binary semaphore type shall be called. This is different from xSemaphoreCreateBinary, which need FreeRTOS task context. This is obscure, so xQueue API will be replaced by xSemaphore API (create mutex, create binary semaphore, create recursive mutex). -
( ➡ !461 (merged)) @brameret started a discussion:Or
vSemaphoreDelete
?@nottin: No. Same response. vQueueDelete used because xQueueXXX is used.
-
( ➡ Fixed) @brameret started a discussion: (+1 comment)I am not convinced that
firstPrevOQP < (wCurrentSlotId - 1)
corresponds to the comment "Slot free before this slot". I would rather test previous slot's value.@nottin: ok, some comments has been added.
-
( ➡ post-poned #603) @brameret started a discussion: (+1 comment)This function takes a
struct T_THREAD_WKS ***
. This may be cleaned by changing types and S2OPC API, cf. !392 (comment 181545723) -
( ➡ Fixed) @brameret started a discussion: (+2 comments)MAX_THREADS
,MAX_WAITERS
, andMAX_P_UTILS_LIST
are synonymous. Can you reduce the number of synonyms? <@nottin : no, limitations are same in that configuration but can be changed on other embedded target>. Moreover,MAX_P_UTILS_LIST
seems to be used in place ofMAX_WAITERS
inp_synchronisation.c
<@nottin: fixed.>
-
( ➡ partially fixed) @brameret started a discussion:Too many indentation levels + change test order to
NULL == variable
.This function can be simplified with early returns and a common
if
to handle all mem alloc error cases. -
( ➡ Fixed) @brameret started a discussion:This function can be simplified (+ change tests to
NULL == variable
). -
( ➡ Fixed) @brameret started a discussion:This function can be simplified (+ change tests to
NULL == variable
). -
( ➡ !461 (merged)) @brameret started a discussion:This function can be simplified (+ change tests to
NULL == variable
+ callxQueueTakeMutexRecursive
before theif
).@nottin : fixed.
-
( ➡ !461 (merged)) @brameret started a discussion:This function can be simplified (+ change tests to
NULL == variable
+ callxQueueGiveMutexRecursive
before theif
).@nottin : fixed.
-
( ➡ Fixed) @brameret started a discussion: (+1 comment)Using
SOPC_ReturnStatus
instead of customeConditionVariableResult
would greatly simplify this function.@nottin: already fixed
-
( ➡ !461 (merged)) @brameret started a discussion: (+2 comments)May overflow. Please check that
uwTimeOutMs <= (uint32_t) portMAX_DELAY / (uint32_t) configTICK_RATE_HZ
and setxTimeToWait
toportMAX_DELAY
otherwise.@nottin: already fixed. @brameret: not yet. @nottin: ok, last fixed has been refixed with your correction :
if ( (TickType_t) uwTimeoutMs / (TickType_t) 1000 >= (TickType_t) portMAX_DELAY / (TickType_t) configTICK_RATE_HZ)
-
( ➡ Won't fix) @brameret started a discussion: (+2 comments)Don't we need to check that
uwClearSignal
has bit31 set and thatuwSignal
has bit31 reset? @nottin: No, uwClearSignal != uwSignal. uwClearSignal is used to as "destroy request" signal of condition variable. uwSignal is waited signal. -
( ➡ !461 (merged)) @brameret started a discussion: (+1 comment)Use a
bool bQuit
andwhile (! bQuit)
. @nottin: ok, !bQuit used. If I understand correctly, we callxTaskNotifyWait
while the signal is not our signal, but we break if it is not our signal and we timed out anyway. Is that correct? @nottin: yes -
( ➡ !461 (merged)) @brameret started a discussion:Please move this function call out of the
if
statement. -
@brameret started a discussion: These functions have been deleted in a commit, and can be reverted later, you can leave them there and delete them again ;)
-
( ➡ !461 (merged)) @brameret started a discussion:Why is this reverted to 32?
-
( ➡ !461 (merged)) @brameret started a discussion:Use
SOPC_P_TIME_H_
(this format must also be changed in other headers) -
( ➡ !461 (merged)) @brameret started a discussion:These includes are not required by this
.h
but rather by the implementation. Please move them back there.Moreover, there might be useless imports (limits? stdlib? sys/time.h? task.h? ...)
-
( ➡ Fixed) @brameret started a discussion:Simplify to
while (UINT16_MAX != wCurrentSlotId && bSignalAll);
-
( ➡ !461 (merged)) @brameret started a discussion:Invert operands order.
-
( ➡ !461 (merged)) @brameret started a discussion:while (! bQuit)
-
( ➡ !461 (merged)) @brameret started a discussion:You added elements to the list (function with side effects), you cannot use an early return here.
-
( ➡ !461 (merged)) @brameret started a discussion:These includes were added lately. Why? I think
stdbool
is enough. -
( ➡ !461 (merged)) @brameret started a discussion:Why is this surrounded by
ifndef
?Can we choose a more explicit name, like
HOURS_LOCAL_TO_UTC
?