Oscilloscope Integration

[kartikaysharma01]

Description There are no known bugs in the code. I have tried to make sure there are no memory leaks and the coding style is followed. The coding style to be followed can be found here. The code can be tested by using these commands on sigrok-cli. The code structure is expected to change as we add new instruments.

Known Weakness

• Trigger is not tested properly

• Possible memory leaks (assigned memory not freed)

Issue Resolves #5

Status Ready for review

[kartikaysharma01]

Testing on sigrok-cli

Check ports for pslab device: sigrok-cli --driver pslab-oscilloscope -l 3 --scan

To see available device options: sigrok-cli --driver pslab-oscilloscope -l 3 --show

To see available config options on a particular channel group: sigrok-cli --driver pslab-oscilloscope -l 3 --show -g CH1

To set trigger source and voltage, for ex: 1.1 V trigger on channel 2 sigrok-cli -d pslab-oscilloscope --set --config triggersource=CH1:triggerlevel=1.1

To set range on any channel ( which in turn will set the appropriate gain for the channel) sigrok-cli -d pslab-oscilloscope -l 3 -g CH1 --set --config vdiv=8V

To sample x no of samples of y channels with z samplerate ( say x = 500 ; y=CH1, CH2 ; z = 1000000) sigrok-cli -d pslab-oscilloscope -l 3 --c samplerate=1m -C CH1,CH2 --samples 500

To sample 500 samples on channels CH1, CH2 with samplerate 10k with range 8V on Channel CH1 sigrok-cli -d pslab-oscilloscope -l 3 --c channel_name=CH1:vdiv=8V --c samplerate=10k -C CH1,CH2 --samples 500

Samplerate= 10e6/timgap(in us)

Use log level 5 -l 5 to see all logs.

All this can be done graphically on pulseview (except setting trigger level)

[mariobehling]

Is this ready for review? Please change the review status.

[kartikaysharma01]

Is this ready for review? Please change the review status.

Will do, Trying to squash the commits.

[bessman]

When starting acquisition in Pulseview, I get some errors on stdout:

Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!

The default setting is to capture 10k samples on 4 channels simultaneously. This correctly raises an "invalid argument" error, since the PSLab can only capture up to 2.5k samples in 4-channel mode. Is it possible to reduce the default number of samples so the default settings are valid?

Related to the above, could the "invalid argument" error be more descriptive to help the user understand what is wrong?

I can only capture samples in Pulseview once. When I try to run Pulseview a second time after a successful capture it crashes with:

“./pulseview” terminated by signal SIGSEGV (Address boundary error)

[kartikaysharma01]

When starting acquisition in Pulseview, I get some errors on stdout:

Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!
Error: Listing device key "Volts/div" failed!

This is expected; the reason behind this is when pulseview tries to get and list range(Volts/div) for CH3 and MIC, it throws an error as we do not support range on those channels.

The default setting is to capture 10k samples on 4 channels simultaneously. This correctly raises an "invalid argument" error, since the PSLab can only capture up to 2.5k samples in 4-channel mode. Is it possible to reduce the default number of samples so the default settings are valid?

Yes, we can reduce the default samples to 2000. Should I commit that change right now?

Related to the above, could the "invalid argument" error be more descriptive to help the user understand what is wrong?

No, unfortunately, libsigrok does not allow us to do that. An alternate would be to use the command pulseview -l 4 to run/open pulseview; that way we can look at the logs to better understand what actually went wrong.

I can only capture samples in Pulseview once. When I try to run Pulseview a second time after a successful capture it crashes with:

“./pulseview” terminated by signal SIGSEGV (Address boundary error)

I could not re-produce this before as I would change the channel configuration after I sample once; this happens when we sample with the same channel configuration twice. I have spotted the issue and fixed it (at least can not reproduce it anymore). Should this be fixed with a new issue or a commit in this PR itself

[bessman]

Yes, we can reduce the default samples to 2000. Should I commit that change right now?

Please do.

I could not re-produce this before as I would change the channel configuration after I sample once; this happens when we sample with the same channel configuration twice. I have spotted the issue and fixed it (at least can not reproduce it anymore). Should this be fixed with a new issue or a commit in this PR itself

