Hi,

On Thu, Aug 27, 2015 at 11:21:49AM +0200, Daniel Glöckner wrote:
> The Hung-Chang DSO-2100 is a parallel port PC oscilloscope sold back
> in 1999 under brand names like Protek and Voltcraft.

Thanks a lot for writing the driver!

As per IRC discussion, could you please indeed add your changeѕ on top
of the 'new-driver' generated stubs, and split the code into api.c and
protocol.[ch] as in the other drivers? Thanks!

 
> +#include <ieee1284.h>
> +#include <string.h>
> +#include <glib.h>
> +#include <libsigrok/libsigrok.h>
> +#include "libsigrok-internal.h"
> +
> +#define LOG_PREFIX "hung-chang-dso-2100"
> +
> +/* The firmware can be in the following states:
> + *  0x00     Temporary state during initializazion

initialization


> + *           Automatically transitions to state 0x01
> + *  0x01     Idle, this state updates calibration caps
> + *           Send 0x02 to go to state 0x21
> + *           Send 0x03 to go to state 0x03
> + *           Send 0x04 to go to state 0x14
> + *  0x21     Trigger is armed, caps are _not_ updated
> + *           Send 0x99 to check if trigger event occured
> + *                   if triggered, goes to state 0x03
> + *                   else stays in state 0x21
> + *           Send 0xFE to generate artifical trigger event

artificial


> + *                   returns to state 0x21
> + *                   but next 0x99 will succeed
> + *           Send 0xFF to go to state 0x03 (abort capture)
> + *  0x03     Extracts two 500 sample subsets from the 5000
> + *           sample capture buffer for readout
> + *           When reading samples, the FPGA starts at the
> + *           first of the 1000 samples an automatically

and


> +     for (i = 0; i < 2; i++) {

Maybe add NUM_CHANNELS for this 2 for slightly more readable code.


> +#ifndef SCAN_ALL_PARALLEL_PORTS
> +     if (!conn)
> +             return NULL;
> +#endif

Hm, this should probably go, blindly scanning parallel ports should be
avoided, we don't know which devices are connected and how they
(mis)react to data sent to them (same reason why we don't scan on serial
ports either).


> +static int config_channel_set(const struct sr_dev_inst *sdi,
> +                           struct sr_channel *channel,

Please use "ch" as name for "struct sr_channel *" items to be a
bit more consistent with the rest of the code-base (and "cg" for
channel groups).


> +static void dso2100_skip_samples(struct parport *port, uint8_t ctrl, size_t 
> num)
> +{
> +     while(num--) {
             ^
      missing space here


> +                     if (buf[1000] == 0x01 && buf[1001] == 0xfe && buf[1002] 
> == 0x80) {
> +                             if (!offset) {
> +                             push_samples(sdi, buf, 6);
> +                                             packet.type = SR_DF_TRIGGER;

The indentation doesn't look correct here.


Apart from the minor stuff I noticed in the quick review above, the driver
looks really nice and clean. Great job!


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

Reply via email to