Callback: Trivial default

[kjbracey]

Summary of changes

Turn off callback.non-trivial by default to save ROM space.

Follow-up to #12036. Will need at least one example changed to turn the option back on.

Impact of changes

• If application code uses Callback with a non-trivial functor, they will get a compilation error directing them to turn on platform.callback-nontrivial.

Migration actions required

• Add "platform.callback-nontrivial": true to your mbed_app.json if a build error indicates that it is required - or change your code to only use trivial functors in Callback.

Documentation

ARMmbed/mbed-os-5-docs#1185

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

---

[ciarmcom]

@kjbracey-arm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

This change will break the filesystem example, from previous testing of #12036, so I've just created a PR there to deal with it: ARMmbed/mbed-os-example-filesystem#115

[0xc0170]

Reading docs helped to understand this one.

[kjbracey]

The failure in filesystem suggests a point of documentation update - the Event class you get from EventQueue is the main in-tree example of something needing full functionality. So the Callback option should probably reference that as an example, and whereever the documentation is for Event should note this option.

That Event thing lets you do

 irq.fall(mbed_event_queue()->event(my_handler))

in one go, and that's what filesystem example uses. The Event returned by event() is stored inside InterruptIn's Callback - when called it does a EventQueue::post to queue the call to my_handler.

That uses RAM in the event queue to store the my_handler pointer and any parameters, and that needs to be freed when the Event is destroyed - Event is a bit like a shared pointer into that data. So you need the full Callback implementation, so that when you do irq.fall(nullptr) to remove the handler, the Event gets properly destroyed, and the storage in the event queue is properly released.

All of that is a bit heavy though. This is another approach:

irq.fall([q = mbed_event_queue()] { q->call(my_handler); });

That uses a lambda to do the work. Pros and cons:

Event version:

• Pro: Uses single Event<void()> type with Callback - no template bloat for multiple events
• Pro: bit neater
• Con: needs platform.callback-nontrivial - system-wide cost if the only user
• Con: consumes more RAM in event queue to hold callback(my_handler) as variable
• Pro: calling is reliable once created - RAM for queued event is preallocated.
• Con: no protection against multiple queueing - in trouble if 2nd IRQ occurs before first is handled

Lambda version:

• Con: Uses separate lambda type with Callback for every call - potential template bloat
• Con: Need to understand lambdas (but you could do it with a standalone helper function)
• Pro: Don't need platform.callback-nontrivial
• Pro: Less RAM cost - my_handler is a ROM constant, and the local capture of the event queue is in Callback storage that was reserved anyway.
• Con: calling is not reliable - RAM is allocated at invocation time, could be allocation failure
• Pro: multiple queuing is fine, memory permitting

Ah, but we can also use UserAllocatedEvent , and that is better overall:

 static auto handler_event =  mbed_event_queue()->make_user_allocated_event(my_handler);
 irq.fall(handler_event);

• Pro: reduced template bloat - single UserAllocatedEvent<void()> type with Callback
• Pro: no modern lambda nonsense
• Pro: doesn't need platform.callback-nontrivial (if conditional destructor containing just MBED_ASSERT is stripped from UserAllocatedCallback)
• Pro: All RAM use is static - no dynamic allocation, so easier to track
• Pro: Calling is reliable
• Con: still no protection against multiple queueing

[kjbracey]

Ah, except you can't just pass a UserAllocatedEvent into Callback - it's too big. Event is deliberately a shared pointer-like thing to make it fit into a Callback. You'd need to have a wrapper. I think std:reference_wrapper does the job:

static auto handler_event =  mbed_event_queue()->make_user_allocated_event(my_handler);
irq.fall(std::ref(handler_event));

std::ref gives you a small reference_wrapper object that contains a reference to its argument, and it passes through the () operator. And that is fine as long as reference_wrapper is trivially-copyable. (Guaranteed since C++17, apparently).

The point about trivial copy of the UserAllocatedEvent is irrelevant - you're not actually storing it inside the Callback, just a reference to it.

Apparently the trivial-copyability of reference_wrapper has varied in libraries: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4151.html

Not sure what the current state of our tools is. We could reinstate the local implementation of mstd::reference_wrapper as a fall-back.

[kjbracey]

Alternative fix posted to filesystem: ARMmbed/mbed-os-example-filesystem#116

[0xc0170]

I run initial CI

[mbed-ci]

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_build-ARM
• jenkins-ci/mbed-os-ci_build-GCC_ARM

[kjbracey]

Failure is the expected one until filesystem example is updated

[0xc0170]

PR ARMmbed/mbed-os-example-filesystem#116 was merged, restarting CI

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

[0xc0170]

@ARMmbed/mbed-os-core @bulislaw Please review

[bulislaw]

LGTM

[jeromecoutant]

Hi
I got IAR compilation issue since this PR :-(

[Error] Callback.h@628,10: [Pe1574]: static assertion failed with "F must be TriviallyCopyable. Turn on Mbed configuration option 'platform.callback-nontrivial' to use more complex function objects"

[kjbracey]

Well, that's potentially expected, depending on the app, and what you're doing. You may have to turn that option on for your app, as per description.

Unless it's IAR-specific, and works in other toolchains? Or you're hitting it with something in-tree, and not using Callback yourself? Where is the error actually coming from?

[jeromecoutant]

Seems there are 3 impacted tests:
tests-mbed_hal-rtc
tests-mbedmicro-rtos-mbed-event_flags
tests-mbedmicro-rtos-mbed-signals

[0xc0170]

@jeromecoutant Can you create an issue , and we take it from there? Thanks