Skip to content
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

Jump to bottom

Execute button and OOM event handlers synchronously #1650

Merged
merged 4 commits into from Dec 20, 2018
Merged

Execute button and OOM event handlers synchronously #1650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
222fecf
Execute button event handlers synchronously in the ISR
sergeuz Dec 19, 2018
ffa595b
Wrap APPLICATION_THREAD_CONTEXT_ASYNC() into a function
sergeuz Dec 19, 2018
3a08c04
Run the OOM event handler synchronously
sergeuz Dec 19, 2018
ecaf7b1
Fix SYSTEM_07_fragmented_heap test
technobly Dec 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion system/inc/system_event.h
View file
Open in desktop
Expand Up @@ -97,6 +97,12 @@ enum SystemEventsParam {
time_changed_sync = 1
};

/**
* Flags altering the behavior of the `system_notify_event()` function.
*/
enum SystemNotifyEventFlag {
NOTIFY_SYNCHRONOUSLY = 0x01
};

/**
* Subscribes to the system events given
Expand Down Expand Up @@ -126,5 +132,7 @@ void system_notify_time_changed(uint32_t data, void* reserved, void* reserved1);
* @param event
* @param data
* @param pointer
* @param flags Event flags as defined by the `SystemNotifyEventFlag` enum.
*/
void system_notify_event(system_event_t event, uint32_t data=0, void* pointer=nullptr, void (*fn)(void* data)=nullptr, void* fndata=nullptr);
void system_notify_event(system_event_t event, uint32_t data = 0, void* pointer = nullptr,
void (*fn)(void* data) = nullptr, void* fndata = nullptr, unsigned flags = 0);
10 changes: 5 additions & 5 deletions system/src/main.cpp
View file
Open in desktop
Expand Up @@ -244,7 +244,7 @@ void reset_button_click()
button_current_clicks = 0;
CLR_BUTTON_TIMEOUT();
if (clicks > 0) {
system_notify_event(button_final_click, clicks);
system_notify_event(button_final_click, clicks, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would add the flags as the 2nd parameter, so that it is always a conscious choice.

button_final_clicks = clicks;
#if Wiring_SetupButtonUX
// Certain numbers of clicks can be processed directly in ISR
Expand All @@ -266,7 +266,7 @@ void handle_button_click(uint16_t depressed_duration)
ARM_BUTTON_TIMEOUT(1000);
reset = false;
}
system_notify_event(button_click, button_current_clicks);
system_notify_event(button_click, button_current_clicks, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);
}
if (reset) {
reset_button_click();
Expand All @@ -288,7 +288,7 @@ void HAL_Notify_Button_State(uint8_t button, uint8_t pressed)
pressed_time = HAL_Timer_Get_Milli_Seconds();
if (!wasListeningOnButtonPress) // start of button press
{
system_notify_event(button_status, 0);
system_notify_event(button_status, 0, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);
}
}
else if (pressed_time > 0)
Expand All @@ -297,7 +297,7 @@ void HAL_Notify_Button_State(uint8_t button, uint8_t pressed)
uint16_t depressed_duration = release_time - pressed_time;

