[libgpiod][PATCH] bindings: python: fix LineRequest.set_value only works for the last entry

[libgpiod][PATCH] bindings: python: fix LineRequest.set_value only works for the last entry

[chuang+git]

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
LineRequest.set_value on any former entries.

>>> import gpiod
>>> lr = gpiod.Chip('/dev/gpiochip0').request_lines(
...     config={
...         'LINE0': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT,
...                                     output_value=gpiod.line.Value.INACTIVE),
...         'LINE2': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT, active_low=True,
...                                     output_value=gpiod.line.Value.ACTIVE),
...     }
... )
>>> lr._name_map
{'LINE2': 2}
>>> lr.set_value('LINE0', gpiod.line.Value.ACTIVE)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../gpiod/line_request.py", line 126, in set_value
    self.set_values({line: value})
  File ".../gpiod/line_request.py", line 138, in set_values
    mapped = {
             ^
  File ".../gpiod/line_request.py", line 139, in <dictcomp>
    self._name_map[line] if self._check_line_name(line) else line: values[line]
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../gpiod/line_request.py", line 82, in _check_line_name
    raise ValueError("unknown line name: {}".format(line))
ValueError: unknown line name: LINE0

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

diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
index b3d8e61..ce77d27 100644
--- a/bindings/python/gpiod/chip.py
+++ b/bindings/python/gpiod/chip.py
@@ -279,11 +279,12 @@ class Chip:
         else:
             mapped_output_values = None
 
+        offsets = list()
+        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] bindings: python: fix LineRequest.set_value only works for the last entry

[Kent Gibson]

On Tue, Jun 25, 2024 at 09:15:22AM +0800, chuang+git@melty.land 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
> LineRequest.set_value on any former entries.
>
> >>> import gpiod
> >>> lr = gpiod.Chip('/dev/gpiochip0').request_lines(
> ...     config={
> ...         'LINE0': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT,
> ...                                     output_value=gpiod.line.Value.INACTIVE),
> ...         'LINE2': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT, active_low=True,
> ...                                     output_value=gpiod.line.Value.ACTIVE),
> ...     }
> ... )
> >>> lr._name_map
> {'LINE2': 2}
> >>> lr.set_value('LINE0', gpiod.line.Value.ACTIVE)
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File ".../gpiod/line_request.py", line 126, in set_value
>     self.set_values({line: value})
>   File ".../gpiod/line_request.py", line 138, in set_values
>     mapped = {
>              ^
>   File ".../gpiod/line_request.py", line 139, in <dictcomp>
>     self._name_map[line] if self._check_line_name(line) else line: values[line]
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../gpiod/line_request.py", line 82, in _check_line_name
>     raise ValueError("unknown line name: {}".format(line))
> ValueError: unknown line name: LINE0
>

Rather than including example code and the error, put this, or similar, in a
test case to both demonstrate the problem and the correctness of the fix.

And, as well as describing the problem, you should describe what the fix
is that you have implemented.

The change itself looks ok to me.

Cheers,
Kent.

> Signed-off-by: Chuang Zhu <git@chuang.cz>
> ---
>  bindings/python/gpiod/chip.py | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
> index b3d8e61..ce77d27 100644
> --- a/bindings/python/gpiod/chip.py
> +++ b/bindings/python/gpiod/chip.py
> @@ -279,11 +279,12 @@ class Chip:
>          else:
>              mapped_output_values = None
>
> +        offsets = list()
> +        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] bindings: python: fix LineRequest.set_value only works for the last entry

[Kent Gibson]

On Tue, Jun 25, 2024 at 06:53:42PM +0800, Kent Gibson wrote:
> On Tue, Jun 25, 2024 at 09:15:22AM +0800, chuang+git@melty.land 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
> > LineRequest.set_value on any former entries.
> >
> > >>> import gpiod
> > >>> lr = gpiod.Chip('/dev/gpiochip0').request_lines(
> > ...     config={
> > ...         'LINE0': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT,
> > ...                                     output_value=gpiod.line.Value.INACTIVE),
> > ...         'LINE2': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT, active_low=True,
> > ...                                     output_value=gpiod.line.Value.ACTIVE),
> > ...     }
> > ... )
> > >>> lr._name_map
> > {'LINE2': 2}
> > >>> lr.set_value('LINE0', gpiod.line.Value.ACTIVE)
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> >   File ".../gpiod/line_request.py", line 126, in set_value
> >     self.set_values({line: value})
> >   File ".../gpiod/line_request.py", line 138, in set_values
> >     mapped = {
> >              ^
> >   File ".../gpiod/line_request.py", line 139, in <dictcomp>
> >     self._name_map[line] if self._check_line_name(line) else line: values[line]
> >                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   File ".../gpiod/line_request.py", line 82, in _check_line_name
> >     raise ValueError("unknown line name: {}".format(line))
> > ValueError: unknown line name: LINE0
> >
>
> Rather than including example code and the error, put this, or similar, in a
> test case to both demonstrate the problem and the correctness of the fix.
>
> And, as well as describing the problem, you should describe what the fix
> is that you have implemented.
>
> The change itself looks ok to me.
>

Oh, and wrt the subject, the problem isn't specific to set_value() is it?
Any function using the line name to identify the line would fail too,
right?

Cheers,
Kent.

Re: [libgpiod][PATCH] bindings: python: fix LineRequest.set_value only works for the last entry

[Bartosz Golaszewski]

On Tue, Jun 25, 2024 at 3:25 AM <chuang+git@melty.land> 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
> LineRequest.set_value on any former entries.
>
> >>> import gpiod
> >>> lr = gpiod.Chip('/dev/gpiochip0').request_lines(
> ...     config={
> ...         'LINE0': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT,
> ...                                     output_value=gpiod.line.Value.INACTIVE),
> ...         'LINE2': gpiod.LineSettings(direction=gpiod.line.Direction.OUTPUT, active_low=True,
> ...                                     output_value=gpiod.line.Value.ACTIVE),
> ...     }
> ... )
> >>> lr._name_map
> {'LINE2': 2}
> >>> lr.set_value('LINE0', gpiod.line.Value.ACTIVE)
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File ".../gpiod/line_request.py", line 126, in set_value
>     self.set_values({line: value})
>   File ".../gpiod/line_request.py", line 138, in set_values
>     mapped = {
>              ^
>   File ".../gpiod/line_request.py", line 139, in <dictcomp>
>     self._name_map[line] if self._check_line_name(line) else line: values[line]
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File ".../gpiod/line_request.py", line 82, in _check_line_name
>     raise ValueError("unknown line name: {}".format(line))
> ValueError: unknown line name: LINE0
>
> Signed-off-by: Chuang Zhu <git@chuang.cz>
> ---
>  bindings/python/gpiod/chip.py | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
> index b3d8e61..ce77d27 100644
> --- a/bindings/python/gpiod/chip.py
> +++ b/bindings/python/gpiod/chip.py
> @@ -279,11 +279,12 @@ class Chip:
>          else:
>              mapped_output_values = None
>
> +        offsets = list()
> +        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
>
>

Thanks! Please rework the commit message as advised by Kent.
Especially: explain what effect the changes have.

Bart