GLISTENER_WAITING flag not clearing

[kengineer]

I find that at times the GLISTENER_WAITING flag is not being cleared and so the geventEventWait() call returns immediately. In my application, multiple functions are calling geventEventWait() however these functions are all on the same thread. I'm trying to track down the issue, but since I'm not super familiar with the inner workings of the event system just wanted to run this by you guys first and see if anything came to mind that would cause the flag not to be cleared other than calling geventEventWait() on the same listener from two different threads.

 

Also, if the event system does get an event after waiting, what actually clears the waiting flag?

 

(from geventEventWait)

	// No - wait for one.
	pl->flags &= ~GLISTENER_EVENTBUSY;				// Event buffer is definitely not busy
	pl->flags |= GLISTENER_WAITING;					// We will now be waiting on the thread
	gfxMutexExit(&geventMutex);
	if (gfxSemWait(&pl->waitqueue, timeout))
		return &pl->event;

 

The flag is set and if the semaphore is received during the wait period, how is it cleared as it immediately returns? Then when the wait function re-enters in another function, the flag is still set so it immediately returns. I see that this flag is cleared in some of the geventSendEvent() statements:

(from geventSendEvent)
if ((psl->pListener->flags & GLISTENER_WAITING)) {
			psl->pListener->flags &= ~(GLISTENER_WAITING|GLISTENER_PENDING);
			gfxSemSignal(&psl->pListener->waitqueue);
		} else
			psl->pListener->flags |= GLISTENER_PENDING;

 

However as you can see there are cases in that statement where it is not cleared and I'm not familiar enough with the subsystem to make a good judgement on where or when it might not be getting cleared.

[inmarket]

This is a complex question. I will need to thouroughly review the code as it has been a number of years since I last looked at it. You know how it goes - if something is working you just don't play with it.

My initial suspicions without looking at the code is that perhaps the semaphore is returning in situations that it shouldn't.  Can you please tell me what operating system you are using as the problem could actually be in the GOS implementation.

I will review the code fully when i get time in the next couple of days. Multithreading stuff can be very complex and there could well be a window of opportunity in the code. Thanks for letting us know.

[kengineer]

I'm using FreeRTOS. Yes I was trying to debug it but debugging a multithreaded system is hard enough let alone one I'm not familiar with. Looking forward to you reply, thanks!

[kengineer]

Did you guys ever get a chance to look at this?

 

Also, I noticed something that you should be doing in your examples for all GEVENT related functions:

 

From button example:

		// Get an Event
		pe = geventEventWait(&gl, TIME_INFINITE);

		switch(pe->type) {
			case GEVENT_GWIN_BUTTON:
				if (((GEventGWinButton*)pe)->gwin == ghButton1) {
					// Our button has been pressed
					if (++which >= sizeof(orients)/sizeof(orients[0]))
						which = 0;

					// Setting the orientation during run-time is a bit naughty particularly with
					// GWIN windows. In this case however we know that the button is in the top-left
					// corner which should translate safely into any orientation.
					gdispSetOrientation(orients[which]);
					gdispClear(White);
					gwinRedrawDisplay(GDISP, FALSE);
				}

 

from gEventWait:

// Timeout - clear the waiting flag.
	// We know this is safe as any other thread will still think there is someone waiting.
	gfxMutexEnter(&geventMutex);
	pl->flags &= ~GLISTENER_WAITING;
	gfxMutexExit(&geventMutex);
	return 0;

There's no check in the examples to see if pe is NULL. This probably wouldn't be a problem is that example since we're waiting forever (unless the event is busy like in the problem I'm having) however pe would definitely be NULL in the case of a timeout or immediate return. This doesn't crash on my embedded platform but I recently got this working on a WIN32 platform and switching on pe-type when pe is NULL does crash. I think you should add a check to your examples.

[inmarket]

All the examples use TIME_INFINITE and therefore will never return a NULL. In practice that is what we encourage - to use TIME_INFINITE whereever possible for geventEventWait. There are plenty of other mechanisms in ugfx that could be used for polling so that the main event loop should not need to be cluttered by polling code eg ugfx threads and gtimer.

Sorry, I have not yet had a chance to check out gevent. Coming very shortly.

[kengineer]

I also confirm that this exact issue happens on a WIN32 build as well.

[inmarket]

Thank you for finding that out. As you might have gathered I have been very busy at work (14 hr days) so i have not had time to look yet. Fortunately the current deadline is nearly there

Can I please ask you to put together a small sample Win32 program that demonstrates the problem? It makes it much easier to find in a short space of time.

[kengineer]

Sure I was thinking about doing that exactly, I'll get you one shortly.

[kengineer]

Alright, I wrote some code that replicates it. Maybe this has something to do with calling the next event too quickly? Because in the main screen function if you replace time_immediate with the 1000 ms wait you won't see the bug. However with TIME_IMMEDIATE as soon as you jump to the next screen function if you observe the printfs you will see it does not wait anymore for an event as the event is return immediately. 

 

In this example eventBugExample in main

#include "gfx.h"

typedef enum {
	MAIN_SCREEN,
	SCREEN_A,
	SCREEN_B
} SELECTED_SCREEN_t;

static SELECTED_SCREEN_t selected_screen = MAIN_SCREEN;

/* Function protos */
static void createWidgets(void);
static void mainScreen(void);
static void screenA(void);
static void screenB(void);

/* Container stuff */
static GHandle Container_Window_Handle;
static GContainerObject Container_Window_Object;

/* Button stuff */
static GHandle ButtonA_Handle, ButtonB_Handle, Back_Button_Handle;
static GButtonObject ButtonA_Object, ButtonB_Object, Back_Button_Object;

/* Event stuff */
static GListener gl;

/* Event wait period */
#define EVENT_MAX_WAIT_MS		1000

static void createWidgets(void)
{
	GWidgetInit wi;

	// Apply the container parameters
	gwinWidgetClearInit(&wi);
	wi.g.show = FALSE;
	wi.g.width = 320;
	wi.g.height = 240;
	wi.g.x = 0;
	wi.g.y = 0;
	Container_Window_Handle = gwinContainerCreate(&Container_Window_Object, &wi, 0);

	gwinWidgetClearInit(&wi);
	wi.g.show = FALSE;
	wi.g.width = 120;
	wi.g.height = 40;
	wi.text = "A";
	wi.g.x = 100;
	wi.g.y = 20;
	wi.g.parent = Container_Window_Handle;
	ButtonA_Handle = gwinButtonCreate(&ButtonA_Object, &wi);

	gwinWidgetClearInit(&wi);
	wi.g.show = FALSE;
	wi.g.width = 120;
	wi.g.height = 40;
	wi.text = "B";
	wi.g.x = 100;
	wi.g.y = 80;
	wi.g.parent = Container_Window_Handle;
	ButtonB_Handle = gwinButtonCreate(&ButtonB_Object, &wi);

	gwinWidgetClearInit(&wi);
	wi.g.show = FALSE;
	wi.g.width = 120;
	wi.g.height = 40;
	wi.text = "Back";
	wi.g.x = 100;
	wi.g.y = 20;
	wi.g.parent = Container_Window_Handle;
	Back_Button_Handle = gwinButtonCreate(&Back_Button_Object, &wi);
}

void eventBugExample(void)
{
	gfxInit();

    // Set the widget defaults
    gwinSetDefaultFont(gdispOpenFont("*"));
    gwinSetDefaultStyle(&WhiteWidgetStyle, FALSE);

	createWidgets();

	/* Init and attach our listener to gwin system */
	geventListenerInit(&gl);
	gwinAttachListener(&gl);

	gdispClear(White);

	/* Show our main window */
	gwinShow(Container_Window_Handle);

	while(1)
	{
		/* Run through our screens */
		switch(selected_screen)
		{
			case MAIN_SCREEN:
			{
				mainScreen();
				break;
			}

			case SCREEN_A:
			{
				screenA();
				break;
			}

			case SCREEN_B:
			{
				screenB();
				break;
			}
		}
	}
}

static void mainScreen(void)
{
	GEvent* pe;
	int flag = 0;

	gwinShow(ButtonA_Handle);
	gwinShow(ButtonB_Handle);
	printf("In main screen\n\r");

	while(!flag)
	{
		pe = geventEventWait(&gl, TIME_IMMEDIATE);

		if(NULL != pe)
		{
			if(GEVENT_GWIN_BUTTON == pe->type)
			{
				if(((GEventGWinButton*)pe)->gwin == ButtonA_Handle)
				{
					selected_screen = SCREEN_A;
					printf("Selected screen A!\n\r");
					flag = 1;
				}
				else if(((GEventGWinButton*)pe)->gwin == ButtonB_Handle)
				{
					selected_screen = SCREEN_B;
					printf("Selected screen B!\n\r");
					flag = 1;
				}
			}
		}
	}

	gwinHide(ButtonA_Handle);
	gwinHide(ButtonB_Handle);
}

static void screenA(void)
{
	GEvent* pe;
	int flag = 0;

	gwinShow(Back_Button_Handle);

	while(!flag)
	{
		printf("In screen A\n\r");
		pe = geventEventWait(&gl, EVENT_MAX_WAIT_MS);

		if(NULL != pe)
		{
			if(GEVENT_GWIN_BUTTON == pe->type)
			{
				if(((GEventGWinButton*)pe)->gwin == Back_Button_Handle)
				{
					selected_screen = MAIN_SCREEN;
					printf("Go back to main!\n\r");
					flag = 1;
				}
			}
		}
	}

	gwinHide(Back_Button_Handle);
}

static void screenB(void)
{
	GEvent* pe;
	int flag = 0;

	gwinShow(Back_Button_Handle);

	while(!flag)
	{
		printf("In screen B\n\r");
		pe = geventEventWait(&gl, EVENT_MAX_WAIT_MS);

		if(NULL != pe)
		{
			if(GEVENT_GWIN_BUTTON == pe->type)
			{
				if(((GEventGWinButton*)pe)->gwin == Back_Button_Handle)
				{
					selected_screen = MAIN_SCREEN;
					printf("Go back to main!\n\r");
					flag = 1;
				}
			}
		}
	}

	gwinHide(Back_Button_Handle);
}

 

 

[inmarket]

Wow - that one was tricky to find!

A nasty little multi-threading issue.

The problem came down to the event buffer management and a small window of opportunity created by the code to detect multiple threads trying to call geventEventWait() on the one listener object. The bug was most obvious when calling gventEventWait() with TIME_IMMEDIATE because that polled that code the fasted.

I took out that checking code for multiple threads on the one listener, simplified, simplified, simplified etc.

The new code is now much simpler and doesn't have the same problem. There is one tiny downside compared to the old code, it doesn't error check for multiple calls to geventEventWait() on the one listener object by different threads. In practice, it is now safe to do so provided the implicit geventEventComplete() call in geventEventWait() doesn't cause problems for the application by releasing of the event buffer while the other thread is still using it.

Thanks for bringing it to our attention and sorry it took so long to get to it.

The updated code is in the repo.