Working epoll Interrupt listener with edge options

[julienvincent]

I have built a working epoll interrupt listener as a proposed fix to the watchFile. I have seen other pull requests and forks containing interrupt listeners, however they did not have edge options which doesn't allow you to setup how interrupts are generated for each pin. This fork fixes that.

[JamesBarwell]

Thanks for the code. This is looking very promising. I'd like to get the following sorted out in order to get this progressed into master and into a release:

1. get the tests passing again.
2. test this thoroughly on real hardware.
3. remove a lot of the noise in the branch (formatting changes, stuff auto-created by the editor).

Do you have plans to work on the tests at all? I could either pull this into a branch on the project where those can be worked on, or leave the pull request open if you plan to do further work. I'm happy to sort out the branch noise prior to merging.

Cheers.

[julienvincent]

Only a pleasure. I have been working on a project that involves the GPIO pins on the pi. This means I will be working with this a lot more and hardware tests go along with that. I just fixed a few bugs and did a fast test repair by removing all tests that involved the watchFile() function.

I plan on writing more tests for the additional functionality I added but that might be a while, if you want to revise the tests yourself then that would welcomed.

I haven't done proper hardware tests on the following:

• Unexporting
• Destroy()
• isExported

I need to fix the async poller as it tends to emit multiple interrupts before they get cleared. Also running the setup() method badly can cause multiple watchers for the same pin, not a huge problem but can use unnesesary system resources.

[julienvincent]

I have done some major changes on the listener. It is now an automatic process called in the async waterfall (this broke the working tests, haven't had time to fix) that does not result in change getting emitted multiple times.

Added a stopListening method which can be called to stop listening to individual channels. Also used internally in the destroy() method.

Done extensive hardware tests and it seems this is ready for release as soon as tests have been fixed and updated. If I can leave that to you, let me know.

I did clean up most of the editor noise.

cheers.

[JamesBarwell]

Hi, it's very hard for me to make promises about my time but I am happy to look at the tests when I do get a chance. It shouldn't require a huge effort to get things passing again if they're just incidentally calling the new interrupt code as a side-effect of their set up.

I'm happy in principle for some of the tests around the watcher/interrupt to be removed if they're failing; these tests don't seem to be helping anyway given the problems people have reported with this feature.

[julienvincent]

The only tests I removed where all relating to the watchFile which, in my fork, is completely replaced with an interrupt listener.

I have tried and can't get the tests to pass while the listen method gets called along with setup(), but by way of hardware it seems to work as expected.

[JamesBarwell]

I had a chance to look at the tests last night. It looks like a lot of the failures will be fixed by stubbing out the epoll module. I'm not sure if there's a way to avoid having to do that but it seems like the right direction to go in for now. Just wanted to provide an update to say that it is being looked at. Cheers.

[julienvincent]

Well the listener will no longer work without that module - so probably a good idea to find a another way

[JamesBarwell]

Only stubbing it for the tests - I'm avoiding any significant changes to the module code.

One other thing I've noticed is that the createListener function has more than one 'fd' variable. There's the one created by fs.openSync, and the other in the Epoll constructor callback. Looking at the example in the epoll documentation, the outer variable (assigned from fs.openSync) could just be renamed 'valuefd'. I think that's a safe change to make and it won't affect the bahaviour, but I'll hang on until I've got a clean branch with passing tests.

[julienvincent]

Oh alright that would work.

In the createListener only one fd is created but is used twice. Once by the poller to initiate it and then once inside the pollers callback by the interrupt clearer to reset the interrupts

[JamesBarwell]

I've pulled this into a branch "epoll" (which I've pushed), cleaned up the commit history, and now have fully passing tests and coverage reports. I've not made any changes to your module code nor added any tests. See: https://travis-ci.org/JamesBarwell/rpi-gpio.js/jobs/69715414

Is there anyone else following this who could give this a test on their Rasberry Pi hardware before this gets merged in? Any help would be much appreciated.

The last thing I'd like to do is try and get the test coverage back up. It doesn't have to be where it was, but I'd like to at least get some assertions around the new start and stop listening functions. I'll keep going with this in my branch and see where I can get. For reference, here's the old and new coverage reports:

master

Statements   : 98.52% ( 133/135 )
Branches     : 86.21% ( 50/58 )
Functions    : 100% ( 36/36 )
Lines        : 99.24% ( 130/131 )

epoll

Statements   : 90.76% ( 167/184 )
Branches     : 80.68% ( 71/88 )
Functions    : 86% ( 43/50 )
Lines        : 91.06% ( 163/179 )

[julienvincent]

That looks good. If I get time I will try help you with writing more tests and repairing the coverage.

As for tests - I have been using this module with another dev and we have done tests on Raspberry pi B+ and Raspberry pi 2. So far no errors. Though if someone else can confirm this on another system that would be appreciated.

I would still like to do some more work on this module to stop it requiring sudo permissions to run.

[JamesBarwell]

Hi, a quick update on this... I've done some further work and got a lot more test coverage around creating and removing listeners, the interrupts, and specifying edges.

I've had to make a change to the module code this time - to stopListening. I didn't like that it exposed both the channel and pin through the API. None of the other parts of the API deal with the pins directly, so it seemed confusing. I saw your note that it was only used internally, so I split the method apart. As I say I've got some tests on it now so I'm fairly confident that I haven't regressed the functionality, but thought I'd point it out.

