385 lines
No EOL
16 KiB
Markdown
385 lines
No EOL
16 KiB
Markdown
# 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. |