fix(firmware): batch of P3 OTA/DFU cleanups from the #5915 audit (#5916)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
James Rich
2026-06-23 13:35:43 -05:00
committed by GitHub
parent 60b7908e1b
commit 61c8a3f479
16 changed files with 300 additions and 31 deletions

View File

@@ -619,6 +619,7 @@ firmware_update_title
firmware_update_unknown_error
firmware_update_unknown_hardware
firmware_update_unknown_release
firmware_update_unsupported_transport
firmware_update_uploading
firmware_update_usb_bootloader_warning
firmware_update_usb_failed

View File

@@ -643,6 +643,7 @@
<string name="firmware_update_unknown_error">Unknown error</string>
<string name="firmware_update_unknown_hardware">Unknown hardware model: %1$d</string>
<string name="firmware_update_unknown_release">Unknown remote release</string>
<string name="firmware_update_unsupported_transport">Firmware updates are not supported over this connection for this device. Connect a different way (e.g. Bluetooth) to update.</string>
<string name="firmware_update_uploading">Uploading firmware...</string>
<string name="firmware_update_usb_bootloader_warning">%1$s usually ships with a bootloader that does not support OTA updates. You may need to flash an OTA-capable bootloader over USB before flashing OTA.</string>
<string name="firmware_update_usb_failed">USB Update failed</string>

View File

@@ -22,6 +22,7 @@ import kotlinx.serialization.json.Json
import org.koin.core.annotation.Single
import org.meshtastic.core.database.entity.FirmwareRelease
import org.meshtastic.core.model.DeviceHardware
import org.meshtastic.feature.firmware.ota.FirmwareHashUtil
private val KNOWN_ARCHS = setOf("esp32-s3", "esp32-c3", "esp32-c6", "nrf52840", "rp2040", "stm32", "esp32")
@@ -173,14 +174,41 @@ class FirmwareRetriever(private val fileHandler: FirmwareFileHandler) {
Logger.i { "Manifest resolved OTA firmware: ${otaEntry.name} (${otaEntry.bytes} bytes, md5=${otaEntry.md5})" }
return retrieveArtifact(
release = release,
hardware = hardware,
onProgress = onProgress,
fileSuffix = ".bin",
internalFileExtension = ".bin",
preferredFilename = otaEntry.name,
)
val artifact =
retrieveArtifact(
release = release,
hardware = hardware,
onProgress = onProgress,
fileSuffix = ".bin",
internalFileExtension = ".bin",
preferredFilename = otaEntry.name,
) ?: return null
// Verify the download against the manifest before handing it to the OTA flow. On mismatch we return null so the
// caller falls back to the filename heuristics (exactly like a missing/garbage manifest) — surfacing a bad
// download here beats rebooting the device into OTA mode, which tears down the mesh link, only to fail
// device-side hash verification mid-flash.
return artifact.takeIf { verifyAgainstManifest(it, otaEntry) }
}
/**
* Check a downloaded `.bin` against the manifest entry's [FirmwareManifestFile.bytes] and
* [FirmwareManifestFile.md5] (each enforced only when the manifest actually carries it). The md5 read is skipped
* when the size already fails, avoiding a full re-read of a known-wrong file.
*/
private suspend fun verifyAgainstManifest(artifact: FirmwareArtifact, entry: FirmwareManifestFile): Boolean {
val sizeOk = entry.bytes <= 0 || fileHandler.getFileSize(artifact) == entry.bytes
val md5Ok =
!sizeOk ||
entry.md5.isBlank() ||
FirmwareHashUtil.calculateMd5Hex(fileHandler.readBytes(artifact)).equals(entry.md5, ignoreCase = true)
if (!sizeOk || !md5Ok) {
Logger.w {
"Manifest integrity check failed for ${entry.name} " +
"(expected ${entry.bytes} bytes, md5=${entry.md5}; sizeOk=$sizeOk, md5Ok=$md5Ok)"
}
}
return sizeOk && md5Ok
}
// ── Private helpers ──────────────────────────────────────────────────────

View File

