From 84da6a4a09fe3ee56667749301ca7bc4c7ea5f34 Mon Sep 17 00:00:00 2001 From: Joe Finney Date: Sat, 31 Oct 2015 10:27:38 +0000 Subject: [PATCH] microbit: Minor bug fixes and refinements - Add maximum depth for event queues, to prevent buggy scripts causing total memory exhaustion. - Suppress generation of A/B click events when A+B click is generated - preservation of event ordering on messagebus for resursive event generation cases. - bugfix of message bus processing to prevent occasional dual processing of events - bugfix MicroBitDisplay to behave correctly when delay parameter is zero. --- inc/MicroBitButton.h | 19 ++++++--- inc/MicroBitConfig.h | 7 ++++ inc/MicroBitMessageBus.h | 8 ++-- inc/MicroBitMultiButton.h | 12 ++++-- source/MicroBit.cpp | 4 +- source/MicroBitButton.cpp | 20 +++++---- source/MicroBitDisplay.cpp | 22 +++++++--- source/MicroBitListener.cpp | 17 +++++--- source/MicroBitMessageBus.cpp | 68 +++++++++++++++++++++++------- source/MicroBitMultiButton.cpp | 76 ++++++++++++++++++++++++++-------- 10 files changed, 184 insertions(+), 69 deletions(-) diff --git a/inc/MicroBitButton.h b/inc/MicroBitButton.h index 231cadd..59370d4 100644 --- a/inc/MicroBitButton.h +++ b/inc/MicroBitButton.h @@ -31,6 +31,13 @@ #define MICROBIT_BUTTON_SIGMA_THRESH_LO 2 #define MICROBIT_BUTTON_DOUBLE_CLICK_THRESH 50 +enum MicroBitButtonEventConfiguration +{ + MICROBIT_BUTTON_SIMPLE_EVENTS, + MICROBIT_BUTTON_ALL_EVENTS +}; + + /** * Class definition for MicroBit Button. * @@ -38,12 +45,12 @@ */ class MicroBitButton : public MicroBitComponent { - PinName name; // mBed pin name of this pin. - DigitalIn pin; // The mBed object looking after this pin at any point in time (may change!). + PinName name; // mbed pin name of this pin. + DigitalIn pin; // The mbed object looking after this pin at any point in time (may change!). - unsigned long downStartTime; // used to store the current system clock when a button down event occurs - uint8_t sigma; // integration of samples over time. - uint8_t doubleClickTimer; // double click timer (ticks). + unsigned long downStartTime; // used to store the current system clock when a button down event occurs + uint8_t sigma; // integration of samples over time. We use this for debouncing, and noise tolerance for touch sensing + MicroBitButtonEventConfiguration eventConfiguration; // Do we want to generate high level event (clicks), or defer this to another service. public: @@ -69,7 +76,7 @@ class MicroBitButton : public MicroBitComponent * MICROBIT_BUTTON_EVT_HOLD * @endcode */ - MicroBitButton(uint16_t id, PinName name, PinMode mode = PullNone); + MicroBitButton(uint16_t id, PinName name, MicroBitButtonEventConfiguration eventConfiguration = MICROBIT_BUTTON_ALL_EVENTS, PinMode mode = PullNone); /** * Tests if this Button is currently pressed. diff --git a/inc/MicroBitConfig.h b/inc/MicroBitConfig.h index 31e9416..182a79a 100644 --- a/inc/MicroBitConfig.h +++ b/inc/MicroBitConfig.h @@ -91,6 +91,13 @@ #define MESSAGE_BUS_LISTENER_DEFAULT_FLAGS MESSAGE_BUS_LISTENER_QUEUE_IF_BUSY #endif +// +// Maximum event queue depth. If a queue exceeds this depth, further events will be dropped. +// Used to prevent message queues growing uncontrollably due to badly behaved user code and causing panic conditions. +// +#ifndef MESSAGE_BUS_LISTENER_MAX_QUEUE_DEPTH +#define MESSAGE_BUS_LISTENER_MAX_QUEUE_DEPTH 20 +#endif // // Core micro:bit services // diff --git a/inc/MicroBitMessageBus.h b/inc/MicroBitMessageBus.h index a6b3288..7ee9141 100644 --- a/inc/MicroBitMessageBus.h +++ b/inc/MicroBitMessageBus.h @@ -64,10 +64,11 @@ class MicroBitMessageBus : public MicroBitComponent * or the constructors provided by MicroBitEvent. * * @param evt The event to send. - * @param mask The type of listeners to process (optional). Matches MicroBitListener flags. If not defined, all standard listeners will be processed. - * @return The 1 if all matching listeners were processed, 0 if further processing is required. + * @param urgent The type of listeners to process (optional). If set to true, only listeners defined as urgent and non-blocking will be processed + * otherwise, all other (standard) listeners will be processed. + * @return 1 if all matching listeners were processed, 0 if further processing is required. */ - int process(MicroBitEvent &evt, uint32_t mask = MESSAGE_BUS_LISTENER_REENTRANT | MESSAGE_BUS_LISTENER_QUEUE_IF_BUSY | MESSAGE_BUS_LISTENER_DROP_IF_BUSY | MESSAGE_BUS_LISTENER_NONBLOCKING); + int process(MicroBitEvent &evt, bool urgent = false); /** * Register a listener function. @@ -232,6 +233,7 @@ class MicroBitMessageBus : public MicroBitComponent MicroBitEventQueueItem *evt_queue_head; // Head of queued events to be processed. MicroBitEventQueueItem *evt_queue_tail; // Tail of queued events to be processed. uint16_t nonce_val; // The last nonce issued. + uint16_t queueLength; // The number of events currently waiting to be processed. void queueEvent(MicroBitEvent &evt); MicroBitEventQueueItem* dequeueEvent(); diff --git a/inc/MicroBitMultiButton.h b/inc/MicroBitMultiButton.h index 1a8b34e..f54702a 100644 --- a/inc/MicroBitMultiButton.h +++ b/inc/MicroBitMultiButton.h @@ -3,10 +3,12 @@ #include "MicroBit.h" -#define MICROBIT_MULTI_BUTTON_STATE_1 1 -#define MICROBIT_MULTI_BUTTON_STATE_2 2 -#define MICROBIT_MULTI_BUTTON_HOLD_TRIGGERED_1 4 -#define MICROBIT_MULTI_BUTTON_HOLD_TRIGGERED_2 8 +#define MICROBIT_MULTI_BUTTON_STATE_1 0x01 +#define MICROBIT_MULTI_BUTTON_STATE_2 0x02 +#define MICROBIT_MULTI_BUTTON_HOLD_TRIGGERED_1 0x04 +#define MICROBIT_MULTI_BUTTON_HOLD_TRIGGERED_2 0x08 +#define MICROBIT_MULTI_BUTTON_SUPRESSED_1 0X10 +#define MICROBIT_MULTI_BUTTON_SUPRESSED_2 0x20 /** * Class definition for MicroBitMultiButton. @@ -22,8 +24,10 @@ class MicroBitMultiButton : public MicroBitComponent uint16_t otherSubButton(uint16_t b); int isSubButtonPressed(uint16_t button); int isSubButtonHeld(uint16_t button); + int isSubButtonSupressed(uint16_t button); void setButtonState(uint16_t button, int value); void setHoldState(uint16_t button, int value); + void setSupressedState(uint16_t button, int value); public: diff --git a/source/MicroBit.cpp b/source/MicroBit.cpp index 07634d0..c788afa 100644 --- a/source/MicroBit.cpp +++ b/source/MicroBit.cpp @@ -63,8 +63,8 @@ MicroBit::MicroBit() : #endif MessageBus(), display(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_WIDTH, MICROBIT_DISPLAY_HEIGHT), - buttonA(MICROBIT_ID_BUTTON_A,MICROBIT_PIN_BUTTON_A), - buttonB(MICROBIT_ID_BUTTON_B,MICROBIT_PIN_BUTTON_B), + buttonA(MICROBIT_ID_BUTTON_A,MICROBIT_PIN_BUTTON_A, MICROBIT_BUTTON_SIMPLE_EVENTS), + buttonB(MICROBIT_ID_BUTTON_B,MICROBIT_PIN_BUTTON_B, MICROBIT_BUTTON_SIMPLE_EVENTS), buttonAB(MICROBIT_ID_BUTTON_AB,MICROBIT_ID_BUTTON_A,MICROBIT_ID_BUTTON_B), accelerometer(MICROBIT_ID_ACCELEROMETER, MMA8653_DEFAULT_ADDR), compass(MICROBIT_ID_COMPASS, MAG3110_DEFAULT_ADDR), diff --git a/source/MicroBitButton.cpp b/source/MicroBitButton.cpp index 1b04303..80e6c1e 100644 --- a/source/MicroBitButton.cpp +++ b/source/MicroBitButton.cpp @@ -18,17 +18,16 @@ * MICROBIT_BUTTON_EVT_UP * MICROBIT_BUTTON_EVT_CLICK * MICROBIT_BUTTON_EVT_LONG_CLICK - * MICROBIT_BUTTON_EVT_DOUBLE_CLICK * MICROBIT_BUTTON_EVT_HOLD * @endcode */ -MicroBitButton::MicroBitButton(uint16_t id, PinName name, PinMode mode) : pin(name, mode) +MicroBitButton::MicroBitButton(uint16_t id, PinName name, MicroBitButtonEventConfiguration eventConfiguration, PinMode mode) : pin(name, mode) { this->id = id; this->name = name; + this->eventConfiguration = eventConfiguration; this->downStartTime = 0; this->sigma = 0; - this->doubleClickTimer = 0; uBit.addSystemComponent(this); } @@ -71,12 +70,15 @@ void MicroBitButton::systemTick() { status = 0; MicroBitEvent evt(id,MICROBIT_BUTTON_EVT_UP); - - //determine if this is a long click or a normal click and send event - if((ticks - downStartTime) >= MICROBIT_BUTTON_LONG_CLICK_TIME) - MicroBitEvent evt(id,MICROBIT_BUTTON_EVT_LONG_CLICK); - else - MicroBitEvent evt(id,MICROBIT_BUTTON_EVT_CLICK); + + if (eventConfiguration == MICROBIT_BUTTON_ALL_EVENTS) + { + //determine if this is a long click or a normal click and send event + if((ticks - downStartTime) >= MICROBIT_BUTTON_LONG_CLICK_TIME) + MicroBitEvent evt(id,MICROBIT_BUTTON_EVT_LONG_CLICK); + else + MicroBitEvent evt(id,MICROBIT_BUTTON_EVT_CLICK); + } } //if button is pressed and the hold triggered event state is not triggered AND we are greater than the button debounce value diff --git a/source/MicroBitDisplay.cpp b/source/MicroBitDisplay.cpp index 178501a..95deb94 100644 --- a/source/MicroBitDisplay.cpp +++ b/source/MicroBitDisplay.cpp @@ -478,7 +478,8 @@ void MicroBitDisplay::print(char c, int delay) if (animationMode == ANIMATION_MODE_NONE) { this->printAsync(c, delay); - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } @@ -509,7 +510,9 @@ void MicroBitDisplay::print(ManagedString s, int delay) if (animationMode == ANIMATION_MODE_NONE) { this->printAsync(s, delay); - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + //TODO: Put this in when we merge tight-validation + //if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } @@ -537,7 +540,8 @@ void MicroBitDisplay::print(MicroBitImage i, int x, int y, int alpha, int delay) if (animationMode == ANIMATION_MODE_NONE) { this->printAsync(i, x, y, alpha, delay); - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } @@ -632,7 +636,9 @@ void MicroBitDisplay::scroll(ManagedString s, int delay) this->scrollAsync(s, delay); // Wait for completion. - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + //TODO: Put this in when we merge tight-validation + //if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } @@ -663,7 +669,9 @@ void MicroBitDisplay::scroll(MicroBitImage image, int delay, int stride) this->scrollAsync(image, delay, stride); // Wait for completion. - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + //TODO: Put this in when we merge tight-validation + //if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } @@ -740,7 +748,9 @@ void MicroBitDisplay::animate(MicroBitImage image, int delay, int stride, int st this->animateAsync(image, delay, stride, startingPosition); // Wait for completion. - fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); + //TODO: Put this in when we merge tight-validation + //if (delay > 0) + fiber_wait_for_event(MICROBIT_ID_DISPLAY, MICROBIT_DISPLAY_EVT_ANIMATION_COMPLETE); } } diff --git a/source/MicroBitListener.cpp b/source/MicroBitListener.cpp index d04c320..f85535b 100644 --- a/source/MicroBitListener.cpp +++ b/source/MicroBitListener.cpp @@ -60,16 +60,23 @@ MicroBitListener::~MicroBitListener() */ void MicroBitListener::queue(MicroBitEvent e) { - MicroBitEventQueueItem *q = new MicroBitEventQueueItem(e); + int queueDepth; + MicroBitEventQueueItem *p = evt_queue; if (evt_queue == NULL) - evt_queue = q; + evt_queue = new MicroBitEventQueueItem(e); else { - while (p->next != NULL) - p = p->next; + queueDepth = 1; - p->next = q; + while (p->next != NULL) + { + p = p->next; + queueDepth++; + } + + if (queueDepth < MESSAGE_BUS_LISTENER_MAX_QUEUE_DEPTH) + p->next = new MicroBitEventQueueItem(e); } } diff --git a/source/MicroBitMessageBus.cpp b/source/MicroBitMessageBus.cpp index 675f6bb..4d64f16 100644 --- a/source/MicroBitMessageBus.cpp +++ b/source/MicroBitMessageBus.cpp @@ -15,6 +15,7 @@ MicroBitMessageBus::MicroBitMessageBus() this->listeners = NULL; this->evt_queue_head = NULL; this->evt_queue_tail = NULL; + this->queueLength = 0; } /** @@ -86,7 +87,6 @@ void async_callback(void *param) listener->flags &= ~MESSAGE_BUS_LISTENER_BUSY; } - /** * Queue the given event for processing at a later time. * Add the given event at the tail of our queue. @@ -97,25 +97,55 @@ void MicroBitMessageBus::queueEvent(MicroBitEvent &evt) { int processingComplete; - // Firstly, process all handler regsitered as URGENT. These pre-empt the queue, and are useful for fast, high priority services. - processingComplete = this->process(evt, MESSAGE_BUS_LISTENER_URGENT); + if (queueLength >= MESSAGE_BUS_LISTENER_MAX_QUEUE_DEPTH) + return; - if (!processingComplete) + MicroBitEventQueueItem *item = new MicroBitEventQueueItem(evt); + MicroBitEventQueueItem *prev; + + // Queue this event. We always do this to maintain event ordering... + // It costs a a little time and memory, but is worth it. + __disable_irq(); + + prev = evt_queue_tail; + + if (evt_queue_tail == NULL) { - // We need to queue this event for later processing... - MicroBitEventQueueItem *item = new MicroBitEventQueueItem(evt); + evt_queue_head = evt_queue_tail = item; + } + else + { + evt_queue_tail->next = item; + evt_queue_tail = item; + } - __disable_irq(); + queueLength++; - if (evt_queue_tail == NULL) - evt_queue_head = evt_queue_tail = item; - else - evt_queue_tail->next = item; + __enable_irq(); - __enable_irq(); + // Now process all handler regsitered as URGENT. + // These pre-empt the queue, and are useful for fast, high priority services. + processingComplete = this->process(evt, true); + + if (processingComplete) + { + // No more processing is required... drop the event from the list to avoid the need for processing later. + if (evt_queue_head == item) + evt_queue_head = item->next; + + if (evt_queue_tail == item) + evt_queue_tail = prev; + + if (prev) + prev->next = item->next; + + queueLength--; + + delete item; } } + /** * Extract the next event from the front of the event queue (if present). * @return @@ -135,10 +165,13 @@ MicroBitEventQueueItem* MicroBitMessageBus::dequeueEvent() if (evt_queue_head == NULL) evt_queue_tail = NULL; + + queueLength--; } __enable_irq(); + return item; } @@ -254,20 +287,23 @@ void MicroBitMessageBus::send(MicroBitEvent evt) * This will attempt to call the event handler directly, but spawn a fiber should that * event handler attempt a blocking operation. * @param evt The event to be delivered. - * @param mask The type of listeners to process (optional). Matches MicroBitListener flags. If not defined, all standard listeners will be processed. - * @return The 1 if all matching listeners were processed, 0 if further processing is required. + * @param urgent The type of listeners to process (optional). If set to true, only listeners defined as urgent and non-blocking will be processed + * otherwise, all other (standard) listeners will be processed. + * @return 1 if all matching listeners were processed, 0 if further processing is required. */ -int MicroBitMessageBus::process(MicroBitEvent &evt, uint32_t mask) +int MicroBitMessageBus::process(MicroBitEvent &evt, bool urgent) { MicroBitListener *l; int complete = 1; + bool listenerUrgent; l = listeners; while (l != NULL) { if((l->id == evt.source || l->id == MICROBIT_ID_ANY) && (l->value == evt.value || l->value == MICROBIT_EVT_ANY)) { - if(l->flags & mask && !(l->flags & MESSAGE_BUS_LISTENER_DELETING)) + listenerUrgent = (l->flags & MESSAGE_BUS_LISTENER_IMMEDIATE) == MESSAGE_BUS_LISTENER_IMMEDIATE; + if(listenerUrgent == urgent && !(l->flags & MESSAGE_BUS_LISTENER_DELETING)) { l->evt = evt; diff --git a/source/MicroBitMultiButton.cpp b/source/MicroBitMultiButton.cpp index f477756..3cc1342 100644 --- a/source/MicroBitMultiButton.cpp +++ b/source/MicroBitMultiButton.cpp @@ -66,6 +66,16 @@ int MicroBitMultiButton::isSubButtonHeld(uint16_t button) return 0; } +int MicroBitMultiButton::isSubButtonSupressed(uint16_t button) +{ + if (button == button1) + return status & MICROBIT_MULTI_BUTTON_SUPRESSED_1; + + if (button == button2) + return status & MICROBIT_MULTI_BUTTON_SUPRESSED_2; + + return 0; +} void MicroBitMultiButton::setButtonState(uint16_t button, int value) { @@ -105,6 +115,26 @@ void MicroBitMultiButton::setHoldState(uint16_t button, int value) } } +void MicroBitMultiButton::setSupressedState(uint16_t button, int value) +{ + if (button == button1) + { + if (value) + status |= MICROBIT_MULTI_BUTTON_SUPRESSED_1; + else + status &= ~MICROBIT_MULTI_BUTTON_SUPRESSED_1; + } + + if (button == button2) + { + if (value) + status |= MICROBIT_MULTI_BUTTON_SUPRESSED_2; + else + status &= ~MICROBIT_MULTI_BUTTON_SUPRESSED_2; + } +} + + void MicroBitMultiButton::onEvent(MicroBitEvent evt) { int button = evt.source; @@ -118,30 +148,40 @@ void MicroBitMultiButton::onEvent(MicroBitEvent evt) MicroBitEvent e(id, MICROBIT_BUTTON_EVT_DOWN); break; - - case MICROBIT_BUTTON_EVT_UP: - setButtonState(button, 0); - setHoldState(button, 0); - - if(isSubButtonPressed(otherButton)) - MicroBitEvent e(id, MICROBIT_BUTTON_EVT_UP); - break; - - case MICROBIT_BUTTON_EVT_CLICK: - case MICROBIT_BUTTON_EVT_LONG_CLICK: - setButtonState(button, 0); - setHoldState(button, 0); - if(isSubButtonPressed(otherButton)) - MicroBitEvent e(id, evt.value); - - break; - + case MICROBIT_BUTTON_EVT_HOLD: setHoldState(button, 1); if(isSubButtonHeld(otherButton)) MicroBitEvent e(id, MICROBIT_BUTTON_EVT_HOLD); break; + + case MICROBIT_BUTTON_EVT_UP: + if(isSubButtonPressed(otherButton)) + { + MicroBitEvent e(id, MICROBIT_BUTTON_EVT_UP); + + if (isSubButtonHeld(button) && isSubButtonHeld(otherButton)) + MicroBitEvent e(id, MICROBIT_BUTTON_EVT_LONG_CLICK); + else + MicroBitEvent e(id, MICROBIT_BUTTON_EVT_CLICK); + + setSupressedState(otherButton, 1); + } + else if (!isSubButtonSupressed(button)) + { + if (isSubButtonHeld(button)) + MicroBitEvent e(button, MICROBIT_BUTTON_EVT_LONG_CLICK); + else + MicroBitEvent e(button, MICROBIT_BUTTON_EVT_CLICK); + } + + setButtonState(button, 0); + setHoldState(button, 0); + setSupressedState(button, 0); + + break; + } }