305 lines
8 KiB
Markdown
305 lines
8 KiB
Markdown
# Code Review Fixes (Batch 2) 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 6 issues found in code review: broken client OTA UI flow (SettingsActivity button), missing timeout in OTA polling, recomputeClientOta race condition, committed upload_port, wrong LED startup sequence, and noisy ESP_LOGI diagnostics.
|
|
|
|
**Architecture:** All fixes are isolated single-file edits. No new files. No architecture changes. Version bumps: server 21→22, client 11→12.
|
|
|
|
**Tech Stack:** C++ (Arduino/ESP-IDF), Kotlin (Android), PlatformIO
|
|
|
|
---
|
|
|
|
### Task 1: Fix SettingsActivity button — client+BLE now routes to startClientOtaUpdate
|
|
|
|
**Files:**
|
|
- Modify: `android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt:167-174`
|
|
|
|
- [ ] **Step 1: Replace button click handler**
|
|
|
|
In `setupButtons()` (or wherever the click listener is set), replace lines 167-174 with the corrected logic:
|
|
|
|
```kotlin
|
|
findViewById<Button>(R.id.updateModuleButton).setOnClickListener {
|
|
val mode = PrefsHelper.getDeviceMode(this)
|
|
val transportId = findViewById<RadioGroup>(R.id.radioTransport).checkedRadioButtonId
|
|
if (transportId == R.id.radioUsb) {
|
|
startFlashing()
|
|
} else if (mode == "client") {
|
|
startClientOtaUpdate(BleRelayManager.instance)
|
|
} else {
|
|
startServerOtaUpdate(BleRelayManager.instance)
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Verify the change compiles**
|
|
|
|
```bash
|
|
cd android && ./gradlew compileDebugKotlin 2>&1 | tail -5
|
|
```
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt
|
|
git commit -m "fix(android): client+BLE button now routes to client OTA instead of USB flash"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 2: Add timeout to waitForClientOtaReady polling loop
|
|
|
|
**Files:**
|
|
- Modify: `android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt:393-405`
|
|
|
|
- [ ] **Step 1: Add `import kotlinx.coroutines.withTimeoutOrNull`**
|
|
|
|
The import `kotlinx.coroutines.isActive` already exists at line 35. Just add the timeout:
|
|
|
|
```kotlin
|
|
import kotlinx.coroutines.withTimeoutOrNull
|
|
```
|
|
|
|
- [ ] **Step 2: Wrap polling loop in withTimeoutOrNull**
|
|
|
|
Replace `waitForClientOtaReady` (lines 393-405) with:
|
|
|
|
```kotlin
|
|
private suspend fun waitForClientOtaReady(serverManager: BleRelayManager): Boolean {
|
|
return withTimeoutOrNull(30_000L) {
|
|
var status = -1
|
|
serverManager.setClientOtaStatusListener { s -> status = s }
|
|
try {
|
|
while (status != 0x01) {
|
|
serverManager.readClientOtaStatus()
|
|
delay(2000)
|
|
}
|
|
} finally {
|
|
serverManager.setClientOtaStatusListener(null)
|
|
}
|
|
} ?: false
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Verify the change compiles**
|
|
|
|
```bash
|
|
cd android && ./gradlew compileDebugKotlin 2>&1 | tail -5
|
|
```
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt
|
|
git commit -m "fix(android): add 30s timeout to client OTA ready polling"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 3: Fix recomputeClientOta() race — preserve 0x01 status
|
|
|
|
**Files:**
|
|
- Modify: `src/main.cpp:251`
|
|
|
|
- [ ] **Step 1: Add 0x01 exclusion to recomputeClientOta**
|
|
|
|
Change line 251 from:
|
|
```cpp
|
|
if (!clientOtaAvailable && clientOtaStatus != 0x00 && clientOtaStatus != 0x02) {
|
|
```
|
|
to:
|
|
```cpp
|
|
if (!clientOtaAvailable && clientOtaStatus != 0x00 && clientOtaStatus != 0x01 && clientOtaStatus != 0x02) {
|
|
```
|
|
|
|
- [ ] **Step 2: Commit**
|
|
|
|
```bash
|
|
git add src/main.cpp
|
|
git commit -m "fix(server): avoid resetting CLIENT_OTA_STATUS 0x01 in recomputeClientOta"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 4: Remove committed upload_port from platformio.ini
|
|
|
|
**Files:**
|
|
- Modify: `platformio.ini:16,38`
|
|
|
|
- [ ] **Step 1: Remove both upload_port lines**
|
|
|
|
Delete line 16:
|
|
```
|
|
upload_port = /dev/serial/by-id/usb-Espressif_USB_JTAG_serial_debug_unit_44:1B:F6:F4:CF:14-if00
|
|
```
|
|
Delete line 38:
|
|
```
|
|
upload_port = /dev/serial/by-id/usb-Espressif_USB_JTAG_serial_debug_unit_44:1B:F6:F5:D1:E0-if00
|
|
```
|
|
|
|
- [ ] **Step 2: Verify platformio.ini is still valid**
|
|
|
|
```bash
|
|
grep -n "upload_port" platformio.ini
|
|
```
|
|
Expected: no output.
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add platformio.ini
|
|
git commit -m "fix: remove committed upload_port from platformio.ini"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 5: Restore LED startup sequence to Blue-Red-Blue
|
|
|
|
**Files:**
|
|
- Modify: `src/client.cpp:603-605`
|
|
|
|
- [ ] **Step 1: Change LED startup sequence**
|
|
|
|
Replace lines 603-605:
|
|
```cpp
|
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Green; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Black; FastLED.show();
|
|
```
|
|
with:
|
|
```cpp
|
|
leds[0] = CRGB::Blue; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Blue; FastLED.show(); delay(500);
|
|
leds[0] = CRGB::Black; FastLED.show();
|
|
```
|
|
|
|
- [ ] **Step 2: Commit**
|
|
|
|
```bash
|
|
git add src/client.cpp
|
|
git commit -m "fix(client): restore LED startup sequence to blue-red-blue"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 6: Downgrade diagnostic ESP_LOGI to ESP_LOGD
|
|
|
|
**Files:**
|
|
- Modify: `src/client.cpp:907`
|
|
- Modify: `src/main.cpp:474`
|
|
|
|
- [ ] **Step 1: Change client.cpp diagnostic log level**
|
|
|
|
Change line 907 from:
|
|
```cpp
|
|
ESP_LOGI(TAG, "OTA: status read value=%d bytes empty=%d firstByte=0x%02X",
|
|
```
|
|
to:
|
|
```cpp
|
|
ESP_LOGD(TAG, "OTA: status read value=%d bytes empty=%d firstByte=0x%02X",
|
|
```
|
|
|
|
- [ ] **Step 2: Change server.cpp onRead diagnostic log level**
|
|
|
|
Change line 474 from:
|
|
```cpp
|
|
ESP_LOGI(TAG, "CLIENT_OTA_STATUS: read by conn=%d", connInfo.getConnHandle());
|
|
```
|
|
to:
|
|
```cpp
|
|
ESP_LOGD(TAG, "CLIENT_OTA_STATUS: read by conn=%d", connInfo.getConnHandle());
|
|
```
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add src/client.cpp src/main.cpp
|
|
git commit -m "chore: downgrade diagnostic OTA logs from INFO to DEBUG"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 7: Bump versions and rebuild
|
|
|
|
**Files:**
|
|
- Modify: `version.txt`
|
|
|
|
- [ ] **Step 1: Bump versions**
|
|
|
|
```
|
|
server=22
|
|
client=12
|
|
```
|
|
|
|
- [ ] **Step 2: Commit version bump**
|
|
|
|
```bash
|
|
git add version.txt
|
|
git commit -m "chore: bump versions server 21->22, client 11->12"
|
|
```
|
|
|
|
- [ ] **Step 3: Rebuild both firmware variants**
|
|
|
|
```bash
|
|
pio run
|
|
```
|
|
|
|
Verify both variants compile: `[env:s3lite-server] SUCCESS` and `[env:s3lite-client] SUCCESS`.
|
|
|
|
- [ ] **Step 4: Copy built firmware to Android assets**
|
|
|
|
```bash
|
|
cp .pio/build/s3lite-server/firmware.bin android/app/src/main/assets/firmware/server/firmware.bin
|
|
cp .pio/build/s3lite-client/firmware.bin android/app/src/main/assets/firmware/client/firmware.bin
|
|
```
|
|
|
|
- [ ] **Step 5: Commit firmware binaries**
|
|
|
|
```bash
|
|
git add android/app/src/main/assets/firmware/
|
|
git commit -m "chore: bundle server v22 and client v12 firmware in APK"
|
|
```
|
|
|
|
---
|
|
|
|
### Verification
|
|
|
|
After all tasks are complete:
|
|
|
|
1. **Check git log:**
|
|
```bash
|
|
git log --oneline -8
|
|
```
|
|
Expected: 7 new commits on top of b8facac.
|
|
|
|
2. **Flash server v22:**
|
|
```bash
|
|
# Find ESP32 server port
|
|
ls /dev/serial/by-id/ | grep -i espressif
|
|
# Update upload_port in platformio.ini (or use override), then:
|
|
pio run -t upload -e s3lite-server
|
|
```
|
|
|
|
3. **Flash client v12:**
|
|
```bash
|
|
pio run -t upload -e s3lite-client
|
|
```
|
|
|
|
4. **Verification test — Server serial:**
|
|
Connect to server serial. Confirm startup message shows v22. Confirm no "CLIENT_OTA_STATUS: read by conn=..." spam (only on CORE_DEBUG_LEVEL=4).
|
|
|
|
5. **Verification test — Client LED startup:**
|
|
Power cycle client. Confirm LED shows Blue → Red → Blue → off.
|
|
|
|
6. **Verification test — Client OTA flow via BLE:**
|
|
- Build and install APK: `cd android && ./gradlew assembleDebug`
|
|
- Deploy APK to phone via ADB
|
|
- In MainActivity, verify client OTA available notification
|
|
- Tap notification → SettingsActivity opens with client+BLE pre-selected
|
|
- Tap "Update module" → must show "Wachten op client..." progress dialog (NOT USB dialog)
|
|
- If client has older version, OTA should proceed; if same version, toast "Firmware is up-to-date"
|