Rationalise Mail/Queue/MemoryPool timing APIs #12901
Conversation
Test run: FAILEDSummary: 2 of 3 test jobs failed Failed test jobs:
|
|
CI retriggered |
Test run: FAILEDSummary: 2 of 3 test jobs failed Failed test jobs:
|
|
Fixed thread test. Started CI. |
Test run: FAILEDSummary: 1 of 6 test jobs failed Failed test jobs:
|
alloc APIs were generally inconsistent - take the opportunity to align with
other APIs like Semaphore.
alloc -> try_alloc
alloc_for -> try_alloc_for
alloc_until -> try_alloc_until
In future the name `alloc` can be used for an untimed blocking
allocation.
To line up with MemoryPool/Mail alloc, rework naming of get/put
Queue::get -> try_get, try_get_for
Queue::put -> try_put, try_put_for
Mail::get -> try_get, try_get_for
Mail::put (no change, but assert that it works)
In the future the names `get` and `put` can be used for untimed blocking
operations. In the interim, you have to use
`try_get_for(Kernel::wait_for_u32_forever)`.
`Mail::put` differs in that it has always been a non-blocking call, but
it can be assumed to always succeed when used correctly, because the
Queue has enough room to store a pointer to every block in the
MemoryPool. It could in future be made a `void` return, similar to the
change made to `Mutex::lock`.
|
Fixed Queue test. CI restarted |
Test run: SUCCESSSummary: 6 of 6 test jobs passed |
| { | ||
| osEvent evt = _queue.get(rel_time); | ||
| osEvent evt = _queue.get(millisec); |
There was a problem hiding this comment.
Unfortunately the call to get will create a deprecation warning.
There was a problem hiding this comment.
Yes, but only when this deprecated Mail::get call is used, and this method is instantiated, because it's a templated class.
So it means 2 deprecation messages rather than 1, but none if you're not using a deprecated method.
I think.
If you do see a warning in a normal build, I'll revisit.
| */ | ||
| MBED_DEPRECATED_SINCE("mbed-os-6.0.0", "Pass a chrono duration, not an integer millisecond count. For example use `5s` rather than `5000`.") | ||
| osEvent get(uint32_t millisec) | ||
| MBED_DEPRECATED_SINCE("mbed-os-6.0.0", "Replaced with try_get and try_get_for. In future get will be an untimed blocking call.") |
There was a problem hiding this comment.
In the future the function signature should also change to take a T ** parameter
There was a problem hiding this comment.
The untimed get can actually just be T *get() - it can return it directly.
Would have been nice if try_get() could have returned T * itself, with nullptr return for failure, like try_alloc, but there's no rule saying people can't actually queue nullptr.
If we had std::optional<T *> or equivalent, could have returned that for try_get(). I did consider std::pair<T *, bool> return, which some std library APIs use, but figured it was too unconventional for Mbed OS. Although it is conceptually similar to the CMSIS RTOS 1 osEvent return it was using before. But structure returns like that usually end up less efficient for code size.
Summary of changes
Follow-up adjusting #12425 , improving (in my opinion, at least) the timing for
Mail,QueueandMemoryPool. I think 12425 was too conservative at cleaning up the unsatisfactory state of their timing.alloc APIs were generally inconsistent - take the opportunity to align with other APIs like Semaphore.
In future the name
alloccan be used for an untimed blocking allocation.To line up with MemoryPool/Mail alloc, rework naming of get/put
In the future the names
getandputcan be used for untimed blocking operations. In the interim, you have to usetry_get_for(Kernel::wait_for_u32_forever).Mail::putdiffers in that it has always been a non-blocking call, but it can be assumed to always succeed when used correctly, because the Queue has enough room to store a pointer to every block in the MemoryPool. It could in future be made avoidreturn, similar to the change made toMutex::lock.Impact of changes
Existing timing code will generate deprecation warnings, but there should be no functional change.
Migration actions required
Users should migrate to use Chrono-based APIs as directed by the deprecation messages
Documentation
None - all in Doxygen
Pull request type
This changes a couple of APIs just merged in #12425, but otherwise deprecates 5.15 ones, so I'm classing it as a feature change like that.
Test results
Reviewers