[libgpiod][PATCH v2 1/2] bindings: python: tests: add a test case for line request by name with multiple entries

[libgpiod][PATCH v2 1/2] bindings: python: tests: add a test case for line request by name with multiple entries

[Chuang Zhu]

From: Chuang Zhu <git@chuang.cz>

Signed-off-by: Chuang Zhu <git@chuang.cz>
---
 bindings/python/tests/tests_line_request.py | 34 +++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/bindings/python/tests/tests_line_request.py b/bindings/python/tests/tests_line_request.py
index f99b93d..c9a5c81 100644
--- a/bindings/python/tests/tests_line_request.py
+++ b/bindings/python/tests/tests_line_request.py
@@ -310,6 +310,40 @@ class LineRequestComplexConfig(TestCase):
                 self.assertEqual(chip.get_line_info(3).edge_detection, Edge.BOTH)
 
 
+class LineRequestComplexConfigByName(TestCase):
+    def setUp(self):
+        self.sim = gpiosim.Chip(num_lines=4, line_names={2: "foo", 3: "bar", 1: "baz", 0: "xyz"})
+        self.req = gpiod.request_lines(
+            self.sim.dev_path,
+            {
+                ("baz", "bar"): gpiod.LineSettings(direction=Direction.OUTPUT),
+                ("foo", "xyz"): gpiod.LineSettings(direction=Direction.INPUT)
+            },
+        )
+
+    def tearDown(self):
+        self.req.release()
+        del self.req
+        del self.sim
+
+    def test_set_values_by_name(self):
+        self.req.set_values(
+            {"bar": Value.ACTIVE, "baz": Value.INACTIVE}
+        )
+
+        self.assertEqual(self.sim.get_value(1), SimVal.INACTIVE)
+        self.assertEqual(self.sim.get_value(3), SimVal.ACTIVE)
+
+    def test_get_values_by_name(self):
+        self.sim.set_pull(0, Pull.UP)
+        self.sim.set_pull(2, Pull.DOWN)
+
+        self.assertEqual(
+            self.req.get_values(["foo", "xyz"]),
+            [Value.INACTIVE, Value.ACTIVE],
+        )
+
+
 class RepeatingLinesInRequestConfig(TestCase):
     def setUp(self):
         self.sim = gpiosim.Chip(num_lines=4, line_names={0: "foo", 2: "bar"})
-- 
2.44.0

[libgpiod][PATCH v2 2/2] bindings: python: fix line request by name with multiple entries

[Chuang Zhu]

From: Chuang Zhu <git@chuang.cz>

When multiple entries are requested using line names in
Chip.request_lines, only the the last entry is added to
LineRequest._name_map, causing a ValueError when trying to use functions
like LineRequest.set_value on any former entries.

This patch fixes that by moving the required variables to the correct
scope.

Signed-off-by: Chuang Zhu <git@chuang.cz>
---
 bindings/python/gpiod/chip.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
index 19c62cd..55f0c3e 100644
--- a/bindings/python/gpiod/chip.py
+++ b/bindings/python/gpiod/chip.py
@@ -279,11 +279,12 @@ class Chip:
         else:
             mapped_output_values = None
 
+        name_map = dict()
+        offset_map = dict()
+        global_output_values = list()
+
         for lines, settings in config.items():
             offsets = list()
-            name_map = dict()
-            offset_map = dict()
-            global_output_values = list()
 
             if isinstance(lines, int) or isinstance(lines, str):
                 lines = (lines,)
-- 
2.44.0

Re: [libgpiod][PATCH v2 1/2] bindings: python: tests: add a test case for line request by name with multiple entries

[Kent Gibson]

On Thu, Jun 27, 2024 at 11:31:02PM +0800, Chuang Zhu wrote:
> From: Chuang Zhu <git@chuang.cz>
>

Add a commit comment.

