PubSub DoubleBuffer rewrite
Description
The SOPC_DoubleBuffer
module is not maintainable in its current state, and cross-compilation reveals some alignment problems:
/S2OPC/src/PubSub/real_time_api/sopc_doublebuffer.c:86:36: error: cast increases required alignment of target type [-Werror=cast-align]
pBuffer->pReadersCounter = (uint32_t*) (pBuffer->doubleBuffer + bufferSizeInBytes * 2);
Analysis
This is mainly due to the fact that the double buffer hides its internal structure in an internal blob that is manually aligned.
All fields are uint32_t
, but this size seems not always relevant.
It seems that the main use of this structure is to have safe access to structures in the context of 1 writer and multiple readers. The readers shall be able to access the stored structure in an atomic fashion, and the writer shall be able to update some of its fields. This lead in the current API to lots of duplicate code that mimics classes, such as:
SOPC_ReturnStatus SOPC_InterruptTimer_Instance_SetPeriod(SOPC_InterruptTimer* pTimer, ...);
SOPC_ReturnStatus SOPC_InterruptTimer_Instance_SetOffset(SOPC_InterruptTimer* pTimer, ...);
SOPC_ReturnStatus SOPC_InterruptTimer_Instance_SetCallback(SOPC_InterruptTimer* pTimer, ...);
SOPC_ReturnStatus SOPC_InterruptTimer_Instance_SetData(SOPC_InterruptTimer* pTimer, ...);
which are all based on the same code:
- if the update is to be commited:
GetWriterBuffer
thenWriteBuffer(offsetof(struct, field_to_modify), writeBefore, writeAfter)
which copy the current value then modify the field, thenReleaseWriteBuffer
which commits the new field value, - if a field is required by the writer: avoid
GetReaderBuffer
which would increment the reader counter and use side-effects ofWriteBuffer
withoutReleaseWriteBuffer
to fetch the last value without update.
TODO
Verify the behavior of the SOPC_DoubleBuffer_GetReadBuffer
function when more than nbBuffers
readers are used.
For now, it looks like that the pReadersCounter[i]
value is maxed to nbBuffers
but without error,
which will hide the fact that readers may still have "acquired" the queue element when pReadersCounter[i]
reaches 0 because some of the readers "released" the element.
Then the writer may write in cells still acquired by readers
Another tricky case: in case we write in lastRecord
(i=0
in the for loop in GetWriteBuffer
), it may happen that the buffer is read while we're writing it, so we change its status and reader may read different value depending on when they read.
Another tricky case: the ignorePrevious
flag makes the function copy the most recent change (before the one that is being made) in the buffer. Obviously, this feature should be removed (there should not be a copy of some data in a call to WriteGetPtr
, as this is a striking side-effect), but the MsgBox seems to rely on this behavior. In doubt, reproduce the existing behavior.
Another tricky case: the GetWriteBuffer
and ReleaseWriteBuffer
shall always be called by pair BUT not if you want to cancel the write. Suggestion is to add an argument to cancel the write, but still call the function.
Another tricky case: the GetWriteBuffer
is called in SOPC_InterruptTimer_Instance_LastStatus
without ever calling ReleaseWriteBuffer
, which seems to mean that the write is never intended to be realized. Then we notice that the WriteBufferGetPtr
function is called with ignorePrevious = false
, meaning that the last valid value is copied to the buffer returned by the function. All in all, it seems, without being 100% sure, that the intent is in fact to read the content of the last buffer, without consuming a reader seat.
Changes to apply
-
Make a substructure for the elements, and replace the blob by an array of elements (which should remove the warnings) -
Add a verification that ReadBuffer
andReadBufferPtr
are correctly called between calls toGetReadBuffer
andReleaseReadBuffer
-
Add Mutex/Lock mechanism to write operations to assert there thread safety -
Remove uses of __atomic_
which are not compiler-independent -
Maybe rename/remove the ignorePrevious/After
fromWrite
andRead
functions- Either rename to more explicit
copyPrevious
,copyBefore
,copyAfter
- or move this functionality in other functions (maybe regroup with
WriteBufferErase
)
- Either rename to more explicit
-
Add a cancel argument to ReleaseWriteBuffer
for cases when we don't want to commit data -
Add unit tests