What timescale are you trying to get this in the trunk? Maybe you'll do the first iteration but I'd like to take a look too before it becomes official

is it here?

https://code.launchpad.net/~yordan-karadzhov/maus/devel

please post link

Good idea for Tunnell to have a look (better knowledge of DAQ and recon than me). I want to review the old TOF code for Beamline papers first. But I think this week my job is to have a go at a few code review trackers...

Yes, you need the new unpacking in order to run the new InputRealData.
To get the new unpacking do

bzr co lp:unpacking

Cut n paste from #550

From Rogers:

Multiple inheritance in e.g V1371
    Hard coded pedestal (should be datacard)
    Running over socket requires recompile (to avoid sprawling dependency tree)
        Try pushing up to Unpacker
    More comments
    Run style guide

Files to look at:

src/input/InputCppRealData.hh
src/input/InputCppRealData.cc
src/input/test_InputCppRealData.py
src/input/UnpackEventLib.hh
src/input/UnpackEventLib.cc

src/Utils/DAQChannelMap.hh
src/Utils/DAQChannelMap.cc

I think not (I guess this is Tof Recon stuff):
src/Utils/TOFChannelMap.*

General¶

In general, I like the code, I think it looks pretty robust and nicely polished. I have a few criticisms, maybe also some bugs (or things that look like bugs to me), but that's my job. Apologies.

Unpacking library¶

• Unpacking library should be installed by a bash script, like all the other third party things. Need to make a "good" version of unpacking and put either in MAUS files area or somewhere on web.
• Unpacking needs to build a .so file - requires some hacking in the unpacking makefile to get that to work.
• Would be good to put unpacking library into install/include/unpacking so that includes look like #include "unpacking/MDpartEventV1731.h" or whatever. Just makes it a bit more obvious what is MAUS and what is not.

Doxygen / comments¶

• Good doxygen coverage. Nice.
• Say InputCppRealData is a prototype "RealData" module. No longer prototype I hope!
• V1724 + V1731 brief summary doesn't come through
• Few functions undocumented - could do with one liners; DAQChannelKey functions, zero suppression fADCDataProcessor::get_pedestal_area, fADCDataProcessor::set_zero_suppression, fADCDataProcessor::set_zs_threshold, MDarranger::get/set_json_doc, V1731DataProcessor::set_zero_supression

Code¶

General¶

The code is of a good standard and robust. Of course, I have to go through and make some detailed comments, but in general it's really nice.

InputCppRealData¶

• General
  • Style - public then private; colon on same line - like
    

     public:
      void Foo();
     private:
      void Bah();
    

  • Style - why map instead of _map? (underscore missing) No big deal of course...

• birth()
  • Style - Better to raise a Squeal exception on error instead of calling exit
  • Bug - getenv("MAUS_ROOT_DIR") returns NULL if variable not found => segmentation fault
  • Style - don't do namespace::std please
  • Style - Maybe better to do datacard like "V1724_zero_suppression_threshold" rather than "V1724_zero_suppressio_threshold" (missing n). Same for all these in fact
  • Style - Please use Squeak::mout(Squeak::debug) rather than std::cout; and Squeak::mout(Squeak::error) rather than std::cerr. At some point I want to implement logging output for control room running. Please don't define your own _debug variable, use the VerboseLevel configuration datacard.
  • Style - why not use inheritance to reduce "cut and paste"?

UnpackEventLib¶

• Everywhere you use "pedestals" as the data tree name, did you mean to write "pedestal"?
• You use -99 as an error code. Fine, maybe you could soft code it?

• I didn't understand what the logic in the for loop was doing. My lack of knowledge about DAQ for sure, but perhaps means a comment here would help out?

• Query - everywhere you assume 20 bins to be the length of an event. Did you ever check that this is okay? Would it be better to soft code this?
• set_pedestal() - did you ever worry that there might be a hit in the first 20 bins when you calculate a running pedestal?
• get_pedestal_area - what is the origin of the hard coded 97 here?

• Bug at line 167: I guess xch < V1724_NCHANNELS should be xch < V1731_NCHANNELS? Not sure where this is defined.

• would it be better to use uint32_t for the pointer at line 218 rather than an unsigned int - guarantees that it is 32 bit pointer

• is the idea that tracker group adds more logic here later on? I sort of expected this to be a bit more complicated? But maybe all the logic is in their FPGA?

• Bug at line 307: I guess V1290_NCHANNELS - should be DBB_NCHANNELS? Not sure where this is defined.

DAQChannelMap¶

