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

PWM enable_fd is never closed #197

Closed
JesseMcL opened this issue Nov 21, 2017 · 1 comment
Closed

PWM enable_fd is never closed #197

JesseMcL opened this issue Nov 21, 2017 · 1 comment
Assignees

Comments

@JesseMcL
Copy link
Contributor

Hi Adafruit team,

It seems like there is a problem with file descriptor leaks in the PWM module.

enable_fd is opened every time pwm_setup is called, but never closed on pwm_disable, or if pwm_setup otherwise fails. The PWM is removed from the exported_pwm list, so a new enable_fd is created the next time PWM.start is called
Eventually the python process runs out of file descriptors and so all calls to PWM.start raises

RuntimeError: problem with sysfs file.

Could I suggest that PWMs aren't removed from exported_pwms on PWM.stop until they are actually unexported (ie stop writes enable=0 but does not call disable_pwm), and file descriptors for frequency, duty cycle, polarity and enable are kept open as long as the pwm is exported? This may require PWM.setup and PWM.disable methods to be exported to python so eventually they could be closed, but would have more predictable behaviour. Otherwise, at least closing enable_fd in pwm_disable will stop the leak, but adds overhead in setup and teardown event time start/stop is called.

This occurs on Beaglebone Black running Adafruit_BBIO 1.0.7 (and other earlier versions i've used as well)

uname -a
cat /etc/dogtag
cat /etc/debian_version

Linux keyhound 4.4.9-ti-r25 #1 SMP Thu May 5 23:08:13 UTC 2016 armv7l GNU/Linux
BeagleBoard.org Debian Image 2016-05-13
8.8

to reproduce
test file:

import Adafruit_BBIO.PWM as PWM

for i in range(20,20000): # could just be an infinite loop, will eventually crash with any open file limit
 PWM.start('P8_19',5,i)
 PWM.stop('P8_19')

set open file limit to 1000, for example, then run test script

ulimit -n 1000
python pwmtest.py

Traceback (most recent call last):
File "./pwmtest.py", line 9, in
PWM.start('P8_19',5,i)
RuntimeError: Problem with a sysfs file

You can also check lsof --pid and see that enable_fd is not closed.

Workaround
only call start/stop on each pwm once. rather than disabling, set duty cycle to zero. This isn't ideal as the peripheral is still in use and can cause glitches when changing frequency, as duty cycle is actually stored in sysfs as the on time, not the duty cycle. changing frequency using sysfs doesn't maintain duty cycle. it maintains on time.

@pdp7 pdp7 self-assigned this Nov 21, 2017
pdp7 added a commit that referenced this issue Nov 22, 2017
Issue #197: JesseMcL raised the issue that running
PWM.start() and PWM.start() in a loop will eventually
exhaust the number of open file descriptors and
PWM.start() will raise the error:

  RuntimeError: problem with sysfs file.

The file descriptor for the enable file should
be closed in pwm_disable() to avoid the leak

Signed-off-by: Drew Fustini <drew@pdp7.com>
@pdp7
Copy link
Collaborator

pdp7 commented Nov 22, 2017

@JesseMcL Thanks for raising this issue and the detailed description.

I have added close(pwm->enable_fd) in pwm_disable() to avoid the leak.

I'll look further at the code to see what other leaks might occur.

pdp7 added a commit that referenced this issue Nov 24, 2017
Make sure that the file descriptor for pwm enable
is closed along with the other pwm file descriptors

Signed-off-by: Drew Fustini <drew@pdp7.com>
@pdp7 pdp7 closed this as completed Nov 24, 2017
pdp7 added a commit that referenced this issue Dec 1, 2017
Features:
  * Issue #194: Encoder position cannot be set
  * PR #205: Encoder: add support for reading/writing sysfs attributes

Fixes:
  * Issue #198: use https for DEFAULT_URL in distribute_setup.py
  * Issue #197: Fix leak of pwm enable file descriptor
  * Issue #189: Fix seg fault of PWM in Python 3.6
  * Issue #180: Clarify there is no 0 prefix for pin lables
  * PR #201: Encoder: do kernel check, PEP8 cleanup
  * PR #202: Encoder: corrected kernel check logic
  * PR #207: Encoder: improved usage adocumentation
  * PR #210: Encoder: fix sysfs import, make code Python 3 compatible
  * PR #212: Encoder: fix Python 3 compatibility
  * PR #213: Encoder: fix frequency calculation from period

shortlog:

* David Planella (18):
  * Encoder: initialize only the given channel
  * Sync from master
  * Encoder: do kernel check, PEP8 cleanup
  * Encoder: added sysfs module
  * Encoder: use sysfs to write QEP attributes
  * Encoder: corrected kernel check logic
  * Merge pull request #2 from adafruit/master
  * Encoder: convert get/set methods to properties, update apidoc strings
  * Encoder: updated README
  * Encoder: add README apt install clarification
  * Encoder: copyright assignment note, updated comments
  * Encoder: added usage notes
  * Encoder: improved usage documentation
  * Encoder: minor fix to usage example
  * Encoder: added a note about permissions
  * Encoder: switched sysfs to be a relative import compatible with Python 2 and 3
  * Encoder: use items() instead of iteritems() to be Python 3 compatible
  * Encoder: fix frequency getter

* Drew Fustini (18):
  * use https for DEFAULT_URL in distribute_setup.py (#198)
  * fix except syntax for Python 3
  * use dict.items() instead of dict.iteritems() for Python 3
  * fix error in set_brightness()
  * close enable_fd when stopping PWM output (#197)
  * Merge pull request #199 from dplanella/patch-1
  * Fix leak of pwm enable file descriptor (#197)
  * Merge pull request #201 from dplanella/encoder-cleanup
  * remove test_rotary.py as not valid for pytest
  * Fix seg fault of PWM in Python 3.6 (#189)
  * Merge pull request #202 from dplanella/patch-2
  * Clarify there is no 0 prefix for pin lables (#180)
  * Merge pull request #205 from dplanella/encoder-sysfs
  * assign copyright for new file to Adafruit Industries
  * Add bash scripts to help install and test
  * Merge pull request #212 from dplanella/patch-4
  * Merge pull request #207 from dplanella/patch-3
  * Merge pull request #213 from dplanella/fix-encoder-frequency

Signed-off-by: Drew Fustini <drew@pdp7.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants