Move activity data and progress manipulation into dedicated methods

I did this in trying to understand the code better and to easier allow
for error handling/transaction rollback to be added.

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/miband/operations/FetchActivityOperation.java

@@ -31,28 +31,89 @@ import nodomain.freeyourgadget.gadgetbridge.service.devices.miband.MiBandSupport
 import nodomain.freeyourgadget.gadgetbridge.util.DateTimeUtils;
 import nodomain.freeyourgadget.gadgetbridge.util.GB;
 
+/**
+ * An operation that fetches activity data. For every fetch, a new operation must
+ * be created, i.e. an operation may not be reused for multiple fetches.
+ */
 public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport> {
     private static final Logger LOG = LoggerFactory.getLogger(FetchActivityOperation.class);
     private static final byte[] fetch = new byte[]{MiBandService.COMMAND_FETCH_DATA};
 
     //temporary buffer, size is a multiple of 60 because we want to store complete minutes (1 minute = 3 bytes)
-    private static final int activityDataHolderSize = 3 * 60 * 4; // 8h
+    private static final int activityDataHolderSize = 3 * 60 * 4; // 4h
 
     private static class ActivityStruct {
-        public byte[] activityDataHolder = new byte[activityDataHolderSize];
+        private byte[] activityDataHolder = new byte[activityDataHolderSize];
         //index of the buffer above
-        public int activityDataHolderProgress = 0;
+        private int activityDataHolderProgress = 0;
         //number of bytes we will get in a single data transfer, used as counter
-        public int activityDataRemainingBytes = 0;
+        private int activityDataRemainingBytes = 0;
         //same as above, but remains untouched for the ack message
-        public int activityDataUntilNextHeader = 0;
+        private int activityDataUntilNextHeader = 0;
         //timestamp of the single data transfer, incremented to store each minute's data
-        public GregorianCalendar activityDataTimestampProgress = null;
+        private GregorianCalendar activityDataTimestampProgress = null;
         //same as above, but remains untouched for the ack message
-        public GregorianCalendar activityDataTimestampToAck = null;
+        private GregorianCalendar activityDataTimestampToAck = null;
+
+        public boolean hasRoomFor(byte[] value) {
+            return activityDataRemainingBytes >= value.length;
         }
 
-    private ActivityStruct activityStruct;
+        public boolean isValidData(byte[] value) {
+            //I don't like this clause, but until we figure out why we get different data sometimes this should work
+            return value.length == 20 || value.length == activityDataRemainingBytes;
+        }
+
+        public boolean isBufferFull() {
+            return activityDataHolderSize == activityDataHolderProgress;
+        }
+
+        public void buffer(byte[] value) {
+            System.arraycopy(value, 0, activityDataHolder, activityDataHolderProgress, value.length);
+            activityDataHolderProgress += value.length;
+            activityDataRemainingBytes -= value.length;
+
+            validate();
+        }
+
+        private void validate() {
+            GB.assertThat(activityDataRemainingBytes >= 0, "Illegal state, remaining bytes is negative");
+        }
+
+        public boolean isFirstChunk() {
+            return activityDataTimestampProgress == null;
+        }
+
+        public void startNewBlock(GregorianCalendar timestamp, int dataUntilNextHeader) {
+            GB.assertThat(timestamp != null, "Timestamp must not be null");
+
+            if (isFirstChunk()) {
+                activityDataTimestampProgress = timestamp;
+            } else {
+                if (timestamp.getTimeInMillis() >= activityDataTimestampProgress.getTimeInMillis()) {
+                    activityDataTimestampProgress = timestamp;
+                } else {
+                    // something is fishy here... better not trust the given timestamp and simply
+                    // (re)use the current one
+                    // we do accept the timestamp to ack though, so that the bogus data is properly cleared on the band
+                }
+            }
+            activityDataTimestampToAck = (GregorianCalendar) timestamp.clone();
+            activityDataRemainingBytes = activityDataUntilNextHeader = dataUntilNextHeader;
+            validate();
+        }
+
+        public boolean isBlockFinished() {
+            return activityDataRemainingBytes == 0;
+        }
+
+        public void bufferFlushed(int minutes) {
+            activityDataTimestampProgress.add(Calendar.MINUTE, minutes);
+            activityDataHolderProgress = 0;
+        }
+    }
+
+    private ActivityStruct activityStruct = new ActivityStruct();
 
     public FetchActivityOperation(MiBandSupport support) {
         super(support);
@@ -98,11 +159,6 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
      * @param value
      */
     private void handleActivityNotif(byte[] value) {
-        boolean firstChunk = activityStruct == null;
-        if (firstChunk) {
-            activityStruct = new ActivityStruct();
-        }
-
         if (value.length == 11) {
             // byte 0 is the data type: 1 means that each minute is represented by a triplet of bytes
             int dataType = value[0];
@@ -123,7 +179,7 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
             // after dataUntilNextHeader bytes we will get a new packet of 11 bytes that should be parsed
             // as we just did
 
-            if (firstChunk && dataUntilNextHeader != 0) {
+            if (activityStruct.isFirstChunk() && dataUntilNextHeader != 0) {
                 GB.toast(getContext().getString(R.string.user_feedback_miband_activity_data_transfer,
                         DateTimeUtils.formatDurationHoursMinutes((totalDataToRead / 3), TimeUnit.MINUTES),
                         DateFormat.getDateTimeInstance().format(timestamp.getTime())), Toast.LENGTH_LONG, GB.INFO);
@@ -132,16 +188,14 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
             LOG.info("data to read until next header: " + dataUntilNextHeader + " len: " + (dataUntilNextHeader / 3) + " minute(s)");
             LOG.info("TIMESTAMP: " + DateFormat.getDateTimeInstance().format(timestamp.getTime()).toString() + " magic byte: " + dataUntilNextHeader);
 
-            activityStruct.activityDataRemainingBytes = activityStruct.activityDataUntilNextHeader = dataUntilNextHeader;
+            activityStruct.startNewBlock(timestamp, dataUntilNextHeader);
-            activityStruct.activityDataTimestampToAck = (GregorianCalendar) timestamp.clone();
-            activityStruct.activityDataTimestampProgress = timestamp;
 
         } else {
             bufferActivityData(value);
         }
         LOG.debug("activity data: length: " + value.length + ", remaining bytes: " + activityStruct.activityDataRemainingBytes);
 
-        if (activityStruct.activityDataRemainingBytes == 0) {
+        if (activityStruct.isBlockFinished()) {
             sendAckDataTransfer(activityStruct.activityDataTimestampToAck, activityStruct.activityDataUntilNextHeader);
         }
     }
@@ -154,15 +208,11 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
      * @param value
      */
     private void bufferActivityData(byte[] value) {
+        if (activityStruct.hasRoomFor(value)) {
+            if (activityStruct.isValidData(value)) {
+                activityStruct.buffer(value);
 
-        if (activityStruct.activityDataRemainingBytes >= value.length) {
+                if (activityStruct.isBufferFull()) {
-            //I don't like this clause, but until we figure out why we get different data sometimes this should work
-            if (value.length == 20 || value.length == activityStruct.activityDataRemainingBytes) {
-                System.arraycopy(value, 0, activityStruct.activityDataHolder, activityStruct.activityDataHolderProgress, value.length);
-                activityStruct.activityDataHolderProgress += value.length;
-                activityStruct.activityDataRemainingBytes -= value.length;
-
-                if (this.activityDataHolderSize == activityStruct.activityDataHolderProgress) {
                     flushActivityDataHolder();
                 }
             } else {
@@ -190,22 +240,26 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
         DBHandler dbHandler = null;
         try {
             dbHandler = GBApplication.acquireDB();
+            int minutes = 0;
             try (SQLiteDatabase db = dbHandler.getWritableDatabase()) { // explicitly keep the db open while looping over the samples
+                int timestampInSeconds = (int) (activityStruct.activityDataTimestampProgress.getTimeInMillis() / 1000);
                 for (int i = 0; i < activityStruct.activityDataHolderProgress; i += 3) { //TODO: check if multiple of 3, if not something is wrong
                     category = activityStruct.activityDataHolder[i];
                     intensity = activityStruct.activityDataHolder[i + 1];
                     steps = activityStruct.activityDataHolder[i + 2];
 
                     dbHandler.addGBActivitySample(
-                            (int) (activityStruct.activityDataTimestampProgress.getTimeInMillis() / 1000),
+                            timestampInSeconds,
                             SampleProvider.PROVIDER_MIBAND,
                             (short) (intensity & 0xff),
                             (short) (steps & 0xff),
                             category);
-                    activityStruct.activityDataTimestampProgress.add(Calendar.MINUTE, 1);
+                    // next minute
+                    minutes++;
+                    timestampInSeconds += 60;
                 }
             } finally {
-                activityStruct.activityDataHolderProgress = 0;
+                activityStruct.bufferFlushed(minutes);
             }
         } catch (Exception ex) {
             GB.toast(getContext(), ex.getMessage(), Toast.LENGTH_LONG, GB.ERROR);
@@ -255,7 +309,7 @@ public class FetchActivityOperation extends AbstractBTLEOperation<MiBandSupport>
             builder.write(getCharacteristic(MiBandService.UUID_CHARACTERISTIC_CONTROL_POINT), ack);
             builder.queue(getQueue());
 
-            // flush to the DB after sending the ACK
+            // flush to the DB after queueing the ACK
             flushActivityDataHolder();
 
             //The last data chunk sent by the miband has always length 0.

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/util/GB.java

@@ -339,4 +339,10 @@ public class GB {
     public static GBEnvironment env() {
         return environment;
     }
+
+    public static void assertThat(boolean condition, String errorMessage) {
+        if (!condition) {
+            throw new AssertionError(errorMessage);
+        }
+    }
 }