-
-
Notifications
You must be signed in to change notification settings - Fork 40.3k
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
[Core] quantum: util: add bit and bitmask helpers #24229
Conversation
fb46eda
to
079cf43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. Here are a few suggestions to consider to improve clarity.
quantum/bits.h
Outdated
#define BITS_PER_LONG 32 | ||
#define BITS_PER_LONG_LONG 64 | ||
|
||
#define BIT(nr) (UL(1) << (nr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can appreciate the usefulness of these utilities and their proven track record, considering that they are adopted from the Linux kernel. Nevertheless, they could be made clearer with a minor change.
Each of these macros supposes a certain integer type. A further subtlety is that they are typed by unsigned long
and unsigned long long
, whose bit widths in general are platform dependent. Though I see these widths are assumed above to be specifically 32-bit and 64-bit, it is still confusing.
I suggest using the stdint.h fixed width types uint32_t
and uint64_t
, and to use a naming suffixed with 32
and 64
. This parallels the naming of QMK's existing software timers APIs, for instance. Additionally, adding good doc comments helps, of course. Like:
#include <stdint.h>
/** Mask for the little endian nth bit (0-31) in a 32-bit integer. */
#define BIT32(n) (UINT32_C(1) << (n))
/** Mask for the little endian nth bit (0-63) in a 64-bit integer. */
#define BIT64(n) (UINT64_C(1) << (n))
// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I applied that and added the suffixes to the GENMASK
flavors as well.
quantum/bits.h
Outdated
|
||
/* | ||
* Create a contiguous bitmask starting at bit position @l and ending at | ||
* position @h. For example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document that the h
and l
endpoints are inclusive (if I got that right). Also, same comment as above that suffixing with the type's width "32" or "64" would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
quantum/bits.h
Outdated
#define BIT(nr) (UL(1) << (nr)) | ||
#define BIT_ULL(nr) (ULL(1) << (nr)) | ||
#define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) | ||
#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intended purpose of BIT_MASK()
and BIT_WORD()
is to select the nth bit from a uint32_t array, is that right? Are there other notable use cases besides this? Like:
uint32_t array[N];
// Get nth bit from `array`.
array[BIT_WORD(n)] & BIT_MASK(n)
I have to say, I find this unclear. The notion of a "word" is prone to misunderstanding, since it depends on context what the word data unit is. It seems easy to overlook a mismatch between the array datatype and the word size supposed by these macros. I suggest again that the bit width be stated explicitly. Arguably, we could just as well reuse the BIT32()
macro [né BIT()
] from above, eliminating the _MASK
and _WORD
macros. Like:
// Get nth bit from `array`.
array[n / 32] & BIT32(n % 32)
Would this be an improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I haven't used BIT_MASK()
and BIT_WORD()
myself but that seems to be the intended use-case. In the current iteration they have been removed as I don't see any benefit from having those, just like you said.
b6b5ee9
to
d640e1f
Compare
These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
d640e1f
to
e3003c3
Compare
@getreuer Thanks for giving this some more thought and your great improvements. The new implementations are basically your suggestions so I added you as a co-author to the commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks great! I appreciate the co-authorship =)
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
quantum: util: add bit and bitmask helpers These helpers are handy and can prevent off-by-one errors when working with registers and general low level bit manipulation tasks. The macros themself are inspired by the bits.h macros from the linux kernel source code. Signed-off-by: Stefan Kerkmann <[email protected]> Co-authored-by: Pascal Getreuer <[email protected]>
Description
These helpers are handy and can prevent off-by-one errors when working
with registers and general low level bit manipulation tasks. The macros
themself are inspired by the bits.h macros from the linux kernel source
code.
Types of Changes
Issues Fixed or Closed by This PR
Checklist