Addressing review comments for StrideReadings class and related test cases:

- removing inner class, and recording of heartbeat timestamps
- fixing issue with rollover by replacing % operator with modulo operation
- comparing problematic firmware id against byte[] instead of String
- removing zephyr packet debug loggin
- not reassigning protobuffers Builders
- moving decision to use workaround into setCadence function
- license and javadoc author for testcase class
- proposal: using introspection to access private fields in testcase in order to determine expected availablity of values from getCadence, alternatively modify access to protected.
- modifying rollover test, hopefully more clear now.
- adding asserts during the loop in this test case.

MyTracks/src/com/google/android/apps/mytracks/services/sensors/StrideReadings.java

@@ -19,43 +19,33 @@ import java.util.LinkedList;
 import java.util.List;
 
 /**
- * Storage of a history of readings of the Zephyr stride counter,
+ * A history of Zephyr stride counter reading.
- * in order to derive a correct cadence value from it,
+ * These can be used as an alternate method to calculate the correct cadence.
- * working around an issue with the HxM's firmware. 
+ * This is a work around for an issue with some HxM firmware.
- *
  * @author Dominik Ršttsches
  */
 public class StrideReadings {
-  private static class StrideReading {
-    // TODO: Check whether 1Hz assumption is okay for cadence calculation
-    // otherwise use timeMs, which is taken from heart beat timestamp.
-    @SuppressWarnings("unused")
-    public int timeMs;
-    public int numStrides;
-
-    StrideReading(int newTimeMs, int newNumStrides) {
-      timeMs = newTimeMs;
-      numStrides = newNumStrides;
-    }
-  }
 
   private static final int NUM_READINGS_FOR_AVERAGE = 10;
   private static final int MIN_READINGS_FOR_AVERAGE = 5;
 
   protected static final int CADENCE_NOT_AVAILABLE = -1;
 
-  private List<StrideReading> strideReadingsHistory;
+  // TODO: Check whether 1Hz assumption is okay for cadence calculation
+  // otherwise add heart beat timestamp to this list and compute
+  // cadence from these timestamps.
+  private List<Integer> strideReadingsHistory;
 
   public StrideReadings() {
-    strideReadingsHistory = new LinkedList<StrideReading>();
+    strideReadingsHistory = new LinkedList<Integer>();
   }
 
-  public void updateStrideReading(int timeInMs, int numStrides) {
+  public void updateStrideReading(int numStrides) {
     // HRM/HxM documentation says, transmission frequency is 1 Hz, 
     // let's keep last NUM_READINGS_FOR_AVERAGE readings.
     // TODO: Calibrate this using a reliable footpod / cadence sensor, 
     // otherwise use heartbeat timestamp for calculation. 
-    strideReadingsHistory.add(0, new StrideReading(timeInMs, numStrides));
+    strideReadingsHistory.add(0, numStrides);
     while(strideReadingsHistory.size() > NUM_READINGS_FOR_AVERAGE) {
       strideReadingsHistory.remove(strideReadingsHistory.size()-1);
     }
@@ -66,14 +56,18 @@ public class StrideReadings {
       // Bail out if we cannot really get a meaningful average yet.
       return CADENCE_NOT_AVAILABLE;
     }
-    // compute assuming 1 stride reading/second
+    // Compute assuming 1 stride reading/second.
     int timeSinceOldestReadingSecs = strideReadingsHistory.size() - 1; 
-    int stridesThen = strideReadingsHistory
+    int stridesThen = strideReadingsHistory.get(strideReadingsHistory.size()-1);
-      .get(strideReadingsHistory.size()-1).numStrides;
+    int stridesNow = strideReadingsHistory.get(0);
-    int stridesNow = strideReadingsHistory
+    // Contrary to documentation stride value seems to roll over at 128.
-      .get(0).numStrides;
+    return Math.round( (float)(mod((stridesNow - stridesThen), 128)) /
-    // Contrary to documentation stride value seems to roll over every 127 strides.
-    return Math.round( (float)((stridesNow - stridesThen) % 127) / 
         timeSinceOldestReadingSecs * 60);
   }
+
+  private int mod(int x, int y)
+  {
+      int result = x % y;
+      return result < 0? result + y : result;
+  }
 }

MyTracks/src/com/google/android/apps/mytracks/services/sensors/ZephyrMessageParser.java

@@ -15,10 +15,9 @@
  */
 package com.google.android.apps.mytracks.services.sensors;
 
-import com.google.android.apps.mytracks.Constants;
 import com.google.android.apps.mytracks.content.Sensor;
 
-import android.util.Log;
+import java.util.Arrays;
 
 /**
  * An implementation of a Sensor MessageParser for Zephyr.
@@ -32,27 +31,12 @@ public class ZephyrMessageParser implements MessageParser {
   public static final int ZEPHYR_HXM_BYTE_CRC = 58;
   public static final int ZEPHYR_HXM_BYTE_ETX = 59;
   
-  private static final String CADENCE_BUG_FW_ID = "1A00316550003162";
+  private static final byte[] CADENCE_BUG_FW_ID = {0x1A, 0x00, 0x31, 0x65, 0x50, 0x00, 0x31, 0x62};
   
   private StrideReadings strideReadings;
   
   @Override
   public Sensor.SensorDataSet parseBuffer(byte[] buffer) {
-    StringBuilder sb = new StringBuilder();
-    for (int i = 0; i < buffer.length; i++) {
-      sb.append(String.format("%02X", buffer[i]));
-    }
-    Log.w(Constants.TAG, "Got zephyr data: " + sb);
-
-    // Device Firmware ID, Firmware Version, Hardware ID, Hardware Version
-    // 0x1A00316550003162 produces erroneous values for Cadence and needs
-    // a workaround based on the stride counter.
-    // Firmware values range from field 3 to 11 of the byte buffer, 
-    // since in hex there are two characters for each byte, doubling the values.
-    String hardwareFirmwareId = sb.substring(6, 22);
-    boolean computeCadenceFromStrides = hardwareFirmwareId.equals(CADENCE_BUG_FW_ID);
-    Log.d(Constants.TAG, "FW & HW Ids & Version " + hardwareFirmwareId + " needs workaround: " + computeCadenceFromStrides);
-    
     Sensor.SensorDataSet.Builder sds =
       Sensor.SensorDataSet.newBuilder()
       .setCreationTime(System.currentTimeMillis());
@@ -60,42 +44,44 @@ public class ZephyrMessageParser implements MessageParser {
     Sensor.SensorData.Builder heartrate = Sensor.SensorData.newBuilder()
       .setValue(buffer[12] & 0xFF)
       .setState(Sensor.SensorState.SENDING);
-    sds = sds.setHeartRate(heartrate);
+    sds.setHeartRate(heartrate);
     
     Sensor.SensorData.Builder batteryLevel = Sensor.SensorData.newBuilder()
       .setValue(buffer[11])
       .setState(Sensor.SensorState.SENDING);
-    sds = sds.setBatteryLevel(batteryLevel);
+    sds.setBatteryLevel(batteryLevel);
     
-    // Appends cadence to SensorDataSet builder if available.
+    setCadence(sds, buffer);
-    parseOrComputeCadence(sds, buffer, computeCadenceFromStrides);
     
     return sds.build();
   }
 
-  private void parseOrComputeCadence(Sensor.SensorDataSet.Builder sds, byte[] buffer, boolean computeFromStrides) {
+  private void setCadence(Sensor.SensorDataSet.Builder sds, byte[] buffer) {
-    
+    // Device Firmware ID, Firmware Version, Hardware ID, Hardware Version
+    // 0x1A00316550003162 produces erroneous values for Cadence and needs
+    // a workaround based on the stride counter.
+    // Firmware values range from field 3 to 10 (inclusive) of the byte buffer.
+    byte[] hardwareFirmwareId = Arrays.copyOfRange(buffer, 3, 11);
+    boolean computeFromStrides = Arrays.equals(hardwareFirmwareId, CADENCE_BUG_FW_ID);
+
     Sensor.SensorData.Builder cadence = Sensor.SensorData.newBuilder();
 
-    if(!computeFromStrides) {
+    if(computeFromStrides) {
-      cadence = cadence
-        .setValue(SensorUtils.unsignedShortToIntLittleEndian(buffer, 56) / 16)
-        .setState(Sensor.SensorState.SENDING);
-      sds.setCadence(cadence);
-    } else {
       if(strideReadings == null) {
         strideReadings = new StrideReadings();
       }
-      strideReadings.updateStrideReading(
+      strideReadings.updateStrideReading(buffer[54] & 0xFF);
-          SensorUtils.unsignedShortToIntLittleEndian(buffer, 14), 
-          buffer[54] & 0xFF);
       
       if(strideReadings.getCadence() != StrideReadings.CADENCE_NOT_AVAILABLE) {
         cadence = cadence.setValue(strideReadings.getCadence())
           .setState(Sensor.SensorState.SENDING);
-        sds.setCadence(cadence);
       }
+    } else {
+      cadence = cadence
+      .setValue(SensorUtils.unsignedShortToIntLittleEndian(buffer, 56) / 16)
+      .setState(Sensor.SensorState.SENDING);
     }
+    sds.setCadence(cadence);
   }
 
   @Override

MyTracksTest/src/com/google/android/apps/mytracks/services/sensors/StrideReadingsTest.java

@@ -1,29 +1,86 @@
+/*
+ * Copyright 2010 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
 package com.google.android.apps.mytracks.services.sensors;
 
+import java.lang.reflect.Field;
+
+import junit.framework.Assert;
 import junit.framework.TestCase;
 
+
+/**
+ * @author Dominik Ršttsches
+ */
 public class StrideReadingsTest extends TestCase {
   
+  /**
+   * Provides access to private members in classes.
+   * from http://onjava.com/pub/a/onjava/2003/11/12/reflection.html
+   */
+  private Object getPrivateField (Object o, String fieldName) {
+    /* Check we have valid arguments */
+    Assert.assertNotNull(o);
+    Assert.assertNotNull(fieldName);
+    /* Go and find the private field... */
+    final Field fields[] = o.getClass().getDeclaredFields();
+    for (int i = 0; i < fields.length; ++i) {
+      if (fieldName.equals(fields[i].getName())) {
+        try {
+          fields[i].setAccessible(true);
+          return fields[i].get(o);
+        } catch (IllegalAccessException ex) {
+          Assert.fail ("IllegalAccessException accessing " + fieldName);
+        }
+      }
+    }
+    Assert.fail ("Field '" + fieldName + "' not found");
+    return null;
+  }
+
   public void testNoReadingOnStartup() {
     StrideReadings strideReadings = new StrideReadings();
-    assertTrue(strideReadings.getCadence() == StrideReadings.CADENCE_NOT_AVAILABLE);
+    assertEquals(StrideReadings.CADENCE_NOT_AVAILABLE, strideReadings.getCadence());
   }
   
   public void testAverageCadenceAvailable() {
     StrideReadings strideReadings = new StrideReadings();
-    for(int i=0;i<30;i++) {
+    // 2 steps / second => Cadence is 120 / minute
-      strideReadings.updateStrideReading(i*1000, i*2);
+    for(int i=1;i<=30;i++) {
+      strideReadings.updateStrideReading(i*2);
+      if(i > (Integer)getPrivateField(strideReadings, "NUM_READINGS_FOR_AVERAGE")) {
+        assertEquals(120, strideReadings.getCadence());
+      }
     }
-    assertTrue(strideReadings.getCadence() > 0);
   }
   
-  /* testing rollover at 127, just like the HxM seems to do it */
+  /** Tests for correct calculation after rolling over at 128 strides,
+   * just like the HxM seems to do it. */
   public void testRollover() {
     StrideReadings strideReadings = new StrideReadings();
-    for(int i=0;i<=10;i++) {
+    // 1 step per second => Cadence is 60 / minute
-      strideReadings.updateStrideReading(i*1000, (120 + i*2) );
+    // Updating readings counting upwards from initialStrides -
-    }
+    // initialStrides set to a value below 128 to ensure rollover.
-    assertTrue(strideReadings.getCadence() > 0);
+    int numReadingsRequired = (Integer)getPrivateField(strideReadings, "NUM_READINGS_FOR_AVERAGE");
-  }
+    int initialStrides = 128 - numReadingsRequired  - 5;
 
+    for(int i=1;i<=numReadingsRequired+10;i++) {
+      strideReadings.updateStrideReading((initialStrides + i) % 128);
+      if(i > numReadingsRequired) {
+        assertEquals(60, strideReadings.getCadence());
+      }
+    }
+  }
 }