bindings: implement python bindings for libgpiod v2

This adds a python wrapper around libgpiosim and a set of test cases for the v2 API using python's standard unittest module.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 bindings/python/tests/Makefile.am             |  14 +
 bindings/python/tests/cases/__init__.py       |  12 +
 bindings/python/tests/cases/tests_chip.py     | 157 +++++++
 .../python/tests/cases/tests_chip_info.py     |  59 +++
 .../python/tests/cases/tests_edge_event.py    | 279 +++++++++++
 .../python/tests/cases/tests_info_event.py    | 135 ++++++
 .../python/tests/cases/tests_line_config.py   | 254 ++++++++++
 .../python/tests/cases/tests_line_info.py     |  90 ++++
 .../python/tests/cases/tests_line_request.py  | 345 ++++++++++++++
 bindings/python/tests/cases/tests_misc.py     |  53 +++
 .../tests/cases/tests_request_config.py       |  77 ++++
 bindings/python/tests/gpiod_py_test.py        |  25 +
 bindings/python/tests/gpiosimmodule.c         | 434 ++++++++++++++++++
 13 files changed, 1934 insertions(+)
 create mode 100644 bindings/python/tests/Makefile.am
 create mode 100644 bindings/python/tests/cases/__init__.py
 create mode 100644 bindings/python/tests/cases/tests_chip.py
 create mode 100644 bindings/python/tests/cases/tests_chip_info.py
 create mode 100644 bindings/python/tests/cases/tests_edge_event.py
 create mode 100644 bindings/python/tests/cases/tests_info_event.py
 create mode 100644 bindings/python/tests/cases/tests_line_config.py
 create mode 100644 bindings/python/tests/cases/tests_line_info.py
 create mode 100644 bindings/python/tests/cases/tests_line_request.py
 create mode 100644 bindings/python/tests/cases/tests_misc.py
 create mode 100644 bindings/python/tests/cases/tests_request_config.py
 create mode 100755 bindings/python/tests/gpiod_py_test.py
 create mode 100644 bindings/python/tests/gpiosimmodule.c

bindings/python/tests/Makefile.am new file +0 -0

new file mode 100644
index 0000000..099574f

bindings/python/tests/Makefile.am new file +11 -0

+dist_bin_SCRIPTS = gpiod_py_test.py
+
+pyexec_LTLIBRARIES = gpiosim.la
+
+gpiosim_la_SOURCES = gpiosimmodule.c
+gpiosim_la_CFLAGS = -I$(top_srcdir)/tests/gpiosim/
+gpiosim_la_CFLAGS += -Wall -Wextra -g -std=gnu89 $(PYTHON_CPPFLAGS)
+gpiosim_la_LDFLAGS = -module -avoid-version
+gpiosim_la_LIBADD = $(top_builddir)/tests/gpiosim/libgpiosim.la
+gpiosim_la_LIBADD += $(top_builddir)/bindings/python/enum/libpycenum.la
+gpiosim_la_LIBADD += $(PYTHON_LIBS)

bindings/python/tests/cases/__init__.py new file +12 -0

new file mode 100644
index 0000000..6503663
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <[email protected]>
+
+from .tests_chip import *
+from .tests_chip_info import *
+from .tests_edge_event import *
+from .tests_info_event import *
+from .tests_line_config import *
+from .tests_line_info import *
+from .tests_line_request import *
+from .tests_misc import *
+from .tests_request_config import *

bindings/python/tests/cases/tests_chip.py new file +0 -0

new file mode 100644
index 0000000..844dbfc

bindings/python/tests/cases/tests_chip.py new file +77 -0

+class WaitingForEdgeEvents(unittest.TestCase):
+    def setUp(self):
+        self.sim = gpiosim.Chip(num_lines=8)
+        self.thread = None
+
+    def tearDown(self):
+        if self.thread:
+            self.thread.join()
+        self.sim = None
+
+    def trigger_falling_and_rising_edge(self, offset):
+        time.sleep(0.05)
+        self.sim.set_pull(offset, Pull.PULL_UP)
+        time.sleep(0.05)
+        self.sim.set_pull(offset, Pull.PULL_DOWN)
+
+    def trigger_rising_edge_events_on_two_offsets(self, offset0, offset1):
+        time.sleep(0.05)
+        self.sim.set_pull(offset0, Pull.PULL_UP)
+        time.sleep(0.05)
+        self.sim.set_pull(offset1, Pull.PULL_UP)
+
+    def test_both_edge_events(self):
+        with gpiod.request_lines(
+            self.sim.dev_path,
+            gpiod.RequestConfig(offsets=[2]),
+            gpiod.LineConfig(edge_detection=Edge.BOTH),
+        ) as req:
+            buf = gpiod.EdgeEventBuffer()
+            self.thread = threading.Thread(
+                target=partial(self.trigger_falling_and_rising_edge, 2)
+            )
+            self.thread.start()
+
+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.RISING_EDGE)
+            self.assertEqual(event.line_offset, 2)
+            ts_rising = event.timestamp_ns
+
+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.FALLING_EDGE)
+            self.assertEqual(event.line_offset, 2)
+            ts_falling = event.timestamp_ns
+
+            self.assertGreater(ts_falling, ts_rising)
+
+    def test_rising_edge_event(self):
+        with gpiod.request_lines(
+            self.sim.dev_path,
+            gpiod.RequestConfig(offsets=[6]),
+            gpiod.LineConfig(edge_detection=Edge.RISING),
+        ) as req:
+            buf = gpiod.EdgeEventBuffer()
+            self.thread = threading.Thread(
+                target=partial(self.trigger_falling_and_rising_edge, 6)
+            )
+            self.thread.start()
+
+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.RISING_EDGE)
+            self.assertEqual(event.line_offset, 6)
+
+            self.assertFalse(
+                req.wait_edge_event(datetime.timedelta(microseconds=10000))
+            )
+
+    def test_falling_edge_event(self):
+        with gpiod.request_lines(

bindings/python/tests/cases/tests_chip.py new file +42 -0

+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.FALLING_EDGE)
+            self.assertEqual(event.line_offset, 6)
+
+            self.assertFalse(
+                req.wait_edge_event(datetime.timedelta(microseconds=10000))
+            )
+
+    def test_sequence_numbers(self):
+        with gpiod.request_lines(
+            self.sim.dev_path,
+            gpiod.RequestConfig(offsets=[2, 4]),
+            gpiod.LineConfig(edge_detection=Edge.BOTH),
+        ) as req:
+            buf = gpiod.EdgeEventBuffer()
+            self.thread = threading.Thread(
+                target=partial(self.trigger_rising_edge_events_on_two_offsets, 2, 4)
+            )
+            self.thread.start()
+
+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.RISING_EDGE)
+            self.assertEqual(event.line_offset, 2)
+            self.assertEqual(event.global_seqno, 1)
+            self.assertEqual(event.line_seqno, 1)
+
+            self.assertTrue(req.wait_edge_event(datetime.timedelta(seconds=1)))
+            self.assertEqual(req.read_edge_event(buf), 1)
+            self.assertEqual(len(buf), 1)
+            event = buf[0]
+            self.assertEqual(event.type, EventType.RISING_EDGE)
+            self.assertEqual(event.line_offset, 4)
+            self.assertEqual(event.global_seqno, 2)
+            self.assertEqual(event.line_seqno, 1)
+
+

+++ b/bindings/python/tests/cases/tests_line_request.py

bindings/python/tests/cases/tests_chip.py new file +25 -0

@@ -0,0 +1,345 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <[email protected]>
+
+import errno
+import gpiod
+import gpiosim
+import unittest
+
+
+Direction = gpiod.Line.Direction
+Edge = gpiod.Line.Edge
+Bias = gpiod.Line.Bias
+Value = gpiod.Line.Value
+SimVal = gpiosim.Chip.Value
+Pull = gpiosim.Chip.Pull
+
+
+class LineRequestConstructor(unittest.TestCase):
+    def test_line_request_cannot_be_instantiated(self):
+        with self.assertRaises(TypeError):
+            info = gpiod.LineRequest()
+
+
+class ChipLineRequestWorks(unittest.TestCase):
+    def test_chip_line_request(self):

bindings/python/tests/cases/tests_chip.py new file +7 -0

+    def test_module_line_request(self):
+        sim = gpiosim.Chip()
+
+        with gpiod.request_lines(
+            sim.dev_path, gpiod.RequestConfig(offsets=[0]), gpiod.LineConfig()
+        ) as req:
+            pass

bindings/python/tests/cases/tests_chip.py new file +13 -0

+    def test_module_line_request_direction(self):
+        sim = gpiosim.Chip(num_lines=2)
+
+        with gpiod.request_lines(
+            sim.dev_path, lines=[0, 1], direction=Direction.OUTPUT
+        ) as req:
+            with gpiod.Chip(sim.dev_path) as chip:
+                info = chip.get_line_info(0)
+                self.assertEqual(info.direction, Direction.OUTPUT)
+                self.assertTrue(info.used)
+
+    def test_module_line_request_edge_detection(self):
+        sim = gpiosim.Chip()

bindings/python/tests/cases/tests_chip.py new file +34 -0

+    def setUp(self):
+        self.sim = gpiosim.Chip(num_lines=8)
+        self.chip = gpiod.Chip(self.sim.dev_path)
+
+    def tearDown(self):
+        self.chip.close()
+        self.chip = None
+        self.sim = None
+
+    def test_passing_invalid_types_as_configs(self):
+        with self.assertRaises(TypeError):
+            self.chip.request_lines("foobar", gpiod.LineConfig())
+
+        with self.assertRaises(TypeError):
+            self.chip.request_lines(gpiod.RequestConfig(offsets=[0]), "foobar")
+
+    def test_no_offsets(self):
+        with self.assertRaises(ValueError):
+            self.chip.request_lines(gpiod.RequestConfig(), gpiod.LineConfig())
+
+    def test_duplicate_offsets(self):
+        with self.assertRaises(OSError) as ex:
+            self.chip.request_lines(
+                gpiod.RequestConfig(offsets=[2, 5, 1, 7, 5]), gpiod.LineConfig()
+            )
+
+        self.assertEqual(ex.exception.errno, errno.EBUSY)
+
+    def test_offset_out_of_range(self):
+        with self.assertRaises(ValueError):
+            self.chip.request_lines(
+                gpiod.RequestConfig(offsets=[1, 0, 4, 8]), gpiod.LineConfig()
+            )
+

