fivdi / pigpio

Fast GPIO, PWM, servo control, state change notification and interrupt handling with Node.js on the Raspberry Pi
MIT License
948 stars 89 forks source link

Running without daemon #81

Closed cinderblock closed 5 years ago

cinderblock commented 5 years ago

I recently noticed that when using node-pigpio, node starts listening on port 8888. This seems to be the pigpio daemon. I'm confused because I thought the whole point of this module was that it was binding to the native interfaces directly and completely bypassing pigpiod and the associated socket communication layer. Is this incorrect? How does node-pigpio work internally? Leaving a port open like that is a security concern to me. Sure I could use a firewall but I'd rather just get at the root cause.

fivdi commented 5 years ago

Internally the pigpio Node.js module uses the pigpio C Interface to control GPIOs. This means that this module binds to the native interfaces directly completely bypassing pigpiod and the associated socket communication layer. When the pigpio C interface is initialised by calling gpioInitialise the pigpio C interface creates a socket that listens on port 8888. This isn't actually the pigpiod deamon but the socket can be used to do what pigpiod does.

Currently this module does not provide functionality that can be used to prevent the pigpio C interface from creating the socket that listens on port 8888. It could be extended to provide this functionality.

cinderblock commented 5 years ago

Looking at the code a little, it seems that this listening on 8888 is entirely happening in the C? Can you confirm this?

As for an API, I was thinking of configuring the listen port to 0 could tell the C backend to not start the daemon. Alternatively, would adding an argument to gpioInitialise be easier?

PS, shouldn't it be gpioInitialize?

fivdi commented 5 years ago

Looking at the code a little, it seems that this listening on 8888 is entirely happening in the C? Can you confirm this?

Yes, this is correct.

As for an API, I was thinking of configuring the listen port to 0 could tell the C backend to not start the daemon. Alternatively, would adding an argument to gpioInitialise be easier?

The pigpio C interface has a C function called gpioCfgInterfaces which can be used to disable the socket interface. It would be best if the pigpio Node.js module provided access to this function.

PS, shouldn't it be gpioInitialize?

See here.

cinderblock commented 5 years ago

I'm looking into it. This seems pretty simple to implement in the most generic way: https://github.com/cinderblock/pigpio/tree/control-interfaces

I'm looking at making the PI_DISABLE_FIFO_IF and related flags exposed in JavaScript but not happy with the solutions I'm finding. I see that the library currently just replicates the constants in JavaScript

I also see that it looks like you once considered what I'm considering but have decided to not use it for ~some reason. Why is this code commented out?~ performance reasons. I'm surprised this was significant. Is there no way to make a constant value from C++ available in a performant way from JavaScript?

fivdi commented 5 years ago

The pigpio Node.js module has two tests for measuring performance, digital-read-performance.js and digital-write-performance.js. Among other things, these tests are used to make sure that modifications to the code don't have a negative impact on performance.

At some point in the past it was necessary to add a new function to target here. Although this new function had nothing to do with digital-read or digital-write, adding the new function to target resulted in the performance of digital-read and digital-write decreasing by approximately 50%. Removing all the constants from target resolved the performance issue. The number of variables added to target appears to have consequences for performance. I don't know why this is the case and perhaps it's not the case with newer versions of Node.js and the V8 JavaScript engine.

For consistency, I'd suggest adding the the constants that you wish to add in the same way that constants are currently added.

cinderblock commented 5 years ago

Hmmm, that's very interesting. I guessing the issue is that the object grew past some threshold and V8 switched to a different data model storage/access scheme. Maybe splitting the big flat object into hierarchical objects would help.

I'll add the constants as they're currently exposed.

Looking at the definitions however, should these constants be exposed as part of the Gpio object? I'm thinking they should be at a higher level... Have an API in mind here?

I'm thinking:

const pigpio = require('pigpio');

// Must be run before first call to `new Gpio()` ...
pigpio.cfgInterfaces(pigpio.PI_DISABLE_SOCK_IF | pigpio.PI_DISABLE_FIFO_IF);

const io = new pigpio.Gpio(1, {mode: pigpio.Gpio.OUTPUT});

I also like object based notations:

const pigpio = require('pigpio');

// Must be run before first call to `new Gpio()` ...
pigpio.cfgInterfaces({disableSock: true, disableFifo: true});

const io = new pigpio.Gpio(1, {mode: pigpio.Gpio.OUTPUT});

Thoughts?

fivdi commented 5 years ago

As you mention, the constants shouldn't be exposed as part of the Gpio object. At the bottom of pigpio.js there are functions called configureClock and configureSocketPort. The constants CLOCK_PWM and CLOCK_PCM which are also defined at the bottom of pigpio.js are used when calling configureClock. Something similar is needed for configuring interfaces.

I prefer the first variant you suggest above as it's consistent with configureClock which doesn't support object based notation for its arguments. I'd call the new function configureInterfaces rather than cfgInterfaces.

cinderblock commented 5 years ago

Any thoughts about this style of export?

Object.defineProperty(module.exports, 'DISABLE_FIFO_IF', { value: 1, writeable: false });
Object.defineProperty(module.exports, 'DISABLE_SOCK_IF', { value: 2, writeable: false });
Object.defineProperty(module.exports, 'LOCALHOST_SOCK_IF', { value: 4, writeable: false });
Object.defineProperty(module.exports, 'DISABLE_ALERT', { value: 8, writeable: false });

What if configureInterfaces supported both APIs?

cinderblock commented 5 years ago

@fivdi Thoughts on merging these changes?

fivdi commented 5 years ago

The changes look good to me. I'm afraid I'm no where near my development computer at the moment and won't be able to merge till the end of August. Hopefully this doesn't cause too much inconvenience.