TimeframeReader and TimeframeWriter devices#370
Conversation
ktf
force-pushed
the
dev-timeframe-io-devices
branch
2 times, most recently
from
6baf5a3 to
fedab19
Compare
There was a problem hiding this comment.
@mkrzewic @matthiasrichter Do we have a better way of doing what I do below?
There was a problem hiding this comment.
if you mean setting the size: the default ctor does that for you - essentially all the members inherited from BaseHeader are set by the default ctor - they should not be touched by user code. Although since this is test code we might want to explicitly check if they are set correctly.
That being said, probably here you want to construct the header in place in the buffer you allocated.
Now the actual interface to construct buffers that hold metadata is the Stack class which packs a buffer with all the headers you might want to have in the header stack - it is a move only type so it plays nice with the transport layer (especially with the AddMessage(...) method in O2Device class). I need to clean this up a bit and finish separating the public and private interfaces - e.g. we should not initialize the description member with an arbitrary string - this is in the works.
example of how it was meant to be used is in Utilities/O2MessageMonitor.cxx
so in a test you could do something like:
DataHeader dataHeader;
dataHeader = gDataOriginAny; //set origin
dataHeader = gDataDescriptionInfo; //set data description
dataHeader = gSerializationMethodNone; //set serialization method for the payload
o2::Header::Stack headerStack{dataHeader};
byte* buffer = headerStack.data();
size_t size = headerStack.size();
//check the metadata buffer byte by byte somehow....
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
fedab19 to
32d62a7
Compare
matthiasrichter
left a comment
matthiasrichter
left a comment
There was a problem hiding this comment.
Nice! I can compile it and I also ran the test program. It's name should probably be testTimeframeParser.cxx to follow the naming conventions. A few minor comments inline.
There was a problem hiding this comment.
can be dh == Header::DataDescription("TIMEFRAMEINDEX");
There was a problem hiding this comment.
To support the continuous processing, we probably need to write one file per timeframe, with some configurable logic to create a file name from the time frame meta data or a simple enumeration.
There was a problem hiding this comment.
Yes, I am preparing something which generates a dummy timeframe without having to have to set up the whole chain, to use with the tests, but then I can extend the actual devices a bit. Actually I was thinking we might want to have different split logics, e.g.:
- one timeframe
- as many timeframes as they fit in N GBs
I was thinking also that the reader could use inotify or similar mechanism to read files as they get written to a given directory. Anyways, maybe we should spell out the actual user stories we want to support.
There was a problem hiding this comment.
indexHeader == Header::DataDescription("TIMEFRAMEINDEX")
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
32d62a7 to
c0fe7dc
Compare
|
Updated. It now provides an helper to create fake timeframes programmatically, to use them in the tests. This requires: |
ktf
force-pushed
the
dev-timeframe-io-devices
branch
5 times, most recently
from
efdfdc4 to
d015ec3
Compare
matthiasrichter
left a comment
matthiasrichter
left a comment
There was a problem hiding this comment.
I would set the two 'invalid' descriptors simply to 0, and not any string equivalent.
There was a problem hiding this comment.
I would use 'void' instead of 'invalid' in the name, this can probably be changed later, but in any case simply set it to 0
There was a problem hiding this comment.
I disagree. If I then printfs something which includes gDataOriginInvalid, the string will be truncated or empty. That's actually why I changed it (I was debugging things and the message looked ok when I realised something was not getting printed).
There was a problem hiding this comment.
ok, fair enough. 'INV' looks so ugly, 'VOID' would be nicer but then there is no terminating 0.
There was a problem hiding this comment.
keeping in mind what we always said about null termination here - I like it less and less... Maybe it would qualify for a rediscussion?
There was a problem hiding this comment.
then also for the longer ones: FFFFFFF\0 etc... to be consistent with the invalid hex values of 0xFFFFFFFF...
There was a problem hiding this comment.
same here, the invalid/void descriptor should simply be 0
There was a problem hiding this comment.
can this be removed?
There was a problem hiding this comment.
This is currently required to avoid that including SubframeMetadata.h requires including vector before it. What we really should do is to clean up SubframeMetadata.h and remove the various TPCTestCluster and ITSRawData classes which we are currently using to mockup the timeframe.
|
Error while checking build/O2/o2 for 7ddc18ee719cbc4157379600bc1fcdac2328a9f7: Full log here. |
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
7ddc18e to
23a5f15
Compare
ktf
changed the title
|
Error while checking build/O2/o2 for 23a5f15e20944260fc572ae7435ddc962749cf67: Full log here. |
|
@matthiasrichter anything else to do before the we merge this? In the end it's quite orthogonal. |
matthiasrichter
left a comment
matthiasrichter
left a comment
There was a problem hiding this comment.
@ktf I have added a few questions between the lines
Utilities/DataFlow/CMakeLists.txt
Outdated
There was a problem hiding this comment.
this should be removed
There was a problem hiding this comment.
Agree. We really need to have way to build only tests and related libraries in debug mode, though. @dberzano any ideas regarding this?
There was a problem hiding this comment.
Hm it is not clear to me what you want to do. Do you want to compile tests only with the debug options (-O0 -g), independently from the global ones?
There was a problem hiding this comment.
does this buffer need to be deleted? should fakeTimeframeGenerator return a unique_ptr?
There was a problem hiding this comment.
I'm puzzled here. It should be
if ((tpcHeader->dataDescription != o2::Header::DataDescription("CLUSTERS")) ||
(tpcHeader->dataOrigin != o2::Header::DataOrigin("TPC")))or the corresponding definitions. But how does it compile at all when using the const char strings? Neither DataDescription nor DataOrigin (aka Descriptor<4>) does define an operator==(const char*)
Looks like such operators need to be forbidden explicitly, I did a quick test.
There was a problem hiding this comment.
same here
if ((itsHeader->dataDescription != o2::Header::DataDescription("CLUSTERS"))
|| (itsHeader->dataOrigin != o2::Header::DataOrigin("ITS")))
|
I want an easy (e.g. make mytest DEBUG=true) option which will recompile my
test and the library used by the test using debug options. In a past life
of mine this was actually possible, any chance we can implement something
like that in our cmake macros (past life admittedly did not use Cmake)?
…On Mon, May 22, 2017 at 2:39 PM Dario Berzano ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Utilities/DataFlow/CMakeLists.txt
<#370 (comment)>:
> @@ -29,24 +34,31 @@ set(LIBRARY_NAME ${MODULE_NAME})
set(BUCKET_NAME ${MODULE_BUCKET_NAME})
O2_GENERATE_LIBRARY()
+target_compile_options(${LIBRARY_NAME} PUBLIC -O0 -g)
Hm it is not clear to me what you want to do. Do you want to compile tests
only with the debug options (-O0 -g), independently from the global ones?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#370 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAApMClj9O8-k6x92fGjva5UZSJ4tOaNks5r8YG3gaJpZM4NVcuU>
.
|
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
23a5f15 to
626c5d5
Compare
|
Error while checking build/O2/o2 for 626c5d57c75104185144dc8daf9c6aef27b020ee: Full log here. |
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
626c5d5 to
6f95cbf
Compare
|
Error while checking build/O2/o2 for 6f95cbf7881c1f28ba35475724d7e5bac0ec9d26: Full log here. |
|
@ktf I have added an item to the WP3 todo list - to me it does not look so straightforward to implement this exact functionality with CMake. |
Initial attempt at TimeframeReader and TimeframeWriter devices. This includes: - Initial plumbing for the devices themselves. - TimeframeParser helper class can be used to read data from an std:istream and create FairMQParts from it. - The test_TimeframeParser example generates a dummy timeframe and pumps it through the TimeframeParser. - FakeTimeframeGeneratorDevice, which can be used to generate timeframes programmatically. - TimeframeValidationTool which can be used to verify the contents of a timeframe file.
ktf
force-pushed
the
dev-timeframe-io-devices
branch
from
6f95cbf to
ada282f
Compare
|
Error while checking build/O2/o2 for ada282f: Full log here. |
…#370) * Following feedback from HMPID * changing the name of task in 2 places * load readout environment * proper url to files * fix path QC-313 * addresses Piotr's comments
5 participants
Initial commit. Does not work yet, albeit it should compile.