# Code Review — Sylvia Touch Relay Android App

**Datum:** 26 juli 2026
**Reviewer:** opencode (GLM-5.1)
**Scope:** Volledige Android app (`android/`)

---

## Samenvatting

De Android app voor de Sylvia Touch Relay is een goed functionerende BLE-client met HMAC-authenticatie, USB key provisioning en firmware flashing. De codebase is compact (~1300 regels Kotlin) en doet wat het moet doen. Er zijn echter enkele significante verbeterpunten rondom beveiliging, resource management, error handling en code organisatie.

**Beoordeling:** Functioneel en bruikbaar, met aandachtspunten voor productie-kwaliteit.

---

## Bevindingen per categorie

### 1. Beveiliging

#### 1.1 HMAC Key opslag in SharedPreferences (BELANGRIJK)

**Bestand:** `SettingsActivity.kt:162`, `UsbKeyReceiver.kt:215,252`

De HMAC key wordt opgeslagen in plain-text SharedPreferences. Dit is het primaire beveiligingsmechanisme van de app en zou met `EncryptedSharedPreferences` (Jetpack Security) beschermd moeten worden. Op een geroot device is de key nu triviaal uit te lezen via de prefs XML file.

**Aanbeveling:** Gebruik `androidx.security:security-crypto` met `EncryptedSharedPreferences`:
```kotlin
val masterKey = MasterKey.Builder(context)
    .setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
    .build()
val prefs = EncryptedSharedPreferences.create(
    context, PREFS_NAME, masterKey,
    EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
    EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)
```

#### 1.2 Sequence counter niet gepersisteerd (MEDIUM)

**Bestand:** `MainActivity.kt:53`

```kotlin
private var seq = 1
```

De sequence counter reset naar 1 bij elke app-herstart. Na herstart kan de app een sequence sturen die lager is dan de laatste sequence die de ESP heeft ontvangen. De ESP's `lastSeq` reset ook bij reconnect, dus dit is alleen een probleem als de app herstart zonder BLE disconnect. In de praktijk valt dit mee omdat de ESP bij elke nieuwe BLE-verbinding de `lastSeq` reset. Maar persistentie van `seq` zou de replay-bescherming robuuster maken.

**Aanbeveling:** Overweeg de sequence counter in SharedPreferences te persisten, óf документeer dat de ESP-side reset het dekt.

#### 1.3 Log-aanvallen lekken key-materiaal (BELANGRIJK)

**Bestand:** `UsbKeyReceiver.kt:216,229`

```kotlin
Log.d(TAG, "HMAC key saved: $upperKey")
Log.d(TAG, "Generated key: $newKey")
```

De volledige HMAC key wordt naar logcat gelogd op DEBUG-niveau. Dit is een beveiligingslek: elke app met `READ_LOGS` of ADB-toegang kan deze key uitlezen.

**Aanbeveling:** Verwijder deze log statements of vervang door: `Log.d(TAG, "HMAC key saved (${upperKey.length} chars)")`.

---

### 2. Resource Management & Memory Leaks

#### 2.1 BLE scan callback niet afgehandeld bij geen permissies (MEDIUM)

**Bestand:** `MainActivity.kt:227-230`

```kotlin
if (!hasPermissions()) {
    requestPermissions()
    return
}
```

Als permissies geweigerd worden en de gebruiker keert terug naar de app, wordt `startScan()` niet opnieuw aangeroepen. De app blijft hangen in "Geen permissies" zonder feedback.

**Aanbeveling:** Bij geweigerde permissies in `onRequestPermissionsResult`, toon een uitleg of stuur de gebruiker naar app-instellingen.

#### 2.2 Meerdere geneste Handler.postDelayed zonder cancel (MEDIUM)

**Bestand:** `MainActivity.kt:89,147,157`

Er zijn meerdere `handler.postDelayed` calls die BLE scans starten. Als `onResume()` meerdere keren wordt aangeroepen (bijv. bij snelle activity-transities), kunnen er meerdere scan-timers lopen. De `isScanning` guard helpt, maar dequeued niet de overtollige callbacks.

