Implement GPIO support via sysfs with agent interface

[leiflm]

Description
This is only complementary work for #407

Checklist

• Documentation for the feature
• Tests for the feature

• The arguments and description in doc/configuration.rst have been updated

• CHANGES.rst has been updated

• PR has been tested

• Man pages have been regenerated

Complements #407

[leiflm]

I've just noticed, that the output port is not switched as expected.

I'm investigating the issue and will comment as soon as the issue is resolved.

[leiflm]

I've just noticed, that the output port is not switched as expected.

This was a false assumption. The output is indeed toggled. It just turns back to its initial state once the instance is destroyed/the line gets released. This happens really quickly with the current code appearing as if it was never toggled in the first place.

I'm investigating the issue and will comment as soon as the issue is resolved.

Okay, turns out that when lines are released they recover their former state.

I'll modify the code to work around eventual pin default state recovery.

[leiflm]

Since I'm not firm with the Python ecosystem: Where would the distro-specific requirement declaration for libgpiod go? Should I just update the debian file?

[jluebbe]

I've just noticed, that the output port is not switched as expected.

This was a false assumption. The output is indeed toggled. It just turns back to its initial state once the instance is destroyed/the line gets released. This happens really quickly with the current code appearing as if it was never toggled in the first place.

I'm investigating the issue and will comment as soon as the issue is resolved.

Okay, turns out that when lines are released they recover their former state.

I'll modify the code to work around eventual pin default state recovery.

Hmm, it seems that this is a limitation of the /dev/gpiochip interface. The current assumption of the DigitalOutputProtocol is that any configured value is kept until overwritten. This is relevant for usage with labgrid-client io, as no process is around to keep the device node open.

https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/TODO#n11 mentions the plan to write a dbus-based daemon to handle that case, adding even more complexity. Maybe using the sysfs interface is the better approach? ;)

[jluebbe]

Since I'm not firm with the Python ecosystem: Where would the distro-specific requirement declaration for libgpiod go? Should I just update the debian file?

The documentation section on the driver should mention the requirement. Updating the Debian file is also useful.

[leiflm]

I've just noticed, that the output port is not switched as expected.

This was a false assumption. The output is indeed toggled. It just turns back to its initial state once the instance is destroyed/the line gets released. This happens really quickly with the current code appearing as if it was never toggled in the first place.

I'm investigating the issue and will comment as soon as the issue is resolved.

Okay, turns out that when lines are released they recover their former state.
I'll modify the code to work around eventual pin default state recovery.

Hmm, it seems that this is a limitation of the /dev/gpiochip interface. The current assumption of the DigitalOutputProtocol is that any configured value is kept until overwritten. This is relevant for usage with labgrid-client io, as no process is around to keep the device node open.

https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/TODO#n11 mentions the plan to write a dbus-based daemon to handle that case, adding even more complexity. Maybe using the sysfs interface is the better approach? ;)

Yeah. I think that the effort of transforming the libgpiod implementation into an agent was futile. So maybe the state prior to agent implementation (8de1391) could be merged? Should I force-push a state that removes all the commits on top?

[jluebbe]

Yeah. I think that the effort of transforming the libgpiod implementation into an agent was futile. So maybe the state prior to agent implementation (8de1391) could be merged? Should I force-push a state that removes all the commits on top?

That would be premature. Compared to that state, you now have network support.
If you keep the libgpiod handle open in the agent (over multiple get/set calls), you would have the same behaviour as using libgpio directly in the driver (just with added remote support). But even that wouldn't be enough for keeping the output state after the process has exited.

Is there some way in libgpiod to make the state persistent?

[leiflm]

Yeah. I think that the effort of transforming the libgpiod implementation into an agent was futile. So maybe the state prior to agent implementation (8de1391) could be merged? Should I force-push a state that removes all the commits on top?

That would be premature. Compared to that state, you now have network support.

Yes. But with libgpiod the remote system has to have the python bindings for libgpiod installed too.
Compared to simply having a SSHDriver, that seems overly complex, doesn't it?

Maybe I could implement the networked variant using sysfs and keep the local implementation using libgpiod?

If you keep the libgpiod handle open in the agent (over multiple get/set calls), you would have the same behaviour as using libgpio directly in the driver (just with added remote support). But even that wouldn't be enough for keeping the output state after the process has exited.

Yeah. That's true. Maybe instead of implementing the DigitalOutputProtocol it should only implement
the ResetProtocol for the time being?

Is there some way in libgpiod to make the state persistent?

Citing the libgpiod project author here:

A common complaint from users about gpioset is that the state of a line is not retained once the program exits.

His reasoning behind that design:

While this is precisely the way linux character devices work, it's understandable that most users will want some centralized way of controlling GPIOs - similar to how sysfs worked

As all character devices are supposed to behave like this, there must be some general implementation in the kernel? Skimming through the kernel source (drivers/gpio/gpiolib.c) does not show a state recovery besides unassigning various flags.

[jluebbe]

Yeah. I think that the effort of transforming the libgpiod implementation into an agent was futile. So maybe the state prior to agent implementation (8de1391) could be merged? Should I force-push a state that removes all the commits on top?

That would be premature. Compared to that state, you now have network support.

Yes. But with libgpiod the remote system has to have the python bindings for libgpiod installed too.
Compared to simply having a SSHDriver, that seems overly complex, doesn't it?

Maybe I could implement the networked variant using sysfs and keep the local implementation using libgpiod?

I think I'd perfer to just use the sysfs interface via the agent. This avoids the additional dependency and solves the problem of keeping the configured output even after a process exits.

