diff --git a/docs/superpowers/plans/2026-07-26-code-review-fixes.md b/docs/superpowers/plans/2026-07-26-code-review-fixes.md new file mode 100644 index 0000000..1ca8b30 --- /dev/null +++ b/docs/superpowers/plans/2026-07-26-code-review-fixes.md @@ -0,0 +1,499 @@ +# Code Review Fixes Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix all Critical, Important, and selected Minor issues from the code review to make the Android app production-ready. + +**Architecture:** Incremental fixes to existing files. New `UsbUtils.kt` utility object to eliminate code duplication. No architectural changes — just bug fixes, lifecycle hardening, and cleanup. + +**Tech Stack:** Kotlin, Android SDK API 24-34, USB Host API, BLE API, esp32-flash-lib + +--- + +## File Structure + +| File | Action | Responsibility | +|---|---|---| +| `UsbUtils.kt` | **Create** | Shared USB utility functions (findCdcDataInterface, findBulkEndpoint) | +| `MainActivity.kt` | **Modify** | Fix receiver flag, handler cleanup, onPause scan stop, boolean state | +| `SettingsActivity.kt` | **Modify** | Thread lifecycle check, use UsbUtils | +| `EspFlasher.kt` | **Modify** | try/finally connection cleanup, progress mapping, use UsbUtils | +| `UsbSerialTransport.kt` | **Modify** | Throw IOException on read error | +| `AndroidManifest.xml` | **Modify** | allowBackup=false | +| `activity_settings.xml` | **Modify** | Fix download-modus text | + +--- + +### Task 1: Create UsbUtils utility object + +**Files:** +- Create: `android/app/src/main/java/com/eddy/atomrelay/UsbUtils.kt` + +- [ ] **Step 1: Create UsbUtils.kt** + +```kotlin +package com.eddy.atomrelay + +import android.hardware.usb.UsbConstants +import android.hardware.usb.UsbDevice +import android.hardware.usb.UsbEndpoint +import android.hardware.usb.UsbInterface + +object UsbUtils { + private const val CDC_DATA_INTERFACE_CLASS = 0x0A + + fun findCdcDataInterface(device: UsbDevice): UsbInterface? { + for (i in 0 until device.interfaceCount) { + val intf = device.getInterface(i) + if (intf.interfaceClass == CDC_DATA_INTERFACE_CLASS) { + return intf + } + } + return null + } + + fun findBulkEndpoint(intf: UsbInterface, direction: Int): UsbEndpoint? { + for (i in 0 until intf.endpointCount) { + val ep = intf.getEndpoint(i) + if (ep.type == UsbConstants.USB_ENDPOINT_XFER_BULK && ep.direction == direction) { + return ep + } + } + return null + } +} +``` + +- [ ] **Step 2: Build to verify compilation** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 3: Commit** + +```bash +git add android/app/src/main/java/com/eddy/atomrelay/UsbUtils.kt +git commit -m "Add UsbUtils utility object for shared USB functions" +``` + +--- + +### Task 2: Fix UsbSerialTransport.read() — throw IOException on error + +**Files:** +- Modify: `android/app/src/main/java/com/eddy/atomrelay/UsbSerialTransport.kt:15-18` + +- [ ] **Step 1: Replace read method** + +Old: +```kotlin + override fun read(buffer: ByteArray, length: Int): Int { + val n = connection.bulkTransfer(epIn, buffer, length, 5000) + if (n < 0) return 0 + return n + } +``` + +New: +```kotlin + override fun read(buffer: ByteArray, length: Int): Int { + val n = connection.bulkTransfer(epIn, buffer, length, 5000) + if (n < 0) throw IOException("USB read failed") + return n + } +``` + +- [ ] **Step 2: Build to verify** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 3: Commit** + +```bash +git add android/app/src/main/java/com/eddy/atomrelay/UsbSerialTransport.kt +git commit -m "Fix UsbSerialTransport.read() to throw IOException on error" +``` + +--- + +### Task 3: Fix EspFlasher — connection leak + progress mapping + UsbUtils + +**Files:** +- Modify: `android/app/src/main/java/com/eddy/atomrelay/EspFlasher.kt` + +- [ ] **Step 1: Replace doFlash with try/finally and progress mapping** + +Replace the entire `doFlash` method and the `progressCallback` with: + +```kotlin + private fun doFlash(usbManager: UsbManager, device: UsbDevice, callback: FlashCallback) { + Thread.sleep(500) + + val connection = usbManager.openDevice(device) + ?: throw IOException("Kan USB-apparaat niet openen") + + try { + val intf = UsbUtils.findCdcDataInterface(device) + ?: throw IOException("Geen CDC data interface gevonden") + + if (!connection.claimInterface(intf, true)) { + throw IOException("Kan USB interface niet claimen") + } + + val epOut = UsbUtils.findBulkEndpoint(intf, UsbConstants.USB_DIR_OUT) + ?: throw IOException("Geen USB OUT endpoint gevonden") + val epIn = UsbUtils.findBulkEndpoint(intf, UsbConstants.USB_DIR_IN) + ?: throw IOException("Geen USB IN endpoint gevonden") + + postProgress(callback, 0, "Verbinden met ESP bootloader...") + + val transport = UsbSerialTransport(connection, epOut, epIn) + + val assets = context.assets + val bootloader = assets.open("firmware/bootloader.bin").readBytes() + val partitions = assets.open("firmware/partitions.bin").readBytes() + val bootApp0 = assets.open("firmware/boot_app0.bin").readBytes() + val firmware = assets.open("firmware/firmware.bin").readBytes() + + val totalSize = bootloader.size + partitions.size + bootApp0.size + firmware.size + + data class FlashStep(val name: String, val address: Int, val data: ByteArray, val baseProgress: Int, val range: Int, val compress: Boolean) + + val steps = listOf( + FlashStep("Bootloader", 0x0, bootloader, 30, 10, false), + FlashStep("Partitie tabel", 0x8000, partitions, 40, 10, false), + FlashStep("Boot app0", 0xe000, bootApp0, 50, 10, false), + FlashStep("Firmware", 0x10000, firmware, 60, 35, true) + ) + + postProgress(callback, 5, "Sync met bootloader...") + val api = EspFlasherApi.connect(transport) + .withCallBack(object : ProgressCallback { + override fun onProgress(pct: Float) { + Log.d(TAG, "Lib progress: $pct%") + } + override fun onInfo(value: String) { + Log.d(TAG, "Info: $value") + } + }) + + postProgress(callback, 10, "Chip detecteren...") + val detected = api.chipDetect() + val chipId = detected.getChipId() + Log.i(TAG, "Detected chip: $chipId") + postProgress(callback, 15, "Chip: ${chipId.readableName}") + + postProgress(callback, 20, "Stub laden...") + val stub = detected.loadStub() + + postProgress(callback, 25, "Flash wissen...") + stub.eraseFlash() + + for (step in steps) { + val sizeStr = if (step.data.size > 1024) "${step.data.size / 1024}KB" else "${step.data.size} bytes" + postProgress(callback, step.baseProgress, "${step.name} schrijven ($sizeStr)...") + val writeStub = if (step.compress) stub.withCompression(true) else stub.withCompression(false) + writeStub.writeFlash(step.address, step.data) + val written = steps.takeWhile { it !== step }.sumOf { it.data.size } + step.data.size + val overallProgress = step.baseProgress + (step.range * step.data.size / totalSize).coerceAtLeast(1) + step.range + postProgress(callback, minOf(step.baseProgress + step.range, 95), "${step.name} geschreven") + } + + postProgress(callback, 95, "ESP resetten...") + stub.reset() + + postProgress(callback, 100, "Firmware geflashed!") + handler.post { callback.onComplete() } + } finally { + connection.close() + } + } +``` + +- [ ] **Step 2: Remove the now-duplicated private methods** + +Delete `findCdcDataInterface` and `findBulkEndpoint` from EspFlasher.kt (they are now in UsbUtils). + +- [ ] **Step 3: Build to verify** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 4: Commit** + +```bash +git add android/app/src/main/java/com/eddy/atomrelay/EspFlasher.kt +git commit -m "Fix EspFlasher: connection leak, progress mapping, use UsbUtils" +``` + +--- + +### Task 4: Fix SettingsActivity — UsbUtils + thread lifecycle + +**Files:** +- Modify: `android/app/src/main/java/com/eddy/atomrelay/SettingsActivity.kt` + +- [ ] **Step 1: Replace findCdcDataInterface and findBulkEndpoint calls with UsbUtils** + +In `readKeyFromEsp`, replace: +```kotlin + val intf = findCdcDataInterface(device) +``` +with: +```kotlin + val intf = UsbUtils.findCdcDataInterface(device) +``` + +Replace: +```kotlin + val epOut = findBulkEndpoint(intf, UsbConstants.USB_DIR_OUT) +``` +with: +```kotlin + val epOut = UsbUtils.findBulkEndpoint(intf, UsbConstants.USB_DIR_OUT) +``` + +Replace: +```kotlin + val epIn = findBulkEndpoint(intf, UsbConstants.USB_DIR_IN) +``` +with: +```kotlin + val epIn = UsbUtils.findBulkEndpoint(intf, UsbConstants.USB_DIR_IN) +``` + +- [ ] **Step 2: Delete the duplicated private methods** + +Delete `findCdcDataInterface` and `findBulkEndpoint` from SettingsActivity.kt. + +- [ ] **Step 3: Add lifecycle check in thread callback** + +In `doKeyExchange`, replace the `Thread` block: + +```kotlin + Thread { + val result = readKeyFromEsp(usbManager, device) + handler.post { +``` + +with: + +```kotlin + Thread { + val result = readKeyFromEsp(usbManager, device) + if (isFinishing || isDestroyed) return@Thread + handler.post { +``` + +- [ ] **Step 4: Build to verify** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 5: Commit** + +```bash +git add android/app/src/main/java/com/eddy/atomrelay/SettingsActivity.kt +git commit -m "Fix SettingsActivity: use UsbUtils, add lifecycle guard" +``` + +--- + +### Task 5: Fix MainActivity — receiver flag, handler cleanup, onPause, boolean state + +**Files:** +- Modify: `android/app/src/main/java/com/eddy/atomrelay/MainActivity.kt` + +- [ ] **Step 1: Fix registerReceiver flag** + +In `onCreate`, replace: +```kotlin + registerReceiver(bondReceiver, IntentFilter(BluetoothDevice.ACTION_BOND_STATE_CHANGED)) +``` +with: +```kotlin + registerReceiver(bondReceiver, IntentFilter(BluetoothDevice.ACTION_BOND_STATE_CHANGED), Context.RECEIVER_NOT_EXPORTED) +``` + +- [ ] **Step 2: Add SCAN_TIMEOUT_MS constant** + +In companion object, add: +```kotlin + private const val SCAN_TIMEOUT_MS = 10000L +``` + +- [ ] **Step 3: Add boolean for scan timeout state** + +Add field: +```kotlin + private var scanTimedOut = false +``` + +- [ ] **Step 4: Fix stopScan to use boolean** + +Replace `stopScan`: +```kotlin + @SuppressLint("MissingPermission") + private fun stopScan() { + if (!isScanning) return + isScanning = false + val adapter = BluetoothAdapter.getDefaultAdapter() + adapter?.bluetoothLeScanner?.stopScan(scanCallback) + if (statusText.text == "Scanning...") { + statusText.text = "Device not found" + } + } +``` +with: +```kotlin + @SuppressLint("MissingPermission") + private fun stopScan() { + if (!isScanning) return + isScanning = false + val adapter = BluetoothAdapter.getDefaultAdapter() + adapter?.bluetoothLeScanner?.stopScan(scanCallback) + if (scanTimedOut) { + statusText.text = "Device not found" + } + } +``` + +- [ ] **Step 5: Fix startScan to use constant and boolean** + +In `startScan`, replace: +```kotlin + handler.postDelayed({ + if (isScanning) stopScan() + }, 10000) +``` +with: +```kotlin + scanTimedOut = true + handler.postDelayed({ + if (isScanning) stopScan() + }, SCAN_TIMEOUT_MS) +``` + +And add `scanTimedOut = false` after `isScanning = true`: +```kotlin + isScanning = true + scanTimedOut = false + statusText.text = "Scanning..." +``` + +- [ ] **Step 6: Add onPause** + +Add after `onResume`: +```kotlin + override fun onPause() { + super.onPause() + stopScan() + } +``` + +- [ ] **Step 7: Fix onDestroy — add handler cleanup** + +Replace `onDestroy`: +```kotlin + @SuppressLint("UnspecifiedRegisterReceiverFlag") + override fun onDestroy() { + super.onDestroy() + stopScan() + bluetoothGatt?.close() + unregisterReceiver(bondReceiver) + } +``` +with: +```kotlin + override fun onDestroy() { + super.onDestroy() + handler.removeCallbacksAndMessages(null) + stopScan() + bluetoothGatt?.close() + try { unregisterReceiver(bondReceiver) } catch (e: Exception) {} + } +``` + +- [ ] **Step 8: Build to verify** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 9: Commit** + +```bash +git add android/app/src/main/java/com/eddy/atomrelay/MainActivity.kt +git commit -m "Fix MainActivity: receiver flag, handler cleanup, onPause, boolean state" +``` + +--- + +### Task 6: Fix AndroidManifest — allowBackup=false + +**Files:** +- Modify: `android/app/src/main/AndroidManifest.xml:14` + +- [ ] **Step 1: Change allowBackup** + +Replace: +```xml + android:allowBackup="true" +``` +with: +```xml + android:allowBackup="false" +``` + +- [ ] **Step 2: Commit** + +```bash +git add android/app/src/main/AndroidManifest.xml +git commit -m "Disable allowBackup to protect HMAC key from extraction" +``` + +--- + +### Task 7: Fix activity_settings.xml — download-modus text + +**Files:** +- Modify: `android/app/src/main/res/layout/activity_settings.xml:31` + +- [ ] **Step 1: Fix text** + +Replace: +```xml + android:text="Sluit de ESP aan op USB en zet de ESP in download-modus (houd reset 2s in tot de groene LED knippert). Tap dan op 'Firmware Flashen'." +``` +with: +```xml + android:text="Zet de ESP in download-modus (houd reset 2s in tot de groene LED aangaat) en tik op 'Firmware Flashen'." +``` + +- [ ] **Step 2: Commit** + +```bash +git add android/app/src/main/res/layout/activity_settings.xml +git commit -m "Fix download-modus instruction text in settings layout" +``` + +--- + +### Task 8: Final build and install + +- [ ] **Step 1: Build** + +Run: `JAVA_HOME=/home/linuxbrew/.linuxbrew/Cellar/openjdk@21/21.0.12/libexec ./gradlew assembleDebug` +Expected: BUILD SUCCESSFUL + +- [ ] **Step 2: Install** + +Run: `adb install -r android/app/build/outputs/apk/debug/app-debug.apk` +Expected: Success + +- [ ] **Step 3: Commit plan document** + +```bash +git add docs/superpowers/plans/2026-07-26-code-review-fixes.md +git commit -m "Add code review fixes implementation plan" +```