ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

CMSIS RTOSv2 API: osSemaphoreClear() is missing #703

Open udoe opened 5 years ago

udoe commented 5 years ago

Specifically in the case of a binary semaphore, there must be a way to reset the semaphore. Other RTOS APIs (e.g. Zephyr), do provide such a feature. Implementation should be trivial.

Thanks Udo

JonatanAntoni commented 5 years ago

Hi Udo,

thanks for your suggestion. Can we elaborate slightly more about the semantics behind osSemaphoreClear or perhaps osSemaphoreReset, please?

Currently a semaphore is created with providing two parameters: Initial count and max count. The max count is stored in the semaphores control block. The initial count is simply put to the semaphore counter right after creation.

To which value do you expect the osSemaphoreReset function to reset the semaphore counter to?

Cheers, Jonatan

udoe commented 5 years ago

Hi Jonatan,

I would expect that osSemaphoreClear sets the semaphore counter to zero (blocking state). Specifically this is useful in the case of a binary semaphore (max_count=1). This kind of semaphore can be used to unblock a thread that's waiting for an asynchronous operation to complete. Typically, one needs a method to reset the semaphore to the not-signaled state before the asynch operation is initiated.

Consider the following example (pseudo code):

// Function that executes an I2C transaction and 
// returns when it completed or timed out (synchronous behavior).
DoI2cTransactionSynch()
  (1) reset semaphore to not-signaled state (counter = zero)
  (2) create and submit transaction to driver
  (3) wait on semaphore (with timeout)
  (4) in case of timeout, cancel transaction and return failure
  (5) in case of completion (with success or error), return completion status

// Callback from driver when transaction completes, typically runs at ISR level.
AsynchDriverCallback()
  signal semaphore

Note that there is a potential race in case 4 (timeout). There is a chance that AsynchDriverCallback runs just after the wait returned in step 3. In this case the semaphore is left in signaled state when DoI2cTransactionSynch returns. This is why we need to reset the semaphore in step (1),

initial_count is just for initialization. It should not be referenced afterwards.

Alternatively, there could be a more generic function osSemaphoreSetCounter(value) which sets the counter to value. But I'm not aware of a use case for this.

Best, Udo

JonatanAntoni commented 5 years ago

Thanks Udo,

I am looking for a more generic approach. In your case clear means setting the count to zero which sounds reasonable. In another context clear might imply setting the count to max (or back to initial value). So there might be different readings of a function called osSemaphoreClear.

I am curious if you can't solve your specific requirements with the current API. I guess your semaphore has a max count of 1. What about just calling osSemaphoreAcquire(id, 0) at (1)? This decrements the count to 0 if it is still 1 and returns osOK. Or it returns osErrorResource if the count is already at 0. In either case your semaphore is properly cleared before triggering the transaction. As the maximum count is 1 (binary semaphore) it cannot be increased beyond even by calling osSemaphoreRelease multiple times.

Does this make sense to you?

Cheers, Jonatan

udoe commented 5 years ago

Hi,

I agree that the term clear or reset can be misleading in this context. Maybe reset is a bit more intuitive as it resets the sem counter to zero (the state where the next acquire call blocks). Other APIs use this term as well, see k_sem_reset here https://docs.zephyrproject.org/latest/reference/kernel/synchronization/semaphores.html for example.

Yes, one can emulate a reset by calling osSemaphoreAcquire with a timeout of zero. But in case of max_count > 1, one needs to call it repeatedly in a loop which sounds less elegant.

My idea was just a suggestion as it occurred to me that osSemaphoreReset would be very easy to implement in the kernel.

Cheers, Udo

JonatanAntoni commented 5 years ago

Hi Udo,

I appreciate your suggestion. And I agree that it sounds useful. But I tried to work out that it might not be as easy to implement as it looks like on the first glance.

Generally speaking, I can imagine a function osSemaphoreReset to reset the Semaphore to its initial state. That can work for whatever semantic you are using the Semaphore for. The drawback is that one would need to store the initial count in the control block at cost of 4 bytes more RAM usage. Alternatively one could add the count to reset the Semaphore to as a function parameter (like your suggestion osSemaphoreSetCounter).

