Hi, On Sun, Sep 20, 2015 at 07:25:13PM +0200, Martin Lederhilger wrote: > this driver is for the above mentioned oscilloscope series. The patches > in the attachment should apply to master.
Great work, thanks a lot! See below for a quick review with mostly minor cosmetics. Apart from that it looks pretty good and certainly mergeable in the next iteration. > oscilloscope. It seems to contain an serial to USB converter, but I was > unable to establish a connection. Are you sure both USB and RS232 have the same protocol and features behind them? The vendor webpage has two different software package names (FreeCapture and FreeView), maybe the USB port is meant for something different, or has another protocol or such? > Please integrate the driver into Sigrok. Let me know if you need a > photograph from the scope - for example for the web page - I do not want > to post it to the mailing list, because the image is rather big. Sure, one or more (self-made) photos would be great for the wiki. Please join us on IRC (#sigrok on FreeNode) for a wiki account if you like to upload the photo and add a wiki page for the device. If that's too much hassle you can also send me a photo via email and I'll upload it for you. Let me know if "Public domain" or "CC-BY-SA 3.0" is fine for you as license for the photo(s). Examples: http://sigrok.org/wiki/Rigol_DS1052E http://sigrok.org/wiki/Rigol_DS1000_series Or a random example page with more photos: http://sigrok.org/wiki/TechTools_DigiView_DV1-100 > +SR_DRIVER([gwinstek gds-800], [gwinstek-gds-800], [libserialport]) The first one should be human-readable, i.e. "GW Instek GDS-800". > +SR_PRIV struct sr_dev_driver gwinstek_gds_800_driver_info = { > + .longname = "gwinstek gds-800", "GW Instek GDS-800 series" please. > + sdi->channel_groups = NULL; > + > + sr_scpi_hw_info_free(hw_info); > + Please remove the extra tabs for the "empty" lines (in the whole driver), there should be no trailing whitespace anywhere. > @@ -88,14 +147,19 @@ static int config_get(uint32_t key, GVariant **data, > const struct sr_dev_inst *s > const struct sr_channel_group *cg) > { > int ret; > - > - (void)sdi; > + struct dev_context *devc; > + > (void)data; data is used below, so you can drop the above line (which is only there to prevent a compiler warning). Same thing in other places maybe. > (void)cg; > + > + if (!sdi || !(devc = sdi->priv)) > + return SR_ERR_ARG; > > ret = SR_OK; > switch (key) { > - /* TODO */ > + case SR_CONF_SAMPLERATE: > + *data = g_variant_new_uint64(devc->sample_rate); > + break; > + /* Initialize device context; */ > + devc->state = START_AQUISITION; Should be START_ACQUISITION (typo). > + if(devc->cur_rcv_buffer_position < data_size) > + return SR_ERR; //not finished yet Please use C-style /* */ comments for more consistency with the rest of the code-base. > + scpi = sdi->conn; > + > + if (!(revents == G_IO_IN || revents == 0)) > + return TRUE; > + > + switch(devc->state) > + { > + case START_AQUISITION: > + if (sr_scpi_send(scpi, ":TRIG:MOD 3") != SR_OK) { The case lines should have the same indentation level as the switch itself. switch(devc->state) { case START_AQUISITION: ... break; case WHATEVER: ... break; } > + if(read_data(sdi, cb_data, scpi, devc, > devc->data_size_digits) == SR_OK) > + { Always one space after "if" (and while/for/...) please, and brace on the same line. > + case WAIT_FOR_TRANSFER_OF_SAMPLE_RATE_COMPLETE: > + if(read_data(sdi, cb_data, scpi, devc, sizeof(float)) > == SR_OK) > + { > + //contrary to the documentation, this field is > transfered with most > + //significant byte first! > + sample_rate = RB32(devc->rcv_buffer); > + memcpy(&devc->sample_rate, &sample_rate, > sizeof(float)); > + devc->state = > WAIT_FOR_TRANSFER_OF_CHANNEL_INDICATOR_COMPLETE; > + > + if(!devc->df_started) { > + std_session_send_df_header(sdi, > LOG_PREFIX); > + > + packet.type = SR_DF_FRAME_BEGIN; > + sr_session_send(sdi, &packet); > + > + devc->df_started = TRUE; > + } > + } These kind of contructs can be inverted often (if you want), that reduces the nesting level and is usually much more readable. case WAIT_FOR_TRANSFER_OF_SAMPLE_RATE_COMPLETE: if (read_data(sdi, cb_data, scpi, devc, sizeof(float)) != SR_OK) break; sample_rate = RB32(devc->rcv_buffer); memcpy(&devc->sample_rate, &sample_rate, sizeof(float)); /* ... */ break; > +#define ANALOG_CHANNELS 2 > +#define VERTICAL_DIVISIONS 10 > +#define MAX_SAMPLES 125000 > +#define MAX_RCV_BUFFER_SIZE (MAX_SAMPLES*2) These #defines are only used in protocol.c, no need to put them in protocol.h, they can be hidden away in protocol.c. > +/** Private, per-device-instance driver context. */ > +struct dev_context { > + enum gds_state state; > + guint64 cur_acq_frame; > + guint64 frame_limit; Please use uint64_t here. Cheers, Uwe. -- http://hermann-uwe.de | http://randomprojects.org | http://sigrok.org ------------------------------------------------------------------------------ _______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
