Re: [sigrok-devel] PULL: sigrok-cli/libsrd: support for multiple protocol decoders

[Karl P. <ka...@tw...>]

Anything further?

Karl Palsson <ka...@tw...> wrote:
> 
> Gerhard Sittig <Ger...@gm...> 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