**Aanbeveling:** Gebruik een specifieke `Runnable` in plaats van lambda's zodat deze met `handler.removeCallbacks(runnable)` afgebroken kunnen worden, óf gebruik een flag die ook pending scans voorkomt.

#### 2.3 SettingsActivity receivers niet veilig geregistreerd op API 14+ (LAAG)

**Bestand:** `SettingsActivity.kt:115-117`

De `RECEIVER_NOT_EXPORTED` flag wordt gebruikt voor `usbPermissionReceiver` met een custom action `ACTION_USB_PERMISSION`. Dit is correct voor custom actions. Echter, de `usbAttachedReceiver` en `usbDetachedReceiver` zouden met `RECEIVER_NOT_EXPORTED` problemen kunnen geven omdat systeem-broadcasts (zoals `ACTION_USB_DEVICE_ATTACHED`) exported moeten zijn om ontvangen te worden.

**Aanbeveling:** Gebruik `RECEIVER_EXPORTED` voor systeem-broadcasts (`ACTION_USB_DEVICE_ATTACHED`/`DETACHED`) en `RECEIVER_NOT_EXPORTED` voor de custom `ACTION_USB_PERMISSION`.

#### 2.4 ProgressDialog is deprecated (MEDIUM)

**Bestand:** `SettingsActivity.kt:153,204,258,272`

`ProgressDialog` is deprecated sinds API level 26. Het werkt nog, maar wordt niet meer onderhouden en kan UI inconsistency veroorzaken op nieuwe Android versies.

**Aanbeveling:** Vervang door `AlertDialog` met een `ProgressBar` of `MaterialAlertDialogBuilder` met een custom layout.

---

### 3. Concurrency & Threading

#### 3.1 Background threads zonder lifecycle-awareness (BELANGRIJK)

**Bestand:** `SettingsActivity.kt:155-178`

```kotlin
Thread {
    val result = readKeyFromEsp(usbManager, device)
    if (isFinishing || isDestroyed) return@Thread
    handler.post { ... }
}.start()
```

De `isFinishing || isDestroyed` check is goed, maar komt ná een potentiële 15-seconden USB operatie. Als de Activity vernietigd wordt tijdens de USB-leesoperatie, draait de thread nutteloos door en wordt er nog USB I/O gedaan op een vernietigde context.

**Aanbeveling:** Gebruik coroutines met `lifecycleScope` voor betere cancellation:
```kotlin
lifecycleScope.launch(Dispatchers.IO) {
    val result = readKeyFromEsp(usbManager, device)
    withContext(Dispatchers.Main) { ... }
}
```

#### 3.2 EspFlasher thread kan niet geannuleerd worden (MEDIUM)

**Bestand:** `EspFlasher.kt:29-37`

De flasher draait in een raw `Thread` die niet annuleerbaar is. Als de gebruiker de Activity verlaat tijdens het flashen (wat de `setCancelable(false)` probeert te voorkomen), draait de thread door.

**Aanbeveling:** Overweeg een cancelable mechanisme of gebruik coroutines.

#### 3.3 BLE writes zonder acknowledgment fallback (LAAG) — **AFGESLOTEN, bewuste keuze**

**Bestand:** `MainActivity.kt:277-289`

`WRITE_TYPE_NO_RESPONSE` betekent dat er geen bevestiging komt van de ESP. Als de packet verloren gaat, heeft de app geen retry mechanisme. De knop-toestand wijzigt wel ("ON"), maar het relais schakelt niet.

**BESLUIT:** Afgewezen om dezelfde reden als 6.1 — ACK-based writes veroorzaken dat loslaten wordt genegeerd als de ACK te lang duurt, waardoor het relais blijft hangen. Zie _Ontwerpbeslissingen_ in README.

---

### 4. Code Organisatie & Onderhoudbaarheid

#### 4.1 UsbKeyReceiver dupliceert SettingsActivity logica (BELANGRIJK)

**Bestand:** `UsbKeyReceiver.kt` vs `SettingsActivity.kt:314-395`

