Hi, sorry for the delay!
On Thu, Nov 26, 2015 at 09:35:47AM +0100, Stefan Ekenberg wrote: > I noticed that there has been no comments on this patch. Just let > me know if you would like to have some more explanation of the > problem, or in general, if you would like me to do anything in order > to ease the integration of this patch! > > On Wed, Nov 11, 2015 at 04:46:51PM +0100, Stefan Ekenberg wrote: > > Fixes the problem that find_clk_edge() mistakenly detects a clock edge > > on the very first sampled value in case SPI mode 1 is used and the > > first sampled clock value i 0. This happens because oldclk is > > initialized to 1 and a clock sample of 0 will then mistakenly trigger a > > 1->0 transition (falling edge). Problem is solved by initializing > > oldclk with the first sampled value. The method is OK in theory, I think we're doing something similar in other PDs. However, unfortunately it breaks in some important use-cases: when the user triggers on certain pins. In such cases (e.g. trigger on CLK rising edge), the very first sample (sample 0) will be 1. But the sample before that (sample -1), which we didn't capture, was 0. Using the method above will not allow us to see this transition since we're basically setting sample -1 to whatever sample 0 is, so there's never a transition there. This is a generic problem in *all* PDs really, when combined with edge triggering (not SPI specific). My proposed solution (for all PDs consistently) is something like this: a) Provide sane defaults in the PD for the initial/assumed sample. For some PDs this is easy and useful, e.g. for I²C you want SCL=1 and SDA=1 as both pins have pullups and their idle state is 1. There are more complex cases though, e.g. on UART RX=1 and TX=1 makes sense often (the idle state is high, a falling edge indicates a start bit). Alas, the PD also has options to invert RX/TX, so here it depends on the PD options which default makes sense. This is fine and can be implemented no problem. Then there's also the SPI case where e.g. the CLK default depends on the SPI mode, which is also a PD option. The CS# default also depends on PD options (the CS# polarity), etc. But even with sane defaults you can still get incorrect decoding for some triggering setups. Thus: b) We also need to provide (in addition to per-PD sane defaults, if possible) a generic method for the user to set/override the initial/assumed value (0 or 1) for *any* of the input pins for a PD. This allows the user to override the defaults in cases where the user knows they're wrong (and the PD cannot possibly know that). I'm happy to hear other opinions on this topic and/or better ways to handle all cases. The above is the best I could come up with so far (that covers all cases). Facility a) can easily be implemented in all PDs right now, just set some defaults in __init__() for the pins. This would cover *most* cases, but not all of them. Facility b) needs to be designed and implemented in libsigrokdecode first, and will cover *all* cases then, AFAICS. Btw, for the specific SPI issue, checkout the new files in spi/allmodes/ in the sigrok-dumps repo. I added a bunch of permutations of all kinds of settings there, which you can use to check/verify any potential fixes (I might add some more later). If anyone feels like doing this, we'll also want "sigrok-test" repo test-cases for all of these SPI files. Please mention here if anyone starts working on this to avoid duplicate work. Cheers, Uwe. -- http://hermann-uwe.de | http://randomprojects.org | http://sigrok.org ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140 _______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
