driver: usbsdmuxdriver: add GPIO get and set support#1891
Conversation
Add two new methods to USBSDMuxDriver: - get_gpio(gpio): reads the state of a GPIO pin (0 or 1), returns 'high' or 'low' - set_gpio(gpio, value): sets a GPIO pin to 'high'/'1' or 'low'/'0' Both methods use the 'gpio' subcommand of the usbsdmux CLI tool, which is available on hardware revisions that support open-drain GPIO outputs (e.g. USB-SD-Mux Fast). Signed-off-by: Hamza TOUIZRAT <hamzatouizrat@yahoo.com>
|
|
||
| @Driver.check_active | ||
| @step(title="sdmux_gpio_get", args=["gpio"]) | ||
| def get_gpio(self, gpio): |
There was a problem hiding this comment.
Not using the DigitalOutputProtocol here means that we can't make use of the IOs in the client and discoverability is difficult (since the function is named differently)
But the DigitalOutputProtocol requires that the index of the GPIO is set beforehand.
@Bastian-Krause @jluebbe do you have a better idea here?
There was a problem hiding this comment.
@Emantor @jluebbe @SmithChart Maybe this should be a dedicated resource/driver then? Is it safe to run the usbsdmux tool in parallel?
There was a problem hiding this comment.
The tool is not safe for parallel use:
We do some read-modify-write over the I2C-bus on the device (e.g. here ). With two instances in parallel one could loose and may get it's changes overwritten by the other instance.
The tool would exit without error - but the state of the USB-SD-Mux would not have changed as expected.
But isn't labgrid strictly serial in it's execution?
There was a problem hiding this comment.
After thinking about it more, I think having separate resources/drivers per IO could actually introduce a problem if two clients each acquire a different IO simultaneously, since they’d still be hitting the same USB device underneath. And honestly I’m not sure there’s a real use case for it beyond being able to implement the protocol, which we could achieve differently anyway.
Would it be possible at the driver level to have two internal indexes and implement DigitalOutputProtocol for each of them within the same driver? That way we keep a single point of access to the USB device and still get the discoverability benefit.
There was a problem hiding this comment.
After thinking about it more, I think having separate resources/drivers per IO could actually introduce a problem if two clients each acquire a different IO simultaneously, since they’d still be hitting the same USB device underneath.
That's exactly what I meant when I asked about the parallel access.
And honestly I’m not sure there’s a real use case for it beyond being able to implement the protocol, which we could achieve differently anyway.
The common interface is the benefit. Every GPIO can be used the same way. And labgrid client io won't work otherwise.
Would it be possible at the driver level to have two internal indexes and implement DigitalOutputProtocol for each of them within the same driver? That way we keep a single point of access to the USB device and still get the discoverability benefit.
I don't think that's possible. If you could live with using only one the GPIOs, we could either hard-code it in the driver or configure it in the USBSDMuxDevice/NetworkUSBSDMuxDevice resource. Either that or add a new resource + driver and some locking or exclusive access + wait/timeout in the usbsdmux tool. But both options are not really ideal.
There was a problem hiding this comment.
Thanks for the clarification.
You mentioned that adding locking or exclusive access in the usbsdmux tool is not really ideal, could you elaborate on why?
There was a problem hiding this comment.
@hamztouiz There was no need for locking in the tool until now. Usage of the tool was always strictly linear, not parallel.
I do not see why we should not add a lock to the tool if it turns out to be useful.
But I do not understand why access can happen in parallel. Labgrid will lock the resource, so only a single user will access the resource. And isn't labgrid strictly linear in executing by design? How could a parallel access ever happen?
There was a problem hiding this comment.
@SmithChart If we would turn the usbsdmux GPIOs into dedicated resources and add a driver for them implementing the DigitalOutputProtocol, these new GPIO and USBSDMuxDevice/NetworkUSBSDMuxDevice resources could be accessed during the same time from different places.
@hamztouiz With "not ideal" I tried to hint at the rather high effort needed for such a simple operation as toggling a GPIO (add locking to usbsdmux, add a new resource, add a new driver).
There was a problem hiding this comment.
I'll let you decide on the best approach here, I don't have enough context on the project conventions to make the call.
I'm also fine with not implementing the protocol and skipping the dedicated resource and driver for now.
|
Hello, just bumping this PR. Happy to rebase on latest main or address any feedback. Let me know how you’d like to proceed! |
4 participants
Add two new methods to
USBSDMuxDriver:get_gpio(gpio): reads the state of a GPIO pin (0 or 1), returns'high'or'low'set_gpio(gpio, value): sets a GPIO pin to'high'/'1'or'low'/'0'Both methods use the
gpiosubcommand of theusbsdmuxCLI tool,which is available on hardware revisions that support open-drain GPIO
outputs (e.g. USB-SD-Mux Fast).
Signed-off-by: Hamza TOUIZRAT hamzatouizrat@yahoo.com