If you keep the libgpiod handle open in the agent (over multiple get/set calls), you would have the same behaviour as using libgpio directly in the driver (just with added remote support). But even that wouldn't be enough for keeping the output state after the process has exited.

Yeah. That's true. Maybe instead of implementing the DigitalOutputProtocol it should only implement
the ResetProtocol for the time being?

That would be much less useful. :/

Is there some way in libgpiod to make the state persistent?

Citing the libgpiod project author here:

A common complaint from users about gpioset is that the state of a line is not retained once the program exits.

His reasoning behind that design:

While this is precisely the way linux character devices work, it's understandable that most users will want some centralized way of controlling GPIOs - similar to how sysfs worked

As all character devices are supposed to behave like this, there must be some general implementation in the kernel? Skimming through the kernel source (drivers/gpio/gpiolib.c) does not show a state recovery besides unassigning various flags.

Given that even gpiolib still has that missing daemon as a TODO, the pragmatic approach for labgrid seems to come down to using the sysfs interface via the agent, as that does everything we need and fits our current model better. If there is a daemon in the future (or we decide to build that ourselves), we can still add an additional driver for the same resource.

It should be possible to translate the gpiochip number + offset information the the global index also via sysfs. That would allow us to use the same resource for both alternative drivers.

[leiflm]

Given that even gpiolib still has that missing daemon as a TODO, the pragmatic approach for labgrid seems to come down to using the sysfs interface via the agent, as that does everything we need and fits our current model better. If there is a daemon in the future (or we decide to build that ourselves), we can still add an additional driver for the same resource.

Alright. For the given reasons, I'll change the agent to use the (deprecated) sysfs instead of libgpiod then.

[codecov]

Codecov Report

Merging #468 into master will increase coverage by 0.1%.
The diff coverage is 68.5%.

@@           Coverage Diff            @@
##           master    #468     +/-   ##
========================================
+ Coverage    57.5%   57.7%   +0.1%     
========================================
  Files         118     120      +2     
  Lines        7569    7693    +124     
========================================
+ Hits         4358    4443     +85     
- Misses       3211    3250     +39

Impacted Files	Coverage Δ
labgrid/remote/client.py	40% <0%> (-0.4%)	⬇️
labgrid/driver/__init__.py	100% <100%> (ø)	⬆️
labgrid/resource/base.py	100% <100%> (ø)	⬆️
labgrid/remote/exporter.py	36.6% <41.6%> (+0.2%)	⬆️
labgrid/driver/gpiodriver.py	63.6% <63.6%> (ø)
labgrid/resource/remote.py	82.5% <75%> (-0.4%)	⬇️
labgrid/util/agents/sysfsgpio.py	84.2% <84.2%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9186d3...3401bca. Read the comment docs.

[jluebbe]

Could you squash your own commits together? They seem to change code that is changed again in your rewrite.

[leiflm]

About squashing: Sure. Which range? All of my code? All of the libgpiod code? Feel free to do it in a way you like/see fit.

[Emantor]

Some small improvment requests, which could also be done after merge. Thanks!

[jluebbe]

I've squashed the incremental changes into one commit. The original commits by @bgermann from PR #407 are unchanged.

[bgermann]

Thanks!

[pmelange]

I've just noticed, that the output port is not switched as expected.

This was a false assumption. The output is indeed toggled. It just turns back to its initial state once the instance is destroyed/the line gets released. This happens really quickly with the current code appearing as if it was never toggled in the first place.

I'm investigating the issue and will comment as soon as the issue is resolved.

Okay, turns out that when lines are released they recover their former state.
I'll modify the code to work around eventual pin default state recovery.

Hmm, it seems that this is a limitation of the /dev/gpiochip interface. The current assumption of the DigitalOutputProtocol is that any configured value is kept until overwritten. This is relevant for usage with labgrid-client io, as no process is around to keep the device node open.

https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/TODO#n11 mentions the plan to write a dbus-based daemon to handle that case, adding even more complexity. Maybe using the sysfs interface is the better approach? ;)

I am currently working on trying to implement libgpio into labgrid instead of using sysfs. And I believe I have found the solution to the problem of the configured value being kept uptil overwritten.

When creating a line request, do it with direction=Direction.AS_IS. This will keep the state of the line as is was before being requested, and therefore no longer causing the value of the line to be changed. Afterwards, it's possible to set the line to output mode without any changes in the line happening. In addition, it is possible to get the current value of the line in the case where a line value needs to be toggled. Here is a small code snippet based on the toggle-line-value example in the libgpiod code.

def toggle_line_value(chip_path, line_offset):
    value_str = {Value.ACTIVE: "Active", Value.INACTIVE: "Inactive"}
    value = Value.ACTIVE

    with gpiod.request_lines(
        chip_path,
        consumer="toggle-line-value",
        config={
            line_offset: gpiod.LineSettings(
                direction=Direction.AS_IS
            )
        },
    ) as request:
        print("requesting the value")
        old_value = request.get_value(line_offset)
        print("old value {}={}".format(line_offset, value_str[old_value]))
        time.sleep(1)
        print("changing to output mode")
        request.reconfigure_lines(config={ line_offset: gpiod.LineSettings(
            direction=Direction.OUTPUT, output_value=old_value) })
        time.sleep(1)
        value = toggle_value(old_value)
        print("setting a new value")
        request.set_value(line_offset, value)
        time.sleep(1)
        print("requestion the value")
        old_value = request.get_value(line_offset)
        print("confirming value {}={}".format(line_offset, value_str[value]))