rust-embedded / rust-sysfs-gpio

A Rust Interface to the Linux sysfs GPIO interface (https://www.kernel.org/doc/Documentation/gpio/sysfs.txt)
Apache License 2.0
383 stars 45 forks source link

Bad performance #30

Open BerndAmend opened 7 years ago

BerndAmend commented 7 years ago

Hi, many functions (e.g. Pin::set_direction, Pin::set_value, Pin::get_value, Pin::set_edge, Pin::get_poller) take longer than somebody would expect. Making them unsuitable for many tasks. The reason for the bad performance is that the sysfs-gpio-files direction,edge, and value are opened on demand and closed afterwards. This makes it impossible to read a sensor like the DHT22 (https://github.com/Filkolev/DHT22-sensor-driver) or performing any time critical operation. Whereas a C/C++/Python/... application would be able to do so. Code example:

let pin = Pin::new(17);
pin.set_direction(Direction::Out);
pin.set_value(1);
[...]
pin.set_direction(Direction::In);
pin.set_edge(Edge::BothEdges);
let mut poller = pin.get_poller().unwrap();
// At this point I already have lost parts of the sensor reply
// if I use pin.get_value() I only receive 1/8 of the desired values
[...]

At the moment I don't see any way to fix this issue without breaking the API. Any suggestions?

Best regards, Bernd

nastevens commented 7 years ago

The Linux sysfs interface for GPIOs (which is the lower-level interface that rust-sysfs-gpio uses) is not the right interface for doing anything with any sort of timing constraints, such as reading the DHT22. It's really only meant for reading/writing slow events like toggle switches, LEDs, buzzers, etc. where a difference of +/- 100ms is not a problem. The decision was made to open and close the files on demand in rust-sysfs-gpio because those operations require a relatively small amount of time compared to the timing of the sysfs GPIO interface, and it made the API more ergonomic.

The project that you linked to actually includes a Linux kernel module, which uses the kernel-level GPIO functions to communicate with the DHT22. The timing inside the kernel is much, much tighter. For your use, it'd probably be better to use the kernel driver you linked to and then use Rust to read the values out of the /sys/kernel/dht22 directory it creates.

BerndAmend commented 7 years ago

I forgot to include a link to a project that uses the Linux sysfs interface for GPIOs for accessing the DHT22. Here is the missing link https://github.com/ondrej1024/foxg20/blob/master/dhtlib/dht_gpio.c The code has a higher success rate than most other applications I tried.

nastevens commented 7 years ago

I still wouldn't count on that code - it may work some percent of the time but then fail other times. The sysfs interface does not have any sort of guaranteed timing and isn't the right place to be writing drivers that have any kind of timing constraints.

If you'd still like to implement something like the dhtlib solution in Rust, I'd recommend just opening and reading/writing directly to the sysfs files from your code.

I don't want it to sound like I'm being dismissive of your issue - there is no doubt that opening and closing the files on demand is a performance hit. My argument is that based on what the sysfs interface should be used for having a more ergonomic API takes precedence over performance.

BerndAmend commented 7 years ago

I briefly tested and measured two things. I compared the success rate of the kernel space implementation and the code from the last repository. Both failed at the same rate, even if all cores are utilized, therefore there is no reason to execute the code in kernel space.

I just briefly measured the performance difference, between the current rust implementation and an optimized rust version. The code can be found in my fork, the code was a quick hack to measure the difference. The performance difference is around 30x for set_value on a Raspberry PI 2.

This behavior should at least be documented and the efficient should be removed from the introduction. Other people might wrongfully assume that they get performance equally to other implementations using the same kernel interface.

nastevens commented 7 years ago

Looks like you've got a good start for PR in your branch. Why not clean up and submit that?

BerndAmend commented 7 years ago

The main reason, is that I currently see no way to fix it in a safe way without breaking the API. At the moment I settled for https://crates.io/crates/rppal and https://crates.io/crates/dht22_pi. Rppal lacks the interrupt feature, due to the usage of mmap (not possible using the sysfs api) it is dramatically (>100x) faster than the fastest sysfs GPIO implementation I wrote so far.

posborne commented 7 years ago

Yeah, although I tried not to make anything here unnecessarily slow, there are cases where things could be done more efficiently with the sysfs GPIO interface by leaving files open and the like to avoid the number of system calls that need to be made (in kernel space, everything should be pretty quick still). These probably would cause enough problems in other contexts that I wonder if we should even try to support all these cases with the same API.

@tptb Would you consider starting with a PR that adds notes on cases which may not perform well to the README? Feel free to include pointers to other libraries that may be better suited for cases like switching pin direction quickly or bit banging.