Probably errant delays in GPIO/PWM

[SJFriedl]

With a BeagleBone Green Wireless and the latest everything (Debian Buster 2020-04-06, Python 3.7, Adafruit-BBIO 1.2.0), I'm finding spurious delays in GPIO and PWM signals that don't seem right . Full version.sh info at the end.

This is an embedded application that uses the GPIO bits for machine control, and PWM to generate speaker beeps to alert an operator of runtime conditions. Some of these errant delays are actually problematic.

I think I've tracked most of these down and will look at creating some pull requests to make them so, though my utter unfamiliarity with udev might make some of them sub-optimal.

GPIO delays

When doing simple GPIO bit flipping, the code seems to add a 200msec delay during pin setup to allow the udev export stuff to settle down even if the pin has already been exported.

At the end of the function gpio_export() found in event_gpio.c is this bit of code:

BBIO_err gpio_export(unsigned int gpio)
{
    ...
    usleep(200000);      // Hack to wait for newly exported pins to get correct ownership
}

and this seems reasonable enough if it needs to let things settle, but my proposed fix would exclude this if the pin has already been exported (mostly pseudocode);

BBIO_err gpio_export(unsigned int gpio)
{
   int hack_settletime = 200000;  // 200 msec

   if (this has already been exported)
   {
       // make a syslog note
       hack_settletime = 0;  // no delay needed
       goto exit;
    }
    // lots more other stuff
exit:
    ...
    if (hack_settletime > 0) usleep(hack_settletime);

   return ret;
}

Actual code fixing this seems to work, though I think the errant delay is not a huge deal because it's just once per execution. Still, worthy of a fix.

PWM delays

Much more problematic are PWM setup delays, which are repeatable. Generating a beep that's 50 msec at 1 kHz, then 50 msec at 2 kHz, then another 50 msec back at 1 kHz:

#!/usr/bin/python3

import time
import Adafruit_BBIO.PWM  as PWM

pwmPin = "P8_46"

def runBeep(freq, dur):
    global pwmPin

    PWM.start(pwmPin, duty_cycle = 50, frequency = freq)

    time.sleep(dur)

    PWM.stop(pwmPin)

runBeep(1000, 0.050)
runBeep(2000, 0.050)
runBeep(1000, 0.050)

The stock library takes around 220 milliseconds between each set of beeps, which feels excessive.

Looking into the code, there is at least one obvious bug that gets one of the udev names wrong, plus I think a second test is wrong as well - this stuff is confusing to me - and after "fixing" it, it looks a lot better:

The pwm_setup() function in c_pwm.c is where I think all the problematic behavior is, and early in the function are character arrays for a raft of different related names.

The clear bug is in:

snprintf(ecap_path_udev, sizeof(ecap_path_udev), "%s/pwm-%c:%d", pwm_chip_path, dmtimer_pin ? pwm_path[47] : pwm_path[67], p->index);

because pwm_path[67] is a slash character on my platform. creating a filename like (check out the far right side of the path):

/sys/devices/platform/ocp/48304000.epwmss/48304200.pwm/pwm/pwmchip7/pwm-/:1

Using pwm_path[66] gets the last character of the pwmchip# number, which gets us more in the ballpark:

/sys/devices/platform/ocp/48304000.epwmss/48304200.pwm/pwm/pwmchip7/pwm-7:1

The deeper issue is that the code seems to think it needs to keep exporting the same PWM pin even after it's been exported previously, and this takes more runtime plus has some built-in intentional delays.

The code first tests the pwm_path, then the pwm_path_udev, and I think it should be doing it in the other order.

    // Export PWM if hasn't already been
    e = stat(pwm_path, &s);    // **SHOULD BE pwm_path_udev***
    if (-1 == e) {

and at the end of the else for this test, we copy pwm_path_pwm to pwm_path, and the rest of the code seems to do the right thing.

This feels terribly brittle and is unlikely to be right in the general case; I'm not sure how to vet this more broadly.

[pdp7]

When doing simple GPIO bit flipping, the code seems to add a 200msec delay during pin setup to allow the udev export stuff to settle down even if the pin has already been exported.

Thanks, this is a good point and worthy of being fixed.

The deeper issue is that the code seems to think it needs to keep exporting the same PWM pin even after it's been exported previously, and this takes more runtime plus has some built-in intentional delays.

I agree that it seems like the tracking of exported state needs to be improved. I need to think more about that because it has been awhile since I looked at this.

Using pwm_path[66] gets the last character of the pwmchip# number, which gets us more in the ballpark:

I made a fix for pwm_path in #342 which is also related to the array index. Do you have commit https://github.com/adafruit/adafruit-beaglebone-io-python/pull/346/commits/fe2e30a8a622de66a98b88ee307d6ed459fc4f85 in your build? I am wondering if maybe I caused a regression.

Overall, the current path handling code does seem complicated and brittle but I am hesitant to make changes for fear of regressions. I don't have a good way to test across all the kernel versions that there is logic for.

This feels terribly brittle and is unlikely to be right in the general case; I'm not sure how to vet this more broadly.

I have to admit that a lot of this library has built over time to cover many different kernel versions and device tree overlay strategies and there are many brittle parts.

Adafruit Blinka has been receiving more focus over the last 2 years. There is support for some of the functionality on the BeagleBone and PocketBeagle.

I think there are people that still prefer the semantics of Adafruit_BBIO (which in turn is based on the rather old RPi.GPIO) but I would recommend people first try Adafruit_Blinka for new Python projects on BeagleBone or PocketBeagle before they try Adafruit_BBIO. It might be more productive to improve the Blinka support than try to continue developing Adafruit_BBIO.

[SJFriedl]

So re: the issue with #342, there's another place the array index needed fixed a few lines above:

I'm not sure why mine took a different code path than the original, probably because I was messing around with the various filenames as I flailed while seeking an answer.

I looked at Blinka, and I don't think it will work for my application, mainly because the code says that variable_frequency is not supported; my application needs to generate different kinds of beeps to alert an operator.

But it seems we've both converged on the term "brittle" to describe this part of the code, and given I've managed to hack my way through something that works for my application, it would probably be a better use of your time to spend it beefing up Blinka than decoding this maze of twisty pathways.

I'm grateful for the time you spent helping me.