Project

General

Profile

Actions

Development #1366

closed

Development #1353: Code cleanup for OCT19

Code cleanup: data/CbmMuchDigi.cxx

Added by Volker Friese almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Start date:
09/16/2019
Due date:
09/27/2019
% Done:

100%

Estimated time:
0.50 h
Spent time:

Description

Compiler warning:

data/much/CbmMuchDigi.cxx: In member function 'void CbmMuchDigi::SetAdc(Int_t)':
[CTest: warning matched] /home/uhlig/cbm/release/oct19/data/much/CbmMuchDigi.cxx:61:13: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  if(fCharge < 0) fCharge=0;

fCharge is defined as UShort_t, so it cannot be negative. You probably meant

if ( adc < 0 ) fCharge = 0;

Please fix.

Actions #1

Updated by Florian Uhlig almost 3 years ago

I changed the code now such that it looks like

void CbmMuchDigi::SetAdc(Int_t adc) {
  //ADC value should not be more than saturation
  if (adc < 0) {
    fCharge=0;
  } else {
    fCharge=adc;
  }
  // if Saturation
  //              Int_t saturation = (1<<12); //2 ^ 12 - 1;
  Int_t saturation = (1<<5); //2 ^ 5 - 1; // 32 for 5 bit adc*** modified by Ekata Nandy on 25/06/19***
  if(fCharge >= saturation){
    //fCharge=saturation-1;
    fCharge=saturation; //As ADC value starts from 1, so -1 removed. Modified by Ekata Nandy on 25/06/19
    fSaturationFlag=1;
  }
//      if(fCharge < 0) fCharge=0;
}

While doing the change I stumbled over 2 things.

1. The variable fSaturationFlag is only set but never used. So here the question arises if the variable is needed at all.
2. The saturation value is set to 32 in the class CbmMuchDigi but in CbmMuchSignal the saturation value is 4095. Is this the intention?

Actions #2

Updated by Vikas Singhal almost 3 years ago

1. The variable fSaturationFlag is only set but never used. So here the question arises if the variable is needed at all.

During modification of CbmMuchDigi for TimeBased development, it was thought to use this in the QA class. But till QA class not modified to use the same. Probably we should remove this, as saturation can be estimated if fCharge > 32. It will save space also.

But this modification will create backward compatibility issue? Present root file containing CbmMuchDigi branch cannot be used by modified class?

2. The saturation value is set to 32 in the class CbmMuchDigi but in CbmMuchSignal the saturation value is 4095. Is this the intention?

No, it is due to code modifications by different developers. We will rectify the same. Will use one static constant for the highest ADC value of the Chip.

Will it OK if we rectify the same after 30Sept?

Regards
Vikas

Actions #3

Updated by Volker Friese almost 3 years ago

  • Status changed from Assigned to In Progress
  • % Done changed from 0 to 70

Vikas Singhal wrote:

Will it OK if we rectify the same after 30Sept?

Sure. The warning have disappeared already.

Actions #4

Updated by Florian Uhlig almost 3 years ago

During modification of CbmMuchDigi for TimeBased development, it was thought to use this in the QA class. But till QA class not modified to use the same. Probably we should remove this, as saturation can be estimated if fCharge > 32. It will save space also.

But this modification will create backward compatibility issue? Present root file containing CbmMuchDigi branch cannot be used by modified class?

The backward compatibility shouldn't be a problem. The Schema Evolution of ROOT should take care of it. If you run old files with new code the variable is read from the file and simply dropped when filling the data into the object. When reading new files with old code the variable isn't in the file but ROOT will take care that the variable is initialized to the value defined in the constructor of CbmMuchDigi. Since the variable was never used anywhere in the code it doesn't matter which value it has.

No, it is due to code modifications by different developers. We will rectify the same. Will use one static constant for the highest ADC value of the Chip.

OK. This is what I have expected.

Will it OK if we rectify the same after 30Sept?

Sure. I add the issues only to the ticket such that they are not forgotten. Is the change in CbmMuchDigi OK for you?

Actions #5

Updated by Volker Friese over 2 years ago

Dear Vikas,

please comment on the status and close the issue if done. We want to freeze OCT19.

Actions #6

Updated by Vikas Singhal over 2 years ago

Dear Volker,

As Florian already committed the code which takes care for -ve fCharge value, therefore you may close this issue.

There is a Boolean fSaturationFlag under CbmMuchDigi which is not needed so can be removed. Probably this modification is not urgent for Oct19 release.

Florian: There is no saturation value is defined under CbmMuchSignal class, however will use static constant for the same in CbmMuchDigi.

Regards
Vikas

Actions #7

Updated by Volker Friese over 2 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 70 to 100
Actions

Also available in: Atom PDF