Skip to content

Don't require platform.callback-nontrivial #116

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.

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

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Apr 7, 2020

Don't use an Event with Callback - it's non-trivial, so needs the "full" form of Callback. Callback is being switched to not support non-trivial functors by default to save ROM.

Use a reference_wrapper to a UserAllocatedEvent instead, which works fine with the trivial-only Callback.

A reference_wrapper is small enough to fit in a Callback, and is trivially-copyable, so works to decouple the event lifetime from the callback. Having decoupled it, the event has program lifetime, so may as well be UserAllocatedEvent for simplicity.

(UserAllocatedEvent could be trivial, but is too big to be placed in a Callback directly in any case).

@kjbracey
Copy link
Contributor Author

kjbracey commented Apr 7, 2020

Replacement for #115. Only merge one of the PRs.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 29, 2020

@kjbracey-arm Just to confirm - travis failures are expected as Mbed OS PR not yet integrated. So the proposal steps will be to merge this one, merge Mbed OS PR (it will test this example so if passes we are all ok).

@kjbracey
Copy link
Contributor Author

kjbracey commented Apr 30, 2020

Actually, both this and #115 should be passing with current master, they don't depend on the PR.

But they're prerequisites to the PR passing.

@kjbracey
Copy link
Contributor Author

Hmm, error indicates the CI here is not being built as C++11. Eh? Really old Mbed OS?

[ERROR] ./main.cpp:72:8: warning: 'auto' changes meaning in C++11; please remove it [-Wc++11-compat]
 static auto erase_event = mbed_event_queue()->make_user_allocated_event(erase);
        ^~~~
./main.cpp:72:13: error: 'erase_event' does not name a type; did you mean 'equeue_event'?
 static auto erase_event = mbed_event_queue()->make_user_allocated_event(erase);
             ^~~~~~~~~~~
             equeue_event
./main.cpp: In function 'int main()':
./main.cpp:80:19: error: 'ref' is not a member of 'std'
     irq.fall(std::ref(erase_event));
                   ^~~
./main.cpp:80:23: error: 'erase_event' was not declared in this scope
     irq.fall(std::ref(erase_event));
                       ^~~~~~~~~~~
./main.cpp:80:23: note: suggested alternative: 'equeue_event'
     irq.fall(std::ref(erase_event));

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 30, 2020

yes, master points to very old sha: https://github.com/ARMmbed/mbed-os-example-filesystem/blob/master/mbed-os.lib , update in this PR to resolve it

@kjbracey
Copy link
Contributor Author

To master or a 6.0-beta, or 5.15?

Don't use an `Event` with `Callback` - it's non-trivial, so needs the
"full" form of `Callback`. `Callback` is being switched to not support
non-trivial functors by default to save ROM.

Use a `reference_wrapper` to a `UserAllocatedEvent` instead, which works
fine with the trivial-only `Callback`.

A `reference_wrapper` is small enough to fit in a `Callback`, and is
trivially-copyable, so works to decouple the event lifetime from the
callback. Having decoupled it, the event has program lifetime, so may as
well be `UserAllocatedEvent` for simplicity.

(`UserAllocatedEvent` could be trivial, but is too big to be placed in a
`Callback` directly in any case).
@kjbracey
Copy link
Contributor Author

Updated to Mbed OS 5.15.2

@0xc0170 0xc0170 merged commit 6ee2bfa into ARMmbed:master Apr 30, 2020
@kjbracey kjbracey deleted the event2 branch April 30, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants