Skip to content

Add type for HGC/CALOROC output #101

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

ruse-traveler
Copy link
Contributor

@ruse-traveler ruse-traveler commented Feb 9, 2025

Briefly, what does this PR introduce?

This PR introduces the RawCALOROCHit type, which accommodates the output of both type 1A/1B HGC/CALOROC chips.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

@ruse-traveler
Copy link
Contributor Author

ruse-traveler commented Feb 9, 2025

Leaving this as draft for now while discussion progresses.

One thing that was discussed in 89, was to potentially add ToT (and ToA if needed) to RawCalorimeterHit. I don't have strong opinions, but considering that systems other than calorimeters (e.g. the PFRICH) will be using HGCROCs, my preference leans towards this being a distinct type... Perhaps another way of framing it is as a RawSiPMHit?

@sly2j
Copy link
Contributor

sly2j commented Feb 10, 2025

Nice work! I have a couple of questions/comments:

  • The way we will likely configure the H2GCROC/CALOROC boards for calorimeters would have 5 amplitude reads for each hit so we can do some waveform template fitting. The first read would be before the ToT (to sample the pedestal), the other 4 are during the pulse. Maybe we should consider using something like std::array<int32_t, 5> for amplitude. If we think hard-coding the 5 samples is safe at this point
  • I would personally call the structure after the future EIC ship which would be CALOROC. In particular because there are so many other HGCROC variants out there. The technical name for the current version would be H2GCROC3 for the SiPM version.
  • How do you suggest we deal with storing the phase of the ADC samples? Due to the nature of the 100MHz bunch crossing rate versus the 40MHz clock on the CALOROC board there is a bit of a drift in the timing offset w.r.t. the EIC clock depending on where we are in the 40MHz cycle (both clocks sync up every 50 ns and after that the ASIC sampling drifts by 10ns until they are back in phase). Should we store the phase in a dedicated field? I believe the ASIC (or FEB) can add the phase to the data stream?

@wdconinc @veprbl what do you think? I believe it's important to get this right from the start as changes to the model are expensive (invalidate old files).

@veprbl veprbl requested a review from simonge February 10, 2025 22:27
@ruse-traveler
Copy link
Contributor Author

Thanks for the feedback @sly2j ! I've in-lined a few thoughts below...

  • The way we will likely configure the H2GCROC/CALOROC boards for calorimeters would have 5 amplitude reads for each hit so we can do some waveform template fitting. The first read would be before the ToT (to sample the pedestal), the other 4 are during the pulse. Maybe we should consider using something like std::array<int32_t, 5> for amplitude. If we think hard-coding the 5 samples is safe at this point

Since they will be configurable (@novitzky also pointed this out to me offline), I wonder if we could go ahead and make the ToT a std::vector<int32_t>?

  • I would personally call the structure after the future EIC ship which would be CALOROC. In particular because there are so many other HGCROC variants out there. The technical name for the current version would be H2GCROC3 for the SiPM version.

Gotcha! Makes sense!

  • How do you suggest we deal with storing the phase of the ADC samples? Due to the nature of the 100MHz bunch crossing rate versus the 40MHz clock on the CALOROC board there is a bit of a drift in the timing offset w.r.t. the EIC clock depending on where we are in the 40MHz cycle (both clocks sync up every 50 ns and after that the ASIC sampling drifts by 10ns until they are back in phase). Should we store the phase in a dedicated field? I believe the ASIC (or FEB) can add the phase to the data stream?

Good question! 😅 I'd definitely be in favor of adding a field for the phase! I feel like that would be the easiest approach for book keeping...

@veprbl
Copy link
Member

veprbl commented Feb 18, 2025

Is the clock counter available in *ROC's?

Also, I think, it would do us good if this structure could also be used to support ongoing beam tests data-taking. cc @tlprotzman

@simonge
Copy link
Contributor

simonge commented Feb 18, 2025

What is the use case for storing the phase for every hit? This should always be easily calculatable from the global time of arrival and some calibration offset for the detector at an event level.

