Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updates to reader --- mainly to fix iTPC fee_words=0 problem #688

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

jml985
Copy link
Contributor

@jml985 jml985 commented Jun 20, 2024

No description provided.

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

This commit is important for FastOffline for the current data-taking at the experiment. There are many modifications I know nothing about, but I have commented on the items I identified which spurred Tonko to change the code, and consequentially Jeff to make this commit. I will approve from that perspective. Thanks, Jeff, for helping get this in.

@@ -1371,6 +1391,8 @@ itpc23::itpc23()

fmt = 0 ;

fee_words = 0 ;
Copy link
Contributor

Choose a reason for hiding this comment

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

This initialization is for me the critical part as an uninitialized fee_words was causing random issues for reconstructing iTPC data.

@@ -718,7 +719,7 @@ u_int *itpc23::fee_non_trgd(u_int *start)
int fee_words = 0 ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeff and I have noted to Tonko that this local fee_words should perhaps not be local, but we believe this is not worth holding up this commit as even without any change to this line, this commit is an important step forward.

@@ -540,7 +540,7 @@ int fcs_trg_base::end_event()

verify_event_io() ; // verify interconnectivity

//int dsmout = 0; moved to .h file
int dsmout = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Akio's concern seems legitimate https://github.com/star-bnl/star-sw/pull/688/files#r1651407708
This new definition should not be added since it shadows a class member with the same name that was introduced about 4 years ago: 4f5a804?diff=unified&w=0#diff-dd89e161b0be358807922c6c11eac6634ce3fcbfbb642e31f914a88942cd28baR257

Copy link
Member

Choose a reason for hiding this comment

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

@jml985 @tonko-lj can you follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody seems to know what's going on here. Obviously, this code is wrong because the local variable shadows the class variable. As far as I can tell this has been true since the FCS came online. It runs in the FCS though, and I think its Akio's code. Akio, are you asking me to change this? Are you asking Tonko?

@jml985
Copy link
Contributor Author

jml985 commented Jun 27, 2024

I talked to Akio and resolved the dsmout in the fcs_trg_base, This should now be ready to go.

@genevb genevb merged commit e23deba into star-bnl:main Jun 28, 2024
148 checks passed
jdbrice pushed a commit to jdbrice/star-sw-1 that referenced this pull request Aug 17, 2024
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.

4 participants