Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver/sigrok: various fixes #1346

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

flxzt
Copy link

@flxzt flxzt commented Apr 2, 2024

Description

This fixes some things in the sigrok driver implementation:

  • explicitly set time field in csv conversion,
    apparently some versions of sigrok-cli on some distros (for example Ubuntu 22.04) default to false,
    resulting in invalid parsing
  • analyze method checks/uses wrong (local) file path when the resource is remote
  • capture file is transferred to wrong path when specified filename is not absolute and using a remote sigrok resource.
  • avoid deadlock when sigrok-cli process failed
  • unsuccessful sigrok-cli termination when using sigrok driver remotely

Checklist

  • PR has been tested

apparently some versions of sigrok-cli on some distros (for example Ubuntu 22.04) default to false,
resulting in invalid parsing

Signed-off-by: Felix Zwettler <felix.zwettler@duagon.com>
…urce is remote

Signed-off-by: Felix Zwettler <felix.zwettler@duagon.com>
when specified filename is not absolute and using
a remote sigrok resource.

Usually when an absolute path is supplied as `filename`
the call `abs.path.join()` resolves to `filename`.
But when the file path is relative,
it resolves it to a relative path appended to the tmp dir,
instead of from where the user calls this method.

Signed-off-by: Felix Zwettler <felix.zwettler@duagon.com>
@flxzt flxzt changed the title driver/sigrok various fixes driver/sigrok: various fixes Apr 2, 2024
@Emantor Emantor requested review from Emantor and jluebbe April 9, 2024 12:46
@Emantor Emantor added the fix label Apr 9, 2024
Emantor
Emantor previously approved these changes Apr 9, 2024
@flxzt
Copy link
Author

flxzt commented Apr 25, 2024

I pushed an additional fix:

When the sigrok-cli process fails prematurely (which can happen when for example the device is used from another process, or the user does not have the permissions to access the device), the driver implementation waited forever for the creation of the capture file.

The implemented fix polls the subprocess while waiting for the capture file creation, and raises an Exception if it has failed. For easier debugging, it also logs the stdout and stderr of the failed sigrok-cli call.

@flxzt
Copy link
Author

flxzt commented Apr 25, 2024

Another issue I found is that the SIGINT signal that is supposed to terminate to the sigrok-cli process does not work when the command is executed through ssh. (here). I don't yet know how to fix it, but I want to document it.

Edit:

Forcing to allocate a pty by adding the ssh -tt argument here makes SIGINT propagate to the sigrok-cli process. I didn't test if adding this would break other drivers, and I am unsure if this is the right approach. It seems to be not ideal for non-interactive use in general, sigrok-cli is the exception here.

I think the better approach to this would be to detach the process entirely and send a kill <pid> when it should be stopped

@Emantor Emantor self-requested a review April 26, 2024 10:31
@flxzt
Copy link
Author

flxzt commented Apr 29, 2024

Turns out sigrok-cli can be terminated through any keypress (see http://sigrok.org/gitweb/?p=sigrok-cli.git;a=blob;f=anykey.c;h=b1ac9ef430a80df681ee1442e99d8bbc6712fe11;hb=HEAD#l57 ).

So the fix actually is rather simple - instead of sending SIGINT, write any key to stdin. This works fine with ssh

flxzt added 2 commits May 6, 2024 16:05
The driver expected that the sigrok-cli is successful and starts
capturing in continuous mode. When the call is unsuccessful,
the driver then waited for the creation of the capture file forever.

Fixed by checking if the sigrok-cli process terminated prematurely
while waiting for the existence of the capture file.

Signed-off-by: Felix Zwettler <Felix.Zwettler@duagon.com>
…otely

the signal that is supposed to terminate the process SIGINT was not being propagated through ssh.
The driver waited for termination forever.

fixed by quitting sigrok-cli through an emulated keypress by sending a char over ssh.

Signed-off-by: Felix Zwettler <Felix.Zwettler@duagon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants