Address style nits to improve readablity and future maintenance.
Move system header includes before application level includes. Move
assignments out of variable declaration blocks. Separate function calls
and assignments from error checks and flow control. Break a long line.
Add braces around conditional multi line statements.
All these adjustments are cosmetical. Behaviour does not change.
With Keysight 34465A the VOLT:AC and FREQ functions keep suffering from
USB timeouts and pipe errors. Add a 10ms delay between initiating the
acquisition and checking for completion before fetching the result.
The weird thing is that when successful responses are seen after
temporary failure, enlarging the delay does not make them go away.
Instead the USB errors keep moving to a different spot. Measurements do
arrive though, so this is just annyoing but not a blocker.
sr: [00:00.134038] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:00.136043] scpi: Got response: '"FREQ +3.00000000E+00,+3.00000000E-06"', length 38.
sr: [00:00.136119] scpi-dmm: dev_acquisition_start: Precision: '"FREQ +3.00000000E+00,+3.00000000E-06"'
sr: [00:00.136251] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:00.184211] scpi: Got response: '1', length 1.
sr: [00:00.184481] scpi_usbtmc: Successfully sent SCPI command: 'INIT'.
sr: [00:00.234800] session: bus: Received SR_DF_HEADER packet.
cli: Received SR_DF_HEADER.
sr: [00:00.245597] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.246050] scpi_usbtmc: USBTMC bulk in transfer error: LIBUSB_ERROR_TIMEOUT.
sr: [00:01.256495] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.291871] scpi_usbtmc: USBTMC invalid bulk in header.
sr: [00:01.302392] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.401959] scpi: Got response: '1', length 1.
sr: [00:01.402135] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.403392] scpi: Got response: '1', length 1.
sr: [00:01.403566] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:01.405769] scpi: Got response: '"FREQ +3.00000000E+00,+3.00000000E-06"', length 38.
sr: [00:01.406020] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.407500] scpi: Got response: '1', length 1.
sr: [00:01.407674] scpi_usbtmc: Successfully sent SCPI command: 'FETCH?'.
sr: [00:01.409107] scpi: Got response: '+9.00555531E+01', length 15.
Experimentation suggests that 200ms timeouts reduce the probability of
these USB communication errors. But if functions tend to respond that
differently, are delay time specs per MQ required? The implementation of
the driver currently assumes a single delay spec.
Some models are slow to start a measurement acquisition in specific MQs
(meter functions). An "*OPC?" query went unanswered after "INIT" was
sent. This was observed on 34405A in temperature, frequency, resistance,
AC voltage and current mode.
Add an optional per-model measurement delay that is used when the MQ
specifies that the delay is applicable. Because functions are changed
rarely but measurements are taken many times, only the first reading
is delayed while later readings are seen at the expected interval. Not
delaying the first reading will result in device errors.
sr: [00:00.875162] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:00.881666] scpi: Got response: '"FREQ +1.00000E+00,+1.00000E-05"', length 32.
sr: [00:00.881700] scpi-dmm: dev_acquisition_start: Precision: '"FREQ +1.00000E+00,+1.00000E-05"'
sr: [00:00.881840] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:00.883966] scpi: Got response: '1', length 1.
sr: [00:00.884139] scpi_usbtmc: Successfully sent SCPI command: 'INIT'.
sr: [00:01.884420] session: bus: Received SR_DF_HEADER packet.
cli: Received SR_DF_HEADER.
sr: [00:01.895260] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:02.895766] scpi_usbtmc: USBTMC bulk in transfer error: LIBUSB_ERROR_TIMEOUT.
sr: [00:02.906302] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:02.910703] scpi_usbtmc: USBTMC invalid bulk in header.
sr: [00:02.921244] scpi_usbtmc: USBTMC bulk out transfer error: LIBUSB_ERROR_PIPE.
sr: [00:02.931802] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:02.932276] scpi: Got response: '1', length 1.
sr: [00:02.932425] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:02.934790] scpi: Got response: '1', length 1.
sr: [00:02.935155] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:02.941564] scpi: Got response: '"FREQ +1.00000E+00,+1.00000E-05"', length 32.
sr: [00:00.173408] hwdriver: scpi-dmm: Starting acquisition.
sr: [00:00.173569] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:00.334312] scpi: Got response: '1', length 1.
sr: [00:00.334544] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:00.341070] scpi: Got response: '"RES +1.00000E+02,+1.00000E-03"', length 31.
sr: [00:00.341307] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:00.343636] scpi: Got response: '1', length 1.
sr: [00:00.343872] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:00.350093] scpi: Got response: '"RES +1.00000E+02,+1.00000E-03"', length 31.
sr: [00:00.350157] scpi-dmm: dev_acquisition_start: Precision: '"RES +1.00000E+02,+1.00000E-03"'
sr: [00:00.350342] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:00.352524] scpi: Got response: '1', length 1.
sr: [00:00.352832] scpi_usbtmc: Successfully sent SCPI command: 'INIT'.
sr: [00:00.352901] session: bus: Received SR_DF_HEADER packet.
cli: Received SR_DF_HEADER.
sr: [00:00.363675] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:01.364363] scpi_usbtmc: USBTMC bulk in transfer error: LIBUSB_ERROR_TIMEOUT.
sr: [00:01.374904] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:02.375380] scpi_usbtmc: USBTMC bulk in transfer error: LIBUSB_ERROR_TIMEOUT.
sr: [00:02.385872] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:03.232457] scpi_usbtmc: USBTMC invalid bulk in header.
sr: [00:03.243003] scpi_usbtmc: USBTMC bulk out transfer error: LIBUSB_ERROR_PIPE.
sr: [00:03.253464] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:03.256568] scpi: Got response: '1', length 1.
sr: [00:03.256902] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:03.259440] scpi: Got response: '1', length 1.
sr: [00:03.259713] scpi_usbtmc: Successfully sent SCPI command: 'CONF?'.
sr: [00:03.266070] scpi: Got response: '"RES +1.00000E+08,+1.00000E+3"', length 30.
sr: [00:03.266456] scpi_usbtmc: Successfully sent SCPI command: '*OPC?'.
sr: [00:03.268501] scpi: Got response: '1', length 1.
sr: [00:03.268961] scpi_usbtmc: Successfully sent SCPI command: 'READ?'.
sr: [00:03.353589] scpi: Got response: '+9.90000000E+37 ', length 16.
Declare a 2500ms measurement initiation delay for 34405A which was found
by experimentation. This is huge but was found to be necessary.
Of course all other meter functions are unaffected by this commit. As
are the meters which specify no delay.
Some models are slow to switch meter functions to specific MQs. This was
observed with 34405A and AC V/I modes, where USB connections timed out
and suffered from pipe errors.
Add an optional delay after "CONF:<mode> [<range>]" SCPI requests. The
duration of the delay is per model, the use of the delay is per MQ on
that model. The 200ms delay for 34405A was found by experimentation.
Speaking about 34405A: Only the "INIT" request in temperature mode
remains as an issue, which is not covered by this commit's approach.
Don't use the "[SENS:]<mode>:RANGE..." request on Agilent protocol
speaking devices. Use "CONF:<mode> [<range>]" instead. This was tested
with all devices which reference this model.
In theory there is the question whether previously configured ranges
should be re-used when (exclusively) the MQ is switched. It's assumed
that this is not useful, instead users or their applications should
select the function first and optionally pick the range then. Re-use of
e.g. 10k resistance range when switching to capacitance might be seen as
confusing (and results in errors in the device).
Use the combination of INIT and FETCH? to get measurements. The READ?
request is more complex and might "undo" what INIT already initiated.
This improves perception of the 34405A meter's operation.
Rephrase the SCPI read timeouts in scpi-dmm model declarations. Shuffle
the order of product terms such that the milliseconds "unit" goes last,
and the milliseconds value goes first. Assume that "2500 * 1000 [us]"
better reflects the intention than "1000 * 2500" does.
The Fluke 45 driver's ECHO_TEST broke other people's SCPI device
identification and was disabled in 2018-11. Remove the dead code which
is impossible to resurrect. Its purpose was questionable to begin with.
Should users be affected, then the echo would break the identification
phase. The check cannot be done before identification without affecting
other connected devices, cannot be done after the identification either
when that very identification systematically fails. Should a user meet
this condition then applications' logs are available to see why *IDN?
fails, assuming that users are not aware of their serial port or device
configuration. The request's reflection in the place of a response gives
a strong hint.
Only scan for hp-3457a devices when the user specified which connection
to use. This avoids breaking SCPI devices by emitting non-standard
requests before reliable device identification.
Only scan for hpib-pps devices when a user provided conn= specification
was given. This avoids breaking SCPI devices by emitting non-standard
requests before reliable device identification.
Improve readability and simplify maintenance of the sr_scpi_scan()
routine.
Move initial assignment and subsequent update of variables in closer
proximity, don't hide assignments in declaration blocks. Use common
helper code where available. Split complex conditions into individual
terms which are easier to review and reason about. Don't mix calls and
value checks and flow control in the same statement.
Device or model or protocol variant specific commands should only be
used after the device reliably got identified. The "CHDR OFF" command
upsets other devices and needs to move after *IDN? and model checks.
The HP 59306A device was made in 1973 and would not reliably identify
by means of SCPI queries. The previous scan() implementation would have
matched any connected SCPI device, then upset these devices by emitting
non-SCPI requests.
Tighten the scope of the hp-59306a probe, only scan for devices when a
conn= spec was provided. This avoids false positives and malfunction of
other devices, and still lets users address the problematic device. This
is similar to serial-dmm and unspecific cables and has proven to work
there.
How to reproduce the issue:
(while any auto enumerating SCPI device is connected)
$ sigrok-cli --scan
Pre-assign default values before having user specified options scanned.
This avoids the necessity to post-process the result. Move the initial
assignment to where its corresponding update is to improve readability.
The sr_serial_extract_options() routine is a hidden gem, has existed for
almost eight years, and is exclusively used by the motech-lps-301 driver
(was also used by hameg-hmo at some point in the past). Being so useful
and doing something to terribly popular, more drivers should use it.
Extend the API to make either of the out parameters genuinely optional,
and guarantee that pre-assigned defaults will survive in the absence of
user specs. Update the Doxygen comment.
While queries for the range are supported for voltage, current,
resistence, capacitance, the same queries in temperature, frequency,
diode, continuity modes not only fail but even lose the USB connection
to the device. This was consistently observed with 34405A and 34465A.
Suppress get and set range requests for the known problematic modes of
the Agilent protocol speaking meters.
Although the 34405A does not support this function, other Agilent
protocol using meters do (currently 34465A, very probably others which
are about to get added in the future).
It's assumed that announcing the availability of that MQ for a set of
meters while only some of them support it is preferrable over exploding
numbers of copies of tables which only slightly vary among each other.
This mechanical edit adds a flags field and the initializer element to
make the compiler happy. Behaviour does not change. See a word diff for
improved review experience.
Add support to get and set the auto/manual range on Agilent protocol
speaking devices (34405A, 34465A). The range values are mere numbers
without a unit or MQ associated with them. Support to list available
ranges is prepared but not used (not needed on these meters). Common
logic could open code the lists of ranges from the model description
if desired in a future implementation.
Though measurement values come in, the 34405A and 34465A meters kept
beeping when acquisition is started. Stopped beeping when the MEAS
keyword was changed to INIT, which is used for other Agilent devices
using the HP protocol variant.
This change is acceptable because it was tested on all DMMs which use
the Agilent protocol (according to scpi-dmm profiles).
Increase the default digit count in the 34465A model description. Though
most measurements get the precision from the current reading, this data
is used as a fallback when the response won't provide the detail.
Assume that *IDN? requests don't involve expensive processing, and thus
don't require *OPC? before their execution. This makes scpi-dmm follow
the good tradition to only use non-standard or model dependent features
after the device was successfully identified. Which avoids confusion on
unrelated devices.
Exclusively check for *OPC? availability on Owon devices, because these
motivated the introduction of the test in the first place. This avoids
confusion on other devices where short timeouts result in errors because
the response is not retrieved although the command is supported (observed
on Agilent meters connected via USBTMC).
Improve readability of SCPI uint8 and float vector get routines. Move
assignment and use of variables closer together to simplify review.
Allocate the glib array based on the text split result's length. Move
data processing to the "straight" path and handle failed conversion as
an exceptional condition in an error path.
Eliminate a redundant data type reference in a malloc call.
The SCPI get routines may allocate memory for response data which
callers have to free after use.
The implemented approach is modelled after pull request 114 by
Matti Eiden, to get the leak fixes in while the PR is queued.
This addresses part of bug #1683.
Submitted-By: Matti Eiden <snaipperi@gmail.com>
The SCPI get routines may allocate memory for response data which
callers have to free after use.
Move an existing assigment such that the initial assignment, the memory
allocation, use of response data, and resource release are in closer
proximity, and thus are easier to reason about during maintenance.
Behaviour does not change.
This is motivated by bug #1683.
The SCPI get routines may allocate memory for response data which
callers have to free after use.
The approach to release allocated memory is simlar or identical to the
corresponding parts of pull request 95, in an attempt to get the reported
leak fixes in, yet reduce the potential for conflicts where this commit
deviates from the PR. Most work was done by Ralf. Thank you!
This addresses part of bug #1683.
Submitted-By: Reported-By: Ralf <jr-oss@gmx.net>
Tested-By: Reported-By: Ralf <jr-oss@gmx.net>
The SCPI get routines may allocate memory for response data which
callers have to free after use.
Move an existing release call to an earlier location, to avoid leaking
memory when an error path is taken.
This addresses part of bug #1683.
The SCPI get routines may allocate memory for response data which
callers have to free after use.
This implementation is incomplete. The fluke-45 driver's context holds
a "global" copy of the most recently received response. While the data
is freed in the next receive call, one item remains allocated for the
driver's remaining life time. Which is considered non-critical.
Also moves an operator in a complex multi-line expression to a different
location to follow the project's style.
This addresses part of bug #1683.
Some of the SCPI getter routines allocate memory for response data which
callers need to free after use. Document this responsibility in more
places, and rephrase existing locations for consistency. See a word-diff
for the essence of the commit.
It's assumed that the separate paragraph in the function description is
better perceived than something that's hidden in the parameter or return
value discussion. Also reference the corresponding free routine by name
for callers' convenience (data types will differ among routines). Some
free routines can be called unconditionally, others require NULL checks
at the caller's.
Reported-By: Ralf <jr-oss@gmx.net>
Assign NULL early in those SCPI get routines which return allocated
memory to their callers. So that the return value is either a valid
pointer to memory or NULL regardless of the routine's exit code. This
simplifies call sites and increases robustness.
Opening a serial port involves two libserialport calls, while only the
second call's return value gets inspected. Check the lookup before the
open, too, and provide its details in a diag message.
When CSV import grabs channel names from the first line in the file then
the text strictly between column separators (a comma) was used. Which
resulted in channel names like " channel 1". Trim leading and trailing
space before unquote, to better fulfill most users' expectation. Users
still can quote the input data for special cases if desired.
The DPS devices default to a rather low UART communication bitrate. The
generous retrieval of more Modbus registers than strictly necessary for
the periodic acquisition of analog channel values and state tracking may
or may not be an issue.
Instrument all call sites in api.c to the device state gathering calls
such that the protocol.c side transparently can reduce the transfer
volume depending on which level of detail the caller may expect given
its current context. Under optimal circumstances this might cut the
traffic in half, which DPS devices may benefit from.
This commit adds the infrastructure to potentially reduce transfer
volume, but does not change behaviour. The "config" reasons may need
further partitioning if Modbus register access really turns out to be
a severe bottleneck. The API lends itself to that extension easily. The
approach which gets prepared in this commit needs to get runtime tested
by those who got access to the hardware.
The RD devices differ from DPS devices in their default communication
bitrate (115200), register content endianess (BE), and register set
layout (addresses for registers). In either case 16bit registers get
accessed by means of Modbus communication over a serial channel. The
interpretation of the registers' values mostly is the same as for DPS
devices once a register with the same meaning got identified. Device
identification includes a 32bit serial number which DPS devices appear
to not provide.
All other product features are the same, and map to identical sigrok
mechanisms. That's why re-using the rdtech-dps driver to add rdtech-rd
support is considered desirable. This implementation shares all code
"above" the raw register addressing and raw value interpretation. All
logical processing, configuration interface, state tracking and data
submission to the session feed, are shared among the different device
types.
Declare support for the RD6006, RD6012, and RD6018 models. Their specs
were taken from the 2020.12.2 instruction manual. The driver was tested
with an RD6006 device (firmware version 1.28). Unfortunately automatic
device detection is not possible or will not be reliable, which is why
users need to specify the respective model by picking one of the two
drivers. Within a driver variant the device identification and use of
the device are automatically dealt with.
The existing power supply driver for Riden DPS/DPH devices unfortunately
duplicated intimate details of modbus communication and register layout
including interpretation of raw register values across several source
files. Do separate the physical transport and the register layout and
register fields interpretation layers. This brings the driver in line
with the usual api.c and protocol.c arrangement of responsibilities, and
prepares the future addition of other similar devices.
Address a few style nits while we are here. Include <config.h> in all
source files, and alpha-sort include directives. Address minor data type
nits. Reduce indentation in code paths. Propagate more error codes.
Update comments which lagged behind the code, adjust grammer (eliminate
"actual" false friends). Eliminate a few magic numbers and redundancies,
switch to the more recent endianess aware byte stream reader API where
beneficial, to eliminate redundant offset math.
Other nits seem to have gone unnoticed before: The indices of all sigrok
channels were identical (all zero). Signed print formats were used for
unsigned data.
This implementation compiles, but wasn't runtime tested due to lack of
hardware. A rough estimate of transfer volume and transport throughput
suggests that the 10ms cycle time no longer can be kept, may end up
around 25ms (40 cycles per second). This can get avoided by adding more
code to tell the configuration requests, the acquisition start, and the
measurements grabbing during acquisition apart. It's assumed though that
the 10ms glib poll interval did not translate to such fast measurement
gathering. The firmware may typically provide data at lower rates anyway.
The generic source code may look overly complicated or redundant at
first glance, but compilers' generated code will greatly get optimized.
Simplified maintenance because of reduced cognitive load is considered
more important.
Improve readability by addressing the trivial style nits in the driver.
Break the channel list into two times eight instead of nine plus seven
items. Put braces around all branches if one of them has braces. Add
braces to multi line branches despite most of the branch just being a
comment -- it's still many lines. Break extra long text lines, adjust
the position of operators when single expressions span multiple lines.
Drop unnecessary parentheses around single variables and auto
incrementing pointer dereferences. Adjust whitespace especially for
pointer data types, and arith operators.
Gerhard Sittig
Sylvain Pelissier
Rafael G. Martins
Frank Stettner
Soeren Apel