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

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Conversation

flxzt
Copy link
Contributor

@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

@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
Contributor 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
Contributor 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
Contributor 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

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 <[email protected]>
happens only when when the resource is remote

Signed-off-by: Felix Zwettler <[email protected]>
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 <[email protected]>
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 <[email protected]>
…motely

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 <[email protected]>
@Emantor Emantor merged commit cecb82f into labgrid-project:master Jun 26, 2024
9 checks passed
@flxzt flxzt deleted the sigrok-driver-fixes branch August 19, 2024 08:32
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.

3 participants