colinoflynn / pico-python

PicoScope Python Interface
Other
101 stars 80 forks source link

ps3404D AWG Signal period wrong #186

Open MitchiLaser opened 1 year ago

MitchiLaser commented 1 year ago

Hello,

although the ps3404D (not MSO) is not listed as a supported device I tried using its waveform generator. It works perfectly fine except for the fact that I have to divide time for the signal duration by the constant 5. My Code works completely fine on some ps2000a devices and a ps3204A.

I just wanted to notice this behavior, maybe it can be taken into account when it comes to the implementation for support for the ps3404D.

hmaarrfk commented 1 year ago

our the device driver are hand written and manually vetted.

PRs welcome to address any issues with specific models.

Their capabilities vary quite a bit.

prestegaard commented 1 year ago

Seeing similar issues with PS6424E. The signal period is depending on number of samples in the buffer, and when using a small number of samples the period is off by around a factor 5.

Update: After some finding out turned the driver from PicoScope does not the playback frequency per sample, but the period for the whole buffer. The PS6242E has a maximum playback frequency of 200 MHz, meaning 5 ns per sample point. If you try to play back a pattern faster than this it is truncated and the total playback time remains as specified.

jcafhe commented 1 year ago

Hi,

Currently I'm trying to implement the AWG capability for the ps4000a series, and I noticed an issue with the deltaphase calculation, which seems to lead to improper signal duration.

https://github.com/colinoflynn/pico-python/blob/f0926415d848ece8aced89d3d34b67f7d574729a/picoscope/picobase.py#L1061-L1063

After digging in programmer's guides of the 3000a, 4000a, 6000 and 6000a series, it appears that the waveform length (number of samples) parameter is missing in the formula. Depending on the guide, the deltaPhase formula is rearranged differently, but all the variations are equivalent :

ps6000a guide (p.110):

deltaPhase = outputFrequency/dacFrequency × arbitraryWaveformSize × 2**(phaseAccumulatorSize - bufferAddressWidth)

With phaseAccumulatorSize and bufferAddressWidth as a number of bits.

or in ps3000a, ps4000a, ps6000 guides, respectively on pages 98, 101, 82:

deltaPhase = outputFrequency/dacFrequency × arbitraryWaveformSize × phaseAccumulatorSize / awgBufferSize

With phaseAccumulatorSize as a maximum value (e.g. 232), and awgBufferSize equivalent to `2bufferAddressWidth`.

From these two definitions, we can see that the arbitraryWaveformSize parameter is missing, i.e. the "length in samples of the user-defined waveform".

Since this calculation is done by the _PicoscopeBase, I imagine this bug affects all implementations of the AWG.

I can make a PR to fix this bug if needed, but it means modifying the signature of the "public" methods getAWGDeltaPhase() and getAWGTimeIncrement() by adding the waveform length as a parameter to the methods. This will affects all the implementation modules that enable AWG capability. Do you think it is acceptable or maybe there's an alternative ? @hmaarrfk @bmoneke @prestegaard @MitchiLaser

jcafhe commented 1 year ago

I guess we can just create two new methods with new names which compute deltaPhase and timeIncrement with the right formulas. The setAWGSimple and setAWGSimpleDeltaPhase methods will now use these new created methods.

For the legacy methods, we just throw a warning or a DeprecationWarning when one calls getAWGDeltaPhase or getAWGTimeIncrement directly, explaining that the returned values might not be valid, so we don't break anything but the user is notified.

jcafhe commented 1 year ago

My bad, after further investigations, it seems I was mistaken about the waveform size. In fact, it is already included in the sampling_interval parameter of the getAWGDeltaPhase method, which equal to outputFrequency × arbitraryWaveformSize.

I guess I was misleading because, as the op, I noticed some weird behavior when working with a waveform with a small number of samples at a low frequency, where the deltaPhase value tends to 0, i.e. where the resolution error becomes significant.

Although, the pico-python slightly differs from calling directly the clib function psXXXXSigGenFrequencyToPhase, where the deltaPhase value seems to be smartly rounded instead of using a floor function.

Round instead of floor would extends the limits of the generator with low frequency and small number of samples, at the expense of a increased error, but it would be aligned with the clib function (will try include this in a PR).

As a rule of thumb, it is preferable to work with a waveform discretized by the maximum number of sample allowed by the device, i.e. 2**psAWGBufferAddressWidth.