Quoted 39 lines · Bartosz Golaszewski

This adds a python wrapper around libgpiosim and a set of test cases for the v2 API using python's standard unittest module.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 bindings/python/tests/Makefile.am             |  14 +
 bindings/python/tests/cases/__init__.py       |  12 +
 bindings/python/tests/cases/tests_chip.py     | 157 +++++++
 .../python/tests/cases/tests_chip_info.py     |  59 +++
 .../python/tests/cases/tests_edge_event.py    | 279 +++++++++++
 .../python/tests/cases/tests_info_event.py    | 135 ++++++
 .../python/tests/cases/tests_line_config.py   | 254 ++++++++++
 .../python/tests/cases/tests_line_info.py     |  90 ++++
 .../python/tests/cases/tests_line_request.py  | 345 ++++++++++++++
 bindings/python/tests/cases/tests_misc.py     |  53 +++
 .../tests/cases/tests_request_config.py       |  77 ++++
 bindings/python/tests/gpiod_py_test.py        |  25 +
 bindings/python/tests/gpiosimmodule.c         | 434 ++++++++++++++++++
 13 files changed, 1934 insertions(+)
 create mode 100644 bindings/python/tests/Makefile.am
 create mode 100644 bindings/python/tests/cases/__init__.py
 create mode 100644 bindings/python/tests/cases/tests_chip.py
 create mode 100644 bindings/python/tests/cases/tests_chip_info.py
 create mode 100644 bindings/python/tests/cases/tests_edge_event.py
 create mode 100644 bindings/python/tests/cases/tests_info_event.py
 create mode 100644 bindings/python/tests/cases/tests_line_config.py
 create mode 100644 bindings/python/tests/cases/tests_line_info.py
 create mode 100644 bindings/python/tests/cases/tests_line_request.py
 create mode 100644 bindings/python/tests/cases/tests_misc.py
 create mode 100644 bindings/python/tests/cases/tests_request_config.py
 create mode 100755 bindings/python/tests/gpiod_py_test.py
 create mode 100644 bindings/python/tests/gpiosimmodule.c

bindings/python/tests/Makefile.am new file +0 -0

new file mode 100644
index 0000000..099574f

bindings/python/tests/Makefile.am line 3

Quoted 3 lines

bindings/python/tests/Makefile.am new file +3 -0

@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+

It is 2022?

Which email address are you going with? gmail here and bgdev below.

Quoted 9 lines

bindings/python/tests/Makefile.am new file +9 -0

+
+    def test_falling_edge_event(self):
+        with gpiod.request_lines(
+            self.sim.dev_path,
+            gpiod.RequestConfig(offsets=[6]),
+            gpiod.LineConfig(edge_detection=Edge.FALLING),
+        ) as req:
+            buf = gpiod.EdgeEventBuffer()
+            self.thread = threading.Thread(

bindings/python/tests/Makefile.am

Quoted 4 lines

bindings/python/tests/Makefile.am new file +4 -0

+                target=partial(self.trigger_falling_and_rising_edge, 6)
+            )
+            self.thread.start()
+

Benefit of the thread? (and elsewhere a background thread is used) The sleeps therein are only necessary because it is run in the background.

Quoted 18 lines

bindings/python/tests/Makefile.am new file +18 -0

+    def test_module_line_request_direction(self):
+        sim = gpiosim.Chip(num_lines=2)
+
+        with gpiod.request_lines(
+            sim.dev_path, lines=[0, 1], direction=Direction.OUTPUT
+        ) as req:
+            with gpiod.Chip(sim.dev_path) as chip:
+                info = chip.get_line_info(0)
+                self.assertEqual(info.direction, Direction.OUTPUT)
+                self.assertTrue(info.used)
+
+    def test_module_line_request_edge_detection(self):
+        sim = gpiosim.Chip()
+
+        with gpiod.request_lines(
+            sim.dev_path, lines=[0], edge_detection=Edge.BOTH
+        ) as req:
+            sim.set_pull(0, Pull.PULL_UP)

bindings/python/tests/Makefile.am

Quoted 5 lines

bindings/python/tests/Makefile.am new file +5 -0

+            self.assertTrue(req.wait_edge_event())
+            self.assertEqual(req.read_edge_event()[0].line_offset, 0)
+
+
+class RequestingLinesFailsWithInvalidArguments(unittest.TestCase):

These tests should be in tests_chip.py, as they are testing the Chip.request_lines() method.

