search

ARMmbed / mbed-os-example-filesystem

The Mbed OS file system example
https://mbed.com
Apache License 2.0
16 stars 33 forks source link

Turn on platform.callback-nontrivial #115

Closed kjbracey closed 4 years ago

kjbracey commented 4 years ago

This example uses Event with Callback which means platform.callback-nontrivial needs to be true. Turn this on in the app, as we're planning to make false the default in Mbed OS. (https://github.com/ARMmbed/mbed-os/pull/12761)

This option is new on master (https://github.com/ARMmbed/mbed-os/pull/12036), so this change would stop the example compiling on 5.15 or earlier.

kjbracey commented 4 years ago

An alternative fix is to remove the dependency, by only storing a trivial object in the irq.fall callback. For example, this works:

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

That stores a reference_wrapper pointing at a UserAllocatedEvent, and the wrapper is trivially copyable (guaranteed in C++17, and seems to be the case for current tools). UserAllocatedEvent itself is too big to store directly in a Callback - Event only fits because it works like a shared pointer to storage in the event queue, but that shared pointer is what makes it non-trivial.

The following more minimal change would also work, by just pulling the Event out of the callback via the reference.

static auto erase_event = mbed_event_queue->event(erase);
irq.fall(std::ref(erase_event));

But once the event is program-lifetime anyway it seems simpler to just let it be user-allocated.

The user-allocated form saves 52 bytes of flash, but uses 44 more bytes of static memory - it's using that rather than the event queue storage. But avoiding using the event queue storage would let that be configured down (or removed entirely).

Overall we save 200+ bytes of flash by keeping the platform.callback-nontrivial off in this image - but the saving is proportional to number of users of Callback. Every user has to pay the cost, regardless of how many need the nontrivial.

See also my comments on https://github.com/ARMmbed/mbed-os/pull/12761

So I think I'll switch to the first form if no-one has any objections.

All these forms are still a bit unsafe though in that they'll fail if a 2nd IRQ triggers before the first one is dispatched. Neither Event nor UserAllocatedEvent have proper protection for multiple queuing, as far as I can see. Which seems unfortunate - they're seem intended to be used in this situation where that can happen.

kjbracey commented 4 years ago

Alternative fix placed in #116. Only merge 1 of the PRs.

kjbracey commented 4 years ago

116 was merged instead.