> I added missing (C) lines to your code and fixed one or two typos, > everything else looked quite good.
Ok, it was intended more as "for commenting" at this point and I was hoping to polish it more. But I guess what's done is done, and I'll have leave any "polishing" into incremental patches. > That's ok, though the flags system may change or be extended in the > future indeed. I actually already proposed an alternative that saves two flags and is at least in some ways more generic. Any comments on that? > 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. Ok. > 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... Yes, but if there ever is a driver for another meter that uses the same chipset and RS-232 connection, it will need the exact same code. Of course it can be copied, but I would hope that would not be necessary. On the other hand it could be argued that if the chipset is the same and the interface is the same too, why have the new driver at all. If there only was this one driver, then the serial code could definitely live in the driver itself. > 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. Fair enough, although I must repeat that especially in the beginning of the driver I wasn't writing it as much as just stitching together snippets I copied from some other driver. I know the "modern" SW design generally frowns upon midlayers, but IMO the alternative should not be tons of copy-paste but intelligent combining of generic helper functions with the driver specific exceptions coded where required. >> 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. Are the driver ops (the function pointers) exported on purpose? If not, they could be put into some opaque struct referenced by sr_dev_driver. >> 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. I'd like sr_dev_driver.cleanup() to get the address of the sr_dev_driver struct as a parameter. This would allow multiple drivers to share a common routine where feasible. ------------------------------------------------------------------------------ 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 sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
