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

sys/bitfield: add support for bit-wise bitfield operations #17710

Merged
merged 3 commits into from Mar 9, 2022

Conversation

benpicco
Copy link
Contributor

Contribution description

This adds support for bit-wise (AND, OR, XOR, NOT) operations to bitfields.

Testing procedure

The tests-bitfield unit test has been extended with the new operations.

Issues/PRs references

@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Feb 26, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 26, 2022
* @param[in] a The first bitfield
* @param[in] b The second bitfield
* @param[in] len The size of the bitfield in bytes
*/
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a @pre stating that the size for a, b, and c must be greater than len.

Also, I would rephrase to something like

  * @param[in]     len   The number of bytes to which the operation is applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the size for a, b, and c must be greater than len.

Shouldn't it be at least len?
I assumed one would typically run the operation on the whole bitfield, not arbitrary byte-sized chunks.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it should just be documented exactly to make sure that it is the responsibility of the caller to check these preconditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like this is stating the obvious, but there you go

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

Exiting functions/macros use size as the number of bits.

There already is an operation int bf_get_unset(uint8_t field[], int size); which has a size parameter, they should all use the same type - seems size_t would be correct.

bitfield.h also seems to miss some kind of initialization illustrated by the example l202 and l203

@kaspar030
Copy link
Contributor

kaspar030 commented Feb 28, 2022

There already is an operation int bf_get_unset(uint8_t field[], int size); which has a size parameter, they should all use the same type - seems size_t would be correct.

I agree on this. Also, using bits vs. bytes doesn't fail as horrible as if someone supplies the number of bits as bytes argument. (does this sentence make sense?)

@kaspar030
Copy link
Contributor

I'd like to see a line or two on the unused bits of a bitfield and how they are affected by the ops. I mean, a "bitfield" of 10 bits uses 2 bytes of storage. the unused 6 bits will get modified by the current and/or/xor implementations. That is fine IMO, but should be documented.

@benpicco benpicco force-pushed the sys/bitfield-ops branch 2 times, most recently from 8ab7441 to 2a616ab Compare March 1, 2022 12:27
sys/bitfield/bitfield.c Show resolved Hide resolved
sys/include/bitfield.h Outdated Show resolved Hide resolved
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 3, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 8, 2022
@benpicco benpicco requested a review from kfessel March 9, 2022 12:31
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

murdock test are sufficient in this case, and murdock is green.

@benpicco benpicco merged commit 60fb36e into RIOT-OS:master Mar 9, 2022
@benpicco benpicco deleted the sys/bitfield-ops branch March 9, 2022 21:09
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants