Add manual firmware flash in developer settings#6202
Conversation
aaravgarg
force-pushed
the
atlas/manual-firmware-flash
branch
from
a017738 to
d6245e3
Compare
Greptile SummaryThis PR adds a "Flash Custom Firmware" entry to Developer Settings that lets a connected user pick a Key issues found:
Confidence Score: 1/5Not safe to merge — the PR will not compile due to a type mismatch, and its core feature (using the user-selected firmware file) is broken even after fixing the compile error. There is a P0 compile-time failure (wrong provider type), a P1 logic bug that makes the feature functionally useless for legacy DFU devices (the default), and a P1 annotation error that produces an analyzer error. Multiple blocking issues need to be resolved before this can ship. app/lib/pages/settings/developer.dart — all issues are concentrated here and in the referenced firmware_mixin.dart (startLegacyDfu/startDfu need to forward zipFilePath). Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant DeveloperPage as DeveloperSettingsPage
participant FilePicker
participant FlashPage as _ManualFirmwareFlashPage
participant FirmwareMixin
participant LegacyDFU as startLegacyDfu
participant MCU as startMCUDfu
User->>DeveloperPage: Tap "Flash Custom Firmware"
DeveloperPage->>FilePicker: pickFiles(type: zip)
FilePicker-->>DeveloperPage: PlatformFile (path, name)
DeveloperPage->>FlashPage: push(zipFilePath, fileName, device)
User->>FlashPage: Tap "Flash Firmware"
FlashPage->>FirmwareMixin: startDfu(device, zipFilePath)
note over FirmwareMixin: isLegacySecureDFU=true (default, never set)
alt isLegacySecureDFU == true (always)
FirmwareMixin->>LegacyDFU: startLegacyDfu(device) ← zipFilePath DROPPED
LegacyDFU-->>FlashPage: errors via callback only, not thrown
note over FlashPage: _error never set on failure
else isLegacySecureDFU == false
FirmwareMixin->>MCU: startMCUDfu(device, zipFilePath) ✓
MCU-->>FlashPage: progress + success via setState
end
Reviews (1): Last reviewed commit: "style: final cleanup" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (provider.isConnected && provider.pairedDevice != null) ...[ | ||
| const SizedBox(height: 24), | ||
| _buildSectionHeader('Firmware', subtitle: 'Flash custom firmware builds'), | ||
| const SizedBox(height: 8), | ||
| _buildManualFirmwareFlash(provider), |
There was a problem hiding this comment.
DeveloperModeProvider passed where DeviceProvider is expected
The outer Consumer at line 410 yields a DeveloperModeProvider instance named provider. At line 1870, provider.isConnected and provider.pairedDevice are accessed on this DeveloperModeProvider, which has neither field — this will produce a compile-time error. The same provider is then passed to _buildManualFirmwareFlash(DeviceProvider provider), which expects a DeviceProvider.
The fix requires obtaining the DeviceProvider via context (e.g. context.read<DeviceProvider>()) and passing it separately.
final deviceProvider = context.read<DeviceProvider>();
if (deviceProvider.isConnected && deviceProvider.pairedDevice != null) ...[
const SizedBox(height: 24),
_buildSectionHeader('Firmware', subtitle: 'Flash custom firmware builds'),
const SizedBox(height: 8),
_buildManualFirmwareFlash(deviceProvider),
],Alternatively, wrap the section in a Consumer<DeviceProvider> so the section rebuilds when connection state changes.
| if (mounted) { | ||
| setState(() => _error = e.toString()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Scaffold( | ||
| backgroundColor: Colors.black, |
There was a problem hiding this comment.
_ManualFirmwareFlashPageState mixes in FirmwareMixin, which defaults isLegacySecureDFU = true. _startFlash calls startDfu(widget.device, zipFilePath: widget.zipFilePath), but when isLegacySecureDFU is true (which it always is here because getLatestVersion() is never called), startDfu delegates to startLegacyDfu which ignores the zipFilePath argument entirely and hardcodes the path to ${appDocumentsDirectory}/firmware.zip.
This means the firmware file the user selected via the file picker is never used. The feature is functionally broken for all legacy DFU devices (the default).
The fix requires forwarding zipFilePath through startLegacyDfu:
// In FirmwareMixin.startLegacyDfu:
Future<void> startLegacyDfu(BtDevice btDevice, {bool fileInAssets = false, String? zipFilePath}) async {
String firmwareFile = zipFilePath ?? '${(await getApplicationDocumentsDirectory()).path}/firmware.zip';
// ...
}
// And in startDfu:
Future<void> startDfu(BtDevice btDevice, {bool fileInAssets = false, String? zipFilePath}) async {
if (isLegacySecureDFU) {
return startLegacyDfu(btDevice, fileInAssets: fileInAssets, zipFilePath: zipFilePath);
}
return startMCUDfu(btDevice, fileInAssets: fileInAssets, zipFilePath: zipFilePath);
}| @override | ||
| Widget _buildManualFirmwareFlash(DeviceProvider provider) { |
There was a problem hiding this comment.
@override misplaced — stolen from build and applied to a non-overriding method
The @override annotation at line 359 was originally attached to Widget build(BuildContext context). Because the new _buildManualFirmwareFlash method was inserted between the annotation and build, the annotation now incorrectly applies to _buildManualFirmwareFlash, which does not override any member from a superclass. The Dart analyzer will flag this as an error. Meanwhile build (line 407), which genuinely overrides State.build, has lost its annotation.
Remove @override from _buildManualFirmwareFlash and restore it on build:
| @override | |
| Widget _buildManualFirmwareFlash(DeviceProvider provider) { | |
| Widget _buildManualFirmwareFlash(DeviceProvider provider) { |
@override
Widget build(BuildContext context) {| ), | ||
| body: Padding( | ||
| padding: const EdgeInsets.all(24), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| // File info | ||
| Container( | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: const Color(0xFF1C1C1E), | ||
| borderRadius: BorderRadius.circular(12), | ||
| ), |
There was a problem hiding this comment.
_startFlash wraps startDfu in a try/catch and sets _error on exception. However, startLegacyDfu (the default path) handles errors via an onError callback that calls setState(() { isInstalling = false; }) without throwing — so the error never propagates out of startDfu as an exception. When a legacy DFU failure occurs, _error stays null and the user sees no feedback; the UI simply stops showing "Installing…" with no explanation.
One fix is to use a Completer inside startLegacyDfu and complete it with an error in the onError callback:
onError: (deviceAddress, error, errorType, message) {
setState(() { isInstalling = false; });
deviceProvider.resetFirmwareUpdateState();
completer.completeError(Exception('DFU error: $message'));
},This allows the exception to propagate up to _startFlash's catch block and be shown to the user.
|
|
||
| const _ManualFirmwareFlashPage({ | ||
| required this.zipFilePath, | ||
| required this.fileName, | ||
| required this.device, | ||
| }); | ||
|
|
||
| @override | ||
| State<_ManualFirmwareFlashPage> createState() => _ManualFirmwareFlashPageState(); | ||
| } | ||
|
|
||
| class _ManualFirmwareFlashPageState extends State<_ManualFirmwareFlashPage> with FirmwareMixin { | ||
| bool _confirmed = false; | ||
| String? _error; | ||
|
|
||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| } | ||
|
|
||
| @override | ||
| void dispose() { | ||
| killMcuUpdateManager(); | ||
| super.dispose(); | ||
| } | ||
|
|
||
| Future<void> _startFlash() async { | ||
| setState(() { | ||
| _confirmed = true; | ||
| _error = null; | ||
| }); | ||
| try { | ||
| await startDfu(widget.device, zipFilePath: widget.zipFilePath); | ||
| } catch (e) { | ||
| if (mounted) { | ||
| setState(() => _error = e.toString()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return Scaffold( | ||
| backgroundColor: Colors.black, | ||
| appBar: AppBar( | ||
| backgroundColor: Colors.black, | ||
| title: const Text('Flash Firmware', style: TextStyle(color: Colors.white)), | ||
| iconTheme: const IconThemeData(color: Colors.white), | ||
| elevation: 0, | ||
| ), | ||
| body: Padding( | ||
| padding: const EdgeInsets.all(24), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| // File info | ||
| Container( | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: const Color(0xFF1C1C1E), | ||
| borderRadius: BorderRadius.circular(12), | ||
| ), | ||
| child: Row( | ||
| children: [ | ||
| const FaIcon(FontAwesomeIcons.file, color: Colors.deepPurple, size: 20), | ||
| const SizedBox(width: 12), | ||
| Expanded( | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Text(widget.fileName, style: const TextStyle(color: Colors.white, fontSize: 16, fontWeight: FontWeight.w600)), | ||
| const SizedBox(height: 4), | ||
| Text('Target: ${widget.device.name}', style: TextStyle(color: Colors.grey.shade400, fontSize: 13)), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| const SizedBox(height: 24), | ||
|
|
||
| // Warning | ||
| if (!_confirmed) ...[ | ||
| Container( | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: Colors.orange.shade900.withValues(alpha: 0.3), | ||
| borderRadius: BorderRadius.circular(12), | ||
| border: Border.all(color: Colors.orange.shade700.withValues(alpha: 0.5)), | ||
| ), | ||
| child: Row( | ||
| children: [ | ||
| Icon(Icons.warning_amber_rounded, color: Colors.orange.shade300, size: 24), | ||
| const SizedBox(width: 12), | ||
| Expanded( | ||
| child: Text( | ||
| 'Flashing custom firmware can brick your device. Make sure this is a valid Omi firmware build. Do not disconnect during the update.', | ||
| style: TextStyle(color: Colors.orange.shade300, fontSize: 13), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| const Spacer(), | ||
| SizedBox( | ||
| width: double.infinity, | ||
| height: 52, | ||
| child: ElevatedButton( | ||
| onPressed: _startFlash, | ||
| style: ElevatedButton.styleFrom( | ||
| backgroundColor: Colors.deepPurple, | ||
| shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(12)), | ||
| ), | ||
| child: const Text('Flash Firmware', style: TextStyle(color: Colors.white, fontSize: 16, fontWeight: FontWeight.w600)), | ||
| ), | ||
| ), | ||
| ], | ||
|
|
||
| // Progress | ||
| if (_confirmed && !isInstalled) ...[ | ||
| const SizedBox(height: 16), | ||
| Text( | ||
| isInstalling ? 'Installing...' : 'Preparing...', | ||
| style: const TextStyle(color: Colors.white, fontSize: 18, fontWeight: FontWeight.w600), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| LinearProgressIndicator( | ||
| value: installProgress / 100, | ||
| backgroundColor: const Color(0xFF2A2A2E), | ||
| valueColor: const AlwaysStoppedAnimation<Color>(Colors.deepPurple), | ||
| minHeight: 8, | ||
| borderRadius: BorderRadius.circular(4), | ||
| ), | ||
| const SizedBox(height: 8), | ||
| Text('${installProgress}%', style: TextStyle(color: Colors.grey.shade400, fontSize: 14)), | ||
| ], | ||
|
|
||
| // Success | ||
| if (isInstalled) ...[ | ||
| const SizedBox(height: 32), | ||
| const Center( | ||
| child: Column( | ||
| children: [ | ||
| Icon(Icons.check_circle, color: Colors.green, size: 64), | ||
| SizedBox(height: 16), | ||
| Text('Firmware flashed successfully!', style: TextStyle(color: Colors.white, fontSize: 18, fontWeight: FontWeight.w600)), | ||
| SizedBox(height: 8), | ||
| Text('Your device will restart.', style: TextStyle(color: Colors.grey, fontSize: 14)), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
|
|
||
| // Error | ||
| if (_error != null) ...[ | ||
| const SizedBox(height: 16), | ||
| Container( | ||
| padding: const EdgeInsets.all(12), | ||
| decoration: BoxDecoration( | ||
| color: Colors.red.shade900.withValues(alpha: 0.3), | ||
| borderRadius: BorderRadius.circular(8), | ||
| ), | ||
| child: Text(_error!, style: TextStyle(color: Colors.red.shade300, fontSize: 13)), | ||
| ), | ||
| ], | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Per the Flutter localization guide, all user-facing strings must use l10n. The _ManualFirmwareFlashPage and _buildManualFirmwareFlash contain many hardcoded strings that should be defined in ARB files and accessed via context.l10n.*. Examples include:
'Flash Custom Firmware'(line 396)'Select firmware ZIP file'(line 368)'Flash Firmware'(AppBar title / button label)'Target: ${widget.device.name}''Flashing custom firmware can brick your device...''Installing...'/'Preparing...''Firmware flashed successfully!''Your device will restart.''Firmware'section header and'Flash custom firmware builds'subtitle (line 1872)
Context Used: Flutter localization - all user-facing strings mus... (source)
| style: TextStyle(color: Colors.orange.shade300, fontSize: 13), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| const Spacer(), | ||
| SizedBox( | ||
| width: double.infinity, | ||
| height: 52, | ||
| child: ElevatedButton( | ||
| onPressed: _startFlash, | ||
| style: ElevatedButton.styleFrom( |
There was a problem hiding this comment.
When flashing completes (isInstalled == true), the success state is shown but there is no button to dismiss the page. The user must discover the AppBar back button themselves. Consider adding a "Done" button below the success message to make the flow feel complete.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adds a "Flash Custom Firmware" option in Developer Settings that lets users pick a `.zip` firmware file and flash it directly to their connected Omi device. - Only visible when device is connected - Uses existing MCUmgr/Nordic DFU pipeline - File picker → warning screen → progress bar → success - No new dependencies
1 participant
Adds a "Flash Custom Firmware" option in Developer Settings that lets users pick a
.zipfirmware file and flash it directly to their connected Omi device.