[New Feature] variable waitTime for parallel sbitrate scans

AndrewLevin commented 5 years ago

This is one of four pull requests to four repositories that make the data collection time for parallel sbitrate scans configurable.

Description

The time to collect the data was previously hard-coded to 1005 milliseconds. This pull requests sets it to the new argument waitTime.

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

This was requested in https://github.com/cms-gem-daq-project/ctp7_modules/issues/154

How Has This Been Tested?

Yes, I performed an sbitThresh scan with the waitTime set to 1000 and the waitTime set to 10000, and found the rates were very close while the second scan took around 10 times longer.

Screenshots (if appropriate):

Checklist:

[ ] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

jsturdy commented 5 years ago

In addition to waiting before reading, you need to ensure that the counter is counting for the proper duration, as it is programmable in the FW, see, e.g.,

https://github.com/cms-gem-daq-project/ctp7_modules/blob/dbe9e1d65464bdaf9d54160f378bded7bdd1ef41/src/calibration_routines.cpp#L1009-L1010

Hmm, maybe not for this scan though...

AndrewLevin commented 5 years ago

In addition to waiting before reading, you need to ensure that the counter is counting for the proper duration, as it is programmable in the FW, see, e.g.,

https://github.com/cms-gem-daq-project/ctp7_modules/blob/dbe9e1d65464bdaf9d54160f378bded7bdd1ef41/src/calibration_routines.cpp#L1009-L1010

Hmm, maybe not for this scan though...

Yes, I believe because we set SBIT_CNT_PERSIST to 0x1 in this scan, the counter will not be reset unless we reset it.

jsturdy commented 5 years ago

In addition to waiting before reading, you need to ensure that the counter is counting for the proper duration, as it is programmable in the FW, see, e.g., https://github.com/cms-gem-daq-project/ctp7_modules/blob/dbe9e1d65464bdaf9d54160f378bded7bdd1ef41/src/calibration_routines.cpp#L1009-L1010

Hmm, maybe not for this scan though...

Yes, I believe because we set SBIT_CNT_PERSIST to 0x1 in this scan, the counter will not be reset unless we reset it.

After confirming with the FW designers, I think we should change the mechanism that this tool is using. Rather than using a rolling counter, we should use the SBIT_CNT_PERSIST = 0 mode and set the SBIT_CNT_MAX_TIME. We'd still have to wait for a bit longer than the delay before reading, but this would make the results more accurate. We just need to ensure that the counter is reset after changing the DAC value (which should already be the case). If we don't manage to read all counters within the same integration time, it doesn't matter, because the counter register holds the value for the duration of the integration time, and we'd just read the next window (which would correspond to the same DAC value)

This mechanism is already used in the lines I showed above for the s-bit rate with pulse function, and I think the only changes would be where the CNT_PERSIST is set to 1, we'd set it to 0 and simultaneously set the CNT_MAX_TIME

AndrewLevin commented 5 years ago

ok, I see currently we are just using the sleep function on the ctp7, so we are not accounting for delays in the ctp7 processer, the time taken to execute other lines of code before we read the result, the delay between the ctp7 and the OH, etc.

AndrewLevin commented 5 years ago

We just need to ensure that the counter is reset after changing the DAC value (which should already be the case). If we don't manage to read all counters within the same integration time, it doesn't matter, because the counter register holds the value for the duration of the integration time, and we'd just read the next window (which would correspond to the same DAC value)

I don't understand this. We should read the counters after the integration time.

AndrewLevin commented 5 years ago

In addition to waiting before reading, you need to ensure that the counter is counting for the proper duration, as it is programmable in the FW, see, e.g., https://github.com/cms-gem-daq-project/ctp7_modules/blob/dbe9e1d65464bdaf9d54160f378bded7bdd1ef41/src/calibration_routines.cpp#L1009-L1010

Hmm, maybe not for this scan though...

Yes, I believe because we set SBIT_CNT_PERSIST to 0x1 in this scan, the counter will not be reset unless we reset it.

Do we understand where 0x02638e98 comes from?

AndrewLevin commented 5 years ago

We just need to ensure that the counter is reset after changing the DAC value (which should already be the case). If we don't manage to read all counters within the same integration time, it doesn't matter, because the counter register holds the value for the duration of the integration time, and we'd just read the next window (which would correspond to the same DAC value)

I don't understand this. We should read the counters after the integration time.

According to the comment in the code

https://github.com/cms-gem-daq-project/ctp7_modules/blob/develop/src/calibration_routines.cpp#L1002

the counter is reset after the SBIT_CNT_TIME_MAX. So, then we have to read it before it is reset, and so we still need to use the linux sleep function to measure the time. So, is there any benefit to using SBIT_CNT_TIME_MAX for this calibration routine actually?

jsturdy commented 5 years ago

We just need to ensure that the counter is reset after changing the DAC value (which should already be the case). If we don't manage to read all counters within the same integration time, it doesn't matter, because the counter register holds the value for the duration of the integration time, and we'd just read the next window (which would correspond to the same DAC value)

I don't understand this. We should read the counters after the integration time.

For this scan the sequence should be (after doing the initial setup): 1) set ARM_DAC on all chips 1) reset all s-bit counters 1) wait waitTime 1) read all s-bit counters 1) repeat

jsturdy commented 5 years ago

In addition to waiting before reading, you need to ensure that the counter is counting for the proper duration, as it is programmable in the FW, see, e.g., https://github.com/cms-gem-daq-project/ctp7_modules/blob/dbe9e1d65464bdaf9d54160f378bded7bdd1ef41/src/calibration_routines.cpp#L1009-L1010

