mbed-ce / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
37 stars 12 forks source link

Starting thread in another thread hangs forever #244

Open lefebvresam opened 4 months ago

lefebvresam commented 4 months ago

I have the following situation:

Two threads and two flags are defined in a class:

        Thread _thread;
        Thread* _buzzerthread;
        atomic_bool _running;
        atomic_bool _buzzerrunning;

The first thread is initialized in the constructor:

_thread(osPriorityBelowNormal, 1024, nullptr, "display")

When the object is started with a start method, I start the first thread:

    _running = true;
    _thread.start(callback(this, &Display::touchProcessor));

The first thread runs as long as running is true and it is calling a function when specific events are occuring:

void Display::touchProcessor() {
    point_t tp;
    while(_running) {
        _touchUpdate = false;
        _queue->event(this, &Display::touchPanelGet).post(&tp);
        while (!_touchUpdate); // hammer time
        ThisThread::sleep_for(1ms); 

        if (_activepage) {
            switch (_tc) {
                case touch: {           ///< touch is detected
                    touchPressHandler(tp);
                    break;
                }
                case no_touch: break;   ///< no touch is detected
                case held: break;       ///< held after touch
                case release: {         ///< release is detected
                    touchReleaseHandler(tp);
                    break;
                }
                case no_cal: break;     ///< no calibration matrix is available
                default:
                    break;
            };
        }
    }
    _thread.terminate();
}

When calling touchPressHandler a software buzzer is started and stopped:

void Display::touchPressHandler(point_t tp) {
    bool found = false;
    static Timeout t;
    widget_map::reverse_iterator it; // reverse order for layering by modifing alphabetic order of object names
    for (it  = _activepage->getWidgets().rbegin(); it != _activepage->getWidgets().rend(); ++it) {
        if (it->second->isVisibleType() && it->second->checkTouchArea(tp)) {
            uint8_t r = it->second->eventTouchPress();
            if (r == 2) continue;
            if (r == 0 && BUZZER_ON) {
                startBuzzer();
                wait_us(100000);
                stopBuzzer();
                // t.attach(callback(this, &Display::stopBuzzer), BUZZER_DURATION);
            }   
            found = true;
            break;                       
        }
    }
    if (!found) {
        FINE("touch press event for [%s]", _activepage->getObjName());
    }
}

I first did a test with Timeout object but get the same issue with a wait.

In the initialization method of the class I prepared the second thread:

        _buzzerthread = new Thread(osPriorityRealtime, 1024, nullptr, "buzzer");
        _buzz = new DigitalOut(_buzzerpin);

When startbuzzer is called, I should start the second thread:

    if (_buzzerthread != nullptr) { // Buzzer thread
        _buzzerrunning = true;
        _buzzerthread->start(callback(this, &Display::buzzBuzzer));
    }

Stopbuzzer should stop the thread:

_buzzerrunning = false;

But as soon as the thread started to run, the software hangs forever:

void Display::buzzBuzzer() {
    while(_buzzerrunning) {
            wait_us(1000);
    }
    _buzzerthread->terminate();
}

and it really hangs at this point, I did a test by pulling a line high before and after and checked with the scope. The thread itself is starting but the call is not returning.

_buzzerthread->start(callback(this, &Display::buzzBuzzer));

lefebvresam commented 4 months ago

I did a trial where I let the thread close itself:

void Display::buzzBuzzer() {
    for (int i = 0; i < 20; i++) {
        _buzz->write(1);
        wait_us(100);
        _buzz->write(0);
        wait_us(100);
    }
    _buzzerthread->terminate();
}

If I do startBuzzer:

_buzzerthread->start(callback(this, &Display::buzzBuzzer));

It runs only once and stops. Running again is not possible, until I reallocate the object at every start:

        _buzzerthread = new Thread(osPriorityRealtime, 1024, nullptr, "buzzer");
        _buzzerthread->start(callback(this, &Display::buzzBuzzer));

But this seems dirty. What I'm missing here?

lefebvresam commented 4 months ago