`UsbKeyReceiver` bevat een volledige kopie van de CDC key-exchange logica die ook in `SettingsActivity.readKeyFromEsp()` staat. Beide implementeren:
- CDC interface finding
- CDC control setup (line coding, DTR/RTS)
- KEY? command + response parsing
- SET_KEY command + OK verification
- Random key generation

Daarnaast heeft `UsbKeyReceiver` zijn eigen kopie van `findCdcDataInterface`, `findBulkEndpoint`, en `findCdcControlInterface` — deze bestaan ook al in `UsbUtils`.

**Aanbeveling:** Extracteer de key-exchange logica naar een gedeelde class (bijv. `UsbKeyExchange`) die door beide callers gebruikt wordt. Gebruik `UsbUtils` consequent.

#### 4.2 UsbKeyReceiver niet geregistreerd in AndroidManifest (MEDIUM)

**Bestand:** `UsbKeyReceiver.kt` — niet in `AndroidManifest.xml`

`UsbKeyReceiver` is defined als een `BroadcastReceiver` maar wordt nergens geregistreerd (noch statisch in AndroidManifest, noch dynamisch in een Activity). Het is dode code.

**Aanbeveling:** Of registreer de receiver (dynamisch in `MainActivity` of statisch in Manifest met `intent-filter` voor `ACTION_USB_DEVICE_ATTACHED`), of verwijder de class als de functionaliteit via `SettingsActivity` voldoende is.

#### 4.3 Magic numbers en hardcoded waarden (LAAG)

**Bestand:** `MainActivity.kt:43-44`, `SettingsActivity.kt:34-37`

Constants zoals `SCAN_TIMEOUT_MS`, `ESP_VENDOR_ID`, `KEY_TIMEOUT_MS` zijn gedefinieerd — dit is goed. Maar er zijn ook ongedefinieerde magic numbers:
- `MainActivity.kt:250` — `SCAN_TIMEOUT_MS` (gedefinieerd, goed)
- `SettingsActivity.kt:334` — retry count `3` als literal
- `SettingsActivity.kt:336,347,373` — `Thread.sleep(500)` hardcoded
- `UsbSerialTransport.kt:16,22` — timeout `5000` hardcoded als magic number

**Aanbeveling:** Definieer deze als constants in de companion objects.

#### 4.4 getString() niet gebruikt voor user-facing tekst (LAAG)

**Bestand:** meerdere — bijv. `MainActivity.kt:155,234,245`, `SettingsActivity.kt:222,237`

User-facing strings zijn hardcoded in Kotlin:
```kotlin
statusText.text = "Geen HMAC sleutel - open Instellingen"
statusText.text = "Verbinden..."
```

`strings.xml` bevat slechts 5 entries die grotendeels ongebruikt zijn.

**Aanbeveling:** Verplaats alle user-facing strings naar `strings.xml` en gebruik `getString(R.string.xxx)` voor lokalisatie-ondersteuning en consistentie.

#### 4.5 Geen proguard rules voor de flasher library (LAAG)

**Bestand:** `app/build.gradle:26`

`minifyEnabled false` voor release. Als men in de toekomst minify aanzet, kan de `esp32-flash-lib` obfuscation-breuk lijden zonder proguard rules.

**Aanbeveling:** Voeg proguard rules toe voor `org.dkaukov.esp32.*` wanneer minify geactiveerd wordt.

---

### 5. Error Handling

#### 5.1 Geen handling voor BLE write failure (BELANGRIJK)

**Bestand:** `MainActivity.kt:284-287`

```kotlin
char.value = packet
char.writeType = BluetoothGattCharacteristic.WRITE_TYPE_NO_RESPONSE
gatt.writeCharacteristic(char)
seq++
```

Op Android API < 13 retourneert `writeCharacteristic()` een boolean die succes/falen aangaf. Op API 13+ is het callback-based. Er wordt geen `onCharacteristicWrite` callback geïmplementeerd in `gattCallback`. Als de write faalt (bijv. characteristic niet beschikbaar, buffer vol), wordt de sequence counter toch opgehoogd, wat leidt tot desync.

**Aanbeveling:** Implementeer `onCharacteristicWrite` callback en verhoog `seq` pas na bevestiging.

