From: Gerhard Sittig <redacted>
Date: Mon, 27 Jul 2020 19:58:55 +0000 (+0200)
Subject: sle44xx: rework data bits accumulation, and byte presentation
X-Git-Url: https://sigrok.org/gitaction?a=commitdiff_plain;h=80c76d2092814d2cd7d0f9fc6ddd6c0c937106dc;p=libsigrokdecode.git

sle44xx: rework data bits accumulation, and byte presentation

The 'databyte' is strictly local to the routine when 8 bits were seen.
The 'bitcount' is redundant and becomes obsolete when bits[] is a Python
list. The comment and the code disagreed, the wire is said to communicate
bits in LSB first order, the implemenation kept accumulating bits in the
reverse order (the annotation part, not the data byte math). Prefer the
common helper to convert bits to bytes.

There is uncertainty about the bit width "estimation" logic. The main
loop's .wait() conditions suggest that data bit values are valid for the
period of high CLK, which provides an easier and more robust condition
for annotation boundaries. Add a comment for now. The order of bit and
byte values' annotation emission is unfortunate, too.
---

diff --git a/decoders/sle44xx/pd.py b/decoders/sle44xx/pd.py
index 02bea42..a53a1ea 100644
--- a/decoders/sle44xx/pd.py
+++ b/decoders/sle44xx/pd.py
@@ -17,6 +17,7 @@
 ## along with this program; if not, see <http://www.gnu.org/licenses/>.
 ##
 
+from common.srdhelper import bitpack_lsb
 import sigrokdecode as srd
 
 class Pin:
@@ -81,8 +82,6 @@ class Decoder(srd.Decoder):
 
     def reset(self):
         self.ss = self.es = self.ss_byte = -1
-        self.bitcount = 0
-        self.databyte = 0
         self.bits = []
         self.cmd = 'RESET'
 
@@ -105,7 +104,6 @@ class Decoder(srd.Decoder):
         self.cmd = 'RESET'
         cls, texts = lookup_proto_ann_txt(self.cmd, {})
         self.putx([cls, texts])
-        self.bitcount = self.databyte = 0
         self.bits = []
         self.cmd = 'ATR' # Next data bytes will be ATR
 
@@ -115,47 +113,49 @@ class Decoder(srd.Decoder):
         # If I/O is rising -> command START
         # if I/O is falling -> command STOP and response data incoming
         self.cmd = 'CMD' if (io == 0) else 'DATA'
-        self.bitcount = self.databyte = 0
         self.bits = []
 
     # Gather 8 bits of data
     def handle_data(self, pins):
         rst, clk, io = pins
 
-        # Data is transmitted LSB-first.
-        self.databyte |= (io << self.bitcount)
-
-        # Remember the start of the first data/address bit.
-        if self.bitcount == 0:
+        # Remember the start of the first data/address bit. Collect
+        # bits in LSB first order. "Estimate" the bit's width at first,
+        # update end times as better data becomes available.
+        # TODO This estimation logic is imprecise and fragile. A single
+        # slightly stretched clock period throws off the following bit
+        # annotation. Better look for more reliable conditions. Available
+        # documentation suggests bit values are valid during high CLK.
+        if not self.bits:
             self.ss_byte = self.samplenum
-
-        # Store individual bits and their start/end samplenumbers.
-        # In the list, index 0 represents the LSB (SLE44xx transmits LSB-first).
-        self.bits.insert(0, [io, self.samplenum, self.samplenum])
-        if self.bitcount > 0:
-            self.bits[1][2] = self.samplenum
-        if self.bitcount == 7:
-            self.bitwidth = self.bits[1][2] - self.bits[2][2]
-            self.bits[0][2] += self.bitwidth
-
-        # Return if we haven't collected all 8 bits, yet.
-        if self.bitcount < 7:
-            self.bitcount += 1
+        bit_val = io
+        bit_ss = self.samplenum
+        bit_es = bit_ss # self.bitwidth is not known yet.
+        if self.bits:
+            self.bits[-1][2] = bit_ss
+        self.bits.append([bit_val, bit_ss, bit_es])
+        if len(self.bits) < 8:
             return
+        bitwidth = self.bits[-1][1] - self.bits[-2][1]
+        self.bits[-1][2] += bitwidth
 
-        self.ss, self.es = self.ss_byte, self.samplenum + self.bitwidth
+        # Get the data byte value, and byte's ss/es.
+        databyte = bitpack_lsb(self.bits, 0)
+        self.ss_byte = self.bits[0][1]
+        self.es_byte = self.bits[-1][2]
 
-        self.putb([Bin.SEND_DATA, bytes([self.databyte])])
+        self.ss, self.es = self.ss_byte, self.es_byte
+        self.putb([Bin.SEND_DATA, bytes([databyte])])
 
+        # TODO Present bit values earlier. As soon as their es is known.
         for bit_val, bit_ss, bit_es in self.bits:
             cls, texts = lookup_proto_ann_txt('BIT', {'bit': bit_val})
             self.put(bit_ss, bit_es, self.out_ann, [cls, texts])
 
-        cls, texts = lookup_proto_ann_txt(self.cmd, {'data': self.databyte})
+        cls, texts = lookup_proto_ann_txt(self.cmd, {'data': databyte})
         self.putx([cls, texts])
 
         # Done with this packet.
-        self.bitcount = self.databyte = 0
         self.bits = []
 
     def decode(self):
@@ -163,6 +163,7 @@ class Decoder(srd.Decoder):
             # Signal conditions tracked by the protocol decoder:
             # - RESET condition (R): RST = rising
             # - Incoming data (D): RST = low, CLK = rising.
+            #   TODO Add "RST low, CLK fall" for "end of DATA" here?
             # - Command mode START: CLK = high, I/O = falling.
             # - Command mode STOP: CLK = high, I/O = rising.
             (COND_RESET, COND_DATA, COND_CMD_START, COND_CMD_STOP,) = range(4)