Implement pwm_get_mask_enabled #3046

2026-06-29T18:33:41-05:00

sprickett commented

2026-06-29 18:33:41 -05:00

(Migrated from github.com)

Fixes #3045, pwm_set_mask_enabled missing complementary pwm_get_mask_enabled

It also addresses a small typo in pwm_set_mask_enabled comments, Bitmap --> Bitmask

Fixes [#3045](https://github.com/raspberrypi/pico-sdk/issues/3045), pwm_set_mask_enabled missing complementary pwm_get_mask_enabled It also addresses a small typo in pwm_set_mask_enabled comments, Bitmap --> Bitmask

sprickett (Migrated from github.com) reviewed

2026-06-29 18:35:40 -05:00

src/rp2_common/hardware_pwm/include/hardware/pwm.h

					
				@ -564,2 +564,4 @@

				}

				/** \brief Get PWM slice enabled status

				 *  \ingroup hardware_pwm

or Get PWM enabled slice status?

Wren6991 (Migrated from github.com) reviewed

2026-08-18 20:26:59 -05:00

Wren6991 (Migrated from github.com) left a comment

That's not a typo. Bitmap is a correct term to use here. I'd even argue bitmask is incorrect because it's not masking anything.

What's the use case for this? Any combination of get() + set() is a read-modify-write which causes a data race with concurrent writers. The correct way to modify the register is something like:

hw_set_bits(&pwm_hw->en, (1 << x) | (1 << y));

...which is fairly straightforward and doesn't have the same failure modes.

That's not a typo. Bitmap is a correct term to use here. I'd even argue bitmask is incorrect because it's not masking anything. What's the use case for this? Any combination of `get()` + `set()` is a read-modify-write which causes a data race with concurrent writers. The correct way to modify the register is something like: ```c hw_set_bits(&pwm_hw->en, (1 << x) | (1 << y)); ``` ...which is fairly straightforward and doesn't have the same failure modes.

sprickett commented

2026-08-19 19:42:27 -05:00

(Migrated from github.com)

@Wren6991 Thanks for commenting.

I don't have much skin in the game here. To argue my case...

Regarding pwm_get_mask_enabled(void)
I was addressing what i saw as an asymmetry with pwm_set_mask_enabled(uint32_t mask) which is implemented simply as pwm_hw->en = mask;

It would seem from your comment that this should maybe be implemented as hw_set_bits(&pwm_hw->en, mask);?

Regardless it still does not solve the conundrum I was facing when implementing logic that utilised multiple pwm pins.
I wanted logic that was reusable and self contained. pwm_set_mask_enabled(uint32_t mask) offers an attractive way to toggle all the associated slices in one go. This does not work when there is some other self contained logic that is using pwm because one disables the other. The initial solution was to read pwm_hw->en directly. This felt a bit wrong so I proposed this getter. This getter also would allow the logic to assert a slice conflict.

Regarding concurrency. I might have the wrong idea of some fundamentals here. Its great that the SDK provides functions that handle concurrent writes but for a simple use case utilising one core is there any concurrency to guard against?

Ultimately I used pwm_set_enabled(uint slice_num, bool enabled) on each of the slices individually. I used a mask to keep track of which slices had already been enabled by other pins in the set. I have also since noted that this function enables/disables a slice using different register addresses to pwm_set_mask_enabled(uint32_t mask).

Regarding Bitmap vs Bitmask, to me they both describe an integral type with zero or more bits set.
They are both used throughout the SDK interchangeably. Bitmask heavily outweighs Bitmap.
pwm.h has eight instances of Bitmask and one instance of Bitmap
Overall there are roughly 144 instances of Bitmask to 13 instances of Bitmap
There is only one instance of "Bitmap" exact.
The suggested change was for more for consistency with the SDK as a whole than to say it is wrong.

Happy to close if you feel this is not well reasoned.

@Wren6991 Thanks for commenting. I don't have much skin in the game here. To argue my case... Regarding `pwm_get_mask_enabled(void)` I was addressing what i saw as an asymmetry with `pwm_set_mask_enabled(uint32_t mask)` which is implemented simply as `pwm_hw->en = mask;` It would seem from your comment that this should maybe be implemented as `hw_set_bits(&pwm_hw->en, mask);`? Regardless it still does not solve the conundrum I was facing when implementing logic that utilised multiple pwm pins. I wanted logic that was reusable and self contained. `pwm_set_mask_enabled(uint32_t mask)` offers an attractive way to toggle all the associated slices in one go. This does not work when there is some other self contained logic that is using pwm because one disables the other. The initial solution was to read `pwm_hw->en` directly. This felt a bit wrong so I proposed this getter. This getter also would allow the logic to assert a slice conflict. Regarding concurrency. I might have the wrong idea of some fundamentals here. Its great that the SDK provides functions that handle concurrent writes but for a simple use case utilising one core is there any concurrency to guard against? Ultimately I used `pwm_set_enabled(uint slice_num, bool enabled)` on each of the slices individually. I used a mask to keep track of which slices had already been enabled by other pins in the set. I have also since noted that this function enables/disables a slice using different register addresses to `pwm_set_mask_enabled(uint32_t mask)`. Regarding Bitmap vs Bitmask, to me they both describe an integral type with zero or more bits set. They are both used throughout the SDK interchangeably. Bitmask heavily outweighs Bitmap. `pwm.h` has eight instances of Bitmask and one instance of Bitmap Overall there are roughly 144 instances of Bitmask to 13 instances of Bitmap There is only one instance of "Bitmap" exact. The suggested change was for more for consistency with the SDK as a whole than to say it is wrong. Happy to close if you feel this is not well reasoned.

lurch commented

2026-08-20 04:10:41 -05:00

(Migrated from github.com)

What's the use case for this? Any combination of get() + set() is a read-modify-write which causes a data race with concurrent writers.

@Wren6991 See my comment in the associated issue.

> What's the use case for this? Any combination of `get()` + `set()` is a read-modify-write which causes a data race with concurrent writers. @Wren6991 See my [comment](https://github.com/raspberrypi/pico-sdk/issues/3045#issuecomment-4843075674) in the associated issue.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'sprickett/pwm_get_mask_enabled:sprickett/pwm_get_mask_enabled'

git switch 'sprickett/pwm_get_mask_enabled'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'

git merge --no-ff 'sprickett/pwm_get_mask_enabled'

git switch 'sprickett/pwm_get_mask_enabled'

git rebase 'develop'

git switch 'develop'

git merge --ff-only 'sprickett/pwm_get_mask_enabled'

git switch 'sprickett/pwm_get_mask_enabled'

git rebase 'develop'

git switch 'develop'

git merge --no-ff 'sprickett/pwm_get_mask_enabled'

git switch 'develop'

git merge --squash 'sprickett/pwm_get_mask_enabled'