adafruit / adafruit/adafruit-beaglebone-io-python

Probably errant delays in GPIO/PWM

Open
#350 2 comments 0 reactions 1 assignee Claimed by @pdp7 View on GitHub
bug
Dominant language
C
Stars
479
Forks
214
PR merge metrics
No merged PRs in 30d

Description

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.

![image](https://user-images.githubusercontent.com/30396472/112786988-5b422180-900c-11eb-8c68-93c3529878ca.png)

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:

![image](https://user-images.githubusercontent.com/30396472/112787078-93496480-900c-11eb-8676-58ebedcb5813.png)

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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.