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.

Sign up for GitHub

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

Jump to bottom

Implement measurement decode #131

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement measurement decode #131

wants to merge 4 commits into from

Conversation

rnestler
Copy link
Contributor

No description provided.

fi5ch
fi5ch reviewed Mar 15, 2022
View reviewed changes
@dbrgn dbrgn mentioned this pull request Dec 21, 2022
Merged
@dbrgn dbrgn force-pushed the implement-measurement-decode branch from 570d164 to cc9ebf9 Compare December 21, 2022 20:38
@dbrgn
Copy link
Contributor

dbrgn commented Dec 21, 2022 •
edited
Loading

@rnestler I rebased and continued with this PR. First I wrote some tests and noticed that they always failed. After some debugging, I changed the loading of the bits from LE to BE: cf7f06e To be honest I still find it hard to understand the bitvec crate, but the tests pass now.

From my view, after adding these tests, the PR should now be ready to merge. What do you think?

@dbrgn dbrgn marked this pull request as ready for review December 21, 2022 22:07
@rnestler
Copy link
Contributor Author

I changed the loading of the bits from LE to BE

Well this makes sense, since we store the bits in BE order (most significant bit first).

@rnestler
Copy link
Contributor Author

To be honest I still find it hard to understand the bitvec crate, but the tests pass now.

True. Maybe it would be easier to just hand-roll the masking and shifting of the bits.

rnestler
rnestler commented Dec 22, 2022
View reviewed changes
Copy link
Contributor Author

@rnestler rnestler left a comment

Choose a reason for hiding this comment

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

Yeah I guess we can merge it as is, but maybe consider refactoring it in the future.

common/src/measurement.rs
Comment on lines +215 to +217
// Value: 0000_0101_1010 = 90
0b0000_0101,
0b1010_0000,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit a confusing test case, since it starts with 0000 and gets padded with 0000. I'd use 1000_0101_1010 as the test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbrgn dbrgn dbrgn left review comments

@fi5ch fi5ch fi5ch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rnestler @dbrgn @fi5ch