Go ahead and commit directly to this PR. Issues should only be opened for code that is already merged.

[kartikaysharma01]

Go ahead and commit directly to this PR. Issues should only be opened for code that is already merged.

Should I squash the changes to the current commit too?

[bessman]

Something is wrong with the gain scaling: This is using 4 V/div setting. The actual waveform is +-3.3 V. I will check the math and see if I can figure out what's wrong.

By the way, I figured out an easier way to get the PSLab to output a waveform in a way that can be viewed in Pulseview: First connect to the device using pslab-python, use WaveformGenerator.generate and then disconnect from the device in pslab-python. Then you can connect to it from Pulseview, and as long as you don't powercycle the PSLab it will keep outputting the waveform.

[bessman]

Should I squash the changes to the current commit too?

Hold off on squashing until we're ready to merge.

[kartikaysharma01]

Something is wrong with the gain scaling:

Yeah, I just spotted that while solving the "sampling twice" bug. Have fixed that. The reason was I used the wrong find index function to save memory while cleaning the code.

By the way, I figured out an easier way to get the PSLab to output a waveform in a way that can be viewed in Pulseview: First connect to the device using pslab-python, use WaveformGenerator.generate and then disconnect from the device in pslab-python. Then you can connect to it from Pulseview, and as long as you don't powercycle the PSLab it will keep outputting the waveform.

That's great, Solves a lot of testing problems 🎉

[kartikaysharma01]

Yes, we can reduce the default samples to 2000. Should I commit that change right now?

@bessman Also, should we keep the default samplerate to 200 kHz, right now it is 2 kHz, which is a bit slow?

[bessman]

Selecting CH1 as trigger source crashes Pulseview with:

Caught exception of type N6sigrok5ErrorE (invalid argument)
double free or corruption (fasttop)
“./pulseview” terminated by signal SIGABRT (Abort)

When selecting a different channel as trigger source the trigger works as expected, but the data captured on the trigger channel is also erroneously displayed on CH1: Here CH1 is not connected to anything, so it should be close to zero. Actually I guess it's the trigger channel that is displayed twice, while CH1 is not displayed at all. Either way it's not the expected behavior.

Is there a way to select the trigger level, or disable the trigger entirely?

[bessman]

I see now that you already mentioned that setting the trigger level is not possible in Pulseview.

In sigrok-cli, when selecting a channel other than CH1 as trigger source, it is not possible to capture from CH1. For example:

sigrok-cli -d pslab -l 3 --config samplerate=1m --config triggersource=CH2:triggerlevel=1.1 -C CH1,CH2 --samples 1000

returns:

sr: pslab: Sending version commands to device
sr: pslab: PSLab device found: PSLab V5 on port: /dev/ttyACM3
sr: pslab: Assign channel CH2 from list to target
unknown channel 'CH1'.

[kartikaysharma01]

Either way it's not the expected behavior.

I am taking a look at that.

Is there a way to select the trigger level, or disable the trigger entirely?

For now, the trigger level can be set through the following command,
sigrok-cli -d pslab --set --config triggersource=__:triggerlevel=__ About disabling trigger entirely, shouldn't trigger level=0 at CH1, act the same.

[kartikaysharma01]

@bessman What is the expected behavior when a trigger channel is selected but no trigger voltage is passed?

[bessman]

About disabling trigger entirely, shouldn't trigger level=0 at CH1, act the same.

No, trigger level=0 means that sampling starts when the signal passes 0 V. No trigger means the sampling starts as soon as possible, which could be at any voltage level.

@bessman What is the expected behavior when a trigger channel is selected but no trigger voltage is passed?

Then the default trigger level should be used, which is 0 V.

[kartikaysharma01]

Is there a way to disable the trigger entirely?

The trigger is not enabled until a trigger source is selected from "configure device". It just displays CH1 as default there. The thought process behind that is if somebody passes only trigger_level from sigrok-cli, channel-one-map is considered the default.

[kartikaysharma01]

~Here CH1 is not connected to anything, so it should be close to zero.~ Actually I guess it's the trigger channel that is displayed twice, while CH1 is not displayed at all. Either way it's not the expected behavior.

This one was exhausting, So in trigger_source, I was saving pointers to the channels. So initially, the pointer point to CH1 by default, and then when we select CH2 as a trigger source, it updates the CH1 as CH2. I don't know how I missed this. ~I will do a bit of testing and update the PR.~ I have updated the PR but could not test it. The reported bug has been resolved., should work fine now.

Elaborating the trigger flow a bit. Initially, CH1 is set as the trigger_source, but the trigger is disabled. Now when the user selects a trigger source from the list in pulseview, the trigger is enabled. After the trigger configuration is complete, it is disabled again. Is this correct? @bessman

[bessman]

Elaborating the trigger flow a bit. Initially, CH1 is set as the trigger_source, but the trigger is disabled. Now when the user selects a trigger source from the list in pulseview, the trigger is enabled. After the trigger configuration is complete, it is disabled again. Is this correct? @bessman

Not quite, I think. The trigger should only be disabled through user action, not automatically.

With the most recent commit, the trigger channel is no longer duplicated. However, triggering no longer works at all.

[kartikaysharma01]

Not quite, I think. The trigger should only be disabled through user action, not automatically.

Pulseview is not supporting trigger configs completely as of now. I am not sure, but it seems as tho setting trigger off from pulseview won't be possible. But I believe it won't affect our ultimate goal as all this can be done through cli

[bessman]

Not quite, I think. The trigger should only be disabled through user action, not automatically.

Pulseview is not supporting trigger configs completely as of now. I am not sure, but it seems as tho setting trigger off from pulseview won't be possible. But I believe it won't affect our ultimate goal as all this can be done through cli

Well, selecting trigger channel works now. We're missing trigger level, as well as the ability to disable the trigger once enabled. But as you say, Pulseview is not our main focus so we can live with some missing features.

[kartikaysharma01]

@bessman, for the gain resetting case we discussed in the meeting, I have tried a few approaches; I did find a solution but only to encounter another problem. Talking about why this is happening, pulseview, I believe, uses sessions to handle sampling, whereas the sigrok-cli scans the devices every time we run an acquisition command resetting the range/vdiv and gain to the default values. I did find a workaround for sampling and setting vdiv through a single command, but I believe it is not the right way.

Moreover, even if we optimize(do it the correct way, if any) and use the workaround method, setting vdiv for CH1 and CH2 through a single command does not seem to be possible, which should not be the case. So, the problem kind of stays. I have asked the sigrok community about the same and will hopefully get a reply soon.

[bessman]

We can live with not being able to set the gain through sigrok-cli for now. I think we're ready to merge this, and mark the PR against upstream as ready for review. What do you think @kartikaysharma01?

[kartikaysharma01]

I have been looking into the options we have for integrating other instruments. And a new driver does seem to be the best option. I have informed upstream about our device, use case, and our interest in having different drivers for each instrument. I have not got upstream's view on it yet, but we should continue with the approach.

What do you think @kartikaysharma01?

I think we should rename this driver from pslab to pslab-os or something the community decides, as this is going to stick with us once merged. So I think we should merge after the rename.

[kartikaysharma01]

@bessman I have renamed the driver and tested for some basic commands. It would be great if you too could test for a few commands again, just in case something broke down.
If you have had look at the coding style and memory deallocation, we can squash the commits and continue with the merging.

[kartikaysharma01]

I don't really see why you allocate memory in most cases where you malloc.

In the case of using malloc for dev_context variables like data, the memory allocation is necessary for reading, and so is the case with most of the allocations.

The general rule of thumb is that every malloc should be paired with a free elsewhere in the program. Right now most allocations are never freed as far as I can tell.

Yes, I have made a few changes, and now all the allocated memory is freed, I believe.

[kartikaysharma01]

@bessman should I go ahead with the squash?

[bessman]

Please do!

[kartikaysharma01]

We can live with not being able to set the gain through sigrok-cli for now.

This issue has been resolved, more details can be found here. I have updated the commands in the comment section of this PR too