Anything further?
Karl Palsson <ka...@tweak.net.au> wrote: > > Gerhard Sittig <gerhard.sit...@gmx.net> wrote: > > On Tue, Nov 29, 2016 at 15:59 -0000, Karl Palsson wrote: > > > > > > As discussed on IRC a little recently, and in issue: > > > http://sigrok.org/bugzilla/show_bug.cgi?id=868 > > > > > > First, please consider pulling into sigrok-cli support for > > > multiple protocol decoders and dropping the -S option [1] By > > > updating sigrok-cli first, it gains proper support for stacking > > > decoders based on the IDs returned by libsigrokdecode, instead of > > > it having implicit knowledge of the ids. > > > > > > Branch at: > > > https://github.com/karlp/sigrok-cli/tree/pulls/multipro Commits: > > > + 0b9982b drop -S stack option > > > + f999f28 Support multiple protocol decoder stacks > > > + d9de37a include decoder instance id in normal outputs [2] > > > + 27105be valgrind: Clear more unfreed memory issues > > > > > > [ ... ] > > > > > > [2] This changes the CLI output to include the decoder instance > > > id. Again, how critically important is the CLI output? I would > > > presume that it's not intended for scraping anyway, the decoders > > > and output files are for those purposes. > > > > Does it change the output for the "regular" (currently only > > supported) use with a single decoder session? Can it be made > > identical to previous behaviour, and only prefix the output > > with the decoder's ID when multiple decoder sessions are in > > use? Why am I thinking of grep(1) and -H/-h? Users might want > > to pick the format they will receive. > > Yes, it does. I fairly strongly feel that attempting to > maintain scraping compatibility, even nominally, is a fools > errand, and the entire purpose of having decoders and output > formats. While it's "easy" to add in the code here to make it > "compatible" for single decoders, and "new" for multiple > decoders, that overlooks two issues that I feel trump any > concerns here: > > a) it makes the output _inconsistent_ when you add a second > decoding instance. Now any downstream (unsupported) scraping > has to support two formats, instead of just one. b) there's no > enforcement that the decoders themselves don't change outputs, > so trying to stay compatible at the higher level of sigrok-cli > only is fairly meaningless. > > > > > I would not bet that nobody processes sigrok-cli output in > > programmatic ways. After all it's a command line tool made for > > piped operation. And users can specify in detail which > > annotation of which decoder they'd like to receive, having the > > output change its format might be unexpected. > > Indeed, but as above, I don't feel that the command line output > of a program that doesn't claim it's command line output to be > an API should be maintained as an API. Consider "iw" which > explicitly even states, "don't scrape this output! use libiw!" > (or something to that effect) Same point about decoders > changing things too. > > > > There's another commit which makes -A apply to all decoders of > > the specified type, instead of their instance. I'd question > > this one, and would suggest that the decoder instance ID is > > used for -A associations. > > > > For the regular case of a single stack with multiple > > (different) decoders, the instance ID is the decoder name. For > > cases where multiple decoders of the same type are in a decoder > > stack, or multiple stacks contain the same type(s) of decoder, > > you'd very probably want to address the annotations per > > instance, not per decoder type. Users can specify the decoder > > instance's ID with the -P option, or might as well foretell the > > instance ID since its construction is "intuitive". Depending on > > the implementation it's either unique per decoder stack, or the > > complete set of stacks. > > The commit you refer to is actually just making it stay > consistent with current behaviour. You can only have a single > -A annotation, and it behaves as before, you provide the > decoder name and the annotations you want. You're absolutely > right, that with the ability to have multiple decoders, you may > wish to have different annotations for each decoder. I > deliberately left that out, as I believe it's a separate > feature, and has questions that need further thought. > > a) do you leave -A to do decoder level selecton? > b) should there be parameters in the the -P strings? > c) if you don't merge it into -P, how do you know the ids that > will be used? d) more? > > My work is intended to move from "one decoder stack, one list > of decoder annotations" to "many decoder stacks, one list of > decoder annotations" and the final commit just makes that work. > Remember that the original behaviour of -A, while technically > operating on the instance, was actually only ever operating on > the decoder type, not the instance. (Because you could only > have one) > > I feel fairly strongly that that step above is a rather useful > improvement, and that opinions and requests on "multiple lists > of decoder annotations" should be considered out of scope. > Further, this is exactly how pulseview behaves. You can have > multiple decoders, but you can't control the annotations per > decoder. I can't find any bugs filed against that as a missing > feature either, so clearly it's useful enough already. > > > > Subsequently, Please consider pulling into libsigrokdecode > > > support for multiple instances of a decoder in a session. Branch > > > available at: > > > https://github.com/karlp/libsigrokdecode/tree/pulls/inst-ids > > > Commits: > > > + 8dd7fde valgrind: safely iterate lists > > > + fe20e5e valgrind: free channels > > > + 3b722ff support adding multiple instances of a decoder > > > + fc841fb look up instances by id in the full stack > > > > Maybe invert the order? Make the find routine search the whole > > stack first, and use that feature in the unique ID creation > > afterwards? Split the creation of the unique ID and the cleanup > > of releasing resources maybe? Could improve bisectability and > > maintenance work. Would give proper credit to Soeren if you > > used his patch and then built on top of it. Reduces merge > > conflicts, too. > > the valgrind fixes have already landed now. The latter two > should probably just be squished to one. I don't see any point > in attempting to preserve Soeren's original commit. It was > reported as untested, and leaked memory and was incomplete. > There's none of it remaining, and crediting the idea is the > best I can do. Putting it as a broken commit in the commit log > just to immediately fix it would seem like a poor choice for a > clean history. > > Sincerely, > Karl Palsson
signature.asc
Description: OpenPGP Digital Signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