Here, is the time stamp the 40MHz clock cycle being added by the FEB and the Time of Arrival from the ASIC in the cycle digitized to 25ps? Can these be combined at this point or might there be further calibrations dependent on having a separate Time of Arrival?

@veprbl
Copy link
Member

veprbl commented Feb 19, 2025

What is the use case for storing the phase for every hit? This should always be easily calculatable from the global time of arrival and some calibration offset for the detector at an event level.

Would be nice to check if there is any way and desire to synchronize the phases between different chips. Storing the same value should not be too expensive in columnar format with compression.

@ruse-traveler
Copy link
Contributor Author

ruse-traveler commented Apr 1, 2025

Hi all, I wanted to summarize some of the feedback I received on this type from the 03.19.2025 meetings:

  1. It could be useful to support types 1A, 1B CALOROC chips. This would entail being able to keep track of the ADC counts in both high gain mode & low gain mode.
  2. To account for double hits (e.g. from background), it could be useful to upgrade the ToA and ToT to vector members.
  3. And it would be easier for analyzers to instead work with fixed array rather than vector members due to the PODIO stores one vs. the other. The dimension of the arrays could be made configurable via templating, and we don't expect the dimension to change frequently (e.g. once per running period at most).

I've just pushed some changes to address this feedback:

  • Towards (1), I've added a member to index type 1A vs. 1B (+ extra code to indicate if it's one or the other) and then 2 new vector members to track the amplitudes in low/high gain modes
    • The idea being that if it's a 1A chip, then you would get the ADC values in getAmplitudes()
    • And if it's a 1B chip, then you would get the low/high gain ADC values in Get{Low,High}Amplitudes() respectively
  • Towards (2), the ToA and ToT values have been upgraded to vector members
  • But for (3), I haven't attempted any changes yet:
    • I'd be in favor of making the amplitude members arrays and using templating to configure the number of samples
    • But I'm not sure if we support templating PODIO types in our environment yet...

Lastly, I have naïve question for everyone: should timeStamp be an int32_t -- in units of TDC(??) -- or a float -- in units of ns?

@ruse-traveler ruse-traveler changed the title Add type for HGCROC output Add type for HGC/CALOROC output Apr 1, 2025
@ruse-traveler ruse-traveler marked this pull request as ready for review April 2, 2025 14:46
@ruse-traveler ruse-traveler requested a review from a team as a code owner April 2, 2025 14:46
@simonge
Copy link
Contributor

simonge commented Apr 9, 2025

  1. To account for double hits (e.g. from background), it could be useful to upgrade the ToA and ToT to vector members.
  • Towards (2), the ToA and ToT values have been upgraded to vector members

This will probably be my ignorance of how the CALOROC works, I'm not sure I understand how a single hit will have multiple ToA and ToT values. Could you point me to some documentation/presentation.

Lastly, I have naïve question for everyone: should timeStamp be an int32_t -- in units of TDC(??) -- or a float -- in units of ns?

timeStamp will need to be the digitized value, presumably from the driving 40MHz clock.

Copy link
Contributor Author

@ruse-traveler ruse-traveler left a comment

Choose a reason for hiding this comment

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

Fix typos and make capitalizations/units consistent

@ruse-traveler
Copy link
Contributor Author

This will probably be my ignorance of how the CALOROC works, I'm not sure I understand how a single hit will have multiple ToA and ToT values. Could you point me to some documentation/presentation.

Apologies for the delayed response! 😵

But, unfortunately, I've been having a tough time finding some documentation on how the multiple ToT/ToA will work in practice (e.g. in test beam or running conditions)... @novitzky do you know of any good documentation on this topic?

timeStamp will need to be the digitized value, presumably from the driving 40MHz clock.

Gotcha! Thanks! I'll leave the units as [TDC counts], then!

Copy link
Contributor

Choose a reason for hiding this comment

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

Having just fixed a whole bunch of signed/unsigned comparisons, can we please use unsigned ints where appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Most of these can be uint32_ts...

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

Successfully merging this pull request may close these issues.

5 participants