Skip to content

Conversation

@rin-yokoyama
Copy link
Contributor

This will make UtkUnpacker to build events with given event width in an input config xml.

@ksmith0
Copy link
Contributor

ksmith0 commented Mar 14, 2017

Addresses issue #253

GetEventStartTime(), eventCounter);

//set eventWidth from Globals (loaded from config file)
eventWidth = Globals::get()->GetEventLengthInTicks();
Copy link
Contributor

@ksmith0 ksmith0 Mar 14, 2017

Choose a reason for hiding this comment

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

This line should use the Unpacker::SetEventWidth() method, eventWidth is protected.

Copy link
Contributor

@ksmith0 ksmith0 Mar 14, 2017

Choose a reason for hiding this comment

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

Also, this should not be in ProcessRawEvent() as the command will be called repeatedly. I believe it should be placed in UtkUnpacker::InitializeDriver().

Copy link
Member

@spaulaus spaulaus Mar 21, 2017

Choose a reason for hiding this comment

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

Actually, this should happen even before that. I think it needs to go in this method. Putting it into UtkUnpacker::InitializeDriver() is too late since it's already started to build events.

Copy link
Contributor Author

@rin-yokoyama rin-yokoyama Mar 21, 2017

Choose a reason for hiding this comment

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

You mean the first event is already build when UtkUnpacker::InitializeDriver() called? I will try initializing there then.

Copy link
Contributor

@ksmith0 ksmith0 left a comment

Choose a reason for hiding this comment

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

Looks good. I can not test it, so I can not approve the pull request.

@tking53 tking53 self-requested a review March 20, 2017 13:16
@tking53
Copy link
Collaborator

tking53 commented Mar 20, 2017

I have tested this. It works as expected and doesn't seem to break anything. It will require new Time Calibrations, but this is to be expected. I tested this on kqxhc. Additionally, this one line change was added to my ornl2016 branch ( https://github.com/tking53/paass/tree/ornl2016 ) as well as Andrew Keeler's e14060 branch ( https://github.com/akeeler/paass/tree/e14060 ) and it works there as well.

Linux Flavor
CentOS Linux release 7.3.1611 (Core)  

Linux Kernel 
3.10.0-514.6.1.el7.x86_64 

cmake version 2.8.12.2 

gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)

GSL version 
2.3

Copy link
Collaborator

@tking53 tking53 left a comment

Choose a reason for hiding this comment

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

Works as expected on kqxhc. See my comment for details.

@tking53 tking53 dismissed ksmith0’s stale review March 20, 2017 15:16

Requested changes were addressed.

@tking53 tking53 merged commit d120a5b into pixie16:dev Mar 20, 2017
@rin-yokoyama rin-yokoyama mentioned this pull request Mar 21, 2017
tking53 pushed a commit to rin-yokoyama/paass that referenced this pull request Mar 25, 2017
tking53 pushed a commit that referenced this pull request Mar 25, 2017
spaulaus pushed a commit that referenced this pull request Jun 7, 2017
spaulaus pushed a commit that referenced this pull request Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants