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

Utilization of event data fields in SelEntry class #69

Merged
merged 5 commits into from
Oct 21, 2019

Conversation

Arcticae
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Aug 27, 2019

Coverage Status

Coverage decreased (-0.1%) to 75.02% when pulling 174e8bd on Arcticae:master into 0121bb0 on kontron:master.

Copy link
Contributor

@hthiery hthiery left a comment

Choose a reason for hiding this comment

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

Why splitting these values to different variables? Isn't it better to have an array with these values?

@Arcticae
Copy link
Author

Arcticae commented Sep 2, 2019

It's the same way i did it in the previous PR, and IPMI documentation defines these 3 variables as 3 separate fields, so i thought it would be more readable that way, although array would be fine with me too. If you think it's more convenient, let me know and i'll edit the PR.

@hthiery
Copy link
Contributor

hthiery commented Sep 2, 2019

yes ... sorry for the confusion. I think it would be better to have these kind a values as list/array like value

@Arcticae
Copy link
Author

@hthiery I've changed event_data to store it as an array of integers (more useful for further decoding than plain string), and also corrected the event_type decoding, as it was only taking 6 younger bits of the event (mask was 0x3f), instead of 7 (now mask is 0x7f). Would appreciate if you reviewed the changes and merged them :)

@hthiery hthiery merged commit 6c07fb1 into kontron:master Oct 21, 2019
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.

3 participants