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

reintroduce 200ns delay for CRV event window start #1363

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ehrlich-uva
Copy link
Contributor

  • old digis were produced relative to -protonBunchTime
  • new digis are produced relative to -protonBunchTime-eventTiming.timeFromProtonsToDRMarker

The fcl setting physics.producers.CrvRecoPulses.oldDigis : true allows us to read old digis with the new code.

@FNALbuild
Copy link
Collaborator

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

  • CRVReco

which require these tests: build.

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

⌛ The following tests have been triggered for 99ba8c2: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at 99ba8c2.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 99ba8c2 at 57339a5
build (prof) Log file. Build time: 04 min 14 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.
FIXME, TODO TODO (0) FIXME (0) in 1 files
clang-tidy 🔶 0 errors 62 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 99ba8c2 after being merged into the base branch at 57339a5.

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.

@kutschke kutschke self-assigned this Oct 17, 2024
@kutschke kutschke self-requested a review October 17, 2024 14:51
@ehrlich-uva
Copy link
Contributor Author

I'm going to reintroduce the 200ns delay (eventTiming.timeFromProtonsToDRMarker). So the option for old digis is not needed anymore. Switching this PR to Draft for now.

@ehrlich-uva ehrlich-uva marked this pull request as draft October 17, 2024 20:34
Copy link
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Thanks for reverting this, I think its a step in the right direction. I'd like Richie to have a look and confirm this will result in synchronized event boundaries for tracker and CRV.

CRVResponse/fcl/prolog_v11.fcl Show resolved Hide resolved
@brownd1978
Copy link
Collaborator

brownd1978 commented Oct 18, 2024 via email

@ehrlich-uva
Copy link
Contributor Author

ehrlich-uva commented Oct 18, 2024

With this change, the event window starts (i.e. when TDC=0) at -protonBunchTime->pbtime_, which is the same event window start time the tracker uses. Zero suppressed data will be recorded starting at 200ns (=digitizationStart) after the event start time. The non-zero suppressed data (not implemented, yet) will get recorded starting from the event start time.

@ehrlich-uva ehrlich-uva changed the title added option to deal with older CrvDigis reintroduce 200ns delay for CRV event window start Oct 18, 2024
Copy link
Collaborator

@kutschke kutschke left a comment

Choose a reason for hiding this comment

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

For this round I think that the goal is to make all of the Tracker, Calo and CRV be consistent. If Richie signs off then I will merge.

In the future we may need another iteration to match what the DAQ timing really relative to the DR RF0 Marker will really do. But I think that this is good enough to encapsulate the later changes in one routine per subsystem.

@ehrlich-uva
Copy link
Contributor Author

@kutschke I would like to run a few tests tomorrow to make sure it does what I want it to do. I will let you know when it is ready to merge.

@kutschke
Copy link
Collaborator

@kutschke I would like to run a few tests tomorrow to make sure it does what I want it to do. I will let you know when it is ready to merge.

OK - I will wait for both you and Richie.

@@ -9,7 +9,9 @@
#include "Offline/CommonMC/fcl/prolog.fcl"
BEGIN_PROLOG

DigitizationStart: 400.0 //ns after event window start (i.e. 400ns after first clock tick after POT) --> 400ns...425ns after POT
DigitizationStart : 200.0 //ns after event window start (400ns...425ns after POT)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the tracker we define digitizationStart/end in fcl as the effective time you want referenced to nominal proton arrival time, and then subtract the timeFromProtonsToDRMarker to get the actual value used by the digi making / hardware. Might make it easier to keep in sync if timeFromProtonsToDRMarker changes

@brownd1978
Copy link
Collaborator

It looks like this development has converged, is there anything holding up merging?

@ehrlich-uva
Copy link
Contributor Author

@brownd1978 I sent an email around last week discussion the issue. The problem is that the CRV group is currently not planning to have this 200ns delay. So I'm not sure anymore, if I should have it in the simulation. Perhaps we could have a short Zoom meeting.

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.

5 participants