Additional consequence: What do you expect to happen with an ongoing osSemaphoreAcquire call, i.e. another thread is blocked waiting on the semaphore? This might be relevant when resetting a depleted Semaphore (count at 0) to be available by default (count other than 0). Shall the other thread instantly acquire the Semaphore, reducing the count and osSemaphoreAcquire returning osOK? Or shall the osSemaphoreAcquire rather be canceled, returning osErrorResource?

Any thoughts on these options?

Thanks, Jonatan

udoe commented 5 years ago

Hi,

The simplest approach would be to have osSemaphoreReset set the count to zero, no argument passed, no need to store initial count in the object, no side effects on blocked threads.

If osSemaphoreReset (or osSemaphoreSetCounter) takes an argument and this value sets the counter from zero to non-zero I would expect that the osSemaphoreAcquire call unblocks a waiting thread immediately, reduces the count by one and return osOK. I'm not sure if such a feature is really useful. But it's more generic then a simple reset to zero.

I don't think it's useful to reset the sem to initial count. The initial count specified when the sem is created just defines the initial state. There is no need to store initial count for later use.

Regards, Udo

JonatanAntoni commented 5 years ago

Thanks again Udo,

my doubt is that different users might define reset state differently. For instance what reset means depends on the logic you are implementing.

In your case I suspect you are using a binary semaphore like an event flag, e.g. signaling the completion of your asynchronous task to the calling thread.

Another area of usage of counting semaphores are resource pools, e.g. sharing five resources among multiple users. Resetting such a semaphore blindly to zero would state all resources used. Which I guess is not the intention of a reset in such a situation.

Perhaps my idea above is already what you are actually looking for: Event or Thread Flags. These might be more efficient to signal a task completion than using a semaphore. And Flags provide clear semantics by setting all bits to zero (i.e. not pending).

Cheers, Jonatan

udoe commented 5 years ago

Hi,

I agree that for the specific use case of asynch task completion, thread flags or event flags will do the job. But our firmware is not only for RTOS2. It uses an RTOS abstraction layer which exposes semaphores. Event Flags are not available in every RTOS while a semaphore seems to be quite common. So basically the suggested feature would simplify porting.

The resource pool use case is what can be found in text books and other theoretical discussions. I have never used a sem that way because I don't like the fact that state information (pool items count and sem counter) is duplicated.

Consider the case where you have a queue and an attached sem whose counter represents the available item count in the queue. If you have to do a cleanup (e.g. because the data path is stopped or needs error recovery) then you would discard all items from the queue and you will need to reset the sem counter to zero. It does not sound intuitive to me if I have to do the cleanup item by item and need to call osSemaphoreAcquire for each item just to end up with a sem count of zero.

My point of view is: A semaphore is a very flexible object which has many different use cases. Semantics of the sem API calls depends on the use case. Resource counting is just one case. A binary sem (one-bit event) is another one.

Maybe more generic names for API calls make things easier to understand, for example: inc/dec/reset, give/take/reset, or up/down/reset.

Regards, Udo

JonatanAntoni commented 5 years ago

Hi Udo,

that's a fair point. I had a quick look to POSIX semaphores and FreeRTOS (which is the second reference we used during definition of CMSIS-RTOS2 API). Both seems not to support a reset semantic on semaphores.

That raises yet another issue to conclude on: How do you mimic a semaphore reset on implementations without native reset support?

Cheers, Jonatan

PS: Please don't get this discussion wrong. I am not counter fighting your suggestion. I just want to be really sure that we don't miss a bit before changing an API that affects many other users.

ilg-ul commented 5 years ago

In µOS++ the semantic of semaphore::reset() is to reset the counter to the initial value.

https://github.com/micro-os-plus/micro-os-plus-iii/blob/1ea30f65acfc9b25e8bf6558035e5dd1bc3b9127/src/rtos/os-semaphore.cpp#L708

µOS++ is basically a C++ rewrite of some POSIX calls with extensions where POSIX did not provide solutions.

