Performance Improvement when writing

[alessandromrc]

Before changes:

After changes:

By adding compiler optimisations and changing a little the way the library writes the GPIO state we achieved some performances easily

Measurements done on an Analog Discovery 3.

[sombriks]

That's great, i'll merge and perform a release tomorrow. Thank you so much for your contribution!

[alessandromrc]

That's great, i'll merge and perform a release tomorrow. Thank you so much for your contribution!

No worries! If I can find something else to improve I will give a look, also here's the performances on C:

Tested with the following code:

#include <gpiod.h>
#include <error.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

struct gpiod_chip *chip;
struct gpiod_line_request_config config;
struct gpiod_line_bulk lines;

int main(int argc, char *argv[]) {
    unsigned int offsets[1];
    int values[1];
    int err;

    chip = gpiod_chip_open("/dev/gpiochip4");
    if (!chip) {
        perror("gpiod_chip_open");
        goto cleanup;
    }

    // set pin 21 to 1 (logic high)
    offsets[0] = 17;
    values[0] = 0;

    err = gpiod_chip_get_lines(chip, offsets, 1, &lines);
    if (err) {
        perror("gpiod_chip_get_lines");
        goto cleanup;
    }

    memset(&config, 0, sizeof(config));
    config.consumer = "blink";
    config.request_type = GPIOD_LINE_REQUEST_DIRECTION_OUTPUT;
    config.flags = 0;

    // get the bulk lines setting default value to 0
    err = gpiod_line_request_bulk(&lines, &config, values);
    if (err) {
        perror("gpiod_line_request_bulk");
        goto cleanup;
    }

    // Loop continuously to toggle the LED on and off
    while (1) {
        // output value 1 to turn on the LED
        values[0] = 1;
        err = gpiod_line_set_value_bulk(&lines, values);
        if (err) {
            perror("gpiod_line_set_value_bulk");
            goto cleanup;
        }

        // output value 0 to turn off the LED
        values[0] = 0;
        err = gpiod_line_set_value_bulk(&lines, values);
        if (err) {
            perror("gpiod_line_set_value_bulk");
            goto cleanup;
        }
    }

cleanup:
    gpiod_line_release_bulk(&lines);
    gpiod_chip_close(chip);

    return EXIT_SUCCESS;
}

I guess NaN is also giving some overhead...

[alessandromrc]

At the current state we can do this:

[alessandromrc]

I guess this is the best we can do at the moment which is surely not bad compared to C..

[dmitrydvorkin]

54aa0d6

are you sure? obj->line can be null

[alessandromrc]

54aa0d6

are you sure? obj->line can be null

I am well aware of that, didn't test the case tho.

[splitice]

Thats not too bad at all really. But I still think its worth throwing a check for null line in before calling setValue, it shouldnt cost much. I havent checked the compiler output but it should be a single instruction on most architectures (some variation of JZ) the impact of which will be minimal.

if (!obj->line || gpiod_line_set_value(obj->line, value) == -1) {

Unless of course you have verified the safety of calling that function with NULL.

[sombriks]

@alessandromrc the performance gains are remarkable,.

the null check however would be valuable for people with different purposes.
for example, there is something keeping node-libgpiod to be properly handled by node-red, i don't remember if it was even converted into a ticket but situations involving memory safety my pop out in the future.

would be nice to restore the null check before the merge, or i can merge now and restore the check before making a release so everyone else can benefit from the improvements you wrote!

[alessandromrc]

@alessandromrc the performance gains are remarkable,.

the null check however would be valuable for people with different purposes. for example, there is something keeping node-libgpiod to be properly handled by node-red, i don't remember if it was even converted into a ticket but situations involving memory safety my pop out in the future.

would be nice to restore the null check before the merge, or i can merge now and restore the check before making a release so everyone else can benefit from the improvements you wrote!

I will check right now with the value being null and see what happens, otherwise I will think of something to check it being null without adding much overhead.

[alessandromrc]

Just tested with null and it's the same behaviour as before, null in C++ is just 0...

can be tested with this code here:

const { version, Chip, Line } = require("node-libgpiod");

global.chip = new Chip(4);
global.line = new Line(chip, 17); // led on GPIO17
let count = 10;

console.log(version());
line.requestOutputMode();


let value_out = false;

while (true)
{
  value_out = 1
  line.setValue(null);
  value_out = 0
  line.setValue(null);
}

NAN_METHOD(Line::setValue) {
  Line *obj = ObjectWrap::Unwrap<Line>(info.Holder());
  v8::Local<v8::Context> context = Nan::GetCurrentContext();
  uint32_t value = info[0]->Uint32Value(context).FromJust();
  std::cout << value << std::endl;
  if (gpiod_line_set_value(obj->line, value) == -1) {
    return Nan::ThrowError("setValue() failed.");
  }
}

Result: 0

Running tests:

alessandro@raspberrypi5:~/nodetests/node-libgpiod $ npm run test

> node-libgpiod@0.4.0 test
> mocha



  libgpiod miscellaneous bindings
1.6.3
    ✔ should get libgpiod version
    - should get line instant value
    - should NOT get line instant value due wrong chip name
    - should blink line with instant value

  libgpiod chip bindings
    - should 'create' a new chip by number
    - should 'create' a new chip by name
    - should 'create' a new chip by path
    ✔ should NOT 'create' a chip because it does not exists

  libgpiod line bindings
    - should get a line from the chip
    - should NOT get a nonexistent line from the chip
    - should set line value
    - should get line value
    - should blink line value

  libgpiod Pin sugar
    - should create a Pin fo line 10


  2 passing (10ms)
  12 pending

[alessandromrc]

Like mentioned before C++ compilers generally see null as a nullptr or as 0, in either cases it will be handled as 0, can also be seen here: https://en.cppreference.com/w/cpp/types/NULL

[alessandromrc]

For the fun of it I've tried directly writing directly to the RP1 chip and oh my the performances...

[sombriks]

@alessandromrc is it ok if i merge now?

[alessandromrc]

@alessandromrc is it ok if i merge now?

To me everything seems to work fine, if anyone else wants to test out the changes (maybe even outside the Raspberry world, that's up to them / you).

[sombriks]

people can test in v0.4.1 release 😆

[alessandromrc]

people can test in v0.4.1 release 😆

Let's hope for the best 😃