• DAQChannelKey
  • Style - might be better to make private data really private
  • Bug? - make_DAQChannelKey_id() do you worry about hardware with more than 100 channels? e.g. tracker has > 300 (but maybe not all on same board so it's okay?)
  • Style - operator should take const reference.
  • Style - operator != should take const reference. (Fine just to return NOT ).

I like the way you overloaded the operators here. Makes nice, easy going code. If you get bored, can do < > operators to help with e.g. std::sort, std::find algortihms.

• DAQChannelMap
  • Please don't call exit() - throw an exception
  • Find algorithm is correct, but slow. If it gets used a lot, may prefer to use STL find/sort algorithms

Tests¶

You know what I'm going to say here - tests aren't thorough enough. Even worse, the test fails on my machine (test log attached).

How much checking did you do by hand? For example, did you check that you can unpack every equipment type correctly? Did you check that you can handle errors correctly? Would be great to at least have automated test that runs through every board type and checks that we can unpack every equipment type, some of the switches work, etc. I wanted to get > 80% test coverage, at the moment you have about 70%, and you don't check the output as thoroughly as I would like.

Memory leaks¶

• Valgrind reports memory leak in _dataProcessManager
• Valgrind reports memory leak in _dataFileManager.OpenFile()

Thanks a lot for contributing the code, as I said previously in general it looks really good - just tidying mostly, although getting the tests done might take a bit of effort.

OK thanks,
I will check all points one by one

I know we're back recently from Nufact so things are slow.

Yordan: are you pushing a new version? It would be nice to have this in the trunk by the next software phonecall if there are no major changes. I could have time later in the week to peek at it.

Yes I did the modifications suggested by Chris Rogers, but I would like to have some time to play with the new code just to be sure that everything works properly. Also I have to investigate the memory leak problem.

This is a question to Chris Rogers: do you mean that the memory leak is in the unpacking?

I think so - valgrind can sometimes make mistakes, but that's what it looked like.

ps: My aim is that the sort of checks you do to make sure code is working properly get put into tests that we can rerun every time the code is changed. Sometimes I make a small change that shouldn't break anything, but break stuff by accident. Sometimes changes in other code breaks things in my code. Making it easy to check code stops these sorts of problems (but does take a bit of extra time to set up).

Bump? What's the status and what's the plan?

I pulled down the latest version and still am getting a test fail on my machine.

        self.assertEqual(digester.hexdigest(), '52a4a8021cfdef2d6e220c39c679970b')

Gives an error like

cr67> python build/test_InputCppRealData.py
 Creating the equipment map ...
 --- Equipment Map Dump --- 
80 0xc2c340 VLSB_C 0xc352a0
102 0x7be560 V1290 0x7be4e0
111 0xc2da30 V830 0x7ccd00
120 0xc2e100 V1724 0xc2c280
121 0xc2e0e0 V1731 0xc2e060
141 0xc2b9b0 DBB 0xc343c0
Equipment Map count : 1
Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
File 02873.003 is added
.WARNING in MDequipMap::MDequipMap() : Trying to create a multiple instances of a static singleton class
 --- Equipment Map Dump --- 
80 0xc2c340 VLSB_C 0xc352a0
102 0x7be560 V1290 0x7be4e0
111 0xc2da30 V830 0x7ccd00
120 0xc2e100 V1724 0xc2c280
121 0xc2e0e0 V1731 0xc2e060
141 0xc2b9b0 DBB 0xc343c0
Equipment Map count : 2
Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
File 02873.003 is added
WARNING : The first event is not a START_OF_RUN. Spill count and Event count not accurate. 
++++ End of file 02873.003  ++++
FWARNING in MDequipMap::MDequipMap() : Trying to create a multiple instances of a static singleton class
 --- Equipment Map Dump --- 
80 0xc2c340 VLSB_C 0xc352a0
102 0x7be560 V1290 0x7be4e0
111 0xc2da30 V830 0x7ccd00
120 0xc2e100 V1724 0xc2c280
121 0xc2e0e0 V1731 0xc2e060
141 0xc2b9b0 DBB 0xc343c0
Equipment Map count : 3
Going to search for run 02873 data files in /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
Trying to add file 02873.003 from /home/cr67/G4MICE/MAUS/maus_karadzhov/src/input/InputCppRealData
File 02873.003 is added
WARNING : The first event is not a START_OF_RUN. Spill count and Event count not accurate. 
.
======================================================================
FAIL: test_multi (__main__.InputCppRealDataTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "build/test_InputCppRealData.py", line 61, in test_multi
    self.assertEqual(digester.hexdigest(), '52a4a8021cfdef2d6e220c39c679970b')
AssertionError: 'e17c6bd815a383558761702d46e31dca' != '52a4a8021cfdef2d6e220c39c679970b'

----------------------------------------------------------------------
Ran 3 tests in 3.735s

FAILED (failures=1)

I'll just check that there wasn't some problem in pulling the latest version...

Did a fresh checkout and it passes. Must have been a problem at my end... sorry about that.

I did a first merge - and now am hacking around trying to "improve" some stuff. e.g. I wanted to put unpacking in a subdirectory of include, to make it easier to manage, want the binaries (analyze_mice.py) to be driven by datacards...

So I messed around with a few things:

• unpacking is now in a subdirectory
• analyze_offline binary is driven entirely by datacards
• added small test to check analyze_offline does something (integration test)

I want additionally to move README.txt into the SpillSchema.py, but I don't understand really what the spill looks like, and running analysis looks weird

{
  "spill_num": -1, 
  "daq_event_type": "end_of_burst", 
  "daq_data": null
}
{
  "spill_num": 0, 
  "daq_event_type": "start_of_burst", 
  "daq_data": null
}
{
  "spill_num": 0, 
  "daq_event_type": "physics_event", 
  "daq_data": {
    "V830": {
...
{
  "spill_num": 0, 
  "daq_event_type": "end_of_burst", 
  "daq_data": null
}
{
  "spill_num": 1, 
  "daq_event_type": "start_of_burst", 
  "daq_data": null
}
{
  "spill_num": 1, 
  "daq_event_type": "physics_event", 
  "daq_data": {
    "V830": {
... etc

In DATE we have big number of different DAQ event types. For the moment the MICE DAQ fills information only in "physics_event" but this may change in the future, so I do not want the unpacking to bypass the other event types. Also keep in mind that DATE package has been made for ALICE so they use the LHC terminology. In our case "burst" is equivalent to "spill".

The data should be filled one json document per spill. At the moment, unpacking creates one json document for start_of_burst, one document for end_of_burst, one document for physics_event. These should be all in the same document, say in a "daq_data" branch of the spill document.

We can always add another branch if we get calibration events.