=============================== Coverage summary ===============================
Statements   : 95.63% ( 175/183 )
Branches     : 87.8% ( 72/82 )
Functions    : 92.31% ( 48/52 )
Lines        : 96.07% ( 171/178 )
================================================================================

p.s. I'm not sure how to get Github to track the new branch in this pull request. Makes sense to keep discussion here though.

[julienvincent]

I took a look at your branch and am happy with that change. I ran a hardware test with the change to stopListening and seems to work as expected.

If you want to keep your work tracked on this PR I could give you write access to my fork and you can move your branch there. I can't think of any other way.

[JamesBarwell]

I've finally had a go to give this a test with the proper hardware, having dug out some LEDs and resistors! I'm pleased to say that this is working nicely here too, on a model B rev 2 running Arch.

I had a problem with the new listen function. It doesn't really work for me because during setup there's already a listener added, so it refuses to add another. Given the amount of bug reports popping up over the change events (thanks for helping!) I'm going to leave listen undocumented, get the code published, then clean it up in a future version.

[julienvincent]

I don't follow why listen doesn't work for you? can you post the error you are having? I would like to take a look. Is it maybe because you are using Arch? (I tested this on raspbian).

It will be nice to get this on master though!

[JamesBarwell]

I'm writing from one pin to another.

The more I look at the listen function the more it doesn't look quite right. Maybe I've misunderstood the intended behaviour but it looks like it will always call back with null and then won't fire during a pin value change, because there's no reference to the callback inside createListener. In my example below, you can see both of those plus it gets called a second time with the duplicate error (because a listener is already attached during setup).

It's possible I misunderstood and broke this one in my earlier patch so sorry if that's me!

var gpio = require('rpi-gpio');
var async = require('async');

var writePin = 7 
var readPin = 11

gpio.on('change', function(channel, value) {
    console.log('Channel ' + channel + ' value is now ' + value);
});

async.waterfall([
    function(next) {
        gpio.setup(writePin, gpio.DIR_OUT, next)
    },  
    function(next) {
        gpio.setup(readPin, gpio.DIR_IN, gpio.EDGE_BOTH, next)
    },  
    function(next) {
        gpio.listen(readPin, function() {
            console.log('listener', arguments)
        });
        next();
    },  
    function(next) {
        gpio.write(writePin, 1, next);
    },  
    function(next) {
        setTimeout(next, 1000)
    },  
    function(next) {
        setTimeout(gpio.destroy(next), 1000)
    }   
], function() {
    process.exit(0)
})

DEBUG=* node multipin.js 
  rpi-gpio seen hardware revision 14; using pin mode v2 +0ms
  rpi-gpio set up pin 4 +183ms
  rpi-gpio export pin 4 +42ms
  rpi-gpio set edge NONE on pin 4 +35ms
  rpi-gpio set direction OUT on pin 4 +28ms
  rpi-gpio listen for pin 4 +11ms
  rpi-gpio set up pin 17 +12ms
  rpi-gpio export pin 17 +6ms
  rpi-gpio set edge BOTH on pin 17 +17ms
  rpi-gpio set direction IN on pin 17 +6ms
  rpi-gpio listen for pin 17 +13ms
  rpi-gpio listen for pin 17 +8ms
listener { '0': null }
listener { '0': [Error: Already watching that pin!] }
  rpi-gpio failed to read value after a change on channel 11 +122ms
Channel 11 value is now true
  rpi-gpio failed to read value after a change on channel 11 +8ms
Channel 11 value is now true
  rpi-gpio unexport pin 4 +1s
  rpi-gpio unexport pin 17 +4ms

[julienvincent]

Alright so listen gets called when you call setup. If you call it manually on a pin that has already has a listener on it, then it should not add another and it should callback with a error. It looks to me like it is creating a second listener anyway, and then when a change happens - both attempt to fire but only the first complete without an error.

POLLERS.forEach(function (map) {
    if (map.pin == pin) {
        return process.nextTick(function () {
            cb(new Error('Already watching that pin!'));
        });
    }
});

This is originally what I wrote - but should instead look something like this:

POLLERS.forEach(function (map) {
    if (map.pin == pin) {
        return process.nextTick(function () {
            cb(new Error('Already watching that pin!'));
        });
    } else {
        createListener(channel, pin, function (readChannel) {

            _this.read(readChannel, function (err, value) {
                debug(
                    'failed to read value after a change on channel %d',
                    readChannel
                );
                _this.emit('change', readChannel, value);
            });
        });
        return cb(null)
    }
});

[JamesBarwell]

It's merged at last! Thanks very much for your efforts and patience with this one.

I think I was misunderstanding the way listen worked in my previous comment/code. But I'm still unsure what the use case is, given that the pins are auto-listened during setup anyway. I can see that it might have some use to re-attach listeners after stopListeners is called. But if that's the only use-case (?) then I think it's simpler that events are always emitted and, rather than switch event-emission on/off through the module API, people have switching in their own 'change' handler. Hope that makes sense.

[julienvincent]

awesome! It was a nice project to work on :)

The listen is mostly just for internal use. I agree that there isn't really any proper use for it, though I had it there just incase someone found a use for it. It can either be tweaked to have a more obvious use or just removed from documentation.

Thanks for working on the tests though! That must have been a lot of work.

p.s. Can probably close all the issues related to the watchers now.