Skip to content

Conversation

@zelbitar
Copy link
Contributor

Hi Jérôme,
Major update:
1- Truncated acquisitions pixels are not considered. A test a done in function isAcqSafe() in BoardReaderMIMOSIS.cxx
2- Frames around triggers can be considered in trigger mode. For this two parameters were added in the config file mimosis.cfg which are TriggerOffset and FramesPerTrigger:
*TriggerOffset: Number of frames where to start decoding starting from the trigger frame.
*FramesPerTrigger: Number of frames to decode per trigger.

zelbitar added 6 commits June 9, 2021 14:41
…e added to the configuration file > TriggerOffset that sets the offset of the frameID to be considered starting from trigger > FramespPerTrigger defines the number of frames to be considered per trigger
@jeromebaudot
Copy link
Owner

Ziad,

excellent work, I like your usage of the mechanism transmitting the parameters to handle the trigger from the config file, congrats! I have a few remarks before starting merging this new version.

1/ Could you generate two different config files, corresponding to the case where we use or no the trigger (mimosis.cfg and mimosis-triggered.cfg for instance)?
Also, would it be possible to make the two parameters (TriggerOffset, FramesPerTrigger) non mandatory if TriggerMode==0?
(maybe that could be for a next step)

2/ About ignoring the truncated frames, should we really enforce it (your version) or let the user decide about it? We could use the eventbuildingmode flag (set in the config) to decide that. But maybe it does not worth it.
<== feel free to reject this suggestion!

3/ Could you document a bit what the class is doing in the BoardReaderMIMOSIS.cxx file? I would suggest the following:

  • at the beginning of the file, under "Class Description of BoardReaderMIMOSIS", simply explains that there is two trigger mode (basically ignoring or using the trigger info)
  • replace the current description of the DecodeNextEvent() and DecodeFrame() functions, in particular explains in which conditions they return true or false.

4/ Could you check that any .DS_Store are not commited to TAF. Sorry, I know this is annoying. To avoid this in the future, you should check that your .gitignore file does contain a line about it.

Cheers,
Jerome

@zelbitar
Copy link
Contributor Author

Hi Jérôme,
Thanks for your comments and your suggestions.
About the truncated acquisitions, I was thinking to add a parameter in the configuration file, let's call it "SafeAcquisitionMode" which will be set to 1 if the user would like to consider only safe acquisitions. Of course, the isAcqSafe() method can evolve if required to consider further test on acquisitions "cleanliness".
I will propose an update soon after considering all your comments.

Cheers,
Ziad

@jeromebaudot
Copy link
Owner

Hi Jérôme,
Thanks for your comments and your suggestions.
About the truncated acquisitions, I was thinking to add a parameter in the configuration file, let's call it "SafeAcquisitionMode" which will be set to 1 if the user would like to consider only safe acquisitions. Of course, the isAcqSafe() method can evolve if required to consider further test on acquisitions "cleanliness".
I will propose an update soon after considering all your comments.

Cheers,
Ziad

I insist that you do not need to create a new flag but rather can use EventBuildingBoardMode for that.

Jerome

@zelbitar
Copy link
Contributor Author

Ok, I see.

Thanks

Ziad

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.

2 participants