Semaphore: remove 16-bit limit #10224
Conversation
Semaphore had a historical 16-bit maximum count limit - this was odd, as the initial count was 32-bit. Remove the limitation - change constructors to take uint32_t, and make the default limit 0xffffffff rather than 0xffff. Limits of overload resolution prevent adding new constructors while retaining the old ones. Addresses issue raised in ARMmbed#10216.
| @note You cannot call this function from ISR context. | ||
| */ | ||
| Semaphore(int32_t count = 0); | ||
| Semaphore(uint32_t count = 0); |
There was a problem hiding this comment.
I believe the reason why count was int32_t is due to the fact that wait() returns the number of available tokens or -1 on invalid parameters. With count being uint32_t you cannot differentiate between the invalid parameters and (very unlikely) case where you have (2^32)-1 tokens available (on U2 complement systems).
There was a problem hiding this comment.
Well spotted. That particular wait API is pretty horrible, and I'm intending to deprecate it anyway in favour of a more rational set of try_acquire, acquire, acquire_for and acquire_until (aligning with the current state of Mutex::lock).
Maybe we could fudge wait so it just clamps to 0x7FFFFFFF? You can't really distinguish different positive return values usefully in a non-racey way anyway.
There was a problem hiding this comment.
I think clamping wait() output to 0x7FFFFFFF is really nasty. I'd just go with int32_t for the count and maximum count instead.
There was a problem hiding this comment.
What's nasty is returning a count at all - you can't really distinguish between any return values in the range 1 to 0x7FFFFFFF, as the instantaneous read is not atomic or synchronised in any way. All you know for certain is that you acquired 1.
wait and its count returning is on the chopping block for deprecation - acquire will not return a count. So I'm minded to ignore the current flaws of wait, rather than work around them.
|
@kjbracey-arm, thank you for your changes. |
If a Semaphore has a huge count, it could overflow the int32_t return of Semaphore::wait().
kjbracey
force-pushed
the
semaphore_32bit
branch
from
5611656 to
3cc4d17
Compare
|
I'm afraid that may break binaries in our sources, some of them would use semaphores. Can you confirm they are ok? |
|
Yes, binary breakage is likely. That may be strong enough reason to avoid making the change. I don't think we have any compelling need to up the limits. Another possibility might be to provide access via a |
|
I don't think there's a pressing requirement for this. I don't think it merits a breaking change, and I think my It's just an anomaly versus the underlying CMSIS-RTOS, really. I don't see any real use case for a semphore with >65535 count. Any thoughts from @desowin? |
The only reason why I mentioned this Semaphore 16-bit limit in #10216 was that I thought about stuffing Semaphore instance inside the Mail class. I had no idea that the underlying CMSIS-RTOS API can achieve timed and/or blocking allocation. Pull Request #10225 fully resolves my issue (lack of blocking alloc in Mail). I don't find any practical use for semaphores (nor even Mail boxes) with more than 16-bit count/items in embedded applications that I work on (tbh. even 8-bit would suffice for my uses). |
|
Closed due to general pointlessness. |
7 participants
Description
Semaphore had a historical 16-bit maximum count limit - this was odd, as the initial count was 32-bit.
Remove the limitation - change constructors to take uint32_t, and make the default limit 0xffffffff rather than 0xffff.
Limits of overload resolution prevent adding new constructors while retaining the old ones.
Addresses issue raised in #10216.
Pull request type
Release Notes
Semaphorenow permits the full 32-bit maximum count available in CMSIS RTOS 2. The change to its available constructors breaks binary compatibility, but there should be no source compatibility issues.Semaphoreis now 0xffffffff rather than 0xffff.