Revision 109:310ac82cbbee, committed 2020-04-18

Comitter:

mjr

Date:

Sat Apr 18 19:08:55 2020 +0000

Parent:

108:bd5d4bd4383b

Child:

110:bf332f824585

Commit message:

TCD1103 DMA setup time padding to fix sporadic missed first pixel in transfer; fix TV ON so that the TV ON IR commands don't have to be grouped in the IR command first slots

Changed in this revision

diff -r bd5d4bd4383b -r 310ac82cbbee NewPwm/NewPwm.h
--- a/NewPwm/NewPwm.h	Tue Feb 18 21:33:30 2020 +0000
+++ b/NewPwm/NewPwm.h	Sat Apr 18 19:08:55 2020 +0000
@@ -244,7 +244,7 @@
     }
     
     // wait for the end of the current cycle
-    void waitEndCycle()
+    inline void waitEndCycle()
     {
         // clear the overflow flag (note the usual KL25Z convention for 
         // hardware status registers like this: writing '1' clears the bit)
@@ -282,8 +282,8 @@
     // be OFF 10% of the time and ON 90% of the time.  This is primarily
     // for complex timing situations where the caller has to be able to
     // coordinate the alignment of up/down transitions on the output; in
-    // particularly, it allows the caller to use the waitEndCycle() to sync
-    // with the falling ege on the output.
+    // particular, it allows the caller to use the waitEndCycle() to sync
+    // with the falling edge on the output.
     NewPwmOut(PinName pin, bool invertedCycle = false)
     {
         // determine the TPM unit number and channel

diff -r bd5d4bd4383b -r 310ac82cbbee Plunger/plunger.h
--- a/Plunger/plunger.h	Tue Feb 18 21:33:30 2020 +0000
+++ b/Plunger/plunger.h	Sat Apr 18 19:08:55 2020 +0000
@@ -538,53 +538,47 @@
                 //
                 // There's a shortcut we can use here to make this loop go a
                 // lot faster than the naive approach.  Note that 255 decimal
-                // is 1111111 binary.  Subtracting any other binary number
-                // (in the range 0..255) from 255 will have the effect of 
-                // simply inverting all of the bits in the original number.  
-                // So 255 - X == ~X for any X in 0..255.  That might not sound
-                // like a big deal, but it's actually pretty great, because it
-                // means that we only have to operate on the bits individually,
-                // rather than doing arithmetic on the bytes.  And if we can
-                // operate on the bits individually, we can operate on them
-                // in the largest groups we can with the processor's native
-                // instruction set, which in the case of ARM is 32-bit DWORDs.
-                // In other words, we can iterate over the array as a DWORD
-                // array rather than a BYTE array, which cuts loop iterations
-                // by a factor of 4.
+                // is 1111111 binary.  Subtracting any number in (0..255) from
+                // 255 is the same as inverting the bits in the other number.
+                // That is, 255 - X == ~X for all X in 0..255.  That's useful
+                // because it means that we can compute (255-X) as a purely
+                // bitwise operation, which means that we can perform it on
+                // blocks of bytes instead of individual bytes.  On ARM, we
+                // can perform bitwise operations four bytes at a time via
+                // DWORD instructions.  This lets us compute (255-X) for N
+                // bytes using N/4 loop iterations.
                 //
                 // One other small optimization we can apply is to notice that
-                // ~X == X ^ ~0, and X ^= ~0 happens to optimize to a single
-                // ARM instruction.  So we can make the ARM C++ compiler 
-                // translate this loop into three assembly instructions (XOR
-                // with immediate data and auto-increment pointer, decrement
-                // counter, jump if not zero), which is as fast as we could
-                // write it in assembly by hand.  (This really works in
-                // practice, too: I clocked this loop at 60us for the 
-                // 1500-pixel TCD1103 array.)
+                // ~X == X ^ ~0, and that X ^= ~0 can be performed with a
+                // single ARM instruction.  So we can make the ARM C++ compiler
+                // translate the loop body into just three instructions:  XOR 
+                // with immediate data and auto-increment pointer, decrement 
+                // the counter, and jump if not zero.  That's as fast we could
+                // do it in hand-written assembly.  I clocked this loop at 
+                // 60us for the 1536-pixel TCD1103 array.
+                //
+                // Note two important constraints:
+                //
+                //  - 'pix' must be aligned on a DWORD (4-byte) boundary.
+                //    This is REQUIRED, because the XOR in the loop uses a
+                //    DWORD memory operand, which will halt the MCU with a
+                //    bus error if the pointer isn't DWORD-aligned.
+                //
+                //  - 'n' must be a multiple of 4 bytes.  This isn't strictly
+                //    required, but if it's not observed, the last (N - N/4)
+                //    bytes won't be inverted.
+                //
+                // The only sensor that uses a negative image is the TCD1103.
+                // Its buffer is DWORD-aligned because it's allocated via
+                // malloc(), which always does worst-case alignment.  Its
+                // buffer is 1546 bytes long, which violates the multiple-of-4
+                // rule, but inconsequentially, as the last 14 bytes represent
+                // dummy pixels that can be ignored (so it's okay that we'll 
+                // miss inverting the last two bytes).
                 //
                 uint32_t *pix32 = reinterpret_cast<uint32_t*>(pix);
                 for (int i = n/4; i != 0; --i)
                     *pix32++ ^= 0xFFFFFFFF;
-    
-                // Note!  If we ever needed to do this with a sensor where
-                // the pixel count isn't a multiple of four, we'd have to
-                // add some code here to deal with the stragglers (the one,
-                // two, or three extra pixels after the last group of four).
-                // That's not an issue with any currently supported sensor,
-                // nor is it likely to be in the future (because any large
-                // pixel array will be built out of repeated submodules, 
-                // which inherently makes power-of-two bases likely, and
-                // because engineers tend to have a bias for round numbers
-                // even when they have to choose arbitrarily).  So I'm not
-                // going to test for this possibility, to save the run-time
-                // cost.  And the worst that happens is we see a couple of
-                // glitchy-looking pixels at the end of the array in the 
-                // visualizer on the client.  But just in case, here's the
-                // code that would be needed...
-                //
-                // int extraPix = n & 3;  // remainder of n/4
-                // for (int i = 0; i < extraPix; ++i)
-                //     reinterpret_cast<uint8_t*>(pix32)[i] ^= 0xFF;
             }            
 
             // send the pixels in report-sized chunks until we get them all

diff -r bd5d4bd4383b -r 310ac82cbbee Plunger/tcd1103Sensor.h
--- a/Plunger/tcd1103Sensor.h	Tue Feb 18 21:33:30 2020 +0000
+++ b/Plunger/tcd1103Sensor.h	Sat Apr 18 19:08:55 2020 +0000
@@ -58,7 +58,7 @@
 {
 public:
     PlungerSensorImageInterfaceTCD1103(PinName fm, PinName os, PinName icg, PinName sh)
-        : PlungerSensorImageInterface(1500), sensor(fm, os, icg, sh)
+        : PlungerSensorImageInterface(1546), sensor(fm, os, icg, sh)
     {
     }
 
@@ -89,7 +89,7 @@
 {
 public:
     PlungerSensorTCD1103(PinName fm, PinName os, PinName icg, PinName sh)
-        : PlungerSensorImage(sensor, 1500, 1499, true), sensor(fm, os, icg, sh)
+        : PlungerSensorImage(sensor, 1546, 1545, true), sensor(fm, os, icg, sh)
     {
     }
     
@@ -102,11 +102,41 @@
     // we see starting at the "far" end.
     virtual bool process(const uint8_t *pix, int n, int &pos, int& /*processResult*/)
     {
+        // The TCD1103's pixel file that it reports on the wire has the 
+        // following internal structure:
+        //
+        //   16 dummy elements, fixed at the dark charge level
+        //   13 light-shielded pixels (live pixels, covered with a shade in the sensor)
+        //   3 dummy "buffer" pixels (to allow for variation in shade alignment)
+        //   1500 image pixels
+        //   14 dummy elements (the data sheet doesn't say exactly what these are physically)
+        //
+        // The sensor holds the 16 dummy elements at the dark charge level,
+        // so they provide a reference point for the darkest reading possible.
+        // The light-shielded pixels serve essentially the same purpose, in
+        // that they *also* should read out at the dark charge level.  But
+        // the shaded pixels can be also used for diagnostics, to distinguish
+        // between problems in the CCD proper and problems in the interface
+        // electronics.  If the dummy elements are reading at the dark level
+        // but the shielded pixels aren't, you have a CCD problem; if the
+        // dummy pixels aren't reading at the dark level, the interface
+        // electronics are suspect.
+        //
+        // For our purposes, we can simply ignore the dummy pixels at either
+        // end.  The diagnostic status report for the Config Tool sends the
+        // full view including the dummy pixels, so any diagnostics that the
+        // user wants to do using the dummy pixels can be done on the PC side.
+        //
+        // Deduct the dummy pixels so that we only scan the true image
+        // pixels in our search for the plunger edge.
+        int startOfs = 32;
+        n -= 32 + 14;
+        
         // Scan the pixel array to determine the actual dynamic range 
         // of this image.  That will let us determine what consistutes
         // "bright" when we're looking for the bright spot.
         uint8_t pixMin = 255, pixMax = 0;
-        const uint8_t *p = pix;
+        const uint8_t *p = pix + startOfs;
         for (int i = n; i != 0; --i)
         {
             uint8_t c = *p++;
@@ -120,7 +150,7 @@
         
         // Scan for the first bright-enough pixel.  Remember that we're
         // working with a negative image, so "brighter" is "less than".
-        p = pix;
+        p = pix + startOfs;
         for (int i = n; i != 0; --i, ++p)
         {
             if (*p < threshold)

diff -r bd5d4bd4383b -r 310ac82cbbee TCD1103/TCD1103.h
--- a/TCD1103/TCD1103.h	Tue Feb 18 21:33:30 2020 +0000
+++ b/TCD1103/TCD1103.h	Sat Apr 18 19:08:55 2020 +0000
@@ -159,16 +159,18 @@
         
         // ADC sample conversion time.  This must be calculated based on the
         // combination of parameters selected for the os() initializer above.
-        // See the KL25 Sub-Family Reference Manual, section 28.4.45, for the
-        // formula.
-        const float ADC_TIME = 2.2083333e-6f; // 6-cycle long sampling, no averaging
+        // See the KL25 Sub-Family Reference Manual, section 28.4.4.5, for the
+        // formula.  We operate in single-sample mode, so when you read the
+        // Reference Manual tables, the sample time value to use is the
+        // "First or Single" value.
+        const float ADC_TIME = 2.1041667e-6f; // 6-cycle long sampling, no averaging
 
         // Set the TPM cycle time to satisfy our timing constraints:
         // 
         //   Tm + epsilon1 < A < 2*Tm - epsilon2
         //
         // where A is the ADC conversion time and Tm is the master clock
-        // period, and the epsilons are a margin of safety for any 
+        // period, and the epsilons provide a margin of safety for any 
         // non-deterministic component to the timing of A and Tm.  The
         // epsilons could be zero if the timing of the ADC is perfectly
         // deterministic; this must be determined empirically.
@@ -185,7 +187,7 @@
         // fast plunger motion accurately.  Empirically, we can get reliable
         // results by using half of the ADC time plus a small buffer time.
         //
-        fm.getUnit()->period(masterClockPeriod = ADC_TIME/2 + 0.1e-6f);
+        fm.getUnit()->period(masterClockPeriod = ADC_TIME/2 + 0.25e-6f);
         
         // Start the master clock running with a 50% duty cycle
         fm.write(0.5f);
@@ -249,12 +251,6 @@
             pix = pix2;
             t = t2;
         }
-        
-        // The raw pixel array we transfer in from the sensor on the serial 
-        // connection consists of 32 dummy elements, followed by 1500 actual
-        // image pixels, followed by 14 dummy elements.  Skip the leading 32 
-        // dummy pixels when passing the buffer back to the client.
-        pix += 32;
     }
     
     // release the client's pixel buffer
@@ -370,6 +366,9 @@
     //
     uint32_t gen_SH_ICG_pulse(bool start_dma_xfer)
     {
+        // Make sure the ADC is stopped
+        os.stop();
+
         // If desired, prepare to start the DMA transfer for the ADC data.
         // (Set up a dummy location to write in lieu of the DMA register if
         // DMA initiation isn't required, so that we don't have to take the
@@ -436,9 +435,9 @@
         // with the start of an ADC cycle.  If we get that wrong, all of our
         // ADC samples will be off by half a clock, so every sample will be
         // the average of two adjacent pixels instead of one pixel.  That
-        // would lose half of the image resolution, which would obviously
-        // be bad.  So make certain we're at the tail end of an ADC cycle
-        // by waiting for the ADC "ready" bit to be set.
+        // would have the effect of shifting the image by half a pixel,
+        // which could make our edge detection jitter by one pixel from one
+        // frame to the next.  So we definitely want to avoid this.
         //
         // The end of the SH pulse triggers the start of a new integration 
         // cycle, so note the time of that pulse for image timestamping 
@@ -447,15 +446,12 @@
         // represent the pixels from the last time we pulsed SH.
         //
         icg = logicLow;
-        icg = logicLow;  // for timing, adds about 60ns
-        icg = logicLow;  // ditto, another 60ns, total is now 120ns > min 100ns
-        icg = logicLow;  // one more to be safer
+        icg = logicLow;  // for timing, adds about 150ns > min 100ns
 
         sh = logicHigh;  // take SH high
         
         wait_us(1);      // >1000ns delay
         sh = logicHigh;  // a little more padding to be sure we're over the minimum
-        sh = logicHigh;  // more padding
         
         sh = logicLow;   // take SH low
         
@@ -514,9 +510,6 @@
         //   - End the ICG pulse
         //
         
-        // disable the TPM->ADC trigger and abort the current conversion
-        os.stop();
-
         // Enable the DMA controller for the new transfer from the ADC.
         // The sensor will start clocking out new samples at the ICG rising
         // edge, so the next ADC sample to complete will represent the first
@@ -530,10 +523,24 @@
         // wait for the start of a new master clock cycle
         fm.waitEndCycle();
         
+        // Wait one more cycle to be sure the DMA is ready.  Empirically,
+        // this extra wait is actually required; evidently DMA startup has
+        // some non-deterministic timing element or perhaps an asynchronous
+        // external dependency.  In any case, *without* this extra wait,
+        // the DMA transfer sporadically (about 20% probability) misses the
+        // very first pixel that the sensor clocks out, so the entire image
+        // is shifted "left" by one pixel.  That makes the position sensing
+        // jitter by a pixel from one frame to the next according to whether
+        // or not we had that one-pixel delay in the DMA startup.  Happily,
+        // padding the timing by an fM cycle seems to make the DMA startup
+        // perfectly reliable.
+        fm.waitEndCycle();
+        
         // Okay, a master clock cycle just started, so we have about 1us 
         // (about 16 CPU instructions) before the next one begins.  Resume 
         // ADC sampling.  The first new sample will start with the next
-        // TPM cycle 1us from now.  (This step takes about 3 instructions.)
+        // TPM cycle 1us from now.  This step itself takes about 3 machine
+        // instructions for 180ns, so we have about 820ns left to go.
         os.resume();
         
         // Eerything is queued up!  We just have to fire the starting gun

diff -r bd5d4bd4383b -r 310ac82cbbee main.cpp
--- a/main.cpp	Tue Feb 18 21:33:30 2020 +0000
+++ b/main.cpp	Sat Apr 18 19:08:55 2020 +0000
@@ -4780,8 +4780,11 @@
                 if ((cfg.IRCommand[i].flags & IRFlagTVON) != 0)
                 {
                     // It's a TV ON command - check if it's the one we're
-                    // looking for.
-                    if (n == tvon_ir_state)
+                    // looking for.  We can match any code starting at the
+                    // current state.  (We ignore codes BEFORE the current
+                    // state, because we've already processed them on past
+                    // iterations.)
+                    if (n >= tvon_ir_state)
                     {
                         // It's the one.  Start transmitting it by
                         // pushing its virtual button.