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.

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

Multithreaded pwm at two pins -> pigpiod hangs up | Race condition? #357

Open
br-172 opened this issue Jun 6, 2020 · 14 comments
Open

Multithreaded pwm at two pins -> pigpiod hangs up | Race condition? #357

br-172 opened this issue Jun 6, 2020 · 14 comments

Comments

@br-172
Copy link

br-172 commented Jun 6, 2020

Hello,
this is according to my question here. In short:

  • The pin-state is 0. After the command pi.write(pin, 1) the pin-state is still 0. (unexpected)

  • The connected LED flickers while changing the dutycycle. In detail, when dimming from 0 to 255, up to about 150 everything is fine, then the pin-status changes with a frequency of approx. 1 Hz. (abnormal)

In the meantime, I think I was able to isolate the problem. The problem occurs when two pins are dimmed at exactly the same time. Multithreading was used here to increase the probability of the error occurring.

The following script demonstrates it (using pigpiod (V71) with python3 on Raspberry Pi 3B+):

#!/usr/bin/env python3

# Modified: 2020-06-06
# br-172


import time
import threading

import pigpio


""" Dimming two pins with pigpiod at the same time with pwm """
# To demonstrate the problem, this script generates two threads which are started at the same time.
# Each thread controlls exactly one pin. The pins are dimmed up and down.
# After the dimming process has been completed, the current state is checked against the target state.
# If the condition is different, an error message is printed out on the console.

# Exemplary pin 6 and 12, problem is the same with 13 and 21
# Problem probably has nothing to do with the pin number.
BCM_PIN_A = 6
BCM_PIN_B = 12

PWM_FREQUENCY = 500