Hmm, maybe not for this scan though...

Yes, I believe because we set SBIT_CNT_PERSIST to 0x1 in this scan, the counter will not be reset unless we reset it.

Do we understand where 0x02638e98 comes from?

It's the number of ticks in the 40.08 MHz clock in one second

jsturdy commented 5 years ago

We just need to ensure that the counter is reset after changing the DAC value (which should already be the case). If we don't manage to read all counters within the same integration time, it doesn't matter, because the counter register holds the value for the duration of the integration time, and we'd just read the next window (which would correspond to the same DAC value)

I don't understand this. We should read the counters after the integration time.

According to the comment in the code

https://github.com/cms-gem-daq-project/ctp7_modules/blob/develop/src/calibration_routines.cpp#L1002

the counter is reset after the SBIT_CNT_TIME_MAX. So, then we have to read it before it is reset, and so we still need to use the linux sleep function to measure the time. So, is there any benefit to using SBIT_CNT_TIME_MAX for this calibration routine actually?

The counter will get the new value after SBIT_CNT_TIME_MAX, I should ask @andrewpeck to update the documentation string to make this clear. If you set SBIT_CNT_PERSIST to 0, and set SBIT_CNT_TIME_MAX, then the FW counts the s-bits for a duration of SBIT_CNT_TIME_MAX and when this counter expires, it fills the individual counter values in their respective registers. These registers hold this constant value until the next update, when they are populated with the count from the preceding SBIT_CNT_TIME_MAX integration window

AndrewLevin commented 5 years ago

ok, I have made this change, but it still needs to be tested

lpetre-ulb commented 5 years ago

We'd still have to wait for a bit longer than the delay before reading, but this would make the results more accurate.

Can you quantify "a bit longer"? If I'm not mistaken we would have to wait twice longer if we want to cover for the worst case scenario.

Indeed, the Sbit time counter (used for SBIT_CNT_TIME_MAX) is not reset when the Sbit rate counters themselves are reset. I hope this time diagram will clarify my thoughts:

 | Acquisition window 1 | Acquisition window 2 | Acquisition window 3 |
 | Value from window 0  | Value from window 1  | Value from window 2  |
      ^ Set DAC value           ^ Reset         ^ Filling register
                                                       ^ Read a corrupted value (waitTime after reset)

@andrewpeck, do you confirm this behavior? I think that if the Sbit time counter is reset at the same time as the Sbit counters, it should work as expected.

jsturdy commented 5 years ago

We'd still have to wait for a bit longer than the delay before reading, but this would make the results more accurate.

Can you quantify "a bit longer"? If I'm not mistaken we would have to wait twice longer if we want to cover for the worst case scenario.

Indeed, the Sbit time counter (used for SBIT_CNT_TIME_MAX) is not reset when the Sbit rate counters themselves are reset. I hope this time diagram should clarify my thoughts:
 | Acquisition window 1 | Acquisition window 2 | Acquisition window 3 |
 | Value from window 0  | Value from window 1  | Value from window 2  |
      ^ Set DAC value           ^ Reset         ^ Filling register
                                                       ^ Read a corrupted value
@andrewpeck, do you confirm this behavior? I think that if the Sbit time counter is reset at the same time as the Sbit counters, it should work as expected.

OK, this indeed should be clarified, my understanding was that FPGA.TRIG.CNT.RESET resets everything in this block such that we'd get the "desired" behaviour. In that case, one just has to wait for the value to be put into the register and that should have some deterministic value "just longer" than SBIT_CNT_TIME_MAX, if this is not the case, then I agree with @lpetre-ulb, after issuing a reset, we should wait 2*waitTime before attempting to read back the value

andrewpeck commented 5 years ago

Yes I think Laurent is right that 2*waitTime is the safe period to wait.. sorry for the confusion. I can update the firmware with a very simple 1 line change so that the cnt_reset also resets the strobe so you could just do a cnt_reset then wait for 1*waitTime, but it will only work in new firmware versions

andrewpeck commented 5 years ago

New firmware version is released here: https://github.com/cms-gem-daq-project/OptoHybridv3/releases/tag/3.2.8.X

In this case a CNT_RESET will restart the timer, so you can do a cnt_reset then wait for 1xCNT_TIME_MAX

jsturdy commented 5 years ago

New firmware version is released here: https://github.com/cms-gem-daq-project/OptoHybridv3/releases/tag/3.2.8.X

In this case a CNT_RESET will restart the timer, so you can do a cnt_reset then wait for 1xCNT_TIME_MAX

Thanks @andrewpeck !

jsturdy commented 5 years ago

@AndrewLevin any update on the two different modes (old FW vs new FW)?

AndrewLevin commented 5 years ago

It is ready to be tested. I just made a reservation for this afternoon.

AndrewLevin commented 5 years ago

I have just tested this pull request with the new FW on the coffin and it seems to work:

With waitTime = 1 second:

canv_Summary_Rate1D_vs_THR_ARM_DAC

With waitTime = 10 seconds:

canv_Summary_Rate1D_vs_THR_ARM_DAC

With waitTime = 100 seconds:

canv_Summary_Rate1D_vs_THR_ARM_DAC

jsturdy commented 5 years ago

@AndrewLevin can you rebase this quickly?

AndrewLevin commented 5 years ago

@AndrewLevin can you rebase this quickly?

done

cms-gem-daq-project / ctp7_modules