Fix an implicit conversion from int to unsigned in Events #14176
Conversation
Mbed-os issue : ARMmbed#13105 The function equeue_event_delay() currently takes an int ms parameter and writes the value to the unsigned target field of the event structure. This is hidden from the user. The user should be aware of this conversion and to make this clearer it was decided to make the equeue_event_delay() take an unsigned value instead (thus pushing the conversion higher up and showing that this is an expected behaviour.
|
@adbridge, thank you for your changes. |
| @@ -162,7 +162,7 @@ void equeue_dealloc(equeue_t *queue, void *event); | |||
| // equeue_event_delay - Millisecond delay before dispatching an event | |||
| // equeue_event_period - Millisecond period for repeating dispatching an event | |||
| // equeue_event_dtor - Destructor to run when the event is deallocated | |||
| void equeue_event_delay(void *event, int ms); | |||
| void equeue_event_delay(void *event, unsigned ms); | |||
There was a problem hiding this comment.
all functions in this header file have it declared as int ms , this change would cause confusion?
There was a problem hiding this comment.
This was the solution @kjbracey-arm suggested above...
There was a problem hiding this comment.
I guess Martin means that it's not consistent with other APIs here? Maybe they also need changing.
But if we're using a mixture of signed and unsigned internally and that got reflected in API might look a bit icky. Would be honest at least...
I noticed this issue the other day too: #14077.
So I suspect the code might not be 100% solid as you reach the extremes of the int range anyway. Just as weird stuff happens if you're less then (int) 0, as Anna noted, weird stuff might happen if you're greater than (unsigned) 0x7fffffff, depending on how solid all the maths is.
It's possible this isn't a net benefit. Maybe the working range is only 0-7fffffff, and unsigned doesn't match that any more than int.
There was a problem hiding this comment.
To a large extent many the Chrono APIs avoid this by just saying "screw it - use a 64-bit type". Thats so big you never have to worry about range problems. Maybe we might want to go 64-bit inside and out to avoid #14077-type problems? We have 64-bit for RTOS APIs and all ticker_api-based stuff. This is a 32-bit outlier in the core system (although various subsystems like Netsocket are 32-bitty).
And, thinking of Chrono as it stands now, we have Chrono implemented already in Event. So we currently have a duration<int, milli> there feeding into this. If changing stuff to unsigned here, you should follow through so that Event::delay (and others?) took a duration<unsigned, milli>.
There was a problem hiding this comment.
If we only change equeue_event_delay then we introduce inconsistency in the equeue library with a mixture of signed and unsigned APIs. Additionally I thought the original request was to change EventQueue::call_in to take an unsigned int, which also introduces inconsistency in that library.
But I am worried that the code is not behaving as I expected. I can't repoduce #13105 as described: the event is dispatched right away and not after 49.7 days as we would have expected.
There was a problem hiding this comment.
Sounds to me like this needs much more thought! Don't forget for events we have
static constexpr std::chrono::duration<int, std::milli> non_periodic{-1};
Which allows a -ve duration to represent a non_periodic ie once off event. So we can't just move all durations to unsigned without re-writing the underlying functionality to allow for a non periodic event in another way...
There was a problem hiding this comment.
The other option I guess would be to make it all unsigned but to enforce a maximum value and then reserve a few values above that max to use for the different modes....
There was a problem hiding this comment.
If we only change
equeue_event_delaythen we introduce inconsistency in the equeue library with a mixture of signed and unsigned APIs. Additionally I thought the original request was to changeEventQueue::call_into take an unsigned int, which also introduces inconsistency in that library.But I am worried that the code is not behaving as I expected. I can't repoduce #13105 as described: the event is dispatched right away and not after 49.7 days as we would have expected.
@evedon see my comment here for why it makes more sense to change equeue_event_delay #13105 (comment)
There was a problem hiding this comment.
I like your suggestion above. We should fix the events library properly and use unsigned everywhere, including duration<unsigned, milli>.
As for EventQueue::call_in(int ms, F f), given that the function is already deprecated, it's probably okay to leave it as such until it is removed.
I suggest that we bring in all the events related PRs in the same release so it is less disruptive to our users.
|
As per @evedon 's comment I am marking this as do not merge for now while I consider how much work (and the best way to bring it all together) is required. |
Stream.h contains a public API but is currently not exposed via mbed.h when it should be. This commit fixes that.
Summary of changes
Fixes #13105
The function equeue_event_delay() currently takes an int ms
parameter and writes the value to the unsigned target field of the
event structure. This is hidden from the user. The user should be
aware of this conversion and to make this clearer it was decided to
make the equeue_event_delay() take an unsigned value instead (thus
pushing the conversion higher up and showing that this is an
expected behaviour).
Impact of changes
The equeue_event_delay() API has changed (int to unsigned), however the underlying behaviour is the same.
Migration actions required
None
Documentation
n/a
Pull request type
Test results
Reviewers