if (!network.listening()) {
system_notify_event(button_status, depressed_duration);
system_notify_event(button_status, depressed_duration, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);
handle_button_click(depressed_duration);
}
pressed_time = 0;
Expand Down Expand Up @@ -500,7 +500,7 @@ void handle_out_of_memory(size_t requested) {
}
else {
recurse = true;
system_notify_event(out_of_memory, requested);
system_notify_event(out_of_memory, requested, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);
recurse = false;
}
}
Expand Down
42 changes: 22 additions & 20 deletions system/src/system_event.cpp
View file
Open in desktop
Expand Up @@ -61,9 +61,6 @@ struct SystemEventSubscription {
std::vector<SystemEventSubscription> subscriptions;

void system_notify_event_impl(system_event_t event, uint32_t data, void* pointer, void (*fn)(void* data), void* fndata) {
APPLICATION_THREAD_CONTEXT_ASYNC(system_notify_event_impl(event, data, pointer, fn, fndata));
// run event notifications on the application thread

for (const SystemEventSubscription& subscription : subscriptions) {
subscription.notify(event, data, pointer);
}
Expand All @@ -72,6 +69,12 @@ void system_notify_event_impl(system_event_t event, uint32_t data, void* pointer
}
}

void system_notify_event_async(system_event_t event, uint32_t data, void* pointer, void (*fn)(void* data), void* fndata) {
// run event notifications on the application thread
APPLICATION_THREAD_CONTEXT_ASYNC(system_notify_event_async(event, data, pointer, fn, fndata));
system_notify_event_impl(event, data, pointer, fn, fndata);
}

class SystemEventTask : public ISRTaskQueue::Task {
system_event_t event_;
uint32_t data_;
Expand All @@ -91,7 +94,7 @@ class SystemEventTask : public ISRTaskQueue::Task {
* Notify the system event encoded in this class.
*/
void notify() {
system_notify_event_impl(event_, data_, pointer_, fn_, fndata_);
system_notify_event_async(event_, data_, pointer_, fn_, fndata_);
system_pool_free(this, nullptr);
}

Expand Down Expand Up @@ -132,22 +135,21 @@ void system_unsubscribe_event(system_event_t events, system_event_handler_t* han
{
}

/**
* Notifes all subscribers about an event.
* @param event
* @param data
* @param pointer
*/
void system_notify_event(system_event_t event, uint32_t data, void* pointer, void (*fn)(void* data), void* fndata) {
if (HAL_IsISR()) {
void* space = (system_pool_alloc(sizeof(SystemEventTask), nullptr));
if (space) {
auto task = new (space) SystemEventTask(event, data, pointer, fn, fndata);
SystemISRTaskQueue.enqueue(task);
};
} else {
system_notify_event_impl(event, data, pointer, fn, fndata);
}
void system_notify_event(system_event_t event, uint32_t data, void* pointer, void (*fn)(void* data), void* fndata,
unsigned flags) {
// TODO: Add an API that would allow user applications to control which event handlers can be
// executed synchronously, possibly in the context of an ISR
if (flags & NOTIFY_SYNCHRONOUSLY) {
system_notify_event_impl(event, data, pointer, fn, fndata);
} else if (HAL_IsISR()) {
void* space = (system_pool_alloc(sizeof(SystemEventTask), nullptr));
if (space) {
auto task = new (space) SystemEventTask(event, data, pointer, fn, fndata);
SystemISRTaskQueue.enqueue(task);
};
} else {
system_notify_event_async(event, data, pointer, fn, fndata);
}
}

void system_notify_time_changed(uint32_t data, void* reserved, void* reserved1) {
Expand Down
16 changes: 6 additions & 10 deletions user/tests/wiring/no_fixture/system.cpp
View file
Open in desktop
Expand Up @@ -112,12 +112,7 @@ test(SYSTEM_04_button_mirror)
EXTI_GenerateSWInterrupt(pinmap[D1].gpio_pin);
delay(300);
pinMode(D1, INPUT_PULLUP);

// Make sure all asynchronous events get processed by the application thread
const auto t = millis();
do {
Particle.process();
} while (millis() - t < 300);
delay(300);

assertEqual(s_button_clicks, 3);
}
Expand Down Expand Up @@ -217,7 +212,8 @@ test(SYSTEM_07_fragmented_heap) {

unregister_oom();
register_oom();
block* b = new block[2]; // no room for 2 blocks
const size_t BLOCKS_TO_MALLOC = 3;
block* b = new block[BLOCKS_TO_MALLOC]; // no room for 3 blocks, memory is clearly fragmented
delete b;

// free the remaining blocks
Expand All @@ -227,12 +223,12 @@ test(SYSTEM_07_fragmented_heap) {
delete b;
}

assertMoreOrEqual(half_fragment_block_size, sizeof(block));
assertLessOrEqual(half_fragment_block_size, 2*sizeof(block)+8 /* 8 byte overhead */);
assertMoreOrEqual(half_fragment_block_size, sizeof(block)); // there should definitely be one block available
assertLessOrEqual(half_fragment_block_size, BLOCKS_TO_MALLOC*sizeof(block)-1); // we expect malloc of 3 blocks to fail, so this better allow up to that size less 1
assertMoreOrEqual(half_fragment_free, low_heap+(sizeof(block)*count));

assertTrue(oomEventReceived);
assertMoreOrEqual(oomSizeReceived, sizeof(block)*2);
assertMoreOrEqual(oomSizeReceived, sizeof(block)*BLOCKS_TO_MALLOC);
}

test(SYSTEM_08_out_of_memory_not_raised_for_0_size_malloc)
Expand Down