Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

straw digis from artdaq fragments #1402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pavel1murat
Copy link
Contributor

  • add module reconstructing straw digis from artdaq fragments
  • known missing pieces:
  • correct assignment of strawID's :
    • need to have that uploaded to FPGAs
    • need to decide how to assign the straw IDs to the existing panels
  • lower bits of the TDC words - to be fixed in firmware
  • bit ordering in ADC samples - to be fixed in the firmware

@FNALbuild
Copy link
Collaborator

Hi @pavel1murat,
You have proposed changes to files in these packages:

  • DAQ

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for ccfef14: build (Build queue has 1 jobs)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at ccfef14.

Test Result Details
test with Command did not list any other PRs to include
merge Merged ccfef14 at df7782c
build (prof) Log file. Build time: 04 min 23 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
FIXME, TODO 🔶 TODO (0) FIXME (2) in 1 files
clang-tidy 🔶 16 errors 47 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at ccfef14 after being merged into the base branch at df7782c.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@edcallaghan
Copy link
Contributor

@pavel1murat Thanks for pushing this, Pasha. I can confirm that this runs and produces digis. I will take a look at the code a bit later, but one question to clarify:

* correct assignment of strawID's :
  
  * need to have that uploaded to FPGAs
  * need to decide how to assign the straw IDs to the existing panels

what does this mean for data we have already taken? I thought there was an effectively-fixed straw <-> id mapping, but this implies (to me) that we don't currently know which straws are being hit. Am I misunderstanding?

@rlcee
Copy link
Collaborator

rlcee commented Jan 24, 2025

  • need to decide how to assign the straw IDs to the existing panels

I want to add there is only one map (~ROC number to ~panel number) in offline. Data with only small parts of the detector can be handled in different way for things like calibrations. Please keep me (and I'd also suggest Richie) in the loop for any channel numbering discussions, if it might effect offline. Thanks

@FNALbuild
Copy link
Collaborator

📝 The HEAD of main has changed to 5e27791. Tests are now out of date.

@brownd1978
Copy link
Collaborator

Is there anyone besides Ed who should review this?

Copy link
Contributor

@edcallaghan edcallaghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments trying to think ahead to dealing with a nonzero error rate in the payload. I'm inclined to say that the ProtonBunchTime and IntensityInfoTrackerHits aren't necessary in this module, but if they fit into a larger pattern then it's ok.

Comment on lines +233 to +248
auto adc_packet = (mu2e::TrackerDataDecoder::TrackerADCPacket*)((char*) hit + 16); // the packet size is 16 bytes
while (npackets < nADCPackets_) {
wf[++idx] = adc_packet->ADC0;
wf[++idx] = adc_packet->ADC1();
wf[++idx] = adc_packet->ADC2;
wf[++idx] = adc_packet->ADC3;
wf[++idx] = adc_packet->ADC4();
wf[++idx] = adc_packet->ADC5;
wf[++idx] = adc_packet->ADC6;
wf[++idx] = adc_packet->ADC7();
wf[++idx] = adc_packet->ADC8;
wf[++idx] = adc_packet->ADC9;
wf[++idx] = adc_packet->ADC10();
wf[++idx] = adc_packet->ADC11;
npackets++;
adc_packet++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only general concern I have is that I don't see any integrity checks. It is an efficient deserialization assuming the fragments are error-free, but it may produce uncaught exceptions, or even terminate ungracefully, if there is an error in the payload. The canonical example being the missing-packet problem we've encountered, which L248 for example assumes cannot happen. Without considering that, then this will eventually read an out-of-bounds address, which will either cause process termination or silent data corruption.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DJ will bring this up at the pass1 workshop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guys, I'm trying to reply, but am having difficulties. would be happy to talk - shall we have this discussion on ZOOM or on slack , where there are threads, at the very least ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following an offline discussion, I had misunderstood the nature of the errors we currently expect to encounter. We may go out-of-sequence when parsing the packed data, but there is not a mechanism by which we foresee encountering an out-of-bounds read like I suggested. Pasha will implement a simple check for having gone out-of-sequence, and utilize the StrawDigiFlag data product to mark digis in such an event as having been part of problematic event.

@edcallaghan
Copy link
Contributor

@rlcee Following a discussion with Richie, we think the channel-numbering business amounts to applying the mapping you reference to a per-panel DCS write during configuration, so I don't foresee needing any to tweak anything in Offline.

Copy link
Collaborator

@rlcee rlcee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this module relate to the existing StrawRecoFromFragments?

#define TRACEMF_USE_VERBATIM 1

#include "TRACE/tracemf.h"
#define TRACE_NAME "StrawRecoFromArtdaqFragments"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would be the first actual use of TRACE in offline. If it just being used to log steps, that should be messagelogger. If it is being used for timing studies, then I'd like to have a discussion in the reco meeting on whether we will converge on offline tools or allow developer preference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pasha agree to comment out trace for now, there is no specific need

}
// ======================================================================

class art::StrawRecoFromArtdaqFragments : public EDProducer {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to call this "StrawDigiFrom" instead of "StrawRecoFrom" since there is no reco in the way I'd use the word.


public:

struct RocDataHeaderPacket_t { // 8 16-byte words in total
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this the sort of thing that goes in artdaq_core_mu2e?

// HV side has an extra bit in teh straw ID
//-----------------------------------------------------------------------------
uint16_t straw_id = hit->StrawIndex;
if (straw_id >= 0x80) straw_id = straw_id - 0x80;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from just how the panel id registers are set on powerup right now - if those are written to correctly by the DAQ before taking data this fix is not needed

@bonventre
Copy link
Contributor

One thing that seems to be missing here is the ability to deal with different packet formats - the artdaq_core_mu2e decoder has code to automatically upgrade older formats, how would that work here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants