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