Another method is not using a separate thread and post it on a infinite queue running in the main, but of course because threads are shared the SW PWM is not smooth and is being interrupted by other calls. Better is let the SW just hang for 100ms. And even in that case the PWM is interrupted by interrupts... I think I really need a HW PWM and have to patch my PCB.

JojoS62 commented 4 months ago

startBuzzer(); wait_us(100000); stopBuzzer();

this does not look good to me. wait_us is blocking and waisting cpu cycles, it should be used with short delays only. Sequences can be scheduled by using the EventQueue:

    queue.call(startBuzzer);
    queue.call_in(10s, stopBuzzer");

Waiting for touch events can be done by using the EventFlags, they are built-in in the Thread class, check the Mbed6 API documentation for the Thread class, there is an example how to wait for flags and set_flags). This is more efficient because it avoids context switching for nothing.

BTW, these discussions are better done in the forum, here it is hard to find it again.

lefebvresam commented 4 months ago

First I did:

static Timeout t;
startBuzzer();
t.attach(callback(this, &Display::stopBuzzer), BUZZER_DURATION);

Which is less or more the same. The point is that starting thread in a thread looks like erroneous. But more fundamental I left the idea by creating a buzzer with a IO pin in a thread. It is however interrupted by other threads and sounds not smooth. I accepted to do hardware patches to reroute a PWM capable output pin to the PWM driver fet.

I will check your suggestion about the flags.

JojoS62 commented 4 months ago

Timeout or Ticker is not really the same, the callbacks are running in Interrupt context. These classes were the poor man’s RTOS in mbed2 :) They can be used and you can create an event in the ISR for the EventQueue and execute it in another thread. But if you don’t have a real Interrupt source, it is easier to create an event direct in the code. Check the EventQueue examples, this is something of the coolest things in Mbed.

lefebvresam commented 4 months ago

I did a test with:

                _queue->call(this, &Display::startBuzzer);
                _queue->call_in(BUZZER_DURATION, this, &Display::stopBuzzer);
void Display::stopBuzzer() {
    if (_buzzer != nullptr) {
        _buzzer->suspend();
    }
}

// bug reported at 24/8/20: after resume object should restore settings
void Display::startBuzzer() {
    FINE("Start buzzer");
    if (_buzzer != nullptr) {
        _buzzer->resume();
        _buzzer->period_us(BUZZER_PERIOD_US);
        _buzzer->write(BUZZER_DUTY_CYCLE);
    } else {
        ERROR("Cannot start buzzer");
    }
}

And this is working also. Note that I did not test again starting a thread in this context. I use PWM module now with hardware patch.

I do not really understand the difference between:

                // _queue->event(this, &Display::startBuzzer).post();
                _queue->call(this, &Display::startBuzzer);
lefebvresam commented 4 months ago

When I use the (injected) main queue, the timing is not guaranteed. When I touch the panel, the beep duration is in one case shorter than normal. I think that at that time, a lot of SPI transactions are executed in the main context. (Loading picture) When I use a separate eventqueue (static EventQueue q) in the touchPressHandler(), there is no beep at all. I'm afraid I have to return to the Timeout object, this worked the best until now. I think because this is with interrupt, the timing is better guaranteed.

JojoS62 commented 4 months ago

You can add a thread with higher priority that is just executing thread.start(queue.dispatch _forever());

lefebvresam commented 4 months ago

Indeed with a separate eventqueue there must be a method to dispatch it forever. If I do one dispatch each time the sofware hangs. I could make it as a class member of Display en do the dispatch forever in a separate thread that may hang on that (like main is doing) as you suggested. But there is a simpeler workaround:

// Doing the start in the current thread to execute it immediately
// Because here is the source of the timing issue if you derefer to main queue
startBuzzer();

// Use the injected main queue, and after buzzer duration the main thread is less occupied
_queue->call_in(BUZZER_DURATION, this, &Display::stopBuzzer);

As long as loading pictures is shorter than BUZZER_DURATION (100ms), but it will be in most of the cases.

The problem with threads is that they are very memory consuming. You can setup the thread stack size but if you set it on 512, you consume 512 kB of RAM.

lefebvresam commented 4 months ago

No this implementation is crashing sometimes. With Timeout I never have a crash. So I will revert it.