Huami: Refactor activity data fetching

Activity data fetching on Huami devices was filled with duplicated code,
and the handleActivityFetchFinish was called from multiple places where
it did not make sense. This made us signal to the band that activity
fetch was finished when it sometimes was not, causing some race
condititions that would make activity fetch fail or get stuck.

This refactor defines a clear "processBufferedData" that is called
upstream, signaling to the fetch operation that we have received all
data and the buffer can be processed. All handling of metadata and ack
messages is also delegated to the upstream class.

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/devices/huami/HuamiService.java

@@ -229,8 +229,6 @@ public class HuamiService {
     public static final byte COMMAND_FIRMWARE_CHECKSUM = 0x04; // to UUID_CHARACTERISTIC_FIRMWARE
     public static final byte COMMAND_FIRMWARE_REBOOT = 0x05; // to UUID_CHARACTERISTIC_FIRMWARE
 
-    public static final byte[] RESPONSE_ACTIVITY_DATA_START_DATE_SUCCESS = new byte[] { RESPONSE, COMMAND_ACTIVITY_DATA_START_DATE, SUCCESS};
-
     public static final byte[] WEAR_LOCATION_LEFT_WRIST = new byte[] { 0x20, 0x00, 0x00, 0x02 };
     public static final byte[] WEAR_LOCATION_RIGHT_WRIST = new byte[] { 0x20, 0x00, 0x00, (byte) 0x82};
 

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/HuamiSupport.java

@@ -344,7 +344,7 @@ public abstract class HuamiSupport extends AbstractBTLEDeviceSupport implements
     protected Huami2021ChunkedEncoder huami2021ChunkedEncoder;
     protected Huami2021ChunkedDecoder huami2021ChunkedDecoder;
 
-    private final Queue<AbstractFetchOperation> fetchOperationQueue = new LinkedList<>();
+    private final LinkedList<AbstractFetchOperation> fetchOperationQueue = new LinkedList<>();
 
     public HuamiSupport() {
         this(LOG);
@@ -1725,6 +1725,10 @@ public abstract class HuamiSupport extends AbstractBTLEDeviceSupport implements
         return fetchOperationQueue.poll();
     }
 
+    public LinkedList<AbstractFetchOperation> getFetchOperationQueue() {
+        return fetchOperationQueue;
+    }
+
     @Override
     public void onEnableRealtimeSteps(boolean enable) {
         try {

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/AbstractFetchOperation.java

@@ -28,6 +28,7 @@ import androidx.annotation.NonNull;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.text.DateFormat;
 import java.util.Arrays;
@@ -42,39 +43,41 @@ import nodomain.freeyourgadget.gadgetbridge.devices.huami.HuamiCoordinator;
 import nodomain.freeyourgadget.gadgetbridge.devices.huami.HuamiService;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.BLETypeConversions;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.TransactionBuilder;
-import nodomain.freeyourgadget.gadgetbridge.service.btle.actions.AbstractGattListenerWriteAction;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.actions.SetDeviceBusyAction;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.AbstractHuamiOperation;
-import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.zeppos.ZeppOsSupport;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
-import nodomain.freeyourgadget.gadgetbridge.util.ArrayUtils;
+import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.zeppos.ZeppOsSupport;
+import nodomain.freeyourgadget.gadgetbridge.util.CheckSums;
 import nodomain.freeyourgadget.gadgetbridge.util.GB;
-import nodomain.freeyourgadget.gadgetbridge.util.StringUtils;
 
 /**
- * An operation that fetches activity data. For every fetch, a new operation must
+ * An operation that fetches activity data.
- * be created, i.e. an operation may not be reused for multiple fetches.
  */
 public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
     private static final Logger LOG = LoggerFactory.getLogger(AbstractFetchOperation.class);
 
-    protected byte lastPacketCounter;
-    int fetchCount;
     protected BluetoothGattCharacteristic characteristicActivityData;
     protected BluetoothGattCharacteristic characteristicFetch;
-    Calendar startTimestamp;
-    int expectedDataLength = 0;
 
-    public AbstractFetchOperation(HuamiSupport support) {
+    protected Calendar startTimestamp;
+
+    protected int fetchCount;
+    protected byte lastPacketCounter;
+    protected int expectedDataLength = 0;
+    protected final ByteArrayOutputStream buffer = new ByteArrayOutputStream(1024);
+
+    protected boolean operationValid = true; // to mark operation failed midway (eg. out of sync)
+
+    public AbstractFetchOperation(final HuamiSupport support) {
         super(support);
     }
 
     @Override
-    protected void enableNeededNotifications(TransactionBuilder builder, boolean enable) {
+    protected void enableNeededNotifications(final TransactionBuilder builder, final boolean enable) {
         if (!enable) {
             // dynamically enabled, but always disabled on finish
-            builder.notify(characteristicFetch, enable);
+            builder.notify(characteristicFetch, false);
-            builder.notify(characteristicActivityData, enable);
+            builder.notify(characteristicActivityData, false);
         }
     }
 
@@ -87,7 +90,7 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
         expectedDataLength = 0;
         lastPacketCounter = -1;
 
-        TransactionBuilder builder = performInitialized(getName());
+        final TransactionBuilder builder = performInitialized(getName());
         if (fetchCount == 0) {
             builder.add(new SetDeviceBusyAction(getDevice(), taskDescription(), getContext()));
         }
@@ -114,11 +117,11 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
     protected abstract String getLastSyncTimeKey();
 
     @Override
-    public boolean onCharacteristicChanged(BluetoothGatt gatt,
+    public boolean onCharacteristicChanged(final BluetoothGatt gatt,
-                                           BluetoothGattCharacteristic characteristic) {
+                                           final BluetoothGattCharacteristic characteristic) {
-        UUID characteristicUUID = characteristic.getUuid();
+        final UUID characteristicUUID = characteristic.getUuid();
         if (HuamiService.UUID_CHARACTERISTIC_5_ACTIVITY_DATA.equals(characteristicUUID)) {
-            handleActivityNotif(characteristic.getValue());
+            handleActivityData(characteristic.getValue());
             return true;
         } else if (HuamiService.UUID_UNKNOWN_CHARACTERISTIC4.equals(characteristicUUID)) {
             handleActivityMetadata(characteristic.getValue());
@@ -129,18 +132,15 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
     }
 
     /**
-     * Handles the finishing of fetching the activity.
+     * Handles the finishing of fetching the activity. This signals the actual end of this operation.
-     * @param success whether fetching was successful
-     * @return whether handling the activity fetch finish was successful
      */
-    @CallSuper
+    protected final void onOperationFinished() {
-    protected boolean handleActivityFetchFinish(boolean success) {
         final AbstractFetchOperation nextFetchOperation = getSupport().getNextFetchOperation();
         if (nextFetchOperation != null) {
             LOG.debug("Performing next operation {}", nextFetchOperation.getName());
             try {
                 nextFetchOperation.perform();
-                return true;
+                return;
             } catch (final IOException e) {
                 GB.toast(
                         getContext(),
@@ -148,17 +148,16 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
                         Toast.LENGTH_SHORT,
                         GB.ERROR, e
                 );
-
+                return;
-                return false;
             }
         }
 
         LOG.debug("All operations finished");
 
         GB.updateTransferNotification(null, "", false, 100, getContext());
+        GB.signalActivityDataFinish();
         operationFinished();
         unsetBusy();
-        return true;
     }
 
     /**
@@ -168,72 +167,54 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
      * @param crc32 the expected checksum
      * @return whether the checksum was valid
      */
-    protected abstract boolean validChecksum(int crc32);
+    protected boolean validChecksum(int crc32) {
+        return crc32 == CheckSums.getCRC32(buffer.toByteArray());
+    }
 
-    /**
+    protected abstract boolean processBufferedData();
-     * Method to handle the incoming activity data.
-     * There are two kind of messages we currently know:
-     * - the first one is 11 bytes long and contains metadata (how many bytes to expect, when the data starts, etc.)
-     * - the second one is 20 bytes long and contains the actual activity data
-     * <p/>
-     * The first message type is parsed by this method, for every other length of the value param, bufferActivityData is called.
-     *
-     * @param value
-     */
-    protected abstract void handleActivityNotif(byte[] value);
 
-    protected abstract void bufferActivityData(byte[] value);
+    protected void handleActivityData(final byte[] value) {
+        LOG.debug("{} data: {}", getName(), Logging.formatBytes(value));
 
-    protected void startFetching(TransactionBuilder builder, byte fetchType, GregorianCalendar sinceWhen) {
+        if (!isOperationRunning()) {
-        final String taskName = StringUtils.ensureNotNull(builder.getTaskName());
+            LOG.error("ignoring {} notification because operation is not running. Data length: {}", getName(), value.length);
+            getSupport().logMessageContent(value);
+            return;
+        }
+
+        if ((byte) (lastPacketCounter + 1) == value[0]) {
+            // TODO we should handle skipped or repeated bytes more gracefully
+            lastPacketCounter++;
+            bufferActivityData(value);
+        } else {
+            GB.toast("Error " + getName() + ", invalid package counter: " + value[0] + ", last was: " + lastPacketCounter, Toast.LENGTH_LONG, GB.ERROR);
+            operationValid = false;
+        }
+    }
+
+    protected void bufferActivityData(byte[] value) {
+        buffer.write(value, 1, value.length - 1); // skip the counter
+    }
+
+    protected void startFetching(final TransactionBuilder builder, final byte fetchType, final GregorianCalendar sinceWhen) {
         final HuamiSupport support = getSupport();
-        final boolean isZeppOs = support instanceof ZeppOsSupport;
         byte[] fetchBytes = BLETypeConversions.join(new byte[]{
                         HuamiService.COMMAND_ACTIVITY_DATA_START_DATE,
                         fetchType},
                 support.getTimeBytes(sinceWhen, support.getFetchOperationsTimeUnit()));
-        builder.add(new AbstractGattListenerWriteAction(getQueue(), characteristicFetch, fetchBytes) {
+        builder.write(characteristicFetch, fetchBytes);
-            @Override
-            protected boolean onCharacteristicChanged(BluetoothGatt gatt, BluetoothGattCharacteristic characteristic) {
-                UUID characteristicUUID = characteristic.getUuid();
-                if (HuamiService.UUID_UNKNOWN_CHARACTERISTIC4.equals(characteristicUUID)) {
-                    byte[] value = characteristic.getValue();
-
-                    if (ArrayUtils.equals(value, HuamiService.RESPONSE_ACTIVITY_DATA_START_DATE_SUCCESS, 0)) {
-                        handleActivityMetadata(value);
-                        if (expectedDataLength == 0 && isZeppOs) {
-                            // Nothing to receive, if we try to fetch data it will fail
-                            sendAckZeppOs(true);
-                        } else if (expectedDataLength != 0) {
-                            TransactionBuilder newBuilder = createTransactionBuilder(taskName + " Step 2");
-                            newBuilder.notify(characteristicActivityData, true);
-                            newBuilder.write(characteristicFetch, new byte[]{HuamiService.COMMAND_FETCH_DATA});
-                            try {
-                                performImmediately(newBuilder);
-                            } catch (IOException ex) {
-                                GB.toast(getContext(), "Error fetching debug logs: " + ex.getMessage(), Toast.LENGTH_LONG, GB.ERROR, ex);
-                            }
-                        }
-                        return true;
-                    } else {
-                        handleActivityMetadata(value);
-                    }
-                }
-                return false;
-            }
-        });
     }
 
     private void handleActivityMetadata(byte[] value) {
         if (value.length < 3) {
             LOG.warn("Activity metadata too short: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
         if (value[0] != HuamiService.RESPONSE) {
             LOG.warn("Activity metadata not a response: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
@@ -245,26 +226,26 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
                 handleFetchDataResponse(value);
                 return;
             case HuamiService.COMMAND_ACK_ACTIVITY_DATA:
-                // ignore, this is just the reply to the COMMAND_ACK_ACTIVITY_DATA
                 LOG.info("Got reply to COMMAND_ACK_ACTIVITY_DATA");
+                onOperationFinished();
                 return;
             default:
                 LOG.warn("Unexpected activity metadata: {}", Logging.formatBytes(value));
-                handleActivityFetchFinish(false);
+                onOperationFinished();
         }
     }
 
     private void handleStartDateResponse(final byte[] value) {
         if (value[2] != HuamiService.SUCCESS) {
             LOG.warn("Start date unsuccessful response: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
         // it's 16 on the MB7, with a 0 at the end
         if (value.length != 15 && (value.length != 16 && value[15] != 0x00)) {
             LOG.warn("Start date response length: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
@@ -277,7 +258,8 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
 
         if (expectedDataLength == 0) {
             LOG.info("No data to fetch since {}", startTimestamp.getTime());
-            handleActivityFetchFinish(true);
+            sendAck(true);
+            // do not finish the operation - do it in the ack response
             return;
         }
 
@@ -287,85 +269,108 @@ public abstract class AbstractFetchOperation extends AbstractHuamiOperation {
         GB.updateTransferNotification(taskDescription(),
                 getContext().getString(R.string.FetchActivityOperation_about_to_transfer_since,
                         DateFormat.getDateTimeInstance().format(startTimestamp.getTime())), true, 0, getContext());
+
+        // Trigger the actual data fetch
+        final TransactionBuilder step2builder = createTransactionBuilder(getName() + " Step 2");
+        step2builder.notify(characteristicActivityData, true);
+        step2builder.write(characteristicFetch, new byte[]{HuamiService.COMMAND_FETCH_DATA});
+        try {
+            performImmediately(step2builder);
+        } catch (final IOException e) {
+            GB.toast(getContext(), "Error starting fetch step 2: " + e.getMessage(), Toast.LENGTH_LONG, GB.ERROR, e);
+            onOperationFinished();
+        }
     }
 
     private void handleFetchDataResponse(final byte[] value) {
         if (value[2] != HuamiService.SUCCESS) {
             LOG.warn("Fetch data unsuccessful response: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
         if (value.length != 3 && value.length != 7) {
             LOG.warn("Fetch data unexpected metadata length: {}", Logging.formatBytes(value));
-            handleActivityFetchFinish(false);
+            onOperationFinished();
             return;
         }
 
         if (value.length == 7 && !validChecksum(BLETypeConversions.toUint32(value, 3))) {
             LOG.warn("Data checksum invalid");
-            handleActivityFetchFinish(false);
+            // If we're on Zepp OS, ack but keep data on device
-            sendAckZeppOs(true);
+            if (isZeppOs()) {
+                sendAck(true);
+                // do not finish the operation - do it in the ack response
+                return;
+            }
+            onOperationFinished();
             return;
         }
 
-        boolean handleFinishSuccess;
+        final boolean success = operationValid && processBufferedData();
-        try {
-            handleFinishSuccess = handleActivityFetchFinish(true);
-        } catch (final Exception e) {
-            LOG.warn("Failed to handle activity fetch finish", e);
-            handleFinishSuccess = false;
-        }
 
-        final boolean keepActivityDataOnDevice = HuamiCoordinator.getKeepActivityDataOnDevice(getDevice().getAddress());
+        final boolean keepActivityDataOnDevice = !success || HuamiCoordinator.getKeepActivityDataOnDevice(getDevice().getAddress());
-
+        if (isZeppOs() || !keepActivityDataOnDevice) {
-        sendAckZeppOs(keepActivityDataOnDevice || !handleFinishSuccess);
+            sendAck(keepActivityDataOnDevice);
-    }
+            // do not finish the operation - do it in the ack response
-
-    protected void sendAckZeppOs(final boolean keepDataOnDevice) {
-        if (!(getSupport() instanceof ZeppOsSupport)) {
             return;
         }
 
-        LOG.debug("Sending Zepp OS ack, keepDataOnDevice = {}", keepDataOnDevice);
+        onOperationFinished();
+    }
+
+    protected void sendAck(final boolean keepDataOnDevice) {
+        final byte[] ackBytes;
+
+        if (isZeppOs()) {
+            LOG.debug("Sending ack, keepDataOnDevice = {}", keepDataOnDevice);
 
             // 0x01 to ACK, mark as saved on phone (drop from band)
             // 0x09 to ACK, but keep it marked as not saved
             // If 0x01 is sent, detailed information seems to be discarded, and is not sent again anymore
             final byte ackByte = (byte) (keepDataOnDevice ? 0x09 : 0x01);
+            ackBytes = new byte[]{HuamiService.COMMAND_ACK_ACTIVITY_DATA, ackByte};
+        } else {
+            LOG.debug("Sending ack, simple");
+            ackBytes = new byte[]{HuamiService.COMMAND_ACK_ACTIVITY_DATA};
+        }
 
         try {
-            final TransactionBuilder builder = performInitialized(getName() + " end");
+            final TransactionBuilder builder = createTransactionBuilder(getName() + " end");
-            builder.write(characteristicFetch, new byte[]{HuamiService.COMMAND_ACK_ACTIVITY_DATA, ackByte});
+            builder.write(characteristicFetch, ackBytes);
             performImmediately(builder);
         } catch (final IOException e) {
-            LOG.error("Ending failed", e);
+            LOG.error("Failed to send ack", e);
         }
     }
 
-    private void setStartTimestamp(Calendar startTimestamp) {
+    private void setStartTimestamp(final Calendar startTimestamp) {
         this.startTimestamp = startTimestamp;
     }
 
-    Calendar getLastStartTimestamp() {
+    protected Calendar getLastStartTimestamp() {
         return startTimestamp;
     }
 
-    void saveLastSyncTimestamp(@NonNull GregorianCalendar timestamp) {
+    protected void saveLastSyncTimestamp(@NonNull final GregorianCalendar timestamp) {
-        SharedPreferences.Editor editor = GBApplication.getDeviceSpecificSharedPrefs(getDevice().getAddress()).edit();
+        final SharedPreferences.Editor editor = GBApplication.getDeviceSpecificSharedPrefs(getDevice().getAddress()).edit();
         editor.putLong(getLastSyncTimeKey(), timestamp.getTimeInMillis());
         editor.apply();
     }
 
     protected GregorianCalendar getLastSuccessfulSyncTime() {
-        long timeStampMillis = GBApplication.getDeviceSpecificSharedPrefs(getDevice().getAddress()).getLong(getLastSyncTimeKey(), 0);
+        final long timeStampMillis = GBApplication.getDeviceSpecificSharedPrefs(getDevice().getAddress()).getLong(getLastSyncTimeKey(), 0);
         if (timeStampMillis != 0) {
             GregorianCalendar calendar = BLETypeConversions.createCalendar();
             calendar.setTimeInMillis(timeStampMillis);
             return calendar;
         }
-        GregorianCalendar calendar = BLETypeConversions.createCalendar();
+        final GregorianCalendar calendar = BLETypeConversions.createCalendar();
         calendar.add(Calendar.DAY_OF_MONTH, -100);
         return calendar;
     }
+
+    protected boolean isZeppOs() {
+        return getSupport() instanceof ZeppOsSupport;
+    }
 }

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/AbstractRepeatingFetchOperation.java

@@ -16,30 +16,20 @@
     along with this program.  If not, see <https://www.gnu.org/licenses/>. */
 package nodomain.freeyourgadget.gadgetbridge.service.devices.huami.operations.fetch;
 
-import android.bluetooth.BluetoothGatt;
-import android.bluetooth.BluetoothGattCharacteristic;
-import android.widget.Toast;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileOutputStream;
-import java.io.IOException;
 import java.io.OutputStream;
 import java.text.SimpleDateFormat;
 import java.util.Calendar;
 import java.util.GregorianCalendar;
 import java.util.Locale;
 
-import nodomain.freeyourgadget.gadgetbridge.Logging;
-import nodomain.freeyourgadget.gadgetbridge.devices.huami.HuamiCoordinator;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.TransactionBuilder;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
-import nodomain.freeyourgadget.gadgetbridge.util.CheckSums;
 import nodomain.freeyourgadget.gadgetbridge.util.FileUtils;
-import nodomain.freeyourgadget.gadgetbridge.util.GB;
 
 /**
  * A repeating fetch operation. This operation repeats the fetch up to a certain number of times, or
@@ -49,8 +39,6 @@ import nodomain.freeyourgadget.gadgetbridge.util.GB;
 public abstract class AbstractRepeatingFetchOperation extends AbstractFetchOperation {
     private static final Logger LOG = LoggerFactory.getLogger(AbstractRepeatingFetchOperation.class);
 
-    private final ByteArrayOutputStream byteStreamBuffer = new ByteArrayOutputStream(140);
-
     protected final HuamiFetchDataType dataType;
 
     public AbstractRepeatingFetchOperation(final HuamiSupport support, final HuamiFetchDataType dataType) {
@@ -77,21 +65,14 @@ public abstract class AbstractRepeatingFetchOperation extends AbstractFetchOpera
     protected abstract boolean handleActivityData(GregorianCalendar timestamp, byte[] bytes);
 
     @Override
-    protected boolean handleActivityFetchFinish(final boolean success) {
+    protected boolean processBufferedData() {
-        LOG.info("{} has finished round {}: {}, got {} bytes in buffer", getName(), fetchCount, success, byteStreamBuffer.size());
+        LOG.info("{} has finished round {}, got {} bytes in buffer", getName(), fetchCount, buffer.size());
 
-        if (!success) {
+        if (buffer.size() == 0) {
-            // We need to explicitly ack this, or the next operation will fail fetch will become stuck
+            return true;
-            sendAckZeppOs(true);
-            super.handleActivityFetchFinish(false);
-            return false;
         }
 
-        if (byteStreamBuffer.size() == 0) {
+        final byte[] bytes = buffer.toByteArray();
-            return super.handleActivityFetchFinish(true);
-        }
-
-        final byte[] bytes = byteStreamBuffer.toByteArray();
         final GregorianCalendar timestamp = (GregorianCalendar) this.startTimestamp.clone();
 
         // Uncomment to dump the bytes to external storage for debugging
@@ -100,7 +81,6 @@ public abstract class AbstractRepeatingFetchOperation extends AbstractFetchOpera
         final boolean handleSuccess = handleActivityData(timestamp, bytes);
 
         if (!handleSuccess) {
-            super.handleActivityFetchFinish(false);
             return false;
         }
 
@@ -108,63 +88,12 @@ public abstract class AbstractRepeatingFetchOperation extends AbstractFetchOpera
         saveLastSyncTimestamp(timestamp);
 
         if (needsAnotherFetch(timestamp)) {
-            byteStreamBuffer.reset();
+            buffer.reset();
+
+            getSupport().getFetchOperationQueue().add(0, this);
+        }
 
-            try {
-                final boolean keepActivityDataOnDevice = HuamiCoordinator.getKeepActivityDataOnDevice(getDevice().getAddress());
-                sendAckZeppOs(keepActivityDataOnDevice);
-                startFetching();
         return true;
-            } catch (final IOException ex) {
-                LOG.error("Error starting another round of " + getName(), ex);
-                super.handleActivityFetchFinish(false);
-                return false;
-            }
-        }
-
-        final boolean superSuccess = super.handleActivityFetchFinish(true);
-        postActivityFetchFinish(superSuccess);
-        return superSuccess;
-    }
-
-    protected void postActivityFetchFinish(final boolean success) {
-
-    }
-
-    @Override
-    protected boolean validChecksum(final int crc32) {
-        return crc32 == CheckSums.getCRC32(byteStreamBuffer.toByteArray());
-    }
-
-    @Override
-    public boolean onCharacteristicRead(final BluetoothGatt gatt, final BluetoothGattCharacteristic characteristic, final int status) {
-        LOG.debug("characteristic read: {}: {}", characteristic.getUuid(), Logging.formatBytes(characteristic.getValue()));
-        return super.onCharacteristicRead(gatt, characteristic, status);
-    }
-
-    @Override
-    protected void handleActivityNotif(final byte[] value) {
-        LOG.debug("{} data: {}", getName(), Logging.formatBytes(value));
-
-        if (!isOperationRunning()) {
-            LOG.error("ignoring {} notification because operation is not running. Data length: {}", getName(), value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if ((byte) (lastPacketCounter + 1) == value[0]) {
-            // TODO we should handle skipped or repeated bytes more gracefully
-            lastPacketCounter++;
-            bufferActivityData(value);
-        } else {
-            GB.toast("Error " + getName() + ", invalid package counter: " + value[0] + ", last was: " + lastPacketCounter, Toast.LENGTH_LONG, GB.ERROR);
-            handleActivityFetchFinish(false);
-        }
-    }
-
-    @Override
-    protected void bufferActivityData(final byte[] value) {
-        byteStreamBuffer.write(value, 1, value.length - 1); // skip the counter
     }
 
     private boolean needsAnotherFetch(final GregorianCalendar lastSyncTimestamp) {

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

@@ -129,11 +129,6 @@ public class FetchActivityOperation extends AbstractRepeatingFetchOperation {
         }
     }
 
-    @Override
-    protected void postActivityFetchFinish(final boolean success) {
-        GB.signalActivityDataFinish();
-    }
-
     @Override
     protected boolean validChecksum(final int crc32) {
         // TODO actually check it

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/FetchDebugLogsOperation.java

@@ -37,7 +37,6 @@ import nodomain.freeyourgadget.gadgetbridge.service.btle.BLETypeConversions;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.TransactionBuilder;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
 import nodomain.freeyourgadget.gadgetbridge.util.FileUtils;
-import nodomain.freeyourgadget.gadgetbridge.util.GB;
 
 public class FetchDebugLogsOperation extends AbstractFetchOperation {
     private static final Logger LOG = LoggerFactory.getLogger(FetchDebugLogsOperation.class);
@@ -81,11 +80,11 @@ public class FetchDebugLogsOperation extends AbstractFetchOperation {
 
     @Override
     protected String getLastSyncTimeKey() {
-        return null;
+        return "lastDebugTimeMillis";
     }
 
     @Override
-    protected boolean handleActivityFetchFinish(boolean success) {
+    protected boolean processBufferedData() {
         LOG.info("{} data has finished", getName());
         try {
             logOutputStream.close();
@@ -95,7 +94,7 @@ public class FetchDebugLogsOperation extends AbstractFetchOperation {
             return false;
         }
 
-        return super.handleActivityFetchFinish(success);
+        return true;
     }
 
     @Override
@@ -105,30 +104,13 @@ public class FetchDebugLogsOperation extends AbstractFetchOperation {
         return true;
     }
 
-    @Override
-    protected void handleActivityNotif(byte[] value) {
-        if (!isOperationRunning()) {
-            LOG.error("ignoring notification because operation is not running. Data length: " + value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if ((byte) (lastPacketCounter + 1) == value[0]) {
-            lastPacketCounter++;
-            bufferActivityData(value);
-        } else {
-            GB.toast("Error " + getName() + " invalid package counter: " + value[0], Toast.LENGTH_LONG, GB.ERROR);
-            handleActivityFetchFinish(false);
-        }
-    }
-
     @Override
     protected void bufferActivityData(@NonNull byte[] value) {
         try {
             logOutputStream.write(value, 1, value.length - 1);
         } catch (final IOException e) {
             LOG.warn("could not write to output stream", e);
-            handleActivityFetchFinish(false);
+            operationValid = false;
         }
     }
 }

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/FetchSportsDetailsOperation.java

@@ -23,7 +23,6 @@ import android.widget.Toast;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
@@ -32,7 +31,6 @@ import java.util.GregorianCalendar;
 import androidx.annotation.NonNull;
 
 import nodomain.freeyourgadget.gadgetbridge.GBApplication;
-import nodomain.freeyourgadget.gadgetbridge.Logging;
 import nodomain.freeyourgadget.gadgetbridge.R;
 import nodomain.freeyourgadget.gadgetbridge.database.DBHandler;
 import nodomain.freeyourgadget.gadgetbridge.entities.BaseActivitySummary;
@@ -45,7 +43,6 @@ import nodomain.freeyourgadget.gadgetbridge.service.btle.TransactionBuilder;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.AbstractHuamiActivityDetailsParser;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiActivityDetailsParser;
-import nodomain.freeyourgadget.gadgetbridge.util.CheckSums;
 import nodomain.freeyourgadget.gadgetbridge.util.DateTimeUtils;
 import nodomain.freeyourgadget.gadgetbridge.util.FileUtils;
 import nodomain.freeyourgadget.gadgetbridge.util.GB;
@@ -60,8 +57,6 @@ public class FetchSportsDetailsOperation extends AbstractFetchOperation {
     private final BaseActivitySummary summary;
     private final String lastSyncTimeKey;
 
-    private ByteArrayOutputStream buffer;
-
     FetchSportsDetailsOperation(@NonNull BaseActivitySummary summary,
                                 @NonNull AbstractHuamiActivityDetailsParser detailsParser,
                                 @NonNull HuamiSupport support,
@@ -83,25 +78,27 @@ public class FetchSportsDetailsOperation extends AbstractFetchOperation {
     @Override
     protected void startFetching(TransactionBuilder builder) {
         LOG.info("start " + getName());
-        buffer = new ByteArrayOutputStream(1024);
+        final GregorianCalendar sinceWhen = getLastSuccessfulSyncTime();
-        GregorianCalendar sinceWhen = getLastSuccessfulSyncTime();
         startFetching(builder, HuamiFetchDataType.SPORTS_DETAILS.getCode(), sinceWhen);
     }
 
     @Override
-    protected boolean handleActivityFetchFinish(boolean success) {
+    protected boolean processBufferedData() {
-        LOG.info(getName() + " has finished round " + fetchCount);
+        LOG.info("{} has finished round {}", getName(), fetchCount);
 
-        boolean parseSuccess = true;
+        if (buffer.size() == 0) {
+            LOG.warn("Buffer is empty");
+            return false;
+        }
 
-        if (success && buffer.size() > 0) {
         if (detailsParser instanceof HuamiActivityDetailsParser) {
             ((HuamiActivityDetailsParser) detailsParser).setSkipCounterByte(false); // is already stripped
         }
+
         try {
-                ActivityTrack track = detailsParser.parse(buffer.toByteArray());
+            final ActivityTrack track = detailsParser.parse(buffer.toByteArray());
-                ActivityTrackExporter exporter = createExporter();
+            final ActivityTrackExporter exporter = createExporter();
-                String trackType = "track";
+            final String trackType;
             switch (summary.getActivityKind()) {
                 case ActivityKind.TYPE_CYCLING:
                     trackType = getContext().getString(R.string.activity_type_biking);
@@ -121,18 +118,21 @@ public class FetchSportsDetailsOperation extends AbstractFetchOperation {
                 case ActivityKind.TYPE_SWIMMING:
                     trackType = getContext().getString(R.string.activity_type_swimming);
                     break;
+                default:
+                    trackType = "track";
+                    break;
             }
+
             final String rawBytesPath = saveRawBytes();
 
-                String fileName = FileUtils.makeValidFileName("gadgetbridge-" + trackType.toLowerCase() + "-" + DateTimeUtils.formatIso8601(summary.getStartTime()) + ".gpx");
+            final String fileName = FileUtils.makeValidFileName("gadgetbridge-" + trackType.toLowerCase() + "-" + DateTimeUtils.formatIso8601(summary.getStartTime()) + ".gpx");
-                File targetFile = new File(FileUtils.getExternalFilesDir(), fileName);
+            final File targetFile = new File(FileUtils.getExternalFilesDir(), fileName);
 
             boolean exportGpxSuccess = true;
             try {
                 exporter.performExport(track, targetFile);
-                } catch (ActivityTrackExporter.GPXTrackEmptyException ex) {
+            } catch (final ActivityTrackExporter.GPXTrackEmptyException ex) {
                 exportGpxSuccess = false;
-                    GB.toast(getContext(), "This activity does not contain GPX tracks.", Toast.LENGTH_LONG, GB.ERROR, ex);
             }
 
             try (DBHandler dbHandler = GBApplication.acquireDB()) {
@@ -144,32 +144,22 @@ public class FetchSportsDetailsOperation extends AbstractFetchOperation {
                 }
                 dbHandler.getDaoSession().getBaseActivitySummaryDao().update(summary);
             }
-            } catch (Exception ex) {
+        } catch (final Exception e) {
-                GB.toast(getContext(), "Error getting activity details: " + ex.getMessage(), Toast.LENGTH_LONG, GB.ERROR, ex);
+            GB.toast(getContext(), "Error saving activity details: " + e.getMessage(), Toast.LENGTH_LONG, GB.ERROR, e);
-                parseSuccess = false;
+            return false;
-            }
         }
 
-        final boolean superSuccess = super.handleActivityFetchFinish(success);
-
-        if (success && parseSuccess) {
         // Always increment the sync timestamp on success, even if we did not get data
-            GregorianCalendar endTime = BLETypeConversions.createCalendar();
+        final GregorianCalendar endTime = BLETypeConversions.createCalendar();
         endTime.setTime(summary.getEndTime());
         saveLastSyncTimestamp(endTime);
 
         if (needsAnotherFetch(endTime)) {
-                FetchSportsSummaryOperation nextOperation = new FetchSportsSummaryOperation(getSupport(), fetchCount);
+            final FetchSportsSummaryOperation nextOperation = new FetchSportsSummaryOperation(getSupport(), fetchCount);
-                try {
+            getSupport().getFetchOperationQueue().add(0, nextOperation);
-                    nextOperation.perform();
-                } catch (IOException ex) {
-                    LOG.error("Error starting another round of fetching activity data", ex);
-                }
-            }
         }
 
-
+        return true;
-        return superSuccess && parseSuccess;
     }
 
     private boolean needsAnotherFetch(GregorianCalendar lastSyncTimestamp) {
@@ -183,78 +173,30 @@ public class FetchSportsDetailsOperation extends AbstractFetchOperation {
             LOG.info("Hopefully no further fetch needed, last synced timestamp is from today.");
             return false;
         }
+
         if (lastSyncTimestamp.getTimeInMillis() > System.currentTimeMillis()) {
             LOG.warn("Not doing another fetch since last synced timestamp is in the future: {}", DateTimeUtils.formatDateTime(lastSyncTimestamp.getTime()));
             return false;
         }
+
         LOG.info("Doing another fetch since last sync timestamp is still too old: {}", DateTimeUtils.formatDateTime(lastSyncTimestamp.getTime()));
         return true;
     }
 
-    @Override
-    protected boolean validChecksum(int crc32) {
-        return crc32 == CheckSums.getCRC32(buffer.toByteArray());
-    }
-
     private ActivityTrackExporter createExporter() {
-        GPXExporter exporter = new GPXExporter();
+        final GPXExporter exporter = new GPXExporter();
         exporter.setCreator(GBApplication.app().getNameAndVersion());
         return exporter;
     }
 
-    /**
-     * Method to handle the incoming activity data.
-     * There are two kind of messages we currently know:
-     * - the first one is 11 bytes long and contains metadata (how many bytes to expect, when the data starts, etc.)
-     * - the second one is 20 bytes long and contains the actual activity data
-     * <p/>
-     * The first message type is parsed by this method, for every other length of the value param, bufferActivityData is called.
-     *
-     * @param value
-     */
-    @Override
-    protected void handleActivityNotif(byte[] value) {
-        LOG.warn("sports details: " + Logging.formatBytes(value));
-
-        if (!isOperationRunning()) {
-            LOG.error("ignoring sports details notification because operation is not running. Data length: " + value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if (value.length < 2) {
-            LOG.error("unexpected sports details data length: " + value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if ((byte) (lastPacketCounter + 1) == value[0]) {
-            lastPacketCounter++;
-            bufferActivityData(value);
-        } else {
-            GB.toast("Error " + getName() + ", invalid package counter: " + value[0] + ", last was: " + lastPacketCounter, Toast.LENGTH_LONG, GB.ERROR);
-            handleActivityFetchFinish(false);
-        }
-    }
-
-    /**
-     * Buffers the given activity summary data. If the total size is reached,
-     * it is converted to an object and saved in the database.
-     *
-     * @param value
-     */
-    @Override
-    protected void bufferActivityData(byte[] value) {
-        buffer.write(value, 1, value.length - 1); // skip the counter
-    }
-
     @Override
     protected String getLastSyncTimeKey() {
         return lastSyncTimeKey;
     }
 
+    @Override
     protected GregorianCalendar getLastSuccessfulSyncTime() {
-        GregorianCalendar calendar = BLETypeConversions.createCalendar();
+        final GregorianCalendar calendar = BLETypeConversions.createCalendar();
         calendar.setTime(summary.getStartTime());
         return calendar;
     }

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/FetchSportsSummaryOperation.java

@@ -17,19 +17,14 @@
     along with this program.  If not, see <https://www.gnu.org/licenses/>. */
 package nodomain.freeyourgadget.gadgetbridge.service.devices.huami.operations.fetch;
 
-import android.bluetooth.BluetoothGatt;
-import android.bluetooth.BluetoothGattCharacteristic;
 import android.widget.Toast;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
 import java.util.GregorianCalendar;
 
 import nodomain.freeyourgadget.gadgetbridge.GBApplication;
-import nodomain.freeyourgadget.gadgetbridge.Logging;
 import nodomain.freeyourgadget.gadgetbridge.R;
 import nodomain.freeyourgadget.gadgetbridge.database.DBHandler;
 import nodomain.freeyourgadget.gadgetbridge.database.DBHelper;
@@ -43,7 +38,6 @@ import nodomain.freeyourgadget.gadgetbridge.model.ActivitySummaryParser;
 import nodomain.freeyourgadget.gadgetbridge.service.btle.TransactionBuilder;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.AbstractHuamiActivityDetailsParser;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
-import nodomain.freeyourgadget.gadgetbridge.util.CheckSums;
 import nodomain.freeyourgadget.gadgetbridge.util.GB;
 
 /**
@@ -53,7 +47,6 @@ import nodomain.freeyourgadget.gadgetbridge.util.GB;
 public class FetchSportsSummaryOperation extends AbstractFetchOperation {
     private static final Logger LOG = LoggerFactory.getLogger(FetchSportsSummaryOperation.class);
 
-    private ByteArrayOutputStream buffer = new ByteArrayOutputStream(140);
     public FetchSportsSummaryOperation(HuamiSupport support, int fetchCount) {
         super(support);
         setName("fetching sport summaries");
@@ -68,125 +61,56 @@ public class FetchSportsSummaryOperation extends AbstractFetchOperation {
     @Override
     protected void startFetching(TransactionBuilder builder) {
         LOG.info("start" + getName());
-        GregorianCalendar sinceWhen = getLastSuccessfulSyncTime();
+        final GregorianCalendar sinceWhen = getLastSuccessfulSyncTime();
         startFetching(builder, HuamiFetchDataType.SPORTS_SUMMARIES.getCode(), sinceWhen);
     }
 
     @Override
-    protected boolean handleActivityFetchFinish(boolean success) {
+    protected boolean processBufferedData() {
-        LOG.info(getName() + " has finished round " + fetchCount);
+        LOG.info("{} has finished round {}", getName(), fetchCount);
 
+        if (buffer.size() < 2) {
+            LOG.warn("Buffer size {} too small for activity summary", buffer.size());
+            return false;
+        }
 
-        BaseActivitySummary summary = null;
         final DeviceCoordinator coordinator = getDevice().getDeviceCoordinator();
         final ActivitySummaryParser summaryParser = coordinator.getActivitySummaryParser(getDevice());
 
-        boolean parseSummarySuccess = true;
+        BaseActivitySummary summary = new BaseActivitySummary();
-
-        if (success && buffer.size() > 0) {
-            summary = new BaseActivitySummary();
         summary.setStartTime(getLastStartTimestamp().getTime()); // due to a bug this has to be set
         summary.setRawSummaryData(buffer.toByteArray());
         try {
             summary = summaryParser.parseBinaryData(summary);
         } catch (final Exception e) {
             GB.toast(getContext(), "Failed to parse activity summary", Toast.LENGTH_LONG, GB.ERROR, e);
-                summary = null;
+            return false;
-                parseSummarySuccess = false;
+        }
+
+        if (summary == null) {
+            return false;
         }
 
-            if (summary != null) {
         summary.setSummaryData(null); // remove json before saving to database,
         try (DBHandler dbHandler = GBApplication.acquireDB()) {
-                    DaoSession session = dbHandler.getDaoSession();
+            final DaoSession session = dbHandler.getDaoSession();
-                    Device device = DBHelper.getDevice(getDevice(), session);
+            final Device device = DBHelper.getDevice(getDevice(), session);
-                    User user = DBHelper.getUser(session);
+            final User user = DBHelper.getUser(session);
             summary.setDevice(device);
             summary.setUser(user);
             summary.setRawSummaryData(buffer.toByteArray());
             session.getBaseActivitySummaryDao().insertOrReplace(summary);
-                } catch (Exception ex) {
+        } catch (final Exception ex) {
             GB.toast(getContext(), "Error saving activity summary", Toast.LENGTH_LONG, GB.ERROR, ex);
-                    parseSummarySuccess = false;
+            return false;
-                }
-            }
         }
 
-        final boolean superSuccess = super.handleActivityFetchFinish(success);
-        boolean getDetailsSuccess = true;
-
-        if (summary != null) {
         final AbstractHuamiActivityDetailsParser detailsParser = ((HuamiActivitySummaryParser) summaryParser).getDetailsParser(summary);
+        final FetchSportsDetailsOperation nextOperation = new FetchSportsDetailsOperation(summary, detailsParser, getSupport(), getLastSyncTimeKey(), fetchCount);
+        getSupport().getFetchOperationQueue().add(0, nextOperation);
 
-            FetchSportsDetailsOperation nextOperation = new FetchSportsDetailsOperation(summary, detailsParser, getSupport(), getLastSyncTimeKey(), fetchCount);
+        return true;
-            try {
-                nextOperation.perform();
-            } catch (IOException ex) {
-                GB.toast(getContext(), "Unable to fetch activity details: " + ex.getMessage(), Toast.LENGTH_LONG, GB.ERROR, ex);
-                getDetailsSuccess = false;
     }
-        }
-
-        return parseSummarySuccess && superSuccess && getDetailsSuccess;
-    }
-
-    @Override
-    protected boolean validChecksum(int crc32) {
-        return crc32 == CheckSums.getCRC32(buffer.toByteArray());
-    }
-
-    @Override
-    public boolean onCharacteristicRead(BluetoothGatt gatt, BluetoothGattCharacteristic characteristic, int status) {
-        LOG.warn("characteristic read: " + characteristic.getUuid() + ": " + Logging.formatBytes(characteristic.getValue()));
-        return super.onCharacteristicRead(gatt, characteristic, status);
-    }
-
-    /**
-     * Method to handle the incoming activity data.
-     * There are two kind of messages we currently know:
-     * - the first one is 11 bytes long and contains metadata (how many bytes to expect, when the data starts, etc.)
-     * - the second one is 20 bytes long and contains the actual activity data
-     * <p/>
-     * The first message type is parsed by this method, for every other length of the value param, bufferActivityData is called.
-     *
-     * @param value
-     */
-    @Override
-    protected void handleActivityNotif(byte[] value) {
-        LOG.warn("sports summary data: " + Logging.formatBytes(value));
-
-        if (!isOperationRunning()) {
-            LOG.error("ignoring activity data notification because operation is not running. Data length: " + value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if (value.length < 2) {
-            LOG.error("unexpected sports summary data length: " + value.length);
-            getSupport().logMessageContent(value);
-            return;
-        }
-
-        if ((byte) (lastPacketCounter + 1) == value[0]) {
-            lastPacketCounter++;
-            bufferActivityData(value);
-        } else {
-            GB.toast("Error " + getName() + ", invalid package counter: " + value[0] + ", last was: " + lastPacketCounter, Toast.LENGTH_LONG, GB.ERROR);
-            handleActivityFetchFinish(false);
-        }
-    }
-
-    /**
-     * Buffers the given activity summary data. If the total size is reached,
-     * it is converted to an object and saved in the database.
-     *
-     * @param value
-     */
-    @Override
-    protected void bufferActivityData(byte[] value) {
-        buffer.write(value, 1, value.length - 1); // skip the counter
-    }
-
 
     @Override
     protected String getLastSyncTimeKey() {

app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/huami/operations/fetch/FetchStatisticsOperation.java

@@ -19,12 +19,10 @@ package nodomain.freeyourgadget.gadgetbridge.service.devices.huami.operations.fe
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.Calendar;
 import java.util.Date;
 import java.util.GregorianCalendar;
 
 import nodomain.freeyourgadget.gadgetbridge.R;
-import nodomain.freeyourgadget.gadgetbridge.service.btle.BLETypeConversions;
 import nodomain.freeyourgadget.gadgetbridge.service.devices.huami.HuamiSupport;
 
 /**
@@ -52,14 +50,6 @@ public class FetchStatisticsOperation extends AbstractRepeatingFetchOperation {
         return true;
     }
 
-    @Override
-    protected GregorianCalendar getLastSuccessfulSyncTime() {
-        // One minute ago - we don't really care about this data, and we don't want to fetch it all
-        final GregorianCalendar sinceWhen = BLETypeConversions.createCalendar();
-        sinceWhen.add(Calendar.MINUTE, -1);
-        return sinceWhen;
-    }
-
     @Override
     protected String getLastSyncTimeKey() {
         return "lastStatisticsTimeMillis";