udoe commented 5 years ago

Hi,

I don't want to convince anyone to implement this API. This was just a proposal. I think the semaphore will be more flexible if a reset semantic is available. Generally, semantics depends on the specific use case.

For my specific problem described above, there are at least two solutions available in RTOS2: the osSemaphoreAcquire workaround and event flags.

Thanks for this interesting discussion.

Udo

JonatanAntoni commented 5 years ago

In FreeRTOS the suggestion seems to be to call acquire until the count reaches zero. So of course one could put this behind osSemaphoreReset as a compatibility implementation. POSIX unfortunately doesn't give us any hint on reset semantics because it doesn't have it.

Finally, such a feature's semantic would need to be clearly defined. I had certain discussions asking what would you expect a reset function does on a semaphore without giving you further details? Common answer is I would expect the semaphore's count to be reset to the initial value given at creation time. (Like it seems to be implemented in uOS++ by Liviu)

The bad things are: This semantic seems not to be compatible with Zephyr (which always resets to zero). It could be emulated by calling give/take until count is reached in FreeRTOS (and Zephyr?). We could of course add a native implementation to RTX5 similar to uOS++.

So far I am not convinced that putting such a function to CMSIS-RTOS2 is worth the effort. Hence I tend to decline this until we can come up with a clear story about what should be achieved.

Any other votes?

Thanks all for your valuable contribution, Jonatan

ilg-ul commented 5 years ago

The reason for using the value given at creation time in µOS++ and not zero is the result of the intended use, which, based on experience with complex code that performs some kind of internal restart, had no other choice than to destroy the semaphore and create a fresh one, a sequence that in environments with dynamic memory would make things more complicated and possibly lead to fragmentation (otherwise prevented if all allocations are done once at the very beginning).

udoe commented 5 years ago

Obviously, there are two possible semantics of reset: a) reset to initial count (as µOS++ does) b) reset to zero (as Zephyr does)

To me, b) makes more sense because sem behavior depends on its counter state (zero -> wait is blocking, not zero -> not blocking). Reset resets the sem to blocking state. Initial count is just an initialization value used during create.

The most flexible solution (which should make everyone happy) would be a SetCount of course.

Thanks Udo

PS: It also came a bit as a surprise to me that semantics of widely used RTOS APIs is so different.

ilg-ul commented 5 years ago

b) [reset to zero] makes more sense

This is probably because you expect semaphores to be binary and be used for synchronisation, but this is not always the case, they can be counting, and be used to protect multiple shared resources, and in this case they should start with the number of resources.

Plus that, for readability reasons, a function setting the value to 0 is better named xxx.clear().

would be a SetCount of course

Not recommended, since it allows to mess the semantics.

b) reset to zero (as Zephyr does) ... a surprise to me that semantics of widely used RTOS APIs is so different.

I doubt Zephyr did a very thorough analysis of semaphores to come to this conclusion.

For µOS++ I did one, you can read it at http://micro-os-plus.github.io/user-manual/semaphores/; you will probably find the most complete implementation, covering all possible allocation strategies and any possible behaviour.

And the object.reset() method, which returns the object to the initial state after construction, is a general API feature and applies to all objects, it is not specific to semaphores.

udoe commented 5 years ago

That's an interesting link. Thanks. I will also take a close look at µOS++. As we do everything in C++ it's been a long-time discussion if and how the RTOS kernel could be implemented in C++ as well.

You are right, probably. Everybody tends to look at a semaphore from the perspective of his specific use case.

Thanks Udo

ilg-ul commented 5 years ago

As we do everything in C++ it's been a long-time discussion if and how the RTOS kernel could be implemented in C++ as well.

It definitely can and µOS++ proved that the RTOS scheduler and core functionality (abusively called kernel) can be implemented in C++.

I will also take a close look at µOS++

I suggest you start with the User Manual.

The system itself is fully functional and stable for some time now. I postponed development in order to create some new tools (the xPack Project) to allow better modularity and testability.

If you have any comments/suggestions/etc on µOS++, please use the newly created Tapatalk forums.