#### 5.2 GATT status codes niet gecontroleerd (MEDIUM)

**Bestand:** `MainActivity.kt:73,94,100`

```kotlin
override fun onConnectionStateChange(gatt: BluetoothGatt, status: Int, newState: Int) {
```

De `status` parameter wordt genegeerd. Wanneer `status != GATT_SUCCESS`, betekent dit dat de verbinding onverwacht verbroken is (niet een normale disconnect). De app behandelt en behandelt alle disconnects hetzelfde (reconnect na 2s), maar logt niet wat de oorzaak was.

**Aanbeveling:** Log de status code bij faalsituaties:
```kotlin
if (status != BluetoothGatt.GATT_SUCCESS) {
    Log.w(TAG, "GATT error: $status, state: $newState")
}
```

#### 5.3 Broakkoppel exception swallowing (LAAG)

**Bestand:** `SettingsActivity.kt:123-125`, `393`

```kotlin
try { unregisterReceiver(usbAttachedReceiver) } catch (e: Exception) {}
try { connection.close() } catch (e: Exception) {}
```

Exceptions worden leeg gevangen. Dit kan bugs maskeren.

**Aanbeveling:** Log minimaal op WARNING niveau:
```kotlin
try { unregisterReceiver(usbAttachedReceiver) } catch (e: Exception) {
    Log.w(TAG, "Failed to unregister receiver", e)
}
```

---

### 6. UI & UX

#### 6.1 Knop-state niet gesynchroniseerd met relay-state (MEDIUM) — **AFGESLOTEN, bewuste keuze**

**Bestand:** `MainActivity.kt:207-222`

De knop tekst wijzigt naar "ON" bij `ACTION_DOWN` en "HOLD" bij `ACTION_UP`, ongeacht of het BLE-commando daadwerkelijk is aangekomen. Gezien het gebruik van `WRITE_TYPE_NO_RESPONSE` is er geen bevestiging.

**Oorspronkelijke aanbeveling:** Geef visuele feedback die onderscheid maakt tussen "commando verzonden" en "relais bevestigd" — bijv. een dimmed state of pending indicator.

**BESLUIT:** Afgewezen. Eerdere versies gebruikten `WRITE_TYPE_DEFAULT` (met ACK), maar de BLE write acknowledgement duurde lang genoeg dat de gebruiker de knop kon loslaten vóór de ACK arriveerde. Bij `ACTION_UP` tijdens een pending write werd het loslaten genegeerd en bleven het relais en de app hangen in de ON-status. `WRITE_TYPE_NO_RESPONSE` verwijdert die latency. De UI reflecteert de gebruikersintentie in plaats van bevestigde relay-state — een aanvaardbare afweging. Gedocumenteerd in code (`MainActivity.kt`) en README (_Ontwerpbeslissingen_).

#### 6.2 Geen dark mode support (LAAG) — **AFGESLOTEN**

**Bestand:** `AndroidManifest.xml:17`

De app forceert Light theme, ook op devices in dark mode.

**Oplossing:** Overgeschakeld naar `Theme.AppCompat.DayNight` met teal accent-kleuren. Thema-selector in Instellingen met 3 opties: Systeem (volgt telefoon), Licht, Donker. Gekozen voor de best-practice aanpak: geen custom font-slider of contrast-kleurenkiezer — Android's systeem-lettergrootte en hoog-contrast instellingen zijn al voldoende voor de doelgroep.

#### 6.3 Geen visuele indicator voor scan timeout (LAAG)

**Bestand:** `MainActivity.kt:264-265`

Wanneer de scan timeout bereikt wordt:
```kotlin
if (scanTimedOut) {
    statusText.text = "Device not found"
}
```

Er is geen retry-knop of automatische retry. De gebruiker moet de app herstarten.

**Aanbeveling:** Voeg een "Opnieuw scannen" knop toe of retry automatisch na 5 seconden.

---

### 7. Build & Dependencies

#### 7.1 Verouderde AGP versie (MEDIUM)

**Bestand:** `android/build.gradle:2`

