updates to the DAQ modules #158
Conversation
…s and for generating a digi file from a artFragment file
|
Hi @gianipez,
which require these tests: build. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for ref 6051517: build |
|
I don't understand this:
Do you mean that the module that does the conversion of fragments to digis is not found in Offline? Or something else? |
|
Hi Rob, That module is in the Offline. What Tomo was doing before was: converting all the fragments (from calo and tracker) to an intermidiate data product, and then using separate modules for converting this tmp data product into tracker or calo digi. |
|
Thanks Giani,
I understand it now.
Please point me to am art file on disk that contains artdaq::Fragments consistent with this code. And also point me to a fhicl file to run the trigger from these files.
Thanks,
Rob
… On Mar 16, 2020, at 5:58 PM, Gianantonio Pezzullo ***@***.***> wrote:
Hi Rob,
That module is in the Offline. What was doing before was: converting all the fragments (from calo and tracker) to an intermidiate data product, and then using separate modules for converting this tmp data product into tracker or calo digi.
In the new workflow, we use the same module to convert directly fragments (calo or tracker) to digis.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
DAQ/src/SConscript
Outdated
| @@ -18,14 +18,13 @@ rootlibs = env['ROOTLIBS'] | |||
|
|
|||
| mainlib = helper.make_mainlib ( [ 'mu2e_ConditionsService', | |||
| 'mu2e_GeometryService', | |||
| 'mu2e_MCDataProducts', | |||
| # 'mu2e_MCDataProducts', | |||
There was a problem hiding this comment.
It's great that we no longer need MCDataProducts - thanks for looking after that. Please just remove the line instead of comment it out. Same comment for any other place this shows up.
|
Hi Rob,
You can run this command after you compiled the code with the trigger parameter in buildopts set to ‘on’.
mu2e -c DAQ/test/generateDigiFromFragment.fcl -s /mu2e/data/users/gianipez/data/driver_rootOutput.art
Thanks,
Gianantonio Pezzullo, Ph.D.
giani.pezzu@gmail.com
**********************************************************************************************************
Address: Department of Physics, Yale University, 56 Hillhouse, New Haven, CT-06511, USA
Phone : +1 (331) 205-9306
**********************************************************************************************************
… On Mar 16, 2020, at 7:26 PM, Rob Kutschke ***@***.***> wrote:
Thanks Giani,
I understand it now.
Please point me to am art file on disk that contains artdaq::Fragments consistent with this code. And also point me to a fhicl file to run the trigger from these files.
Thanks,
Rob
> On Mar 16, 2020, at 5:58 PM, Gianantonio Pezzullo ***@***.***> wrote:
>
>
> Hi Rob,
>
> That module is in the Offline. What was doing before was: converting all the fragments (from calo and tracker) to an intermidiate data product, and then using separate modules for converting this tmp data product into tracker or calo digi.
> In the new workflow, we use the same module to convert directly fragments (calo or tracker) to digis.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or unsubscribe.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGCD3ECQ4EXLRTD4QDJDRFLRH2YTNANCNFSM4LMUWUTQ>.
|
|
@FNALbuild run build test |
|
Thanks Giani,
The job ran. I have a question about the cardinality of the new format data products.
In this file the calo artdaq::Fragment data product always has 168 entries - is this fixed, maybe one per some some grouping of crystals? Or is it an artifact of this dataset? When we look at cosmic rays, will we always have 168 entries, most of which are empty? If so, how big is an empty artdaq::Fragment?
In this file there is a variable number of artdaq::Fragments. I looks to me as if there is one artdaq::Fragment per StrawDigi. Is that right? If you compare the overhead of a fragment compared to the payload of a single digi, how much is this choice costing us in overhead compared to a more compact representation?
In both cases, is an artdaq::Fragment a fixed size thing?
In the first two cases I have asked questions about added overheads. Maybe the answer is that they are small, or even that they are OK when they are big - in all cases all stakeholders need to be in the loop on this.
Rob
… On Mar 16, 2020, at 8:51 PM, Gianantonio Pezzullo ***@***.***> wrote:
Hi Rob,
You can run this command after you compiled the code with the trigger parameter in buildopts set to ‘on’.
mu2e -c DAQ/test/generateDigiFromFragment.fcl -smu2e/data/users/gianipez/data/driver_rootOutput.art
Thanks,
Gianantonio Pezzullo, Ph.D.
***@***.***
**********************************************************************************************************
Address: Department of Physics, Yale University, 56 Hillhouse, New Haven, CT-06511, USA
Phone : +1 (331) 205-9306
**********************************************************************************************************
> On Mar 16, 2020, at 7:26 PM, Rob Kutschke ***@***.***> wrote:
>
>
> Thanks Giani,
>
> I understand it now.
>
> Please point me to am art file on disk that contains artdaq::Fragments consistent with this code. And also point me to a fhicl file to run the trigger from these files.
>
> Thanks,
> Rob
>
> > On Mar 16, 2020, at 5:58 PM, Gianantonio Pezzullo ***@***.***> wrote:
> >
> >
> > Hi Rob,
> >
> > That module is in the Offline. What was doing before was: converting all the fragments (from calo and tracker) to an intermidiate data product, and then using separate modules for converting this tmp data product into tracker or calo digi.
> > In the new workflow, we use the same module to convert directly fragments (calo or tracker) to digis.
> >
> > —
> > You are receiving this because you commented.
> > Reply to this email directly, view it on GitHub, or unsubscribe.
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <#158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGCD3ECQ4EXLRTD4QDJDRFLRH2YTNANCNFSM4LMUWUTQ>.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Hi Rob, Thanks for making these questions. 168 is the number of ROCs that the calorimeter plans to use. The difference w.r.t. the Tracker is driven by the different data model used to send the data out by the different ROCs. Cheers, Giani |
There was a problem hiding this comment.
Please add configuration validation.
I put in lots of small comments and have run of out time to work on this. The common thread is tha the code is full of numeric literals. I expect that many of these will want to be used in the "unpack" routines so they should be defined in one place for all code that wants to use them. One option would be to put them in a header file that contains lots of constexpr definitions. Another is to put them in Proditions. My vote is generally for Proditions since many things will change with time over the next decade.
setup.sh
Outdated
|
|
||
| # # Get access to raw data formats. | ||
| # setup -B mu2e_artdaq_core v1_02_30a -q${MU2E_UPS_QUALIFIERS}:+${MU2E_ART_SQUALIFIER}:offline | ||
|
|
There was a problem hiding this comment.
Do you need to keep this line for future debugging? If not then just get rid of it. You can diff two different commits to learn what older versions were used when.
| // | ||
| // | ||
| // Original author Tomonari Miyashita | ||
| // |
There was a problem hiding this comment.
Is Tomo really the original author of this? It's appropriate to give him credit if this is substantially based on his work but if it's a total rewrite it's your code.
| // Definitions and typedefs needed for compatibility with HLS codeblock | ||
| #define NUM_PRESAMPLES 4 | ||
| #define START_SAMPLES 5 // 0 indexed | ||
| #define NUM_SAMPLES 15 | ||
| #define LOWER_TDC 16000 | ||
| #define UPPER_TDC 64000 |
There was a problem hiding this comment.
Two things:
Always prefer to use constexpr over #define. Remember that #define is NOT part of C++ it is a C-preprocessor directive. Therefore many of the safety features of the compiler are not available.
As targets of opportunity, please change this everywhere in your code.
In any case these values should not be hard coded. Other code will want to know them as well. So they belong somewhere that many modules have access to them. Unless you can make a very compelling case that these are fixed for all time, the only place for them is in Proditions.
| typedef uint16_t tdc_type; | ||
| typedef uint8_t tot_type; | ||
| typedef uint16_t adc_type; | ||
| typedef uint16_t calib_constant_type; | ||
| typedef uint8_t flag_mask_type; | ||
| typedef uint64_t timestamp; |
There was a problem hiding this comment.
Prefer to use the more modern using syntax, as you do below.
|
|
||
| virtual void beginJob() override; | ||
| virtual void beginRun(art::Run&); | ||
|
|
There was a problem hiding this comment.
Add override everywhere it can be used
| headerData.DTCID = DTCId; | ||
| uint8_t evbMode = 0;//maybe off-spill vs on-spill? | ||
| headerData.EVBMode = evbMode; | ||
| } |
There was a problem hiding this comment.
Use symbolic names for the numeric literals. Which of these are used in other modules/functions? Those should come from a common, authoritative source.
There was a problem hiding this comment.
Hi Rob,
This is on our to-do list. We didn't decide yet with Eric how to set up "EventMode" class in mu2e_artdaq. I was waiting to have some code working for the CRV online reconstruction to start working on it.
There was a problem hiding this comment.
Are you saying the when we have real data, you won't need to fill headerData since artdaq will have already filled it for you? Or something else?
| TrkData.SetWaveform(12, theWaveform[12]); | ||
| TrkData.SetWaveform(13, theWaveform[13]); | ||
| TrkData.SetWaveform(14, theWaveform[14]); | ||
|
|
There was a problem hiding this comment.
Why not write this as a loop?
| calib_constant_type gainrs15 = 1389; | ||
| calib_constant_type inverseionizationenergyls26 = 633; | ||
| calib_constant_type panelTDCoffset = 0, hvoffset = 0, caloffset = 0; | ||
|
|
There was a problem hiding this comment.
that block of code is something from Tomo. I kept in only for keeping track, but so far I have never used that nor I read anything in Tomo's documentation about it.
I can either delete it or comment it.
It seems to reproduce the operations performed already on the DRAC. I have never heard about any use-case for doing that.
There was a problem hiding this comment.
If you expect that you will want this functionality soon, then keep it. If you do not expect to need that functionality then please remove it but put enough information in your commit comment that you can find it again if you do need it.
|
|
||
| // Loop over the DTC/ROC pairs and generate datablocks for each ROC | ||
| for (uint8_t dtcID = 0; dtcID < max_dtc_id; dtcID++) { | ||
|
|
There was a problem hiding this comment.
Is there a check anywhere that max_dtc_id < 255 so that it fits in a uint8_t?
There was a problem hiding this comment.
The total number of ROCs and of DTC (for reading the ROCs) was fixed in the project, so that's why we didn't put any check.
There was a problem hiding this comment.
All data driven code must assume that it will sooner or later see corrupt data and it needs to be safe when that happens.
|
|
||
|
|
||
| using mu2e::ArtBinaryPacketsFromDigis; | ||
| DEFINE_ART_MODULE(ArtBinaryPacketsFromDigis); |
There was a problem hiding this comment.
You can write this in one line:
DEFINE_ART_MODULE(mu2e::ArtBinaryPacketsFromDigis);
In the very old days it had to be done on 2 lines because of limitations in the CPP.
|
Thanks Giani,
I don't need these questions resolved in order to approve the pull request but I would like them addressed over the next few months.
I will ask Eric but I suspect that this is really a Mu2e issue. As I understand it we subclass from artdaq::Fragment so we control the payload and therefore the size. Perhaps I have misunderstood the architecture?
Rob
… On Mar 17, 2020, at 2:18 PM, Gianantonio Pezzullo ***@***.***> wrote:
Hi Rob,
Thanks for making these questions. 168 is the number of ROCs that the calorimeter plans to use. The difference w.r.t. the Tracker is driven by the different data model used to send the data out by the different ROCs.
I think Bertrand wanted to address this issue of optimization of the Fragment format and their un-packing. As it is right now, it's a 1-to-1 translation of the binary file, which currently reflects the Firmware decisions of the sub-detectors. For the specific questions about the artFragments, I think Eric Flumerfelt is the best person for answering them.
Cheers,
Giani
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Giani: will this pull request bring online and offline to the point that we are using the same ups products, the same compiler etc? |
|
Hi Rob, Yes. So probably we want to tag the Offline before and after making the merge. |
| if (parseTRK_){ | ||
| event.getByLabel(trkFragmentsTag_ , trkFragments); | ||
| if (!trkFragments.isValid()){ | ||
| return; |
There was a problem hiding this comment.
We need to have a policy discussions that involve both online and offline.
Suppose that we have an event in which the tracker is present but the calorimeter is not. Or vice versa. That might often happen during commissioning and might happen from time to time during physics running when an error happens. Do you really want to skip the good data just because something else is missing? It's not useful for physics but it might very well be important for debugging.
The second policy is issue is that this code declares that it will produce something and then never produces it. I believe that for Offline we currently have art configured so that it's an error that art will catch and throw an exception. There is run-time configuration to allow failure to put to be allowed. I believe that there is also something like produce_maybe which allows us exempt single modules from this policy.
I am not asking for an action at this time but we need to discuss this the online team.
|
|
||
| size_t totalSize = 0; | ||
| for(size_t idx = 0; idx < trkFragments->size(); ++idx) { | ||
| for(size_t idx = 0; idx < numTrkFrags; ++idx) { | ||
| auto size = ((*trkFragments)[idx]).size() * sizeof(artdaq::RawDataType); | ||
| totalSize += size; | ||
| // std::cout << "\tTRK Fragment " << idx << " has size " << size << std::endl; | ||
| } |
There was a problem hiding this comment.
You can take the multiply out of the loop.
| @@ -171,90 +188,100 @@ void | |||
|
|
|||
| // Print out decimal values of 16 bit chunks of packet data | |||
| for(int i=7; i>=0; i--) { | |||
There was a problem hiding this comment.
Please define a symbolic constant for the value 7. There are other places in the code that need this too.
|
|
||
| eventNumber = timestamp; |
There was a problem hiding this comment.
I am not 100% sure what you are up to here but the variable name might be misleading. If I just see a variable named eventNumber I will assume it's the event number field of an art::EventID, which would be wrong. So it would be a good idea choose a safer name. What will hdr->GetTimestamp() return - is it a time relative to the t=0 within the current event window? Something else?
| mode_ = "CAL"; | ||
| } | ||
| // if(idx>=numTrkFrags && mode_ == "TRK") { |
There was a problem hiding this comment.
Please do not use character string comparisons for flow control, especially in time critical code. This is the perfect case for an enum.
If you need to have a character string representation for debugging, then use the enum-matched-to string template. When doing assignments an comparisons this is as fast and as small as a plain enum. The overhead is incurred only for IO or for initialization from a string.
| } | ||
| if (parseCAL_){ | ||
| event.put(std::move(calo_digis)); | ||
| } | ||
|
|
||
| } // produce() | ||
|
|
There was a problem hiding this comment.
This routine would benefit greatly from major refactoring. I expect that you inherited a lot of this from the previous author. As written the code says:
(loop over the combination of all trk and cal fragments){
if ( track fragment){
process the track fragment
} else if ( cal fragment ) {
process the cal fragment
}
}
There is no common code between the two modes. I would write one function to process all of the trk fragments an another to process all of the cal fragments. I would also extract all of the debug output into helper functions so that the main line of the algorithm is more compact. It's really hard to tell what this code is doing since it's all debug output with random bits of real code mixed in a few lines at a time.
There was a problem hiding this comment.
Hi Rob,
We use two different module instances in the Online reco: one for taking care of the Tracker fragments and a second one for the calorimeter. In this way, we don't break/change the naming convention in the reconstruction sequences in the Online running from fragments or from Digis.
| outputFile : "DTC_packets_calo.bin" | ||
| maxDMABlockSize : 32000 | ||
| bufferSize : 1000 | ||
| } |
There was a problem hiding this comment.
Consider moving the standard module definitions into a prolog sooner rather than later.
|
HI Giani,
Great! I would like to get the two code bases synched back to the same set of ups products.
We will soon need to move to a new art and break it again but having all of the other products the same is important.
Rob
… On Mar 17, 2020, at 10:19 PM, Gianantonio Pezzullo ***@***.***> wrote:
Hi Rob,
Yes. So probably we want to tag the Offline before and after making the merge.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Then we should just make two modules. The downstream configuration should not care so long as you keep the module labels the same.
… On Mar 17, 2020, at 11:18 PM, Gianantonio Pezzullo ***@***.***> wrote:
@gianipez commented on this pull request.
In DAQ/src/StrawAndCaloDigisFromFragments_module.cc:
> @@ -405,8 +448,12 @@ void
event.put(std::unique_ptr<EventNumber_t>(new EventNumber_t( eventNumber )));
// Store the straw digis and calo digis in the event
- event.put(std::move(straw_digis));
- event.put(std::move(calo_digis));
+ if (parseTRK_){
+ event.put(std::move(straw_digis));
+ }
+ if (parseCAL_){
+ event.put(std::move(calo_digis));
+ }
} // produce()
Hi Rob,
We use two different module instances in the Online reco: one for taking care of the Tracker fragments and a second one for the calorimeter. In this way, we don't break/change the naming convention in the reconstruction sequences in the Online running from fragments or from Digis.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
| @@ -64,16 +64,16 @@ CrvDigisFromFragments::CrvDigisFromFragments(fhicl::ParameterSet const& pset) | |||
| , diagLevel_(pset.get<int>("diagLevel",0)) | |||
There was a problem hiding this comment.
Why is this class in the art namespace and not the mu2e namespace? Same question for StrawAndCaloDigisFromFragments_module.cc . I did not check the others.
There was a problem hiding this comment.
I have no clue. I did that because Tomo was doing it. I'm pretty sure the code should work also if I use the mu2e namespace. Probably he was doing that for debugging purposes?
|
Hi Rob, Eric told me that we might need in the coming future that "EventNumber_t" data product for debugging purposes when we will develop the way to assign the event number in the Online reco. So far now I guess it makes sense to keep it. I think I made all the changes you have been asking. Please, let me know if I missed anything. Cheers, Giani |
|
On Mar 23, 2020, at 12:55 AM, Gianantonio Pezzullo ***@***.***> wrote:
Hi Rob,
Eric told me that we might need in the coming future that "EventNumber_t" data product for debugging purposes when we will develop the way to assign the event number in the Online reco. So far now I guess it makes sense to keep it.
OK - that's a good reason to keep it.
… I think I made all the changes you have been asking. Please, let me know if I missed anything.
Cheers,
Giani
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for ref b2108ad: build |
|
|
||
| trkTag : "daq:trk" | ||
| caloTag : "daq:calo" | ||
| } |
There was a problem hiding this comment.
Can you use pre-defined configurations of makeSD and readSD here? Or use them with one or two overrides? Copying it in full makes it fragile to new parameters added to the standard configuration
DAQ/test/generateBinaryFromDigi.fcl
Outdated
| binaryOutput: { | ||
| module_type : ArtBinaryPacketsFromDigis | ||
| # diagLevel : 2 | ||
| diagLevel : 0 | ||
| maxFullPrint : 0 | ||
|
|
||
| includeTracker : 1 | ||
| includeCalorimeter : 1 | ||
| # includeCosmicRayVeto: 1 | ||
| includeCosmicRayVeto: 0 | ||
| includeDMAHeaders: 1 | ||
| # Cosmic ray veto module is currently incomplete | ||
|
|
||
| generateTimestampTable : 1 | ||
| generateBinaryFile : 1 | ||
|
|
||
| # outputFile : "DTC_packets_from_fragment.bin" | ||
| outputFile : "DTC_packets_NoPrimary.bin" | ||
| maxDMABlockSize : 32000 | ||
| bufferSize : 1000 | ||
| } |
There was a problem hiding this comment.
Is this a candidate for going into a prolog?
|
|
||
| trkTag : "daq:trk" | ||
| caloTag : "daq:calo" | ||
| } |
There was a problem hiding this comment.
Same comment as above - can you use a predefined standard definition here?
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for ref fae0954: build |
|
There is one unresolved issue that we cannot address until some other work has been done. I have added it as issue #164 . |
-> added module for creating binary files from digis (for the calorimeter and the tracker)
-> added module to convert the artFragments into calo/tracker digis