70f004c45b
Also un-ignores docs/ directory so project documentation (specs, plans) is tracked in git. Individual research files remain gitignored.
499 lines
15 KiB
Markdown
499 lines
15 KiB
Markdown
# 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"
|
|
```
|