```groovy
id 'com.android.application' version '8.2.2' apply false
```

AGP 8.2.2 is van begin 2024. De nieuwste is 8.5+. Nieuwere versies bevatten bug fixes en betere Kotlin support.

**Aanneveling:** Upgrade AGP naar 8.5+ (compatibel met Gradle 8.14).

#### 7.2 compileSdk 34, niet 35 (LAAG) — **AFGESLOTEN, bewuste keuze**

**Bestand:** `app/build.gradle:8,13`

`compileSdk` en `targetSdk` zijn 34 (Android 14). Android 15 (API 35) is uitgebracht.

**BESLUIT:** Afgewezen. De doelgroep bestaat uit lichamelijk gehandicapte gebruikers die zeer terughoudend zijn met het updaten van hun telefoon. API 34 maximaliseert device-compatibiliteit. Gedocumenteerd in `build.gradle` en README (_Ontwerpbeslissingen_).

#### 7.3 Kotlin 1.9.24, niet 2.x (LAAG) — **AFGESLOTEN, geüpgraded**

Kotlin is geüpgraded naar 2.0.21 (K2 compiler).

---

### 8. Firmware / App inconsistenties

#### 8.1 Line coding byte-array verschilt (MEDIUM)

**Bestand:** `SettingsActivity.kt:430-433` vs `UsbKeyReceiver.kt:164`

```kotlin
// SettingsActivity:
byteArrayOf(0x00, 0xC2.toByte(), 0x01, 0x00, 0x00, 0x08, 0x08)
// UsbKeyReceiver:
byteArrayOf(0x00, 0xC2.toByte(), 0x01, 0x00, 0x00, 0x00, 0x08)
```

Het 6e byte verschilt: `0x08` (SettingsActivity) vs `0x00` (UsbKeyReceiver). De juiste CDC line coding is `115200 8N1`:
- Byte 0-3: baudrate little-endian (115200 = `0x00C20000` → `0x00, 0xC2, 0x01, 0x00`)
- Byte 4: stop bits (0 = 1 stop bit)
- Byte 5: parity (0 = none)
- Byte 6: data bits (8)

`UsbKeyReceiver` heeft de juiste waarde (`0x00` voor parity). `SettingsActivity` heeft `0x08` — dit zegt parity=space. Het werkt waarschijnlijk omdat de ESP32-S3 CDC deze instellingen negeert, maar het is inconsistent.

**Aanbeveling:** Fix `SettingsActivity.kt:432` naar `0x00`  en extracteer naar een gedeelde functie.

---

## Prioritering

| Prioriteit | Aantal | Actie |
|---|---|---|
| **Belangrijk** | 5 | Alle 5 opgelost |
| **MEDIUM** | 8 | Alle 8 opgelost |
| **LAAG** | 8 | Alle 8 opgelost / afgesloten |

---

## Top 5 aanbevelingen (snelste impact)

1. **Verwijder key logging** (`UsbKeyReceiver.kt:216,229`) — 2 regels, direct beveiligingswinst
2. **Schakel over naar EncryptedSharedPreferences** — beschermt de HMAC key op device
3. **Consolideer UsbKeyReceiver** — 318 regels dode code of duplicatie; verwijderen of integreren
4. **Fix line coding byte** (`SettingsActivity.kt:432`) — consistentie bug
5. **Implementeer `onCharacteristicWrite`** — voorkomt sequence desync

---

## Conclusie

De app is solide voor hobbyist-gebruik en bevat doordachte beveiligingsarchitectuur (HMAC, sequence counters, USB key provisioning). Voor productie-kwaliteit zijn de belangrijkste punten:

- **Beveiliging:** Key-opslag versleutelen, logging van key-materiaal verwijderen
- **Code kwaliteit:** Duplicatie weghalen (UsbKeyReceiver), coroutines ipv raw Threads
- **Robustheid:** BLE write acknowledgements, GATT status logging
- **Modernisering:** EncryptedSharedPreferences, DayNight theme, AGP/Kotlin upgrade

De firmware (`src/main.cpp`) is netjes geschreven en heeft geen aanbevelingen buiten scope van deze review.