> Signed-off-by: Chuang Zhu <git@chuang.cz>
> ---
>  bindings/python/tests/tests_line_request.py | 34 +++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/bindings/python/tests/tests_line_request.py b/bindings/python/tests/tests_line_request.py
> index f99b93d..c9a5c81 100644
> --- a/bindings/python/tests/tests_line_request.py
> +++ b/bindings/python/tests/tests_line_request.py
> @@ -310,6 +310,40 @@ class LineRequestComplexConfig(TestCase):
>                  self.assertEqual(chip.get_line_info(3).edge_detection, Edge.BOTH)
>
>
> +class LineRequestComplexConfigByName(TestCase):

Nothing complex about the config, so LineRequestMixedConfigByName?

> +    def setUp(self):
> +        self.sim = gpiosim.Chip(num_lines=4, line_names={2: "foo", 3: "bar", 1: "baz", 0: "xyz"})
> +        self.req = gpiod.request_lines(
> +            self.sim.dev_path,
> +            {
> +                ("baz", "bar"): gpiod.LineSettings(direction=Direction.OUTPUT),
> +                ("foo", "xyz"): gpiod.LineSettings(direction=Direction.INPUT)
> +            },
> +        )
> +
> +    def tearDown(self):
> +        self.req.release()
> +        del self.req
> +        del self.sim
> +
> +    def test_set_values_by_name(self):
> +        self.req.set_values(
> +            {"bar": Value.ACTIVE, "baz": Value.INACTIVE}
> +        )
> +
> +        self.assertEqual(self.sim.get_value(1), SimVal.INACTIVE)
> +        self.assertEqual(self.sim.get_value(3), SimVal.ACTIVE)
> +
> +    def test_get_values_by_name(self):
> +        self.sim.set_pull(0, Pull.UP)
> +        self.sim.set_pull(2, Pull.DOWN)
> +
> +        self.assertEqual(
> +            self.req.get_values(["foo", "xyz"]),
> +            [Value.INACTIVE, Value.ACTIVE],
> +        )
> +

Add "baz" to the lines to get so this test fails as well - expected to
be INACTIVE.

Cheers,
Kent.

> +
>  class RepeatingLinesInRequestConfig(TestCase):
>      def setUp(self):
>          self.sim = gpiosim.Chip(num_lines=4, line_names={0: "foo", 2: "bar"})
> --
> 2.44.0
>

Re: [libgpiod][PATCH v2 2/2] bindings: python: fix line request by name with multiple entries

[Kent Gibson]

On Thu, Jun 27, 2024 at 11:31:03PM +0800, Chuang Zhu wrote:
> From: Chuang Zhu <git@chuang.cz>
>
> When multiple entries are requested using line names in
> Chip.request_lines, only the the last entry is added to
> LineRequest._name_map, causing a ValueError when trying to use functions
> like LineRequest.set_value on any former entries.
>

Chip.request_lines()

> This patch fixes that by moving the required variables to the correct
> scope.
>

Use imperitive, "Move the required...."

> Signed-off-by: Chuang Zhu <git@chuang.cz>

Use the same address to sign off and send the submission email.

With a multi-patch series you should include a cover letter that
describes the series the changes between versions.

e.g. here you have fixed my review comments, but also fixed not moving the
offsets list into the outer scope.

Cheers,
Kent.

> ---
>  bindings/python/gpiod/chip.py | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
> index 19c62cd..55f0c3e 100644
> --- a/bindings/python/gpiod/chip.py
> +++ b/bindings/python/gpiod/chip.py
> @@ -279,11 +279,12 @@ class Chip:
>          else:
>              mapped_output_values = None
>
> +        name_map = dict()
> +        offset_map = dict()
> +        global_output_values = list()
> +
>          for lines, settings in config.items():
>              offsets = list()
> -            name_map = dict()
> -            offset_map = dict()
> -            global_output_values = list()
>
>              if isinstance(lines, int) or isinstance(lines, str):
>                  lines = (lines,)
> --
> 2.44.0
>