And they should have module level equivalents (don't assume one wraps the other).

Quoted 34 lines

bindings/python/tests/Makefile.am new file +34 -0

+    def setUp(self):
+        self.sim = gpiosim.Chip(num_lines=8)
+        self.chip = gpiod.Chip(self.sim.dev_path)
+
+    def tearDown(self):
+        self.chip.close()
+        self.chip = None
+        self.sim = None
+
+    def test_passing_invalid_types_as_configs(self):
+        with self.assertRaises(TypeError):
+            self.chip.request_lines("foobar", gpiod.LineConfig())
+
+        with self.assertRaises(TypeError):
+            self.chip.request_lines(gpiod.RequestConfig(offsets=[0]), "foobar")
+
+    def test_no_offsets(self):
+        with self.assertRaises(ValueError):
+            self.chip.request_lines(gpiod.RequestConfig(), gpiod.LineConfig())
+
+    def test_duplicate_offsets(self):
+        with self.assertRaises(OSError) as ex:
+            self.chip.request_lines(
+                gpiod.RequestConfig(offsets=[2, 5, 1, 7, 5]), gpiod.LineConfig()
+            )
+
+        self.assertEqual(ex.exception.errno, errno.EBUSY)
+
+    def test_offset_out_of_range(self):
+        with self.assertRaises(ValueError):
+            self.chip.request_lines(
+                gpiod.RequestConfig(offsets=[1, 0, 4, 8]), gpiod.LineConfig()
+            )
+

[snip]

bindings/python/tests/Makefile.am line 3

Quoted 5 lines

+++ b/bindings/python/tests/cases/tests_misc.py

bindings/python/tests/Makefile.am new file +3 -0

@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <[email protected]>
+

The tests in this file are all module scope, and cover functions from module.c, so rename to tests_module.py.

Quoted 51 lines

bindings/python/tests/Makefile.am new file +50 -0

+import gpiod
+import gpiosim
+import os
+import re
+import unittest
+
+
+class LinkGuard:
+    def __init__(self, src, dst):
+        self.src = src
+        self.dst = dst
+
+    def __enter__(self):
+        os.symlink(self.src, self.dst)
+
+    def __exit__(self, type, val, tb):
+        os.unlink(self.dst)
+
+
+class IsGPIOChip(unittest.TestCase):
+    def test_is_gpiochip_bad(self):
+        self.assertFalse(gpiod.is_gpiochip_device("/dev/null"))
+        self.assertFalse(gpiod.is_gpiochip_device("/dev/nonexistent"))
+
+    def test_is_gpiochip_good(self):
+        sim = gpiosim.Chip()
+
+        self.assertTrue(gpiod.is_gpiochip_device(sim.dev_path))
+
+    def test_is_gpiochip_link_good(self):
+        link = "/tmp/gpiod-py-test-link.{}".format(os.getpid())
+        sim = gpiosim.Chip()
+
+        with LinkGuard(sim.dev_path, link):
+            self.assertTrue(gpiod.is_gpiochip_device(link))
+
+    def test_is_gpiochip_link_bad(self):
+        link = "/tmp/gpiod-py-test-link.{}".format(os.getpid())
+
+        with LinkGuard("/dev/null", link):
+            self.assertFalse(gpiod.is_gpiochip_device(link))
+
+
+class VersionString(unittest.TestCase):
+    def test_version_string(self):
+        self.assertTrue(
+            re.match(
+                "^[0-9][1-9]?\\.[0-9][1-9]?([\\.0-9]?|\\-devel)$", gpiod.__version__
+            )
+        )

bindings/python/tests/cases/tests_request_config.py +0 -0

[snip]

A complete audit of the test coverage would be beneficial.
I haven't attempted that - only pointed out any gaps I happened to notice. Are there any coverage tools available for Python C modules?

Quoted 47 lines · Kent Gibson

Quoted 39 lines · Bartosz Golaszewski

This adds a python wrapper around libgpiosim and a set of test cases for the v2 API using python's standard unittest module.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 bindings/python/tests/Makefile.am             |  14 +
 bindings/python/tests/cases/__init__.py       |  12 +
 bindings/python/tests/cases/tests_chip.py     | 157 +++++++
 .../python/tests/cases/tests_chip_info.py     |  59 +++
 .../python/tests/cases/tests_edge_event.py    | 279 +++++++++++
 .../python/tests/cases/tests_info_event.py    | 135 ++++++
 .../python/tests/cases/tests_line_config.py   | 254 ++++++++++
 .../python/tests/cases/tests_line_info.py     |  90 ++++
 .../python/tests/cases/tests_line_request.py  | 345 ++++++++++++++
 bindings/python/tests/cases/tests_misc.py     |  53 +++
 .../tests/cases/tests_request_config.py       |  77 ++++
 bindings/python/tests/gpiod_py_test.py        |  25 +
 bindings/python/tests/gpiosimmodule.c         | 434 ++++++++++++++++++
 13 files changed, 1934 insertions(+)
 create mode 100644 bindings/python/tests/Makefile.am
 create mode 100644 bindings/python/tests/cases/__init__.py
 create mode 100644 bindings/python/tests/cases/tests_chip.py
 create mode 100644 bindings/python/tests/cases/tests_chip_info.py
 create mode 100644 bindings/python/tests/cases/tests_edge_event.py
 create mode 100644 bindings/python/tests/cases/tests_info_event.py
 create mode 100644 bindings/python/tests/cases/tests_line_config.py
 create mode 100644 bindings/python/tests/cases/tests_line_info.py
 create mode 100644 bindings/python/tests/cases/tests_line_request.py
 create mode 100644 bindings/python/tests/cases/tests_misc.py
 create mode 100644 bindings/python/tests/cases/tests_request_config.py
 create mode 100755 bindings/python/tests/gpiod_py_test.py
 create mode 100644 bindings/python/tests/gpiosimmodule.c

bindings/python/tests/Makefile.am new file +0 -0

new file mode 100644
index 0000000..099574f

bindings/python/tests/Makefile.am line 3

Quoted 3 lines

bindings/python/tests/Makefile.am new file +3 -0

@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
+

It is 2022?

Quoted 2 lines

Which email address are you going with? gmail here and bgdev below.

These patches will be squashed together anyway. When I wrote this part I used this email and then switched to [email protected]. It's just copyright anyway. I can fix it up later.

[snip!]

Quoted 9 lines

Quoted 9 lines

bindings/python/tests/Makefile.am new file +9 -0

+
+    def test_falling_edge_event(self):
+        with gpiod.request_lines(
+            self.sim.dev_path,
+            gpiod.RequestConfig(offsets=[6]),
+            gpiod.LineConfig(edge_detection=Edge.FALLING),
+        ) as req:
+            buf = gpiod.EdgeEventBuffer()
+            self.thread = threading.Thread(

Quoted 9 lines

bindings/python/tests/Makefile.am

Quoted 4 lines

bindings/python/tests/Makefile.am new file +4 -0

+                target=partial(self.trigger_falling_and_rising_edge, 6)
+            )
+            self.thread.start()
+

Benefit of the thread? (and elsewhere a background thread is used) The sleeps therein are only necessary because it is run in the background.

Just to make it similar to real-life applications. I did the same for C++ and C. And no: if I triggered multiple events without any sleeps in between, then some of them would risk not being registered. You can try it for yourself with gpiosim. It happens because when the kernel irq_work is busy adding an interrupt, new ones get ignored.

[nsip]

Quoted 4 lines

These tests should be in tests_chip.py, as they are testing the Chip.request_lines() method.

I would argue that there's some overlap in where the test cases should live. For instance - if we moved the line watching out of tests_info_event into tests_chip then not much would be left. I would leave these here as they test the general idea of requesting lines rather than the functionality of class LineRequest. Same for the module level line requests.

This is the implementation of the new python API for libgpiod v2.

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

bindings/python/module.c line 280

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 26 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

bindings/python/module.c line 280

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Quoted 4 lines

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

bindings/python/module.c line 280

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

bindings/python/module.c line 280

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

bindings/python/module.c line 280

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

"The optional timeout argument specifies a time-out as a floating point number in seconds. When the timeout argument is omitted the function blocks until at least one file descriptor is ready. A time-out value of zero specifies a poll and never blocks."

Cheers,
Kent.

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Quoted 73 lines · Bartosz Golaszewski

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

Quoted 7 lines

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

That is what I said - you get timeout=None if the kwarg is not specified.

Quoted 4 lines

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

Quoted 4 lines

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Cos you are forcing the user to create a timedelta, which is a PITA, and both time.sleep and select.select (i.e. standard Python modules) do it that way. The float is the Pythonic way.

Quoted 94 lines · Kent Gibson

Quoted 73 lines · Bartosz Golaszewski

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

Quoted 7 lines

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

That is what I said - you get timeout=None if the kwarg is not specified.

Quoted 8 lines

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Quoted 4 lines

Cos you are forcing the user to create a timedelta, which is a PITA, and both time.sleep and select.select (i.e. standard Python modules) do it that way. The float is the Pythonic way.

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Quoted 100 lines · Bartosz Golaszewski

Quoted 98 lines · Kent Gibson

Quoted 73 lines · Bartosz Golaszewski

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

Quoted 7 lines

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

That is what I said - you get timeout=None if the kwarg is not specified.

Quoted 8 lines

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Cos you are forcing the user to create a timedelta, which is a PITA, and both time.sleep and select.select (i.e. standard Python modules) do it that way. The float is the Pythonic way.

Quoted 4 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Quoted 101 lines · Kent Gibson

Quoted 100 lines · Bartosz Golaszewski

Quoted 98 lines · Kent Gibson

Quoted 73 lines · Bartosz Golaszewski

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

Quoted 7 lines

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

That is what I said - you get timeout=None if the kwarg is not specified.

Quoted 8 lines

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Cos you are forcing the user to create a timedelta, which is a PITA, and both time.sleep and select.select (i.e. standard Python modules) do it that way. The float is the Pythonic way.

Quoted 9 lines

Quoted 4 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Same issue for chip.wait_info_event(), btw.
Still working through a full review - but it'll probably take a while.

Wrt the wait, does the C API have a blocking wait, or do you have to poll() the fd?

And can you add a description of the timeout=0 behaviour to gpiod_chip_wait_info_event() etc, as 0 is sometimes taken as block.

Quoted 113 lines · Kent Gibson

Quoted 101 lines · Kent Gibson

Quoted 100 lines · Bartosz Golaszewski

Quoted 98 lines · Kent Gibson

Quoted 73 lines · Bartosz Golaszewski

Quoted 56 lines · Kent Gibson

Quoted 54 lines · Kent Gibson

Quoted 33 lines · Kent Gibson

Quoted 32 lines · Bartosz Golaszewski

Quoted 30 lines · Kent Gibson

Quoted 2 lines · Bartosz Golaszewski

This is the implementation of the new python API for libgpiod v2.

[snip]

Review

Quoted 10 lines

(quoted diff) +10 -0

+     }
+
+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

Building against python 3.9 (the min required by configure.ac) gives:

module.c:276:7: warning: implicit declaration of function ‘Py_IsNone’; did you mean ‘Py_None’? [-Wimplicit-function-declaration]

  276 |  if (!Py_IsNone(res)) {
      |       ^~~~~~~~~
      |       Py_None

Py_IsNone didn't get added to the Stable ABI until 3.10.

Cheers,
Kent.

Quoted 7 lines

Quoted 4 lines

It seems like most distros still ship python 3.9, I don't want to make 3.10 the requirement. This can be replaced by if (res != Py_None). Are there any more build issues?

No, that was the only one.

But I am seeing a test failure:

$ sudo bindings/python/tests/gpiod_py_test.py
.............................................................................F................................
======================================================================
FAIL: test_module_line_request_edge_detection (cases.tests_line_request.ModuleLineRequestWorks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dev/libgpiod/bindings/python/tests/cases/tests_line_request.py", line 71, in test_module_line_request_edge_detection
    self.assertTrue(req.wait_edge_event())
AssertionError: False is not true

Quoted 8 lines

Quoted 5 lines

----------------------------------------------------------------------
Ran 110 tests in 2.652s

FAILED (failures=1)

The req.wait_edge_event() does not wait without a timeout parameter, which is a bit nonintuitive, so the test has a race.

Ah, makes sense.

Quoted 4 lines

Adding a timeout=datetime.timedelta(microseconds=1) (the shortest possible) works for me, so anything that triggers a context switch is probably sufficient, though a longer timeout probably wouldn't hurt.

Quoted 7 lines

I'll change that.

Quoted 2 lines

The Python API should take timeout=NONE to mean wait indefinitely, and 0 as a poll.

This makes sense but I'd still want to have some default behavior for when timeout is not given. Maybe wait indefinitely?

That is what I said - you get timeout=None if the kwarg is not specified.

Quoted 8 lines

Quoted 3 lines

And it should take the timeout as a float, not a
timedelta, as per select.select. From its doc:

I don't necessarily want to mirror select's interface. Why would we prefer a float over a class that's the standard python interface for storing time deltas?

Cos you are forcing the user to create a timedelta, which is a PITA, and both time.sleep and select.select (i.e. standard Python modules) do it that way. The float is the Pythonic way.

Quoted 9 lines

Quoted 4 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Quoted 6 lines

Same issue for chip.wait_info_event(), btw.
Still working through a full review - but it'll probably take a while.

Wrt the wait, does the C API have a blocking wait, or do you have to poll() the fd?

Blocking wait is simply reading the event without checking if an event is there to be read. select() (the system call) waits indefinitely if the timeval struct is NULL, ppoll() behaves the same for a NULL timespec, poll() does the same for a negative timeout (which is an int). We take an uint64_t so we can't do it. Either we need to switch to int64_t and interpret a negative value as indefinite wait or just not do it at all and tell users to just call the (blocking) read_edge_event().

Bart

Quoted 1 line · Bartosz Golaszewski

Quoted 27 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Quoted 6 lines

Same issue for chip.wait_info_event(), btw.
Still working through a full review - but it'll probably take a while.

Wrt the wait, does the C API have a blocking wait, or do you have to poll() the fd?

Blocking wait is simply reading the event without checking if an event is there to be read. select() (the system call) waits indefinitely if the timeval struct is NULL, ppoll() behaves the same for a NULL timespec, poll() does the same for a negative timeout (which is an int). We take an uint64_t so we can't do it. Either we need to switch to int64_t and interpret a negative value as indefinite wait or just not do it at all and tell users to just call the (blocking) read_edge_event().

Quoted 2 lines

Bart

I'm still on the fence about using timespec. It seems that the more recent linux interfaces avoid timespec and timeval altogether due to the year 2038 problem and the subsequent change in the struct layout. On the other hand the timeouts are unlikely to be set to years. :)

What do you think?

Quoted 2 lines · Bartosz Golaszewski

Quoted 1 line · Bartosz Golaszewski

....

Quoted 30 lines

Quoted 27 lines

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Quoted 6 lines

Same issue for chip.wait_info_event(), btw.
Still working through a full review - but it'll probably take a while.

Wrt the wait, does the C API have a blocking wait, or do you have to poll() the fd?

Blocking wait is simply reading the event without checking if an event is there to be read. select() (the system call) waits indefinitely if the timeval struct is NULL, ppoll() behaves the same for a NULL timespec, poll() does the same for a negative timeout (which is an int). We take an uint64_t so we can't do it. Either we need to switch to int64_t and interpret a negative value as indefinite wait or just not do it at all and tell users to just call the (blocking) read_edge_event().

Quoted 2 lines

Bart

Quoted 7 lines

I'm still on the fence about using timespec. It seems that the more recent linux interfaces avoid timespec and timeval altogether due to the year 2038 problem and the subsequent change in the struct layout. On the other hand the timeouts are unlikely to be set to years. :)

What do you think?

I prefer not using timespecs. I prefer the int64 microseconds/nanoseconds or float seconds approaches.
Especially for the time scales we are concerned with.
I only use timespecs where existing APIs such as ppoll() require it.

For the C API I'd go with int64 nsec, for Python float secs. (though as already covered - with Python you could accept float secs, int nsec, or a timedelta all in the one parameter)

Quoted 1 line · Kent Gibson

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Timedelta constructor is much more explicit than a float IMO. How about a compromise and taking both (mutually exclusive)? timeout=datettime.timedelta(seconds=1) == timeout_sec=float(1.0)?

Maybe, but float seconds seems to be the way they do it.
If you insist on both then just the one timeout parameter and work the type out on the fly. (it's Python, so dynamic typing...)

Quoted 6 lines

Same issue for chip.wait_info_event(), btw.
Still working through a full review - but it'll probably take a while.

Wrt the wait, does the C API have a blocking wait, or do you have to poll() the fd?

Blocking wait is simply reading the event without checking if an event is there to be read. select() (the system call) waits indefinitely if the timeval struct is NULL, ppoll() behaves the same for a NULL timespec, poll() does the same for a negative timeout (which is an int). We take an uint64_t so we can't do it. Either we need to switch to int64_t and interpret a negative value as indefinite wait or just not do it at all and tell users to just call the (blocking) read_edge_event().

This is the implementation of the new python API for libgpiod v2.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 bindings/python/.gitignore          |    1 +
 bindings/python/Makefile.am         |   40 +
 bindings/python/chip-info.c         |  126 +++
 bindings/python/chip.c              |  606 ++++++++++++
 bindings/python/edge-event-buffer.c |  330 +++++++
 bindings/python/edge-event.c        |  191 ++++
 bindings/python/exception.c         |  182 ++++
 bindings/python/info-event.c        |  175 ++++
 bindings/python/line-config.c       | 1373 +++++++++++++++++++++++++++
 bindings/python/line-info.c         |  286 ++++++
 bindings/python/line-request.c      |  803 ++++++++++++++++
 bindings/python/line.c              |  239 +++++
 bindings/python/module.c            |  557 +++++++++++
 bindings/python/module.h            |   58 ++
 bindings/python/request-config.c    |  320 +++++++
 configure.ac                        |    3 +-
 16 files changed, 5289 insertions(+), 1 deletion(-)
 create mode 100644 bindings/python/.gitignore
 create mode 100644 bindings/python/Makefile.am
 create mode 100644 bindings/python/chip-info.c
 create mode 100644 bindings/python/chip.c
 create mode 100644 bindings/python/edge-event-buffer.c
 create mode 100644 bindings/python/edge-event.c
 create mode 100644 bindings/python/exception.c
 create mode 100644 bindings/python/info-event.c
 create mode 100644 bindings/python/line-config.c
 create mode 100644 bindings/python/line-info.c
 create mode 100644 bindings/python/line-request.c
 create mode 100644 bindings/python/line.c
 create mode 100644 bindings/python/module.c
 create mode 100644 bindings/python/module.h
 create mode 100644 bindings/python/request-config.c

bindings/python/.gitignore new file +1 -0

new file mode 100644
index 0000000..bee8a64
@@ -0,0 +1 @@
+__pycache__

bindings/python/Makefile.am new file +0 -0

new file mode 100644
index 0000000..3f7ee5f

bindings/python/Makefile.am new file +37 -0

+pyexec_LTLIBRARIES = gpiod.la
+
+gpiod_la_SOURCES = \
+	chip.c \
+	chip-info.c \
+	edge-event.c \
+	edge-event-buffer.c \
+	exception.c \
+	info-event.c \
+	line.c \
+	line-config.c \
+	line-info.c \
+	line-request.c \
+	module.c \
+	module.h \
+	request-config.c
+
+gpiod_la_CFLAGS = -I$(top_srcdir)/include/
+gpiod_la_CFLAGS += -Wall -Wextra -g -std=gnu89 $(PYTHON_CPPFLAGS)
+gpiod_la_CFLAGS += -include $(top_builddir)/config.h
+gpiod_la_LDFLAGS = -module -avoid-version
+gpiod_la_LIBADD = $(top_builddir)/lib/libgpiod.la $(PYTHON_LIBS)
+gpiod_la_LIBADD += $(top_builddir)/bindings/python/enum/libpycenum.la
+
+SUBDIRS = enum .
+
+if WITH_TESTS
+
+SUBDIRS += tests
+
+endif
+
+if WITH_EXAMPLES
+
+SUBDIRS += examples
+
+endif

bindings/python/chip-info.c new file +9 -0

new file mode 100644
index 0000000..e48cf74
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+// SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <[email protected]>
+
+#include "module.h"
+
+typedef struct {
+	PyObject_HEAD;
+	struct gpiod_chip_info *info;
+} chip_info_object;

bindings/python/chip-info.c new file +65 -0

+static void chip_info_finalize(chip_info_object *self)
+{
+	if (self->info)
+		gpiod_chip_info_free(self->info);
+}
+
+PyDoc_STRVAR(chip_info_name_doc,
+"Name of the chip as represented in the kernel.");
+
+static PyObject *chip_info_name(chip_info_object *self,
+				void *Py_UNUSED(ignored))
+{
+	return PyUnicode_FromString(gpiod_chip_info_get_name(self->info));
+}
+
+PyDoc_STRVAR(chip_info_label_doc,
+"Label of the chip as represented in the kernel.");
+
+static PyObject *chip_info_label(chip_info_object *self,
+				 void *Py_UNUSED(ignored))
+{
+	return PyUnicode_FromString(gpiod_chip_info_get_label(self->info));
+}
+
+PyDoc_STRVAR(chip_info_num_lines_doc,
+"Number of GPIO lines exposed by the chip.");
+
+static PyObject *chip_info_num_lines(chip_info_object *self,
+				     void *Py_UNUSED(ignored))
+{
+	return PyLong_FromUnsignedLong(
+			gpiod_chip_info_get_num_lines(self->info));
+}
+
+static PyGetSetDef chip_info_getset[] = {
+	{
+		.name = "name",
+		.get = (getter)chip_info_name,
+		.doc = chip_info_name_doc,
+	},
+	{
+		.name = "label",
+		.get = (getter)chip_info_label,
+		.doc = chip_info_label_doc,
+	},
+	{
+		.name = "num_lines",
+		.get = (getter)chip_info_num_lines,
+		.doc = chip_info_num_lines_doc
+	},
+	{ }
+};
+
+static PyObject *chip_info_str(PyObject *self)
+{
+	PyObject *name, *label, *num_lines, *str = NULL;
+
+	name = PyObject_GetAttrString(self, "name");
+	label = PyObject_GetAttrString(self, "label");
+	num_lines = PyObject_GetAttrString(self, "num_lines");
+	if (!name || !label || !num_lines)
+		goto out;
+
+	str = PyUnicode_FromFormat("<gpiod.ChipInfo name=\"%S\" label=\"%S\" num_lines=%S>",
+				   name, label, num_lines);

bindings/python/chip-info.c new file +19 -0

+static PyGetSetDef edge_event_getset[] = {
+	{
+		.name = "type",
+		.get = (getter)edge_event_get_type,
+		.doc = edge_event_get_type_doc,
+	},
+	{
+		.name = "timestamp_ns",
+		.get = (getter)edge_event_timestamp_ns,
+		.doc = edge_event_timestamp_ns_doc,
+	},
+	{
+		.name = "line_offset",
+		.get = (getter)edge_event_line_offset,
+		.doc = edge_event_line_offset_doc,
+	},
+	{
+		.name = "global_seqno",
+		.get = (getter)edge_event_global_seqno,

bindings/python/chip-info.c new file +6 -0

+static const struct exception_desc exceptions[] = {
+	{
+		.name = "ChipClosedError",
+		.base = "Exception",
+		.doc = "Error raised when an already closed chip is used.",
+	},

bindings/python/chip-info.c new file +10 -0

+
+static PyGetSetDef info_event_getset[] = {
+	{
+		.name = "type",
+		.get = (getter)info_event_get_type,
+		.doc = info_event_get_type_doc,
+	},
+	{
+		.name = "timestamp_ns",
+		.get = (getter)info_event_timestamp_ns,

bindings/python/chip-info.c new file +11 -0

+PyDoc_STRVAR(line_config_set_props_default_doc,
+"Set the defaults for properties.\n"
+"\n"
+"Args:\n"
+"  direction:\n"
+"    Default direction.\n"
+"  edge_detection:\n"
+"    Default edge detection.\n"
+"  bias:\n"
+"    Default bias.\n"
+"  drive:\n"

bindings/python/chip-info.c new file +10 -0

+				PyMem_Free(values);
+				PyMem_Free(offsets);
+				return NULL;
+			}
+		}
+	}
+
+	Py_BEGIN_ALLOW_THREADS;
+	ret = gpiod_line_request_get_values_subset(self->request, num_values,
+						   offsets, values);

bindings/python/chip-info.c new file +18 -0

+static const PyCEnum_EnumDef line_enums[] = {
+	{
+		.name = "Value",
+		.values = value_enum_vals
+	},
+	{
+		.name = "Direction",
+		.values = direction_enum_vals
+	},
+	{
+		.name = "Bias",
+		.values = bias_enum_vals
+	},
+	{
+		.name = "Drive",
+		.values = drive_enum_vals
+	},
+	{

bindings/python/chip-info.c new file +29 -0

+static PyObject *
+make_line_cfg_kwargs(PyObject *direction, PyObject *edge_detection,
+		     PyObject *bias, PyObject *drive, PyObject *active_low,
+		     PyObject *debounce_period, PyObject *event_clock,
+		     PyObject *output_value, PyObject *output_values)
+{
+	static const char *const keys[] = {
+		"direction",
+		"edge_detection",
+		"bias",
+		"drive",
+		"active_low",
+		"debounce_period",
+		"event_clock",
+		"output_value",
+		"output_values",
+	};
+
+	PyObject *kwargs, *vals[9];
+	int ret, i;
+
+	vals[0] = direction;
+	vals[1] = edge_detection;
+	vals[2] = bias;
+	vals[3] = drive;
+	vals[4] = active_low;
+	vals[5] = debounce_period;
+	vals[6] = event_clock;
+	vals[7] = output_value;

bindings/python/chip-info.c new file +13 -0

+		if (!vals[i])
+			continue;
+
+		ret = PyDict_SetItemString(kwargs, keys[i], vals[i]);
+		if (ret) {
+			Py_DECREF(kwargs);
+			return NULL;
+		}
+	}
+
+	return kwargs;
+}
+

bindings/python/chip-info.c new file +15 -0

+	Py_DECREF(res);
+
+	return line_cfg;
+}
+
+static PyObject *
+module_request_lines(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+	static char *kwlist[] = {
+		"path",
+		"req_cfg",
+		"line_cfg",
+		"lines",
+		"direction",
+		"edge_detection",

bindings/python/chip-info.c new file +20 -0

+	PyObject *path, *req_cfg = NULL, *line_cfg = NULL, *lines = NULL,
+		 *direction = NULL, *edge_detection = NULL, *bias = NULL,
+		 *drive = NULL, *active_low = NULL, *debounce_period = NULL,
+		 *event_clock = NULL, *output_value = NULL,
+		 *output_values = NULL, *dict, *chip, *req, *line_cfg_kwargs;
+	int ret;
+
+	ret = PyArg_ParseTupleAndKeywords(args, kwargs, "O|OO$OOOOOOOOOO",
+					  kwlist, &path, &req_cfg, &line_cfg,
+					  &lines, &direction, &edge_detection,
+					  &bias, &drive, &active_low,
+					  &debounce_period, &event_clock,
+					  &output_value, &output_values);
+	if (!ret)
+		return NULL;
+
+	dict = PyModule_GetDict(self);
+	if (!dict)
+		return NULL;
+

configure.ac +1 -0

 		[AC_MSG_ERROR([python3 not found - needed for python bindings])])
 	AC_CHECK_PROG([has_python_config], [python3-config], [true], [false])
 	if test "x$has_python_config" = xfalse

@@ -243,6 +243,7 @@ AC_CONFIG_FILES([Makefile
 		 bindings/cxx/examples/Makefile
 		 bindings/cxx/tests/Makefile
 		 bindings/python/Makefile
+		 bindings/python/enum/Makefile
 		 bindings/python/examples/Makefile
 		 bindings/python/tests/Makefile
 		 man/Makefile])

Signature · 1 line

2.34.1

bindings/python/chip.c line 352

Quoted 11 lines

bindings/python/chip.c new file +11 -0

+PyDoc_STRVAR(chip_get_line_offset_from_name_doc,
+"Map a line's name to its offset within the chip.\n"
+"\n"
+"Args:\n"
+"  name:\n"
+"    Name of the GPIO line to map.\n"
+"\n"
+"Returns:\n"
+"  Line offset corresponding with the name or None if a line with given name\n"
+"  is not exposed by this chip.");
+

It should throw if the name search fails.

If name is already an int then just return the int. (to allow the method to be used as a mapping function on a mixed list.) Though ironically the name isn't the best then. Perhaps just get_line_offset() or map_line_offset()?

[snip]

Quoted 19 lines

bindings/python/edge-event.c new file +19 -0

+static PyGetSetDef edge_event_getset[] = {
+     {
+             .name = "type",
+             .get = (getter)edge_event_get_type,
+             .doc = edge_event_get_type_doc,
+     },
+     {
+             .name = "timestamp_ns",
+             .get = (getter)edge_event_timestamp_ns,
+             .doc = edge_event_timestamp_ns_doc,
+     },
+     {
+             .name = "line_offset",
+             .get = (getter)edge_event_line_offset,
+             .doc = edge_event_line_offset_doc,
+     },
+     {
+             .name = "global_seqno",
+             .get = (getter)edge_event_global_seqno,

bindings/python/edge-event.c line 126

Quoted 10 lines

bindings/python/edge-event.c new file +10 -0

+             .doc = edge_event_global_seqno_doc,
+     },
+     {
+             .name = "line_seqno",
+             .get = (getter)edge_event_line_seqno,
+             .doc = edge_event_line_seqno_doc,
+     },
+     { }
+};
+

Provide a helper to convert the timestamp_ns into a time.datetime. (for event_clock_realtime)

[snip]

Quoted 6 lines

bindings/python/exception.c new file +6 -0

+static const struct exception_desc exceptions[] = {
+     {
+             .name = "ChipClosedError",
+             .base = "Exception",
+             .doc = "Error raised when an already closed chip is used.",
+     },

bindings/python/exception.c line 30

Quoted 10 lines

bindings/python/exception.c new file +10 -0

+     {
+             .name = "RequestReleasedError",
+             .base = "Exception",
+             .doc = "Error raised when a released request is used.",
+     },
+     {
+             .name = "BadMappingError",
+             .base = "Exception",
+             .doc = "Exception thrown when the core C library returns an invalid value for any of the line properties.",
+     },

Name is too vague - a bad mapping could mean anything - including its own name ;-).
How about "UnknownPropertyValueError"? "unknown" rather than "invalid" as the likely cause is an updated C library.
Or even just a ValueError might work.

[snip]

Quoted 17 lines

bindings/python/info-event.c new file +17 -0

+
+static PyGetSetDef info_event_getset[] = {
+     {
+             .name = "type",
+             .get = (getter)info_event_get_type,
+             .doc = info_event_get_type_doc,
+     },
+     {
+             .name = "timestamp_ns",
+             .get = (getter)info_event_timestamp_ns,
+             .doc = info_event_timestamp_ns_doc,
+     },
+     {
+             .name = "line_info",
+             .get = (getter)info_event_line_info,
+             .doc = info_event_line_info_doc,
+     },

bindings/python/chip-info.c line 72

Quoted 3 lines

bindings/python/chip-info.c new file +3 -0

+     { }
+};
+

Provide a helper to convert timestamp_ns to time.datetime. This one is a bit trickier as the kernel only ever provides monotonic clock, so need to perform the monotonic -> realtime conversion. (for reference my proposed gpiowatch tool does this)

[snip]

Quoted 11 lines

bindings/python/line-config.c new file +11 -0

+PyDoc_STRVAR(line_config_set_props_default_doc,
+"Set the defaults for properties.\n"
+"\n"
+"Args:\n"
+"  direction:\n"
+"    Default direction.\n"
+"  edge_detection:\n"
+"    Default edge detection.\n"
+"  bias:\n"
+"    Default bias.\n"
+"  drive:\n"

bindings/python/line-config.c line 353

Quoted 10 lines

bindings/python/line-config.c new file +10 -0

+"    Default drive.\n"
+"  active_low:\n"
+"    Default active-low setting.\n"
+"  debounce_period:\n"
+"    Default debounce period.\n"
+"  event_clock:\n"
+"    Default event clock.\n"
+"  output_value:\n"
+"    Default output value.");
+

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

[snip]

bindings/python/line-config.c line 549

Quoted 12 lines

bindings/python/line-config.c new file +12 -0

+PyDoc_STRVAR(line_config_get_props_default_doc,
+"Get default values for a set of line properties.\n"
+"\n"
+"Args:\n"
+"  properties:\n"
+"    List of properties (gpiod.LineConfig.Property) for which to get default\n"
+"    values.\n"
+"\n"
+"Returns:\n"
+"  List of default values for properties specified in the argument list and\n"
+"  in the same order");
+

As per the set, consider merging the _default and _offset forms by adding an offset kwarg.

[snip]

bindings/python/line-info.c line 255

Quoted 3 lines

bindings/python/line-info.c new file +3 -0

+PyDoc_STRVAR(line_info_type_doc,
+"Line info object contains an immutable snapshot of a line's status.");
+

Either "LineInfo" or "Immutable object containing..." as you use elsewhere (I'd go with the latter for consistency).

[snip]

bindings/python/line-request.c line 280

Quoted 11 lines

bindings/python/line-request.c new file +11 -0

+     } else {
+             for (i = 0; i < num_values; i++) {
+                     offset = PyList_GetItem(offsets_obj, i);
+                     if (!offset) {
+                             PyMem_Free(values);
+                             PyMem_Free(offsets);
+                             return NULL;
+                     }
+
+                     offsets[i] = Py_gpiod_PyLongAsUnsignedInt(offset);
+                     if (PyErr_Occurred()){

^ missing whitespace.

Quoted 10 lines

bindings/python/line-request.c new file +10 -0

+                             PyMem_Free(values);
+                             PyMem_Free(offsets);
+                             return NULL;
+                     }
+             }
+     }
+
+     Py_BEGIN_ALLOW_THREADS;
+     ret = gpiod_line_request_get_values_subset(self->request, num_values,
+                                                offsets, values);

[snip]

Quoted 18 lines

bindings/python/line.c new file +18 -0

+static const PyCEnum_EnumDef line_enums[] = {
+     {
+             .name = "Value",
+             .values = value_enum_vals
+     },
+     {
+             .name = "Direction",
+             .values = direction_enum_vals
+     },
+     {
+             .name = "Bias",
+             .values = bias_enum_vals
+     },
+     {
+             .name = "Drive",
+             .values = drive_enum_vals
+     },
+     {

bindings/python/line.c line 134

Quoted 10 lines

bindings/python/line.c new file +10 -0

+             .name = "Edge",
+             .values = edge_enum_vals
+     },
+     {
+             .name = "Clock",
+             .values = event_clock_enum_vals
+     },
+     { }
+};
+

Clock -> EventClock here and elsewhere

[snip]

Quoted 29 lines

bindings/python/module.c new file +29 -0

+static PyObject *
+make_line_cfg_kwargs(PyObject *direction, PyObject *edge_detection,
+                  PyObject *bias, PyObject *drive, PyObject *active_low,
+                  PyObject *debounce_period, PyObject *event_clock,
+                  PyObject *output_value, PyObject *output_values)
+{
+     static const char *const keys[] = {
+             "direction",
+             "edge_detection",
+             "bias",
+             "drive",
+             "active_low",
+             "debounce_period",
+             "event_clock",
+             "output_value",
+             "output_values",
+     };
+
+     PyObject *kwargs, *vals[9];
+     int ret, i;
+
+     vals[0] = direction;
+     vals[1] = edge_detection;
+     vals[2] = bias;
+     vals[3] = drive;
+     vals[4] = active_low;
+     vals[5] = debounce_period;
+     vals[6] = event_clock;
+     vals[7] = output_value;

bindings/python/module.c line 237

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     vals[8] = output_values;
+
+     if (memcmp(vals, "\0\0\0\0\0\0\0\0\0", 9) == 0)
+             return NULL;
+
+     kwargs = PyDict_New();
+     if (!kwargs)
+             return NULL;
+
+     for (i = 0; i < 9; i ++) {

^ extra whitespace

Quoted 13 lines

bindings/python/module.c new file +13 -0

+             if (!vals[i])
+                     continue;
+
+             ret = PyDict_SetItemString(kwargs, keys[i], vals[i]);
+             if (ret) {
+                     Py_DECREF(kwargs);
+                     return NULL;
+             }
+     }
+
+     return kwargs;
+}
+

[snip]

bindings/python/module.c line 280

Quoted 8 lines

bindings/python/module.c new file +8 -0

+     res = PyObject_Call(method, args, line_cfg_kwargs);
+     Py_DECREF(args);
+     Py_DECREF(method);
+     if (!Py_IsNone(res)) {
+             Py_DECREF(res);
+             return NULL;
+     }
+

As mentioned in a separate mail, Py_IsNone() requires Python 3.10, while the configure.ac allows 3.9.

Quoted 22 lines

bindings/python/module.c new file +22 -0

+     Py_DECREF(res);
+
+     return line_cfg;
+}
+
+static PyObject *
+module_request_lines(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+     static char *kwlist[] = {
+             "path",
+             "req_cfg",
+             "line_cfg",
+             "lines",
+             "direction",
+             "edge_detection",
+             "bias",
+             "drive",
+             "active_low",
+             "debounce_period",
+             "event_clock",
+             "output_value",
+             "output_values",

bindings/python/chip.c line 19

Quoted 3 lines

bindings/python/chip.c new file +3 -0

+             NULL
+     };
+

My suggestion to provide a lines parameter here was actually a poor one, given the LineConfig only deals with offsets - which is totally reasonable as supporting line names in LineConfig would be complicated.
I would prefer the two to be consistent, and so use offsets.

If get_line_offset_from_name() was better for mapping (in the Python sense) then you could just use a list comprehension to convert a list of names/offsets into a list of offsets to pass in here.

So I would change lines to offsets here and make get_line_offset_from_name() more useful for mapping (more on that where it is defined).

Quoted 20 lines

bindings/python/module.c new file +20 -0

+     PyObject *path, *req_cfg = NULL, *line_cfg = NULL, *lines = NULL,
+              *direction = NULL, *edge_detection = NULL, *bias = NULL,
+              *drive = NULL, *active_low = NULL, *debounce_period = NULL,
+              *event_clock = NULL, *output_value = NULL,
+              *output_values = NULL, *dict, *chip, *req, *line_cfg_kwargs;
+     int ret;
+
+     ret = PyArg_ParseTupleAndKeywords(args, kwargs, "O|OO$OOOOOOOOOO",
+                                       kwlist, &path, &req_cfg, &line_cfg,
+                                       &lines, &direction, &edge_detection,
+                                       &bias, &drive, &active_low,
+                                       &debounce_period, &event_clock,
+                                       &output_value, &output_values);
+     if (!ret)
+             return NULL;
+
+     dict = PyModule_GetDict(self);
+     if (!dict)
+             return NULL;
+

bindings/python/module.c line 335

Quoted 10 lines

bindings/python/module.c new file +10 -0

+     chip = make_chip(dict, path);
+     if (!chip)
+             return NULL;
+
+     req_cfg = make_req_cfg(dict, chip, req_cfg, lines);
+     if (!req_cfg) {
+             close_chip(chip);
+             return NULL;
+     }
+

What if lines is None or empty?

A failed name -> offset mapping in make_req_cfg() and set_lines() results in a returned NULL here? Shouldn't it provide a meaningful error or throw an exception?
Change to passing in offsets and this problem goes away.

[snip]

configure.ac AM_CONDITIONAL([WITH_BINDINGS_PYTHON], [test "x$with_bindings_python" = xtrue]) · line 201

Quoted 8 lines

configure.ac +1 -1

@@ -198,7 +198,7 @@ AM_CONDITIONAL([WITH_BINDINGS_PYTHON], [test "x$with_bindings_python" = xtrue])
 
 if test "x$with_bindings_python" = xtrue
 then
-     AM_PATH_PYTHON([3.0], [],
+     AM_PATH_PYTHON([3.9], [],

Given this requirement, make sure it compiles with Python 3.9.

Quoted 12 lines

configure.ac +1 -1

              [AC_MSG_ERROR([python3 not found - needed for python bindings])])
      AC_CHECK_PROG([has_python_config], [python3-config], [true], [false])
      if test "x$has_python_config" = xfalse

@@ -243,6 +243,7 @@ AC_CONFIG_FILES([Makefile
               bindings/cxx/examples/Makefile
               bindings/cxx/tests/Makefile
               bindings/python/Makefile
+              bindings/python/enum/Makefile
               bindings/python/examples/Makefile
               bindings/python/tests/Makefile
               man/Makefile])
--

Quoted 2 lines

2.34.1

Nothing major really.
I would personally like to have a slightly higher level of abstraction, but given you are going for a minimalist wrapper around libgpiod, it seems totally reasonable.

Cheers,
Kent.

Quoted 1 line · Kent Gibson

[snip]

Quoted 14 lines

bindings/python/chip.c line 352

Quoted 11 lines

bindings/python/chip.c new file +11 -0

+PyDoc_STRVAR(chip_get_line_offset_from_name_doc,
+"Map a line's name to its offset within the chip.\n"
+"\n"
+"Args:\n"
+"  name:\n"
+"    Name of the GPIO line to map.\n"
+"\n"
+"Returns:\n"
+"  Line offset corresponding with the name or None if a line with given name\n"
+"  is not exposed by this chip.");
+

It should throw if the name search fails.

Quoted 5 lines

If name is already an int then just return the int. (to allow the method to be used as a mapping function on a mixed list.) Though ironically the name isn't the best then. Perhaps just get_line_offset() or map_line_offset()?

Do you think we should change the C function name to gpiod_chip_map_line_offset() too? And possibly make it parse strings representing integers as well?

Quoted 7 lines

[snip]

Provide a helper to convert timestamp_ns to time.datetime. This one is a bit trickier as the kernel only ever provides monotonic clock, so need to perform the monotonic -> realtime conversion. (for reference my proposed gpiowatch tool does this)

Should this be put into libgpiod C API directly maybe?

Quoted 29 lines

[snip]

Quoted 11 lines

bindings/python/line-config.c new file +11 -0

+PyDoc_STRVAR(line_config_set_props_default_doc,
+"Set the defaults for properties.\n"
+"\n"
+"Args:\n"
+"  direction:\n"
+"    Default direction.\n"
+"  edge_detection:\n"
+"    Default edge detection.\n"
+"  bias:\n"
+"    Default bias.\n"
+"  drive:\n"

bindings/python/line-config.c line 353

Quoted 10 lines

bindings/python/line-config.c new file +10 -0

+"    Default drive.\n"
+"  active_low:\n"
+"    Default active-low setting.\n"
+"  debounce_period:\n"
+"    Default debounce period.\n"
+"  event_clock:\n"
+"    Default event clock.\n"
+"  output_value:\n"
+"    Default output value.");
+

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Quoted 7 lines

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

What would that look like (in python code) if I wanted to request 5 lines and use the same config for them?

Quoted 19 lines

[snip]

Quoted 7 lines

bindings/python/module.c new file +7 -0

+     static char *kwlist[] = {
+             "path",
+             "req_cfg",
+             "line_cfg",
+             "lines",
+             "direction",
+             "edge_detection",

bindings/python/module.c line 305

Quoted 10 lines

bindings/python/module.c new file +10 -0

+             "bias",
+             "drive",
+             "active_low",
+             "debounce_period",
+             "event_clock",
+             "output_value",
+             "output_values",
+             NULL
+     };
+

Quoted 5 lines

My suggestion to provide a lines parameter here was actually a poor one, given the LineConfig only deals with offsets - which is totally reasonable as supporting line names in LineConfig would be complicated.
I would prefer the two to be consistent, and so use offsets.

I disagree. In the module-wide request function we have the chip already, we can map the names to offsets. It makes perfect sense to do it implicitly here as a pythonic shorthand for opening the chip manually and requesting lines separately. This function already got improved a lot in my v3.

Quoted 1 line · Bartosz Golaszewski

On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <[email protected]> wrote:

Quoted 15 lines

Quoted 7 lines

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Quoted 7 lines

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

Quoted 3 lines

What would that look like (in python code) if I wanted to request 5 lines and use the same config for them?

That is the trivial case - you use the module level gpiod.request_lines() as is and pass in the config parameters and list of lines you want.

req = gpiod.request_lines(chip="gpiochip0", offsets=[1,2,3,4,5],

                          direction="output", values=[1,0,1,0,0])

The more complicated case is where the lines config differs. Then you have to build the LineConfig by adding the config for each set of lines in a separate call to set_props().
Then you provide that LineConfig to the request_lines(), along with the set of lines.

lc.set_props(offsets=[1,2,3], direction="input") lc.set_props(offsets=[4,5], direction="output", values=[1,0]) req = gpiod.request_lines(chip="gpiochip0", line_cfg=lc)

(simplified examples using stringified prop values etc - hope you get the idea)

Building that on top of the C API, you would determine the "default" config based on the most common attribute values, then override the config for the lines that differ from that default.
That is the internal complexity I mentioned.

Quoted 25 lines

Quoted 19 lines

[snip]

Quoted 7 lines

bindings/python/module.c new file +7 -0

+     static char *kwlist[] = {
+             "path",
+             "req_cfg",
+             "line_cfg",
+             "lines",
+             "direction",
+             "edge_detection",

bindings/python/module.c line 305

Quoted 10 lines

bindings/python/module.c new file +10 -0

+             "bias",
+             "drive",
+             "active_low",
+             "debounce_period",
+             "event_clock",
+             "output_value",
+             "output_values",
+             NULL
+     };
+

Quoted 5 lines

My suggestion to provide a lines parameter here was actually a poor one, given the LineConfig only deals with offsets - which is totally reasonable as supporting line names in LineConfig would be complicated.
I would prefer the two to be consistent, and so use offsets.

Quoted 6 lines

I disagree. In the module-wide request function we have the chip already, we can map the names to offsets. It makes perfect sense to do it implicitly here as a pythonic shorthand for opening the chip manually and requesting lines separately. This function already got improved a lot in my v3.

Yeah, good point - the caller of the module level function won't have a Chip object to do the mapping. And forcing them to create one defeats the purpose of having the module level function in the first place.

Cheers,
Kent.

Quoted 3 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <[email protected]> wrote:

[snip]

Quoted 19 lines

Quoted 15 lines

Quoted 7 lines

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Quoted 7 lines

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

Quoted 3 lines

What would that look like (in python code) if I wanted to request 5 lines and use the same config for them?

Quoted 7 lines

That is the trivial case - you use the module level gpiod.request_lines() as is and pass in the config parameters and list of lines you want.

req = gpiod.request_lines(chip="gpiochip0", offsets=[1,2,3,4,5],

                          direction="output", values=[1,0,1,0,0])

This is close to what I have now in my v3 branch. Except that values is called output_values and takes a dictionary like its counterpart in LineConfig but that can be extended to interpreting a list as providing the values for corresponding offsets/lines. Current version of request_lines() takes all LineConfig options and uses them as the defaults.

Quoted 10 lines

The more complicated case is where the lines config differs. Then you have to build the LineConfig by adding the config for each set of lines in a separate call to set_props().
Then you provide that LineConfig to the request_lines(), along with the set of lines.

lc.set_props(offsets=[1,2,3], direction="input") lc.set_props(offsets=[4,5], direction="output", values=[1,0]) req = gpiod.request_lines(chip="gpiochip0", line_cfg=lc)

Quoted 8 lines

(simplified examples using stringified prop values etc - hope you get the idea)

Building that on top of the C API, you would determine the "default" config based on the most common attribute values, then override the config for the lines that differ from that default.
That is the internal complexity I mentioned.

Internal complexity is fine - it's the implicitness of the defaults that make me not like this idea. I think we discussed something similar for the C API and I was against it too. Your examples are fine but the defaults for lines not mentioned in set_props() would be filled by a freshly created LineConfig with its well defined default values. In other words I prefer to keep the override mechanism visible in python but unification of the property setters is something I will consider.

To me it should look like:

lc.set_props(direction=Direction.INPUT, edge_detection=Edge.BOTH) sets the defaults
lc.set_props(offset=4, direction=Direction.OUTPUT) sets a single override lc.set_props(offsets=[5, 1], direction=Direction.OUTPUT, output_value=Value.ACTIVE) sets a set of overrides.

Quoted 60 lines · Bartosz Golaszewski

Quoted 3 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <[email protected]> wrote:

[snip]

Quoted 19 lines

Quoted 15 lines

Quoted 7 lines

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Quoted 7 lines

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

Quoted 3 lines

What would that look like (in python code) if I wanted to request 5 lines and use the same config for them?

Quoted 7 lines

That is the trivial case - you use the module level gpiod.request_lines() as is and pass in the config parameters and list of lines you want.

req = gpiod.request_lines(chip="gpiochip0", offsets=[1,2,3,4,5],

                          direction="output", values=[1,0,1,0,0])

This is close to what I have now in my v3 branch. Except that values is called output_values and takes a dictionary like its counterpart in LineConfig but that can be extended to interpreting a list as providing the values for corresponding offsets/lines. Current version of request_lines() takes all LineConfig options and uses them as the defaults.

Quoted 10 lines

The more complicated case is where the lines config differs. Then you have to build the LineConfig by adding the config for each set of lines in a separate call to set_props().
Then you provide that LineConfig to the request_lines(), along with the set of lines.

lc.set_props(offsets=[1,2,3], direction="input") lc.set_props(offsets=[4,5], direction="output", values=[1,0]) req = gpiod.request_lines(chip="gpiochip0", line_cfg=lc)

Quoted 8 lines

(simplified examples using stringified prop values etc - hope you get the idea)

Building that on top of the C API, you would determine the "default" config based on the most common attribute values, then override the config for the lines that differ from that default.
That is the internal complexity I mentioned.

Quoted 9 lines

Internal complexity is fine - it's the implicitness of the defaults that make me not like this idea. I think we discussed something similar for the C API and I was against it too. Your examples are fine but the defaults for lines not mentioned in set_props() would be filled by a freshly created LineConfig with its well defined default values. In other words I prefer to keep the override mechanism visible in python but unification of the property setters is something I will consider.

I suspect you are right that we've been here before and I'm flogging a dead horse, but you get that - I must think there is still a bit of life in the old nag ;-).

I find it ironic that a feature of the uAPI that is there due to the constraints on the uAPI, i.e. to keep the line_config to a manageable size, gets propagated this highly. In my mind the configuration for each line has always been distinct, and the uAPI line_config is just a reduced form.

(it was also a logical stepping stone from the v1 "all lines have the same config", which maybe where your attachment to a default originates, to "all lines can be configured independently" that I was going for in v2, but practicality limited to "all lines can be configured independently - to a point", basically by supporting a limited number of deltas - which you refer to as overrides)

Quoted 2 lines

To me it should look like:

Quoted 6 lines

lc.set_props(direction=Direction.INPUT, edge_detection=Edge.BOTH) sets the defaults
lc.set_props(offset=4, direction=Direction.OUTPUT) sets a single override lc.set_props(offsets=[5, 1], direction=Direction.OUTPUT, output_value=Value.ACTIVE) sets a set of overrides.

I could argue that I don't like the implictness of lines 1, 2 and 3 here.

The LineConfig defaults are safe. Allowing the user to redefine the defaults that apply to a request, separately from where the request is made mind you, is potentially dangerous.

What if you make the default OUTPUT, and then elsewhere you request lines and include line 6, forgetting to set it to an INPUT or assuming that by default you'll get an INPUT? Kiss that board goodbye.

OTOH the LineConfig "well defined default values" are safe. Forget to define the config for a line - get a vanilla input.

I wasn't overly concerned about this in the uAPI itself as I was assuming that at a higher level the lines would be configured separately, and the higher level language binding would perform the encoding to uAPI. Turns out that isn't always the case :-|.

Quoted 76 lines · Kent Gibson

Quoted 60 lines · Bartosz Golaszewski

Quoted 3 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Tue, Jul 5, 2022 at 4:09 AM Kent Gibson <[email protected]> wrote:

[snip]

Quoted 19 lines

Quoted 15 lines

Quoted 7 lines

How about merging the _default and _offset forms by adding an offsets kwarg?
offsets=None (or unspecified) -> default
offsets=int -> offset
offsets=iterable -> offsets

Quoted 7 lines

Off on a bit of a tangent... why should the end user care about defaults and overrides?
For a higher level abstraction I'd prefer to see the whole "default" concept disappear in favour of the config for each line. That would remove a lot of the complexity from the LineConfig interface. Though it would add complexity to the binding internals.

Quoted 3 lines

What would that look like (in python code) if I wanted to request 5 lines and use the same config for them?

Quoted 7 lines

That is the trivial case - you use the module level gpiod.request_lines() as is and pass in the config parameters and list of lines you want.

req = gpiod.request_lines(chip="gpiochip0", offsets=[1,2,3,4,5],

                          direction="output", values=[1,0,1,0,0])

This is close to what I have now in my v3 branch. Except that values is called output_values and takes a dictionary like its counterpart in LineConfig but that can be extended to interpreting a list as providing the values for corresponding offsets/lines. Current version of request_lines() takes all LineConfig options and uses them as the defaults.

Quoted 10 lines

The more complicated case is where the lines config differs. Then you have to build the LineConfig by adding the config for each set of lines in a separate call to set_props().
Then you provide that LineConfig to the request_lines(), along with the set of lines.

lc.set_props(offsets=[1,2,3], direction="input") lc.set_props(offsets=[4,5], direction="output", values=[1,0]) req = gpiod.request_lines(chip="gpiochip0", line_cfg=lc)

Quoted 8 lines

(simplified examples using stringified prop values etc - hope you get the idea)

Building that on top of the C API, you would determine the "default" config based on the most common attribute values, then override the config for the lines that differ from that default.
That is the internal complexity I mentioned.

Quoted 9 lines

Internal complexity is fine - it's the implicitness of the defaults that make me not like this idea. I think we discussed something similar for the C API and I was against it too. Your examples are fine but the defaults for lines not mentioned in set_props() would be filled by a freshly created LineConfig with its well defined default values. In other words I prefer to keep the override mechanism visible in python but unification of the property setters is something I will consider.

I suspect you are right that we've been here before and I'm flogging a dead horse, but you get that - I must think there is still a bit of life in the old nag ;-).

Quoted 6 lines

I find it ironic that a feature of the uAPI that is there due to the constraints on the uAPI, i.e. to keep the line_config to a manageable size, gets propagated this highly. In my mind the configuration for each line has always been distinct, and the uAPI line_config is just a reduced form.

The limitation of the uAPI is what keeps us from making it true in user-space (that each line can have its own config). As it is, only up to 9-10 lines can have distinct configs and making the API look and behave as if it wasn't the case is more confusing (E2BIG errors) than simply admitting we have the concept of defaults and overrides (to which the interface is greatly simplified in the high-level libraries). The idea about making the most common config attributes become the defaults is simply bad. It would require the user to anticipate how the library will behave for every attribute and lead to

Quoted 10 lines

The limitation of the uAPI is what keeps us from making it true in user-space (that each line can have its own config). As it is, only up to 9-10 lines can have distinct configs and making the API look and behave as if it wasn't the case is more confusing (E2BIG errors) than simply admitting we have the concept of defaults and overrides (to which the interface is greatly simplified in the high-level libraries). The idea about making the most common config attributes become the defaults is simply bad. It would require the user to anticipate how the library will behave for every attribute and lead to

It requires nothing from the user. They are not even aware of the concept of "defaults" or "overrides". They just set config on lines. If that is too complicated, which is quite unlikely, then they get E2BIG and they need to repartition their lines into multiple requests.

Anyway, that horse is dead.

Quoted 1 line · Kent Gibson

[snip]

Quoted 11 lines

Quoted 10 lines

The limitation of the uAPI is what keeps us from making it true in user-space (that each line can have its own config). As it is, only up to 9-10 lines can have distinct configs and making the API look and behave as if it wasn't the case is more confusing (E2BIG errors) than simply admitting we have the concept of defaults and overrides (to which the interface is greatly simplified in the high-level libraries). The idea about making the most common config attributes become the defaults is simply bad. It would require the user to anticipate how the library will behave for every attribute and lead to

Quoted 7 lines

It requires nothing from the user. They are not even aware of the concept of "defaults" or "overrides". They just set config on lines. If that is too complicated, which is quite unlikely, then they get E2BIG and they need to repartition their lines into multiple requests.

Anyway, that horse is dead.

For a python user, this:

lc = gpiod.LineConfig() lc.set_props(offsets=[2, 3], direction=Direction.OUTPUT) req = gpiod.request_lines("/dev/gpiochip0", line_cfg=lc)

is pretty much as simple as it gets. They still don't need to be aware of the underlying split into defaults and overrides. I believe it's GoodEnough™.

I imagine in Rust bindings we'll be able to chain set_props() as is customary and we'll get a one-liner out of that.

Quoted 31 lines · Bartosz Golaszewski

Quoted 1 line · Kent Gibson

[snip]

Quoted 11 lines

Quoted 10 lines

The limitation of the uAPI is what keeps us from making it true in user-space (that each line can have its own config). As it is, only up to 9-10 lines can have distinct configs and making the API look and behave as if it wasn't the case is more confusing (E2BIG errors) than simply admitting we have the concept of defaults and overrides (to which the interface is greatly simplified in the high-level libraries). The idea about making the most common config attributes become the defaults is simply bad. It would require the user to anticipate how the library will behave for every attribute and lead to

Quoted 7 lines

It requires nothing from the user. They are not even aware of the concept of "defaults" or "overrides". They just set config on lines. If that is too complicated, which is quite unlikely, then they get E2BIG and they need to repartition their lines into multiple requests.

Anyway, that horse is dead.

For a python user, this:

lc = gpiod.LineConfig() lc.set_props(offsets=[2, 3], direction=Direction.OUTPUT) req = gpiod.request_lines("/dev/gpiochip0", line_cfg=lc)

Quoted 7 lines

is pretty much as simple as it gets. They still don't need to be aware of the underlying split into defaults and overrides. I believe it's GoodEnough™.

I imagine in Rust bindings we'll be able to chain set_props() as is customary and we'll get a one-liner out of that.

The code I posted here is wrong as it's missing the request config but it made me think: how about in case of req_cfg=None or not passed at all, we derive the lines to request from overridden offsets in the line config? In that case if the user does:

lc = gpiod.LineConfig() lc.set_props(offsets=[0, 1], direction=Direction.OUTPUT,
output_value=Value.ACTIVE)
lc.set_props(offset=4, direction=Direction.INPUT) req = gpiod.request_lines("/dev/gpiochip0", line_cfg=lc)

Then it will be interpreted as lines=[0, 1, 4]?

I'm also thinking that we could allow the output values to be mapped as <line name> -> <value> within gpiod.LineConfig like that:

lc.set_props(lines=["foo", 4], direction=Direction.OUTPUT) lc.set_output_values({"foo": Value.Active, 4: Value.INACTIVE})

It would require us to retrieve the names of all lines from the chip at the time of the request and store them in the request structure (for reconfigure to work) but it would make the entire thing even more "pythonic".

This series adds python bindings for libgpiod v2. The series is split into several patches for easier review.

In general the python bindings follow what we did for C++ in terms of class layout except that we leverage python's flexibility to reduce the number of method variants by allowing different types of arguments.

Because python doesn't have RAII, unlike C++, we provide a module-level request_lines() helper as gpiod.Chip(path).request_lines(...) one-liner could lead to the chip left dangling even after the last reference is dropped.

Because python forces us to dynamically allocate objects all the time even for fundamental types (which are also immutable) there's no point in trying to make the EdgeEventBuffer avoid copying the events like we did in C++ for better performance. Python simply isn't designed around speed.

v1 -> v2:
- all methods now accept keyword arguments even for mandatory positional ones
- added a global request_lines() function that enables easy one-liner liner requests
- improve and unify the pydoc format
- many smaller tweaks and fixes

Bartosz Golaszewski (5):
bindings: python: remove old version
bindings: python: enum: add a piece of common code for using python's
enums from C
bindings: python: add examples for v2 API
bindings: python: add tests for v2 API
bindings: python: add the implementation for v2 API

 bindings/python/.gitignore                    |    1 +
 bindings/python/Makefile.am                   |   19 +-
 bindings/python/chip-info.c                   |  126 +
 bindings/python/chip.c                        |  606 ++++
 bindings/python/edge-event-buffer.c           |  330 ++
 bindings/python/edge-event.c                  |  191 ++
 bindings/python/enum/Makefile.am              |    9 +
 bindings/python/enum/enum.c                   |  208 ++
 bindings/python/enum/enum.h                   |   24 +
 bindings/python/examples/gpiodetect.py        |   13 +-
 bindings/python/examples/gpiofind.py          |   12 +-
 bindings/python/examples/gpioget.py           |   28 +-
 bindings/python/examples/gpioinfo.py          |   39 +-
 bindings/python/examples/gpiomon.py           |   53 +-
 bindings/python/examples/gpioset.py           |   36 +-
 bindings/python/exception.c                   |  182 ++
 bindings/python/gpiodmodule.c                 | 2662 -----------------
 bindings/python/info-event.c                  |  175 ++
 bindings/python/line-config.c                 | 1373 +++++++++
 bindings/python/line-info.c                   |  286 ++
 bindings/python/line-request.c                |  803 +++++
 bindings/python/line.c                        |  239 ++
 bindings/python/module.c                      |  557 ++++
 bindings/python/module.h                      |   58 +
 bindings/python/request-config.c              |  320 ++
 bindings/python/tests/Makefile.am             |   15 +-
 bindings/python/tests/cases/__init__.py       |   12 +
 bindings/python/tests/cases/tests_chip.py     |  157 +
 .../python/tests/cases/tests_chip_info.py     |   59 +
 .../python/tests/cases/tests_edge_event.py    |  279 ++
 .../python/tests/cases/tests_info_event.py    |  135 +
 .../python/tests/cases/tests_line_config.py   |  254 ++
 .../python/tests/cases/tests_line_info.py     |   90 +
 .../python/tests/cases/tests_line_request.py  |  345 +++
 bindings/python/tests/cases/tests_misc.py     |   53 +
 .../tests/cases/tests_request_config.py       |   77 +
 bindings/python/tests/gpiod_py_test.py        |  827 +----
 bindings/python/tests/gpiomockupmodule.c      |  309 --
 bindings/python/tests/gpiosimmodule.c         |  434 +++
 configure.ac                                  |    3 +-
 40 files changed, 7517 insertions(+), 3882 deletions(-)
 create mode 100644 bindings/python/.gitignore
 create mode 100644 bindings/python/chip-info.c
 create mode 100644 bindings/python/chip.c
 create mode 100644 bindings/python/edge-event-buffer.c
 create mode 100644 bindings/python/edge-event.c
 create mode 100644 bindings/python/enum/Makefile.am
 create mode 100644 bindings/python/enum/enum.c
 create mode 100644 bindings/python/enum/enum.h
 create mode 100644 bindings/python/exception.c
 delete mode 100644 bindings/python/gpiodmodule.c
 create mode 100644 bindings/python/info-event.c
 create mode 100644 bindings/python/line-config.c
 create mode 100644 bindings/python/line-info.c
 create mode 100644 bindings/python/line-request.c
 create mode 100644 bindings/python/line.c
 create mode 100644 bindings/python/module.c
 create mode 100644 bindings/python/module.h
 create mode 100644 bindings/python/request-config.c
 create mode 100644 bindings/python/tests/cases/__init__.py
 create mode 100644 bindings/python/tests/cases/tests_chip.py
 create mode 100644 bindings/python/tests/cases/tests_chip_info.py
 create mode 100644 bindings/python/tests/cases/tests_edge_event.py
 create mode 100644 bindings/python/tests/cases/tests_info_event.py
 create mode 100644 bindings/python/tests/cases/tests_line_config.py
 create mode 100644 bindings/python/tests/cases/tests_line_info.py
 create mode 100644 bindings/python/tests/cases/tests_line_request.py
 create mode 100644 bindings/python/tests/cases/tests_misc.py
 create mode 100644 bindings/python/tests/cases/tests_request_config.py
 delete mode 100644 bindings/python/tests/gpiomockupmodule.c
 create mode 100644 bindings/python/tests/gpiosimmodule.c

--
2.34.1