@@ -120,6 +120,7 @@ import org.meshtastic.core.resources.firmware_update_taking_a_while
import org.meshtastic.core.resources.firmware_update_target
import org.meshtastic.core.resources.firmware_update_title
import org.meshtastic.core.resources.firmware_update_unknown_release
import org.meshtastic.core.resources.firmware_update_unsupported_transport
import org.meshtastic.core.resources.firmware_update_usb_bootloader_warning
import org.meshtastic.core.resources.firmware_update_usb_instruction_text
import org.meshtastic.core.resources.firmware_update_usb_instruction_title
@@ -374,6 +375,13 @@ private fun ReadyState(
selectedReleaseType: FirmwareReleaseType,
actions: FirmwareUpdateActions,
) {
// Unknown == no update path for this transport+device combo (e.g. ESP32 over Serial, nRF52 over TCP). Say so
// up front instead of offering a button whose handler would only throw on press.
if (state.updateMethod is FirmwareUpdateMethod.Unknown) {
UnsupportedTransportState()
return
}
var showDisclaimer by remember { mutableStateOf(false) }
val device = state.deviceHardware
val haptic = LocalHapticFeedback.current
@@ -455,6 +463,26 @@ private fun ReadyState(
}
}
@Composable
private fun UnsupportedTransportState() {
Column(horizontalAlignment = Alignment.CenterHorizontally) {
Spacer(Modifier.height(16.dp))
Icon(
MeshtasticIcons.Warning,
contentDescription = null,
modifier = Modifier.size(48.dp),
tint = MaterialTheme.colorScheme.onSurfaceVariant,
)
Spacer(Modifier.height(16.dp))
Text(
stringResource(Res.string.firmware_update_unsupported_transport),
style = MaterialTheme.typography.bodyLarge,
color = MaterialTheme.colorScheme.onSurfaceVariant,
textAlign = TextAlign.Center,
)
}
}
@Composable
internal fun DisclaimerDialog(updateMethod: FirmwareUpdateMethod, onDismiss: () -> Unit, onConfirm: () -> Unit) {
MeshtasticDialog(

View File

@@ -188,7 +188,14 @@ class FirmwareUpdateViewModel(
radioPrefs.isBle() -> FirmwareUpdateMethod.Ble
radioPrefs.isTcp() -> FirmwareUpdateMethod.Wifi
radioPrefs.isTcp() -> {
// WiFi OTA is ESP32-only; nRF52/RP2040 have no TCP update path.
if (deviceHardware.isEsp32Arc) {
FirmwareUpdateMethod.Wifi
} else {
FirmwareUpdateMethod.Unknown
}
}
else -> FirmwareUpdateMethod.Unknown
}

View File

@@ -151,19 +151,16 @@ class BleOtaTransport(
onHandshakeStatus: suspend (OtaHandshakeStatus) -> Unit,
): Result<Unit> = safeCatching {
val command = OtaCommand.StartOta(sizeBytes, sha256Hash)
val packetsSent = sendCommand(command)
sendCommand(command)
// Drive on response *type*, never a fragment/response count: the handshake completes only on an explicit OK.
// ERASING is an interim progress notification the device may emit before OK, so it just continues the wait. At
// a low MTU the command splits into multiple writes; gating completion on a write count would desync against
// the device's single OK — the same fragment-count bug PR #5915 removed from streamFirmware.
var handshakeComplete = false
var responsesReceived = 0
while (!handshakeComplete) {
val response = waitForResponse(ERASING_TIMEOUT)
responsesReceived++
when (val parsed = OtaResponse.parse(response)) {
is OtaResponse.Ok -> {
if (responsesReceived >= packetsSent) {
handshakeComplete = true
}
}
when (val parsed = OtaResponse.parse(waitForResponse(ERASING_TIMEOUT))) {
is OtaResponse.Ok -> handshakeComplete = true
is OtaResponse.Erasing -> {
Logger.i { "BLE OTA: Device erasing flash..." }
@@ -177,9 +174,7 @@ class BleOtaTransport(
throw OtaProtocolException.CommandFailed(command, parsed)
}
else -> {
Logger.w { "BLE OTA: Unexpected handshake response: $response" }
}
else -> Logger.w { "BLE OTA: Unexpected handshake response: $parsed" }
}
}
}
@@ -205,6 +200,11 @@ class BleOtaTransport(
onProgress: suspend (Float) -> Unit,
): Result<Unit> = safeCatching {
val totalBytes = data.size
if (totalBytes == 0) {
// Fail now: an empty image would otherwise skip the loop and then wait out the full
// VERIFICATION_TIMEOUT before failing.
throw OtaProtocolException.TransferFailed("Firmware is empty")
}
var sentBytes = 0
val writePayload = bleConnection.maximumWriteValueLength(BleWriteType.WITHOUT_RESPONSE) ?: SAFE_WRITE_PAYLOAD

View File

@@ -62,6 +62,12 @@ private const val PACKET_SEND_DELAY_MS = 2000L
// Time to wait for BLE GATT to fully release after disconnecting mesh service
private const val GATT_RELEASE_DELAY_MS = 1000L
// A BLE target is a 6-octet MAC (e.g. AA:BB:CC:DD:EE:FF). Matching the exact MAC shape — not merely "contains a colon"
// — keeps an IPv6 WiFi target (which also has colons) from being misrouted to the BLE path.
private val MAC_ADDRESS_REGEX = Regex("([0-9A-Fa-f]{2}:){5}[0-9A-Fa-f]{2}")
internal fun isBleMacAddress(target: String): Boolean = MAC_ADDRESS_REGEX.matches(target)
/**
* KMP handler for ESP32 firmware updates using the Unified OTA protocol. Supports both BLE and WiFi/TCP transports via
* [UnifiedOtaProtocol].
@@ -80,14 +86,14 @@ class Esp32OtaUpdateHandler(
private val dispatchers: CoroutineDispatchers,
) : FirmwareUpdateHandler {
/** Entry point for FirmwareUpdateHandler interface. Routes to BLE (MAC with colons) or WiFi (IP without). */
/** Entry point for FirmwareUpdateHandler interface. Routes to BLE (target is a MAC) or WiFi (anything else). */
override suspend fun startUpdate(
release: FirmwareRelease,
hardware: DeviceHardware,
target: String,
updateState: (FirmwareUpdateState) -> Unit,
firmwareUri: CommonUri?,
): FirmwareArtifact? = if (target.contains(":")) {
): FirmwareArtifact? = if (isBleMacAddress(target)) {
startBleUpdate(release, hardware, target, updateState, firmwareUri)
} else {
startWifiUpdate(release, hardware, target, updateState, firmwareUri)

View File

@@ -31,4 +31,7 @@ object FirmwareHashUtil {
/** Convert byte array to lowercase hex string. */
fun bytesToHex(bytes: ByteArray): String = bytes.toByteString().hex()
/** Calculate the MD5 hex digest of raw bytes — used to verify a download against a firmware manifest's `md5`. */
fun calculateMd5Hex(data: ByteArray): String = data.toByteString().md5().hex()
}

View File

@@ -51,6 +51,16 @@ internal fun parseDfuZipEntries(entries: Map<String, ByteArray>): DfuZipPackage
val entry =
manifest.manifest.primaryEntry ?: throw DfuException.InvalidPackage("No firmware entry found in manifest.json")
// ponytail: app-only Meshtastic OTA zips carry a single image, so we transfer only the primary. A combined
// app+SoftDevice+bootloader package would need sequential DFU sessions we don't implement — warn loudly rather
// than silently flash one image of several. Upgrade path: multi-image sequencing if such packages ever ship.
if (manifest.manifest.imageCount > 1) {
Logger.w {
"DFU: package declares ${manifest.manifest.imageCount} images; flashing only the primary " +
"(${entry.binFile}). Combined app+SoftDevice+bootloader packages are not supported."
}
}
val initPacket =
entries[entry.datFile] ?: throw DfuException.InvalidPackage("Init packet '${entry.datFile}' not found in zip")
val firmware =

View File

@@ -268,6 +268,10 @@ internal data class DfuManifestContent(
/** First non-null entry in priority order. */
val primaryEntry: DfuManifestEntry?
get() = application ?: softdeviceBootloader ?: bootloader ?: softdevice
/** Number of image entries present. >1 means a combined package, of which only [primaryEntry] is flashed. */
val imageCount: Int
get() = listOfNotNull(application, softdeviceBootloader, bootloader, softdevice).size
}
@Serializable

View File

@@ -210,10 +210,12 @@ class SecureDfuTransport(
}
}
} catch (_: TimeoutCancellationException) {
Logger.w {
"DFU: $logLabel buttonless trigger timed out — likely cause: stale BLE bond (Meshtastic " +
"BLEDfu requires SECMODE_ENC_WITH_MITM). User must Forget+Re-pair the device in Android " +
"Bluetooth settings if the next DFU-mode scan also fails."
// Expected on the normal success path: the WITH_RESPONSE write never ATT-ACKs because the device reboots
// into the bootloader before sending it. connectToDfuMode()'s scan confirms whether the trigger actually
// landed; only that scan failing indicates a real problem (e.g. a stale bond that blocked the trigger),
// and it surfaces the Forget+Re-pair guidance there.
Logger.d {
"DFU: $logLabel buttonless trigger write did not ACK before timeout (expected — device rebooting)"
}
}
}
@@ -233,7 +235,11 @@ class SecureDfuTransport(
val device =
scanForDevice { d -> d.address in targetAddresses }
?: throw DfuException.ConnectionFailed("DFU mode device not found. Tried: $targetAddresses")
?: throw DfuException.ConnectionFailed(
"DFU mode device not found (tried $targetAddresses). If the device never rebooted into DFU mode, " +
"a stale BLE bond may be blocking the trigger (Meshtastic BLEDfu requires " +
"SECMODE_ENC_WITH_MITM) — Forget+Re-pair the device in Android Bluetooth settings and retry.",
)
Logger.i { "DFU: Found DFU mode device at ${device.address}, connecting..." }

View File

@@ -22,6 +22,7 @@ import kotlinx.coroutines.test.runTest
import org.meshtastic.core.common.util.CommonUri
import org.meshtastic.core.database.entity.FirmwareRelease
import org.meshtastic.core.model.DeviceHardware
import org.meshtastic.feature.firmware.ota.FirmwareHashUtil
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
@@ -90,6 +91,66 @@ abstract class CommonFirmwareRetrieverTest {
assertEquals("firmware-heltec-v3-2.7.17.bin", result.fileName)
}
@Test
fun `retrieveEsp32Firmware accepts manifest artifact when md5 and size match`() = runTest {
val handler = FakeFirmwareFileHandler()
val retriever = FirmwareRetriever(handler)
// Distinctive app0 name no fallback heuristic would construct, so resolving it proves the verified manifest
// path returned the artifact (rather than a filename-heuristic fallback that happens to share the name).
val name = "firmware-heltec-v3-2.7.17-app0.bin"
val payload = ByteArray(2048) { it.toByte() }
val md5 = FirmwareHashUtil.calculateMd5Hex(payload)
handler.textResponses["$BASE_URL/firmware-2.7.17/firmware-heltec-v3-2.7.17.mt.json"] =
"""{"files":[{"name":"$name","part_name":"app0","md5":"$md5","bytes":${payload.size}}]}"""
handler.existingUrls.add("$BASE_URL/firmware-2.7.17/$name")
handler.fileBytes[name] = payload
val result = retriever.retrieveEsp32Firmware(TEST_RELEASE, TEST_HARDWARE) {}
assertNotNull(result, "Manifest artifact passing the md5+size check should resolve")
assertEquals(name, result.fileName)
}
@Test
fun `retrieveEsp32Firmware rejects manifest artifact on md5 mismatch and falls back`() = runTest {
val handler = FakeFirmwareFileHandler()
val retriever = FirmwareRetriever(handler)
val name = "firmware-heltec-v3-2.7.17-app0.bin"
// Size matches (4) so md5 is computed; the manifest md5 is wrong, so the artifact must be rejected.
handler.textResponses["$BASE_URL/firmware-2.7.17/firmware-heltec-v3-2.7.17.mt.json"] =
"""{"files":[{"name":"$name","part_name":"app0","md5":"deadbeef","bytes":4}]}"""
handler.existingUrls.add("$BASE_URL/firmware-2.7.17/$name")
handler.fileBytes[name] = byteArrayOf(0x01, 0x02, 0x03, 0x04)
// Heuristic fallback the rejection should land on.
handler.existingUrls.add("$BASE_URL/firmware-2.7.17/firmware-heltec-v3-2.7.17.bin")
val result = retriever.retrieveEsp32Firmware(TEST_RELEASE, TEST_HARDWARE) {}
assertNotNull(result)
assertEquals("firmware-heltec-v3-2.7.17.bin", result.fileName)
}
@Test
fun `retrieveEsp32Firmware rejects manifest artifact on size mismatch and falls back`() = runTest {
val handler = FakeFirmwareFileHandler()
val retriever = FirmwareRetriever(handler)
val name = "firmware-heltec-v3-2.7.17-app0.bin"
// Blank md5 → only size is checked; the declared size disagrees with the download, so it must be rejected.
handler.textResponses["$BASE_URL/firmware-2.7.17/firmware-heltec-v3-2.7.17.mt.json"] =
"""{"files":[{"name":"$name","part_name":"app0","md5":"","bytes":9999}]}"""
handler.existingUrls.add("$BASE_URL/firmware-2.7.17/$name")
handler.fileBytes[name] = ByteArray(10)
handler.existingUrls.add("$BASE_URL/firmware-2.7.17/firmware-heltec-v3-2.7.17.bin")
val result = retriever.retrieveEsp32Firmware(TEST_RELEASE, TEST_HARDWARE) {}
assertNotNull(result)
assertEquals("firmware-heltec-v3-2.7.17.bin", result.fileName)
}
@Test
fun `retrieveEsp32Firmware falls back to current naming when manifest unavailable`() = runTest {
val handler = FakeFirmwareFileHandler()
@@ -345,6 +406,9 @@ abstract class CommonFirmwareRetrieverTest {
/** Result returned by [extractFirmwareFromZip]. */
var zipExtractionResult: FirmwareArtifact? = null
/** Bytes (and thus size) reported by [readBytes] / [getFileSize] for a downloaded file, keyed by fileName. */
val fileBytes = mutableMapOf<String, ByteArray>()
// Tracking
val checkedUrls = mutableListOf<String>()
val fetchedTextUrls = mutableListOf<String>()
@@ -401,9 +465,10 @@ abstract class CommonFirmwareRetrieverTest {
preferredFilename: String?,
): FirmwareArtifact? = zipExtractionResult
override suspend fun getFileSize(file: FirmwareArtifact): Long = 0L
override suspend fun getFileSize(file: FirmwareArtifact): Long = (fileBytes[file.fileName]?.size ?: 0).toLong()
override suspend fun readBytes(artifact: FirmwareArtifact): ByteArray = ByteArray(0)
override suspend fun readBytes(artifact: FirmwareArtifact): ByteArray =
fileBytes[artifact.fileName] ?: ByteArray(0)
override suspend fun importFromUri(uri: CommonUri): FirmwareArtifact? = null

View File

@@ -369,6 +369,23 @@ class FirmwareUpdateViewModelTest {
assertIs<FirmwareUpdateMethod.Unknown>(state.updateMethod)
}
@Test
fun `update method is Unknown for TCP nrf52`() = runTest {
// WiFi OTA is ESP32-only — nRF52 over TCP has no update path, so the method must be Unknown (the screen then
// shows an unsupported message instead of an Update button that would only throw on press).
val hardware = DeviceHardware(hwModel = 1, architecture = "nrf52", platformioTarget = "tbeam")
everySuspend { deviceHardwareRepository.getDeviceHardwareByModel(any(), any()) } returns
Result.success(hardware)
every { radioPrefs.devAddr } returns MutableStateFlow("t192.168.1.1")
viewModel = createViewModel()
advanceUntilIdle()
val state = viewModel.state.value
assertIs<FirmwareUpdateState.Ready>(state)
assertIs<FirmwareUpdateMethod.Unknown>(state.updateMethod)
}
@Test
fun `setReleaseType LOCAL produces null release in Ready`() = runTest {
advanceUntilIdle()

View File

@@ -422,6 +422,21 @@ class BleOtaTransportTest {
assertIs<OtaProtocolException.TransferFailed>(result.exceptionOrNull())
}
@Test
fun `streamFirmware fails immediately on empty firmware`() = runTest {
val scanner = FakeBleScanner()
val connection = FakeBleConnection()
val (transport) = createTransport(scanner, connection)
connectTransport(transport, scanner, connection)
// Empty image: must fail right away rather than skipping the loop and waiting out VERIFICATION_TIMEOUT. No
// device response is buffered, so a regression (waiting for a response) would surface as a Timeout, not this.
val result = transport.streamFirmware(ByteArray(0), 512) {}
assertTrue(result.isFailure)
assertIs<OtaProtocolException.TransferFailed>(result.exceptionOrNull())
}
// -----------------------------------------------------------------------
// close()
// -----------------------------------------------------------------------

View File

@@ -0,0 +1,48 @@
/*
* Copyright (c) 2026 Meshtastic LLC
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
package org.meshtastic.feature.firmware.ota
import kotlin.test.Test
import kotlin.test.assertFalse
import kotlin.test.assertTrue
/** Verifies BLE-vs-WiFi target routing in [Esp32OtaUpdateHandler] — specifically that IPv6 literals route to WiFi. */
class Esp32OtaRoutingTest {
@Test
fun `MAC address is a BLE target`() {
assertTrue(isBleMacAddress("AA:BB:CC:DD:EE:FF"))
assertTrue(isBleMacAddress("00:11:22:33:44:55"))
}
@Test
fun `IPv4 address is not a BLE target`() {
assertFalse(isBleMacAddress("192.168.1.100"))
}
@Test
fun `IPv6 literal is not a BLE target`() {
// The bug this guards: an IPv6 literal carries colons but must route to WiFi, not be mistaken for a MAC.
assertFalse(isBleMacAddress("fe80::1"))
assertFalse(isBleMacAddress("2001:db8::ff00:42:8329"))
}
@Test
fun `hostname is not a BLE target`() {
assertFalse(isBleMacAddress("meshtastic.local"))
}
}

View File

@@ -124,4 +124,34 @@ class DfuZipParserTest {
val ex = assertFailsWith<DfuException.InvalidPackage> { parseDfuZipEntries(entries) }
assertEquals("Firmware 'app.bin' not found in zip", ex.message)
}
@Test
fun picksPriorityImageWhenMultiplePresent() {
// Combined packages are unsupported; the parser flashes only the priority (application) image, ignoring the
// softdevice entry. Guards the multi-image limitation documented in parseDfuZipEntries.
val manifestJson =
"""
{
"manifest": {
"application": { "bin_file": "app.bin", "dat_file": "app.dat" },
"softdevice": { "bin_file": "sd.bin", "dat_file": "sd.dat" }
}
}
"""
.trimIndent()
val entries =
mapOf(
"manifest.json" to manifestJson.encodeToByteArray(),
"app.bin" to byteArrayOf(0x01, 0x02),
"app.dat" to byteArrayOf(0x03),
"sd.bin" to byteArrayOf(0x04),
"sd.dat" to byteArrayOf(0x05),
)
val packageResult = parseDfuZipEntries(entries)
assertTrue(packageResult.firmware.contentEquals(byteArrayOf(0x01, 0x02)))
assertTrue(packageResult.initPacket.contentEquals(byteArrayOf(0x03)))
}
}