Hi, On Sat, Oct 11, 2014 at 03:47:26PM +0300, Janne Huttunen wrote: > Here's my WIP version of a driver for my DER EE DE-5000 LCR meter.
Awesome work there, thanks a lot! In addition to supporting a new device (and a whole new device class as well!) your patch series was also very nicely split and the code quality looks pretty good as well :) I added missing (C) lines to your code and fixed one or two typos, everything else looked quite good. > The protocol between the host PC and the device is based on my > own reverse engineering from traffic dumps. As far as I know there > is no public document that describes it (feel free to point me into > it if you know better). It appears that once again the protocol > essentially just sends to the host the same information that is > currently visible on the display of the device. Yeah, this is a common thing for this kind of chips from Cyrustek. Btw, did you get a wiki account already? Please consider making a wiki page with device info, photos etc. for the new device and adding it to the supported hardware list. The protocol should also be documented in the wiki IMHO, we can (optionally) drop it from the code then later on. > I ended up exporting the used measurement model as a set of new > quantities instead of flags because I realized that I would have > required two of them and while the flags are very much a finite > resource, the MQs are almost infinite. That's ok, though the flags system may change or be extended in the future indeed. You're right that there's only a limited number of flags we can add with the current approach so we might switch to something else later (or add more uint64_t variables to extend the number of possible flags). > The test signal frequency was exported as a new config key. I was > not quite sure whether to use integer or float for the value, but > ultimately ended up using integer for it. It does not support > fractions of a hertz, but on the other hand it won't get any > rounding errors either. In a way something like 'double' would > probably have the "best of both worlds", but there seems to be > very little precedent for using it in sigrok and on the other > hand I don't know if fractions of a hertz are ever useful in the > context of ouput frequency(?). We might revisit this later on, we have a problem with integer-only frequencies anyway (e.g. the samplerate of a logic analyzer is an uint64_t, which has issues as well). For now, using an int is OK here. > During my research I also found out that the meter is based on > a common LCR meter chipset, so I separated the chipset code into > it's own module. This made the driver itself only a very thin shim. > All the "heavy lifting" is done by the chipset specific module that > can be shared if any of the other LCR meters based on the same > chipset ever get supported in sigrok. Yup, that's great. We might move some of the helper code elsewhere later, so far the dmm/* parsers contain only the pure parsers themselves, no serial port handling or other driver code. Generally that stuff belongs into the driver itself (unless we factor it out to some common place), but see below... > During the course of the implementation I also found myself wishing > that there were more generic helper functions available for the > drivers. Currently the amount of copy-pasted "boilerplate" the > driver needs is quite staggering compared to size of the actual > parser code. Yes, I know now that there is some way to generate > a "template" for a driver, but I realized that only after I had > already too far in the implementation (I guess real men(tm) don't > read documentation, my bad) and even with then I think the common > parts of the drivers could be generalized in helpers. We have a number of helpers in std.c for some common stuff, yeah. It's certainly possible to add a few more, though we should evaluate whether it's worth doing this for all the functions (might not be for 1-liner functions or such). I'll give this some more thought, and wait to see what other people think. > I have now written some for my ow driver, but at least some of them > could be useful if named properly, polished and put into some common > file so that other drivers could use them too. Feel free to suggest > any such routines in this driver (with desired name, API and > destination file, if possible). For driver helper code we used std.c for that so far. > One thing that I would also like to ask about is the sr_dev_driver > struct. Is it exported for some purpose or should it actually be > split into exported and private parts? Currently any changes in > there are potentially an API/ABI breakage, which seems unnecessary > to me, but maybe it actually is so by design(?). Good question. Not sure about this specific case right now (didn't check), but generally we did export a bit more than would actually be needed in the past. We've fixed some of that over time, but there may be room for more. Generally we certainly should hide non-API stuff from the lib user (SR_PRIV, opaque structs, static functions, etc) as much as possible, indeed. > I would like to > have at least one change in the driver operations, but it is not > so easy to do if the functions are considered to be a part of the > stable API/ABI. Which change do you mean? Please elaborate. There's one TODO (there's a bugzilla bug for it, too) to make scan() return an int (SR_OK, SR_ERR, and so on), for example. As for APIs, in git there's no problem to change APIs as we see fit at any time, we only make API guarantees for (major) tarball releases. E.g. anything in the 0.3.x series will be guaranteed to not contain API/ABI incompatibilities, but the 0.4.x series will certainly have a different API that will not be compatible. Cheers, Uwe. -- http://hermann-uwe.de | http://randomprojects.org | http://sigrok.org ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://p.sf.net/sfu/Zoho _______________________________________________ sigrok-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/sigrok-devel
