Skip to content

Fix an implicit conversion from int to unsigned in Events #14176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix an implicit conversion from int to unsigned in Events #14176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d71d5f
This fixes an implicit conversion from int to unsigned in Events
adbridge Jan 19, 2021
1bae70f
Expose the public API for streams in mbed.h
adbridge Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion events/include/events/equeue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all functions in this header file have it declared as int ms , this change would cause confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the solution @kjbracey-arm suggested above...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

Copy link
Contributor Author

@adbridge adbridge Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@evedon see my comment here for why it makes more sense to change equeue_event_delay #13105 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

void equeue_event_period(void *event, int ms);
void equeue_event_dtor(void *event, void (*dtor)(void *));

Expand Down
2 changes: 1 addition & 1 deletion events/source/equeue.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ void equeue_dispatch(equeue_t *q, int ms)


// event functions
void equeue_event_delay(void *p, int ms)
void equeue_event_delay(void *p, unsigned ms)
{
struct equeue_event *e = (struct equeue_event *)p - 1;
e->target = ms;
Expand Down
1 change: 1 addition & 0 deletions mbed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
#include "platform/ScopedRomWriteLock.h"
#include "platform/ScopedRamExecutionLock.h"
#include "platform/mbed_stats.h"
#include "platform/Stream.h

// mbed Non-hardware components
#include "platform/Callback.h"
Expand Down