Introduced firmware binning switch for CbmStsUnpackAlgoLegacy.

@a.toia,

for how long is this option needed?

Thanks, Dominik!

AFCK-readout-data need the Legacy Unpacker

with BinningFW: mCBM data of 2020
without BinningFW: mCBM data before 2020, and Lab/COSY data past, present and future (untill Binning FW may be installed in Lab)

CRI-readout data need "standard" Unpacker: mCBM data 2021 and 2022.

So I think this scheme is still needed for a while...

Dear @v.friese,

you have been identified as code owner of at least one file which was changed with this merge request.

Please check the changes and approve them or request changes.

added CodeOwners label

Looks Ok to me, let me know if I should review or merge it

I think generally we have to be careful with declaring things "deprecated" too fast: all our online code is for me at the current time only dev status, not pro.
Plus it will anyway probably always remain an overkill with not user-friendly flexibility and monitoring for the low rate lab-setups of the various detectors (I can think of the STS lab setup but also the TOF setups in Heidelberg and Bucarest + their old setups in China).

@p.-a.loizeau,

I agree that we shouldn't outdate code too fast. My point was targeting into a different direction. I am against having only one unpacker which unpacks all data. If there are different format there should be different unpackers, Otherwise we pile up all the legacy in one place and in the end it becomes unhandy or very complicated to remove the legacy code.

On the other hand, if we introduce a new unpacker for each data format, we will likely end up with a situation where we have multiple classes with 99% identical code, which would violate the "do not repeat yourself" principle of clean coding.

CbmStsUnpackAlgo and CbmStsUnpackAlgoLegacy are already very similar.

Then the 99% of identical code should be in a base class and the 1% difference should be in derived classes.

Hi Florian I agree with you, and I think this is reflected in the code: AFCK/CRI have different data format -> two different unpackers In the AFCK format there is a small firmware difference (BinningFW) -> just a switch in one of the unpackers

Not everything here can be easily moved to the base class. The "binning / no binning" switch appears in the middle of larger functions. I think the if-statement is the simplest solution for this particular case.

That said, there are probably some parts of the STS unpacker that could be moved. I would propose to do this in a separate MR.

Okay. You are right. I didn't check the MR careful enough and overlooked e=that it is mainly changing the legacy unpacker. For sure I am fine with that since this is what I was requesting. If there a minor differences between two firmware updates this should be handled in the way it is. What I don't understand why normal unpacker class has to be changed also.

@f.uhlig regarding your question: I had to introduce the binning / no binning switch in the base class. This is kind of unfortunate, because it is only used by the legacy unpacker, not the normal one. But it was necessary, because the steering class, which sets all of the parameters, uses pointers to the base class.

Then I had to modify the normal unpacker such that it produces a warning when this switch is used, noting that it is not implemented.

I could instead use some type-casts together with some safety checks at the level of the steering class. The result would be a more tidy unpacker base class, at the expense of a less tidy steering class. I'm not sure that this is better.

added 3 commits

6ed95860...19ab4b46 - 2 commits from branch computing:master
b5336926 - Introduced firmware binning switch for CbmStsUnpackAlgoLegacy.

Compare with previous version

enabled an automatic merge when the pipeline for b5336926 succeeds

merged

Introduced firmware binning switch for CbmStsUnpackAlgoLegacy.

Merge request reports

Activity