def test_BCM_PIN_A(dimm_to_on):
    pi = pigpio.pi()
    pi.set_mode(BCM_PIN_A, pigpio.OUTPUT)
    pi.set_PWM_frequency(BCM_PIN_A, PWM_FREQUENCY)

    print("'dimm_to_on' for BCM_PIN_A is", dimm_to_on)

    # Dimm from HIGH to LOW
    if dimm_to_on is False:
        for dc in range(255, 0, -5):
            pi.set_PWM_dutycycle(BCM_PIN_A, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pi.write(BCM_PIN_A, 0)

        time.sleep(0.1)

        # Check, if target state is equal to the current state.
        if pi.read(BCM_PIN_A) != 0:
            print("BCM_PIN_A (", BCM_PIN_A, ") should be LOW, but is HIGH")

    # Dimm from LOW to HIGH
    elif dimm_to_on is True:
        for dc in range(0, 255, 5):
            pi.set_PWM_dutycycle(BCM_PIN_A, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pi.write(BCM_PIN_A, 1)

        time.sleep(0.1)

        # Check, if target state is equal to the current state.
        if pi.read(BCM_PIN_A) != 1:
            print("BCM_PIN_A (", BCM_PIN_A, ") should be HIGH, but is LOW")

    pi.stop()


# Method does the same as 'test_BCM_PIN_A' for BCM_PIN_B
def test_BCM_PIN_B(dimm_to_on):
    pi = pigpio.pi()
    pi.set_mode(BCM_PIN_B, pigpio.OUTPUT)
    pi.set_PWM_frequency(BCM_PIN_B, PWM_FREQUENCY)

    print("'dimm_to_on' for BCM_PIN_B is", dimm_to_on)

    # Dimm from HIGH to LOW
    if dimm_to_on is False:
        for dc in range(255, 0, -5):
            pi.set_PWM_dutycycle(BCM_PIN_B, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pi.write(BCM_PIN_B, 0)

        time.sleep(0.1)

        # Check, if target state is equal to the current state.
        if pi.read(BCM_PIN_B) != 0:
            print("BCM_PIN_B (", BCM_PIN_B, ") should be LOW, but is HIGH")

    # Dimm from LOW to HIGH
    elif dimm_to_on is True:
        for dc in range(0, 255, 5):
            pi.set_PWM_dutycycle(BCM_PIN_B, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pi.write(BCM_PIN_B, 1)

        time.sleep(0.1)

        # Check, if target state is equal to the current state.
        if pi.read(BCM_PIN_B) != 1:
            print("BCM_PIN_B (", BCM_PIN_B, ") should be HIGH, but is LOW")

    pi.stop()


def test_two_pins(dimm_to_on):
    # Comment one function out to test only one pin. Than it works correctly.
    listOfTestFunctions = [
        test_BCM_PIN_A,
        test_BCM_PIN_B
    ]

    threads = []
    for testFunction in listOfTestFunctions:
        pinTest_Thread = threading.Thread(target=testFunction, args=(dimm_to_on, ))
        threads.append(pinTest_Thread)

    for thread in threads:
        thread.start()
        #time.sleep(0.1) # Comment this delay in and it works correctly.

    for thread in threads:  # iterates over the threads
        thread.join()


while True:
    print("\nDimming from LOW to HIGH")
    test_two_pins(True)
    time.sleep(3)
    print("\nDimming from HIGH to LOW")
    test_two_pins(False)
    time.sleep(3)

The Script produces the following output:

root@raspberry:/home/pi# python3 test_pwm multithreading pigpiod.py

Dimming from LOW to HIGH
'dimm_to_on' for BCM_PIN_A is True
'dimm_to_on' for BCM_PIN_B is True

Dimming from HIGH to LOW
'dimm_to_on' for BCM_PIN_A is False
'dimm_to_on' for BCM_PIN_B is False

Dimming from LOW to HIGH
'dimm_to_on' for BCM_PIN_A is True
'dimm_to_on' for BCM_PIN_B is True

Dimming from HIGH to LOW
'dimm_to_on' for BCM_PIN_A is False
'dimm_to_on' for BCM_PIN_B is False

Dimming from LOW to HIGH
'dimm_to_on' for BCM_PIN_A is True
'dimm_to_on' for BCM_PIN_B is True
BCM_PIN_A ( 6 ) should be HIGH, but is LOW
BCM_PIN_B ( 12 ) should be HIGH, but is LOW

Dimming from HIGH to LOW
'dimm_to_on' for BCM_PIN_A is False
'dimm_to_on' for BCM_PIN_B is False

Dimming from LOW to HIGH
'dimm_to_on' for BCM_PIN_B is True
'dimm_to_on' for BCM_PIN_A is True
BCM_PIN_A ( 6 ) should be HIGH, but is LOW
BCM_PIN_B ( 12 ) should be HIGH, but is LOW

Dimming from HIGH to LOW
'dimm_to_on' for BCM_PIN_A is False
'dimm_to_on' for BCM_PIN_B is False

[and so on]

You can see, in this case the problem arose after 3 cycles (dimm-to-High and dimm-to-Low). Unfortunately that doesn't seem deterministic. To fix the problem pigpiod has to be restarted with killall pigpiod and pigpiod. But after a few seconds it will be back.
In my understanding pigpiod works with a pipe, so this behavior is not normal.

Can someone confirm the effect?

@guymcswain
Copy link
Collaborator

Can you try to open just a single instance of pi and pass that object into your dimming method. I'm not sure that pi.stop() and pigpio.pi() are thread safe.

@br-172
Copy link
Author

br-172 commented Jun 7, 2020

Many thanks for your response. I tried your suggestion and it seems to work fine.

#!/usr/bin/env python3

# Modified: 2020-06-07
# br-172


import time
import threading

import pigpio


""" Control two Pins with pigpiod at the same time """

BCM_PIN_A = 6
BCM_PIN_B = 12

PWM_FREQUENCY = 500

pi = pigpio.pi()


def test_BCM_PIN_A(dimm_to_on, pigpiodObject):
    #pi = pigpio.pi()
    pigpiodObject.set_mode(BCM_PIN_A, pigpio.OUTPUT)
    pigpiodObject.set_PWM_frequency(BCM_PIN_A, PWM_FREQUENCY)

    print("'dimm_to_on' for BCM_PIN_A is", dimm_to_on)

    # Dimm from HIGH to LOW
    if dimm_to_on is False:
        for dc in range(255, 0, -5):
            pigpiodObject.set_PWM_dutycycle(BCM_PIN_A, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pigpiodObject.write(BCM_PIN_A, 0)

        time.sleep(0.1)

        # Check, if target state is equal to the actual state
        if pigpiodObject.read(BCM_PIN_A) != 0:
            print("BCM_PIN_A (", BCM_PIN_A, ") should be LOW, but is HIGH")

    # Dimm from LOW to HIGH
    elif dimm_to_on is True:
        for dc in range(0, 255, 5):
            pigpiodObject.set_PWM_dutycycle(BCM_PIN_A, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pigpiodObject.write(BCM_PIN_A, 1)

        time.sleep(0.1)

        # Check, if target state is equal to the actual state
        if pigpiodObject.read(BCM_PIN_A) != 1:
            print("BCM_PIN_A (", BCM_PIN_A, ") should be HIGH, but is LOW")

    #pi.stop()


def test_BCM_PIN_B(dimm_to_on, pigpiodObject):
    #pi = pigpio.pi()
    pigpiodObject.set_mode(BCM_PIN_B, pigpio.OUTPUT)
    pigpiodObject.set_PWM_frequency(BCM_PIN_B, PWM_FREQUENCY)

    print("'dimm_to_on' for BCM_PIN_B is", dimm_to_on)

    # Dimm from HIGH to LOW
    if dimm_to_on is False:
        for dc in range(255, 0, -5):
            pigpiodObject.set_PWM_dutycycle(BCM_PIN_B, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pigpiodObject.write(BCM_PIN_B, 0)

        time.sleep(0.1)

        # Check, if target state is equal to the actual state
        if pigpiodObject.read(BCM_PIN_B) != 0:
            print("BCM_PIN_B (", BCM_PIN_B, ") should be LOW, but is HIGH")

    # Dimm from LOW to HIGH
    elif dimm_to_on is True:
        for dc in range(0, 255, 5):
            pigpiodObject.set_PWM_dutycycle(BCM_PIN_B, dc)
            time.sleep(0.02)

        time.sleep(0.1)

        pigpiodObject.write(BCM_PIN_B, 1)

        time.sleep(0.1)

        # Check, if target state is equal to the actual state
        if pigpiodObject.read(BCM_PIN_B) != 1:
            print("BCM_PIN_B (", BCM_PIN_B, ") should be HIGH, but is LOW")

    #pi.stop()


def test_two_pins(dimm_to_on):

    # Comment one function out to test only one pin. Than it works correctly.
    listOfTestFunctions = [
        test_BCM_PIN_A,
        test_BCM_PIN_B
    ]

    threads = []
    for testFunction in listOfTestFunctions:
        pinTest_Thread = threading.Thread(target=testFunction, args=(dimm_to_on, pi ))
        threads.append(pinTest_Thread)

    for thread in threads:
        thread.start()
        #time.sleep(0.1) # Comment this delay in and it works correctly.

    for thread in threads:  # iterates over the threads
        thread.join()


while True:
    print("\nDimm from LOW to HIGH")
    test_two_pins(True)
    time.sleep(3)
    print("\nDimm from HIGH to LOW")
    test_two_pins(False)
    time.sleep(3)

I tried another test:
Apparently pigpiod is not only not thread safe, but simultaneous access via two different processes is also not secure. In the .zip are three files. The scripts test_gpio_A.py and test_gpio_B.py wait for a tcp command and then dim up or down. The script tcpSender.py sends the messages to the two scripts at exactly the same time. I have exactly the same problem as with multithreading. The function pigpio.pi() is only called up exactly once when the test_gpio_A.py and test_gpio_B.py scripts are started.
test_pwm_parallel_tcp_pigpiod.zip

@guymcswain
Copy link
Collaborator

Apparently` pigpiod is not only not thread safe

I would disagree with that statement but instead say the it is related to how the python module is instantiated.

I'm not in a place to unzip and examine your code but I'll just say that pigpiod should handle multiple threads just fine. However, you need to be cautious of APIs that are not thread safe, such as wave_clear().

@guymcswain
Copy link
Collaborator

I know that the python module runs the APIs atomically. I'll need to verify if the same can be said for pigpiod's socket handler. I thought I had run tests in the past with multiple clients hitting pigpiod without issue. I'll take a look at this over the next few days.

@guymcswain
Copy link
Collaborator

@joan2937 , I'm not seeing a lock on myDoCommand. Is that where I should expect to find one? I'm looking to verify atomicity of socket command execution.

@guymcswain
Copy link
Collaborator

guymcswain commented Jun 8, 2020

@br-172 , on second examination of the code and thinking through the use case, pigpiod is not designed to allow multiple clients working simultaneously on the same host in a protected environment. Given the many APIs that have global side effects it would be impractical if not impossible to be "thread safe". Your best alternative is to use the python client, with the built-in atomic API execution and to protect against thread/API conflicts at the application layer.

@br-172
Copy link
Author

br-172 commented Jun 8, 2020

Many thanks for your effort.
But I am not quite sure what you mean by 'python client'. Do you have a link or an example script?

@guymcswain
Copy link
Collaborator

Sorry, I refer to the python module you have been using in your scripts as a client. It connects to pigpiod (server).

@guymcswain
Copy link
Collaborator

guymcswain commented Jun 8, 2020

I'm adding this comment to correct my earlier comments now that I have a better understanding of the behaviors between the original failing script and the edited working version. Hopefully this will help future readers of this issue.

I said:

Can you try to open just a single instance of pi and pass that object into your dimming method. I'm not sure that pi.stop() and pigpio.pi() are thread safe.

It was incorrect for me to assert that pi = pigpio.pi() and pi.stop() were potentially thread unsafe. They have nothing to do with the observed behaviors, per se, other than to create multiple client connections which resulted in multiple threads running socket commands on pigpiod.

In the edited working script, only a single socket is open on pigpiod and this forces serial (or in other words, atomic) execution of the APIs called by the two worker threads.

I never ran the test_pwm_parallel_tcp_pigpiod.zip script versions because they present the same issue as the original script - multiple socket connections to pigpiod.

Bottom line: APIs such as set_PWM_frequency() and set_PWM_dutycycle() (and presumably others) cannot be run multi-threaded. And, the corollary, one can safely run multi-threads on pigpiod via a single instance (connection) of the python module (client).

@br-172
Copy link
Author

br-172 commented Jun 9, 2020

Thank you for your comments.
I could not finally understand why pigpiod shows exactly this unexpected behavior with multi-threaded access. I think the exact cause depends on many side effects and is difficult to find.

Nevertheless, I tried to outline a workaround:
I would very much like to continue using pigpio on the Raspberry Pi because I don't know of any better library.
Therefore I will probably write a kind of proxy for my use case (I call it pigpioMediator.py).
All scripts that previously sent directly to pigpiod now send their commands to the pigpioMediator. The pigpioMediator provides a socket and stores all incoming instructions in a queue. The pigpioMediator processes them one after the other and thus ensures atomized execution and access to pigpiod via exactly one thread. A disadvantage is the increased delay, but this should persist within a few milliseconds. This workaround is therefore not necessarily useful for quasi real-time applications.
For clarification, I attached a graphic with the status quo:

pigpioMediator_statusQuo

The following is the idea described above with the proxy as a graphic. I know that sending strings and then evaluating them with job.split("_") is still not an optimal solution. But that's enough to test for now.

pigpioMediator_proposedSolution

Should I still leave the issue open?

@guymcswain
Copy link
Collaborator

Why can't a single global instance of pi=pigpio.pi() be the workaround for your application? It will serialize all the api calls to pigpiod.

@br-172
Copy link
Author

br-172 commented Jun 9, 2020

I don't know of a way to distribute a single global instance of pi=pigpio.pi() across processes.

I have combined related hardware into logical units. A script controls exactly one logical unit. Such a unit could e.g. a water supply, a video surveillance system or just a lamp. It is important that the actuators are not controlled sequentially, but at the same time. I tried to think of examples where you can see the design decision particularly well:

My water supply consists of a pump, a tank, a temperature sensor on the pump motor against overheating and a water level sensor on the floor, if something should leak.
For this I create a script that is called garden_watersupply.py. This activates the water supply as a result of a command from a server. The script ensures that the tank is always full and the pump is switched off in the event of leaks or overheating.

I want to control a lamp in the kitchen.
For this I create a script that is called kitchen_lamp.py and then controls the lamp at the command of a server. This script only controls this lamp and nothing else.

If I now install a lamp in the living room, I can copy the script kitchen_lamp.py and then call the copy livingroom_lamp.py. In this I only have to adjust the GPIO pin.

Video surveillance is controlled by a script that controls the camera via "motion" and switches on a lamp via pigpio when motion is detected. The script also takes care of storing the camera images on the file system and sending alarm SMS. I call this script door_videoSurveillance.py.

I found the clarity to be an advantage. Even if a script gets stuck due to a serious error, I can specifically end and restart this script based on its PID without the other scripts being affected.
In particular I can e.g. change the software for the water supply without having to shut down all devices. So if I find that my temperature limit for the pump motor was set too low, I only need to change the script garden_watersupply.py and restart this script.

After implementing this concept, I found that after some time (hours to days) malfunctions occurred. The error occurred when two lamps were dimmed at the same time. Unfortunately, the error - apparently - was not reproducible. So I tried to narrow the problem down and designed a minimal example that still demonstrates the error.

If I only want to use one global instance of pi=pigpio.pi(), I can only use one script with threads like above. This would have a lot of imports and would be very long and - in my opinion - confusing.

@guymcswain
Copy link
Collaborator

I see. It sounds fairly complex. I find that having a lot of processes running come with their own baggage but I'm not in your situation. Good luck with it.

I'm going to keep this open a bit longer to remind me. I haven't decided if it should be formally documented or just have a topic on the project wiki pages discussing it.

@guymcswain
Copy link
Collaborator

@br-172 ,

I have a patch that will run the socket threads single threaded and it works on the test script you provided above (start of this issue). The patch is based on a git diff of the current 'develop' branch but you might be able to git apply it to your version. If not, upgrade to the latest release, v77.

In its current form, the patch has side affects that breaks other stuff - you can't run scripts on pigpiod for example. So it would have to be reworked quite a bit to make it into a release (if ever). But, on the other hand, it is so simple for your use case that it may be a better alternative than the architecture you describe above to work around the lack of multi-threading on the daemon.

socket-threading-patch.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants