Agilent 54621d device driver #206
Conversation
There was a problem hiding this comment.
- didn't do an exhaustive check of indent and whitespace
- didn't ensure that all g_str* operations have a matching free (suggest running valgrind)
- inconsistent braces and newlines with some
if/elseand} else if ... {; sometimes should be on the same line. didn't comment inline - suggest running cppcheck
- some float / double inconsistencies, only commented on some. Suggest looking closely at each explicit cast, g_str functions taking a Double argument, and "%E" formatters
- a few excessively long lines
- a number of assignments-in-conditional
- lacking whitespace around math operators, and patterns like
if(...){ - Lazy brain so I paid less attention towards the end. Probably missed things.
grep "2000" *should return only a single match, that number appears way too often- some commented dead code should be removed
- some commits should probably be squashed, since this is an initial implementation ? to check with maintainer
- commit messages should be reworded to follow general format (see log history)
disclaimer : I am neither maintainer nor dev of this project, and there's no guarantee that my suggestions will accelerate processing of this PR.
| #include <math.h> | ||
|
|
||
| #define WAIT_FOR_CAPTURE_COMPLETE_RETRIES 100 | ||
| #define WAIT_FOR_CAPTURE_COMPLETE_DELAY (100*1000) |
There was a problem hiding this comment.
what units are these in ?
| int ret; | ||
| struct sr_scpi_dev_inst *scpi = sdi->conn; | ||
|
|
||
| if ((ret = sr_scpi_open(scpi)) < 0) { |
There was a problem hiding this comment.
assignment in conditional (there are a few of these, will not comment on each)
| return sr_scpi_close(sdi->conn); | ||
| } | ||
|
|
||
| static int check_channel_group(struct dev_context *devc, |
There was a problem hiding this comment.
you're returning enum values, why not set the func return type as the correct enum ?
| case SR_CONF_LIMIT_SAMPLES: | ||
| tmp_int = g_variant_get_uint64(data); | ||
| tmp_d = (double)tmp_int/devc->sample_rate_limit; //total time to be transmitted | ||
| tmp_d2 = 10*((float) (*model->timebases)[state->timebase][0] / (*model->timebases)[state->timebase][1]); //total time shown in display |
There was a problem hiding this comment.
tmp_d2 is double but you're casting some stuff to float
| return SR_ERR_ARG; | ||
| if ((j = std_cg_idx(cg, devc->analog_groups, model->analog_channels)) < 0) | ||
| return SR_ERR_ARG; | ||
| g_ascii_formatd(float_str, sizeof(float_str), "%E", |
There was a problem hiding this comment.
%E means double, but you're casting arg to float
| if(sr_scpi_get_float(sdi->conn, (*config->scpi_dialect)[SCPI_CMD_GET_HORIZ_TRIGGERPOS], &tmp_float) != SR_OK) | ||
| return SR_ERR; | ||
|
|
||
| state->horiz_triggerpos = tmp_float / |
There was a problem hiding this comment.
using a float but casting to double...
| state->horiz_triggerpos *= -1; | ||
| //ToDo: This might be dependent on time ref setting | ||
|
|
||
| if (scope_state_get_array_option(sdi->conn, (*config->scpi_dialect)[SCPI_CMD_GET_TRIGGER_SOURCE], config->trigger_sources, config->num_trigger_sources, &state->trigger_source) != SR_OK) |
There was a problem hiding this comment.
whitespace offence + long line
| if (sr_scpi_get_string(scpi, command, &tmp) != SR_OK) | ||
| return SR_ERR; | ||
|
|
||
| if ((idx = std_str_idx_s(tmp, *array, n)) < 0) { |
There was a problem hiding this comment.
splitting assignment out of conditional would also allow you to g_free(tmp) immediately after, in one place only
| } | ||
|
|
||
| /* Truncate acquisition if a smaller number of samples has been requested. */ | ||
| if (devc->samples_limit > 0 && devc->logic_data->len > devc->samples_limit * devc->pod_count) |
There was a problem hiding this comment.
I know this is technically non-ambiguous but can we have a few extra () please
| switch (ch->type){ | ||
| case SR_CHANNEL_ANALOG: | ||
| info = (struct analog_channel_transfer_info *)ch->priv; | ||
| data = NULL; |
There was a problem hiding this comment.
data=NULL could go outside the switch{} block instead of each case probably
|
Thanks alot @fenugrec . I'll go over the code and implement the changes. |
|
Here is a good explanation how to squash your commits: https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together But keep in mind that your first commit is just the driver skeleton and the second commit is your implementation. Have a look at the HACKING file in the root of the repository. |
Would be good if the driver was called 546xx then, too, so that future support for other models doesn't make the driver name confusing. |
This is my implementation of a device driver for the Agilent 54621d mixed signal oscilloscope.
The driver allows to configure the device and download data from memory or single shot mode.
Downloading data is done in a hacky way:
The device natively only allows downloading either upto 2k Points from the display buffer or the entire captured waveform (upto 8M points). Downloading the entire waveform takes ages (~10Minutes per channel) since the Device only allows upto 57600Baud. To speed up download time I've implemented Downloading so it zooms and pans the captured waveform to use the Scopes display decimation filter to downsample to the requested sample rate and then download chunks of 2k points until the requested number of points is downloaded.
I'm planning to implement high resolution mode and pattern trigger, but other than that the driver is finished from my point of view.
It can be generalized to work with all devices of the 546xx family, however I've not yet gotten around to doing that and cannot test the other devices. (Might be able to get access to a 54624a)