diff mbox series

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

Message ID 20240625011522.1470733-1-chuang+git@melty.land
State New
Headers show
Series [libgpiod] bindings: python: fix LineRequest.set_value only works for the last entry | expand

Commit Message

Chuang Zhu June 25, 2024, 1:15 a.m. UTC 
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(-)

Comments

Kent Gibson June 25, 2024, 10:53 a.m. UTC | #1 
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
>
Kent Gibson June 25, 2024, 11:09 a.m. UTC | #2 
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.
Bartosz Golaszewski June 25, 2024, 12:32 p.m. UTC | #3 
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
diff mbox series

Patch

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,)