Hi, On Fri, Apr 03, 2020 at 10:49:03AM +0000, Perisanu, Teo wrote: > Analog Devices would like to contribute to the sigrok project with 4 > protocol decoders.
Great, thanks a lot! > The patches for these decoders can be found at: A GitHub pull-request against github.com/sigrokproject is fine btw, no need for patch files. The files for sigrok-dumps are all merged, with very minor cosmetic/whitespace fixes in the READMEs squashed in, thanks! The decoders all all merged as well, with a few changes as follows (some of them I squashed into your original commits to avoid cluttering git history too much with directory renames, whitespace changes and such): * The PD names/dirs are now: adxl345, ltc242x, ltc26x7, ad5626, ad79x0. I tried to use a name that allows for additional support for more models (mentioned in the same datasheet) later on. E.g. ltc242x could gain LTC2421 support via a 'chip' option, like you already added in ltc26x7. * Some devices can use multiple protocols (e.g. SPI or I2C), for those it would be great if you could provide some more samples for sigrok-dumps, we might want to add a respective PD for the I2C variant as well later on, for example. Yes, this would somewhat duplicate some code chunks in PDs; if it's not a lot of code, that's fine; for larger chunks, some common things like register tables can be moved to the common/ directory, if needed. If possible, also hook up all pins to the sample files ideally (e.g. LDAC#, CLR# and such). This can be helpful for better understanding what is happening on the lines, as well as for potentially improving PDs later. * This is fully optional, but we're currently in the process of improving the readability of PDs a bit by using SrdIntEnum and friends, so I've converted adxl345 as a quick demo. * For PDs with multiple channels, I've put each one on an extra annotation row, which allows the user to more easily see (graphically) what's going on, e.g. in PulseView. * I fixed two issues I noticed in adxl345, but please double-check, just in case. There's one remaining potential issue I haven't addressed: handle_reg_0x22/_0x23/_0x24 all use "Latent" as string; is that intentional? Or should those be Latent/Window/THRESH_ACT or such? * I've moved the ad5626 sample files from spi/ to dac/. IMHO this doesn't "really" count as SPI even though the SPI decoder works somewhat for more or less any random sync/clocked protocol. I've merged the ad5626 decoder for now, but this one should probably be eventually replaced with a non-stacked version which implements the DAC protocol directly, including LDAC# and CLR# handling, etc. Another such decoder we already have is tlc5620 btw. if you want to have a look at that. That one is not perfect and could use various improvements as well, but it's an example for another non-SPI DAC. And yes, sooner or later we'd like to have support for additional logic input channels for stacked decoders, but that's currently not possible, yet. * Finally, I've added a few test-cases in our sigrok-test repo, based on your .sr files in sigrok-dumps. We use this to (regression-)test decoders as much as possible. Thanks again for your contributions! Cheers, Uwe. -- http://hermann-uwe.de | http://randomprojects.org | http://sigrok.org _______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
