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

Reduce size of status structs. #11

Merged
merged 2 commits into from Aug 8, 2022

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Aug 5, 2022

  • Store returned status directly as u8 instead of as multiple bools.
  • Use bitflags for the status flags to get nicer debug output. Also renamed the flags to match the datasheet.

Copy link
Owner

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Nice improvement, thanks!
I would remove the bitflags dependency, though, since it is only for debug and it brings its own dependencies and additional MSRV policy.
Without that we can keep the MSRV for now until a newer language feature is of use here.

@reitermarkus
Copy link
Contributor Author

I would remove the bitflags dependency, though, since it is only for debug and it brings its own dependencies and additional MSRV policy.

I have another WIP branch which adds dedicated types for all registers using bitflags with many more improvements so removing the dependency here would cause a lot of conflicts and be a pain to rebase.

Also, this is a API-breaking change anyways, so might as well increase the MSRV.

@eldruin
Copy link
Owner

eldruin commented Aug 7, 2022

I see. The MSRV update policy of bitflags is apparently fine (although not properly documented) and no other build dependencies are added if not using features, so ok.
Could you add an entry to the changelog?
Then this would be good to go.

@reitermarkus
Copy link
Contributor Author

@eldruin, I added a changelog entry.

Copy link
Owner

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

@eldruin eldruin merged commit 504ab27 into eldruin:master Aug 8, 2022
@reitermarkus reitermarkus deleted the status-struct-size branch August 8, 2022 08:16
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.

None yet

2 participants