Skip to content

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Sep 8, 2025

Contribution description

Add bit_checkXX functions for kinetis MCUs, analogous to the existing bit_clear and bit_set functions.
A general bit_checkXX implementation was added with #16522 to sys/bit.h, and probably just forgotten for the kinetis override?

cc @benpicco.

Testing procedure

Unfortunately I don't have any kinetis-based boards with me. If someone has them and it is wanted, I can add some unitests for setting/clearing/ reading bits, analogous to the existing tests-bitfield test.

Issues/PRs references

Came up in #21577, which adds a first usage of bit_check (Murdock error).

A general `bit_checkXX` implementation was added with RIOT-OS#16522 to
`sys/bit.h`, but still missing for kinetis MCUs.
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Sep 8, 2025
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation and removed Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Sep 8, 2025
@riot-ci
Copy link

riot-ci commented Sep 8, 2025

Murdock results

✔️ PASSED

db2077f sys/bit: document return values

Success Failures Total Runtime
10515 0 10516 09m:45s

Artifacts

@benpicco
Copy link
Contributor

benpicco commented Sep 8, 2025

Is this not already covered by the CPU_HAS_BITBAND code path?

@github-actions github-actions bot added the Area: sys Area: System label Sep 8, 2025
@elenaf9
Copy link
Contributor Author

elenaf9 commented Sep 8, 2025

Is this not already covered by the CPU_HAS_BITBAND code path?

No, because the whole bit.h module is gated by a #if !BITBAND_FUNCTIONS_PROVIDED, so that path is never entered for the kinetis MCUs that have BITBAND_FUNCTIONS_PROVIDED enabled.

@benpicco
Copy link
Contributor

benpicco commented Sep 8, 2025

Ah the Kinetis Bit Manipulation Engine is indeed different from the ARM bit-banding extension! I would have thought they'd just use the standard functionality under a different name.

*/
static inline bool bit_check32(volatile uint32_t *ptr, uint8_t bit)
{
return *((volatile uint32_t *)(((uintptr_t)ptr) | BME_AND_MASK)) & (uint32_t)(1ul << bit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be

Suggested change
return *((volatile uint32_t *)(((uintptr_t)ptr) | BME_AND_MASK)) & (uint32_t)(1ul << bit);
return bme_bitfield32(ptr, bit, 32);

I have openlabs-kw41z-mini at home and can test

Copy link
Contributor Author

@elenaf9 elenaf9 Sep 10, 2025

Choose a reason for hiding this comment

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

Ah yeah, makes sense. I am not familiar with the kinetis bme at all, so I was just implementing it analogous to the other functions and didn't look at the bitfield functions above properly.
Should I already change it in this PR or do you want to do the testing first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Friendly ping @benpicco :)

Copy link
Contributor

@benpicco benpicco Sep 29, 2025

Choose a reason for hiding this comment

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

Sorry, I didn't get to it before my vaccation - I added a unit test so we can actually test those function and turns out bit_clearXX() is already not working 😕

2025-09-30 00:44:56,723 # START
2025-09-30 00:44:56,723 # .clear bit 0
2025-09-30 00:44:56,723 # clear bit 2
2025-09-30 00:44:56,723 # 
2025-09-30 00:44:56,724 # bit_tests.test_bit8 (tests/unittests/tests-bit/tests-bit.c 36) exp 250 was 251
2025-09-30 00:44:56,724 # .
2025-09-30 00:44:56,735 # bit_tests.test_bit16 (tests/unittests/tests-bit/tests-bit.c 60) exp 65530 was 65531
2025-09-30 00:44:56,735 # .
2025-09-30 00:44:56,740 # bit_tests.test_bit32 (tests/unittests/tests-bit/tests-bit.c 84) exp 4294967290 was 4294967291

If you want to move this forward, you can just set BITBAND_FUNCTIONS_PROVIDED 0 and I'll look into this later since I can actually test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks!

If you want to move this forward, you can just set BITBAND_FUNCTIONS_PROVIDED 0 and I'll look into this later since I can actually test it.

Opened #21750.

@crasbe crasbe added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants