Event/semaphore-related crash?

[steved]

I'm implementing a pop-up window, in a similar manner to that described at http://wiki.ugfx.org/index.php?title=Frame#Overlay. Using chibios (V3 SVN)

I get a semaphore-related system crash during destruction of the popup, which appears to be triggered within the event handler. Not managed to pin it down yet, but it looks as if maybe the event listener created by the popup window isn't getting fully destroyed. Code crashes on line 156 of gevent.c:

gfxSemWait(&psl->pListener->eventlock, TIME_INFINITE);        // Obtain a lock on the listener event buffer

The create and destroy code all runs on the main ugfx event handler loop.

To create the popup:

      gwinSetEnabled(ghFrameScreen, FALSE);    // Disable main screen - all elements are in a container
      ghFramePopup = createKeypadDisplay(0, "Access Code");		// Add some buttons in a frame

To destroy the popup, and reinstate the main screen (in response to a button press on the popup):

        gwinDestroy(ghFramePopup);
        ghFramePopup = NULL;
        gwinSetEnabled(ghFrameScreen, TRUE);

The call stack is as follows:

gfxSemWait() (chibios.c:121)

geventRegisterCallback() (gevent.c:129)

_frameDestroy() (frame.c:42)

gwinDestroy() (gwin.c:175)

At one point the crash didn't occur until the first button press on the main screen (after the popup was destroyed). I added a line of code after line 44 in gevent.c:

44:        psl->pListener = 0;
45:        psl->pSource = 0;               // Added as experiment

At the time that appeared to make a difference; then the problem returned.

I'm still trying to thread my way through the event code

Has anyone else experienced something like this?

[steved]

Looks like there could be two problems!

Initially the popup frame was created without a 'close' button (or min/max).

On investigation, the _frameDestroy() routine 'disconnects' the frame listener regardless. So I added a check:

static void _frameDestroy(GHandle gh) {
  // Following added
  if ((gh->flags & (GWIN_FRAME_CLOSE_BTN|GWIN_FRAME_MINMAX_BTN))) {
	/* Deregister the button callback */
	geventRegisterCallback(&gh2obj->gl, NULL, NULL);
	geventDetachSource(&gh2obj->gl, NULL);
  }
	/* call the gcontainer standard destroy routine */
	_gcontainerDestroy(gh);
}

... and everything works fine.

I then enabled the frame 'Close' button - and the problem recurred.

So there may be a problem with the operation of geventRegisterCallback(&gh2obj->gl, NULL, NULL);

[inmarket]

This is code that has been fairly well tested but there are always opportunities for things to slip through. I will have a good look tomorrow. Thanks for bringing it to my attention.

PS. The frame window is not an implementation I am very happy with as a) it uses a lot of memory, and b) the window move logic won't work properly because it has children windows outside the client area. These things really require a rewrite of the frame window but that can come later when I have more time.

[steved]

PS. The frame window is not an implementation I am very happy with as a) it uses a lot of memory, and b) the window move logic won't work properly because it has children windows outside the client area. These things really require a rewrite of the frame window but that can come later when I have more time.

When you do get round to a rewrite, a method of including 'simple' objects (lines, fixed text, etc) within the scope of redrawing would be nice. One thought is a widget that collects them all into a group, but even that may be overkill. It certainly does seem that any window object is relatively memory hungry, but on a quick look I couldn't see anything surplus.

If you have a frame 'close' button - it (quite correctly) destroys the frame, and other parts of the code need to know that the window had been closed. To solve this I assigned a tag value (as per submitted mod) to each of the frame's buttons:

#define TAG_SCREEN_CLOSE        0x0b    /* Tag value assigned to the 'Close' button on frames - ASCII 'Escape'  */
#define TAG_SCREEN_MAX          0xfe    /* Tag value for the 'Maximise' button on frames */
#define TAG_SCREEN_MIN          0xfd    /* Tag value for the 'Minimise' button on frames */

Thus my control loop became aware of the event and could take action to re-enable the background screen. And only destroy the popup on a 'normal' exit, of course.

[inmarket]

The other thing that will be nice to add is support for a "model" window.

It would make your popup window code so much easier.

Ahh, so much to do

[inmarket]

You are certainly right about window objects taking memory. Unfortunately that is the cost of adding that level of abstraction.

uGFX is a LOT better than most other embedded window systems but we are working to make it better yet.

We have an internal spreadsheet that we use for tracking memory usage of widgets etc for the purpose of driving our optimisation efforts. We are not ready to release that publicly yet however if you send me a private email I will send it to you.

The frame window is particularly bad because it contains a number of "sub-widgets" for the close buttons etc plus special listening code to handle that.

Listeners in particular are expensive as under the current model every listener contains a maximum sized event buffer.

The way to optimise the frame window is to remove the sub-widgets and have the frame window code do the drawing itself for those objects. This has a number of advantages...

• It removes the memory requirements for each of the sub-widgets
  
• It removes the memory requirements for the special listening code
  
• Window move and resize should work correctly as there are no children outside the client area
  
• Simplifies object destruction and other code

There is however more complexity in the frame window drawing code and it will take time to code. Hopefully soon I will fix it.

[inmarket]

I have looked at the code in question. Your fix in frame.c is definitely required when there are no buttons on the frame window. Thanks for finding it.

The change to event.c is NOT required (only the psl->listener field is checked to test for slot availability) and may have other side effects.

I have updated the repository with this change.

Can you please test if this solves your problem?

[steved]

Thanks for looking at this - I've updated my local copy with the changes.

One thing I found is that the test on line 42 of frame.c needs to be on gh->flags (not flags).

Can't comment on whether my problem is solved yet - the various changes seem to have stopped things working at all! Back to debugging.....

[steved]

Behaviour reverted to the original - crashing on first button press on main screen after the popup has been destroyed.

Might have partially fixed it, with a change to line 155 of gevent.c:

//      if (gsh == psl->pSource) {
        if ((gsh == psl->pSource) && psl->pListener) {

Now works fine when a 'normal' button is pressed in the popup frame.

What I think was happening with the 'normal' button press:

1. All button and frame events are registered with a source handle of GWIDGET_SOURCE [defined as ((GSourceHandle)(void *)_gwidgetCreate)]

2. Initially there is one event handler linkage in the Assignments list - all OK

3. Creating the popup adds a second event handler linkage

4. Destroying the popup clears down the Listener field of the second event handler linkage (, but not the source)

5. A subsequent button event checks the list, and gets a source match.

6. Attempt to check the listener - which is zero - so semaphore doesn't exist.

Still locks if the frame is closed with its 'close' button - get a chibiOs SV#5 error from the event handler loop in the main code - 'misplaced chSysUnlock()'. Semaphores again!

[inmarket]

Thanks. I will look in detail at that in the morning.

[steved]

Might have discovered what is going on with the frame 'close' button.

1. Frame 'close' button generates an event

2. Button event handler finds the frame's listener using geventGetSourceListener(), which locks the listener event buffer.

3. Frame's event handler is called, identifies that its 'close' button has been pressed, and destroys the frame

4. The frame destroy routine attempts to kill its listener by calling geventRegisterCallback(&gh2obj->gl, NULL, NULL);

5. geventRegisterCallback() calls gfxSemWait(&pl->eventlock, TIME_INFINITE); to lock the listener.

6. The listener is already locked, so gfxSemWait() results in task suspending

7. Context switch to main loop, which does pe = geventEventWait(&gl, TIME_IMMEDIATE)

In 'normal' circumstances the listener event buffer is unlocked when the event is fully processed. However it looks as if its possible to create deadlock by destroying the frame directly in response to the event.

There's a difference I'd missed before - I normally process events on the 'main' thread. A 'window close' is processed on a ugfx thread. Quite possibly of significance.

[inmarket]

That is great debugging! Multi-threading issues can be such a pain. I will look very closely at it in the next few hours (just got to time it around real paying work).

Thank you for your work in this area.

I think the reason this has gone undetected for so long is because in the past listeners have never been created and destroyed continually. They were typically set up once and never destroyed (like the main application listener). I will make sure we get this fixed asap.

[inmarket]

I have largely rewritten gevent.

As you surmised the problem was the re-entrancy within the event callback handler that the frame window was doing.

Looking at the code it should have never been allowed with the existing implementation.

The updated implementation should have none of those issues.

It also now completely avoids the use of gfxSemCounter() which is algorithmically nasty anyway (it is also hard to implement on some o/s's eg Win32).

I have done basic testing using the frame and widgets demo however I would appreciate you testing the new implementation with your code which appears to test this far more thoroughly than our simple tests.

[steved]

I have largely rewritten gevent.

That sounds a little drastic!

But it does seem to have sorted the problem - on a quick test, no more lockups.

Thanks for quick response.

[inmarket]

It was one of those to - do things. The problems you were having just made now the time to do it.

I am much happier with the code now.

Thanks for your diagnostics and help in identifying problems.