feat: rename omi option #3324
feat: rename omi option #3324
Conversation
|
@krushnarout after renaming, will it show w the new name in devices list when scanning for devices to connect? |
|
@krushnarout can u also attach a demo of renaming process once ure done |
Yeah, It will going to show. |
Sure |
demo: ScreenRecording_11-02-2025.16-51-02_1.MP4 |
|
if its a custom name no need to show the serial number in bracket |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/pages/settings/device_settings.dart (1)
69-86: Don’t overwrite the stored custom name with an empty device name.
getDeviceName()often returns''when no custom label is set. Assigning that value wipes the UI (and downstream widgets) to an empty string. Please fall back to the previously stored name unless the BLE value is non-empty.- String? customName = await connection.getDeviceName(); + String? customName = await connection.getDeviceName(); + final storedName = + SharedPreferencesUtil().getCustomDeviceName(deviceProvider.pairedDevice!.id); + String? resolvedName; + if (customName != null && customName.isNotEmpty) { + resolvedName = customName; + SharedPreferencesUtil().setCustomDeviceName(deviceProvider.pairedDevice!.id, customName); + } else if (storedName.isNotEmpty) { + resolvedName = storedName; + } @@ - setState(() { - _customDeviceName = customName; + setState(() { + if (resolvedName != null) { + _customDeviceName = resolvedName; + }
🧹 Nitpick comments (1)
app/lib/providers/onboarding_provider.dart (1)
433-437: Clarify dual storage of device name.The custom device name is stored in two places:
SharedPreferencesUtil().deviceName(line 435) and viasetCustomDeviceName(line 436). This duplication may lead to inconsistencies.Consider consolidating to a single storage mechanism. If both are intentional (e.g.,
deviceNamestores the currently paired device's name globally, whilesetCustomDeviceNamestores per-device names), add a comment explaining the distinction:// Store current device name globally for quick access SharedPreferencesUtil().deviceName = customName ?? connectedDevice.name; // Store per-device custom name for multi-device scenarios SharedPreferencesUtil().setCustomDeviceName(connectedDevice.id, customName ?? connectedDevice.name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/lib/backend/preferences.dart(1 hunks)app/lib/pages/home/device.dart(4 hunks)app/lib/pages/onboarding/find_device/found_devices.dart(5 hunks)app/lib/pages/settings/device_settings.dart(4 hunks)app/lib/providers/capture_provider.dart(1 hunks)app/lib/providers/device_provider.dart(0 hunks)app/lib/providers/onboarding_provider.dart(3 hunks)app/lib/services/devices/apple_watch_connection.dart(1 hunks)app/lib/services/devices/device_connection.dart(1 hunks)app/lib/services/devices/frame_connection.dart(1 hunks)app/lib/services/devices/omi_connection.dart(2 hunks)app/lib/services/wals.dart(1 hunks)omi/firmware/omi/src/lib/core/settings.h(2 hunks)omi/firmware/omi/src/lib/core/transport.c(5 hunks)omi/firmware/omi/src/settings.c(4 hunks)
💤 Files with no reviewable changes (1)
- app/lib/providers/device_provider.dart
🧰 Additional context used
🧬 Code graph analysis (2)
omi/firmware/omi/src/lib/core/settings.h (1)
omi/firmware/omi/src/settings.c (2)
app_settings_save_device_name(119-136)app_settings_get_device_name(138-153)
omi/firmware/omi/src/lib/core/transport.c (1)
omi/firmware/omi/src/settings.c (2)
app_settings_save_device_name(119-136)app_settings_get_device_name(138-153)
🪛 Clang (14.0.6)
omi/firmware/omi/src/lib/core/settings.h
[error] 4-4: 'stddef.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (14)
omi/firmware/omi/src/lib/core/settings.h (1)
4-4: Static analysis error is a false positive.The clang error about 'stddef.h' file not found is a false positive from the analyzer running without the Zephyr SDK environment. The include is necessary and correct for the
size_ttype used in the function declarations.omi/firmware/omi/src/lib/core/transport.c (4)
6-6: LGTM!Adding
string.his necessary formemcpyused in the device name handlers.
97-107: LGTM!Forward declarations follow the existing pattern for other settings handlers.
174-175: LGTM!The UUID assignment follows the sequential pattern of the Settings Service (0x19B10011 for dim_ratio, 0x19B10012 for mic_gain, 0x19B10013 for device_name).
191-196: LGTM!The characteristic registration correctly grants both read and write permissions, consistent with other settings characteristics.
omi/firmware/omi/src/settings.c (6)
6-6: LGTM!Adding
string.his necessary forstrncpyandstrlenused in the device name functions.
13-19: LGTM! Well-defined constants and proper buffer sizing.The buffer is correctly sized as
[MAX_DEVICE_NAME_LENGTH + 1]to accommodate the maximum name length plus null terminator. However, as noted in settings.h,MAX_DEVICE_NAME_LENGTHshould be moved to the header file to prevent inconsistencies in transport.c.
50-61: LGTM! Proper bounds checking and null-termination.The settings loading logic correctly validates that the stored length doesn't exceed
MAX_DEVICE_NAME_LENGTH, reads the exact number of bytes, and ensures null-termination at the appropriate position.
81-81: LGTM!The extended log message provides useful visibility into all loaded settings, including the new device name.
119-136: LGTM! Safe string handling with proper validation.The implementation correctly:
- Validates input pointer and length before any operations
- Uses
strncpywith the defined limit to prevent buffer overflow- Explicitly null-terminates at
[MAX_DEVICE_NAME_LENGTH]to handle cases where the source is exactlyMAX_DEVICE_NAME_LENGTHcharacters (sincestrncpywon't add a null terminator in that case)- Persists using the actual string length rather than the buffer size
138-153: LGTM! Robust bounds checking and safe buffer operations.The getter function properly:
- Validates all input parameters
- Checks if the name fits in the provided buffer (including space for null terminator)
- Uses
buffer_size - 1withstrncpyto leave room for the explicit null terminator- Guarantees null-termination at
[buffer_size - 1]This defensive approach prevents buffer overflows even if the caller provides a small buffer.
app/lib/services/devices/omi_connection.dart (1)
17-17: LGTM: New BLE characteristic UUID added.The new
settingsDeviceNameCharacteristicUuidconstant follows the existing UUID pattern and integrates cleanly with the settings service.app/lib/providers/onboarding_provider.dart (1)
520-525: LGTM: Custom names applied correctly during discovery.The logic retrieves stored custom names and applies them to discovered devices via
copyWith, maintaining a consistent display name across the app.app/lib/services/devices/device_connection.dart (1)
441-458: LGTM: Device name API correctly implemented across all subclasses.Verification confirms all
DeviceConnectionsubclasses implement the required abstract methods:
- AppleWatchDeviceConnection: performSetDeviceName (line 267), performGetDeviceName (line 273)
- FrameDeviceConnection: performSetDeviceName (line 205), performGetDeviceName (line 211)
- OmiDeviceConnection: performSetDeviceName (line 551), performGetDeviceName (line 561)
The new device name methods follow the established connectivity guard pattern and are properly integrated throughout the inheritance hierarchy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/lib/pages/home/device.dart (1)
54-70: Consider simplifying the state update logic.The comparison
customName != _customDeviceNamebefore callingsetStateis an unnecessary optimization. Flutter'ssetStatealready handles no-op updates efficiently, and this check adds cognitive overhead without meaningful benefit.Apply this diff to simplify:
String? customName = await connection.getDeviceName(); if (mounted && customName != null && customName.isNotEmpty) { - if (customName != _customDeviceName) { - setState(() { - _customDeviceName = customName; - }); - } + setState(() { + _customDeviceName = customName; + }); SharedPreferencesUtil().setCustomDeviceName(deviceProvider.pairedDevice!.id, customName); }app/lib/services/devices/omi_connection.dart (2)
551-559: Consider adding client-side validation for device name.While the firmware should handle limits, adding client-side validation would improve UX by providing immediate feedback. BLE characteristics typically support up to 512 bytes with long writes, but practical limits may be lower.
Apply this diff to add validation:
@override Future<void> performSetDeviceName(String name) async { try { + if (name.isEmpty) { + debugPrint('OmiDeviceConnection: Device name cannot be empty'); + return; + } final nameBytes = utf8.encode(name); + if (nameBytes.length > 248) { + debugPrint('OmiDeviceConnection: Device name too long (${nameBytes.length} bytes, max 248)'); + return; + } await transport.writeCharacteristic(settingsServiceUuid, settingsDeviceNameCharacteristicUuid, nameBytes); } catch (e) { debugPrint('OmiDeviceConnection: Error setting device name: $e'); } }
561-573: Consider adding resilience for malformed UTF-8 data.To handle potentially corrupted BLE data more gracefully, consider using the
allowMalformedparameter. This prevents exceptions while still returning usable data.Apply this diff:
final value = await transport.readCharacteristic(settingsServiceUuid, settingsDeviceNameCharacteristicUuid); if (value.isNotEmpty) { - return utf8.decode(value); + return utf8.decode(value, allowMalformed: true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/lib/pages/home/device.dart(4 hunks)app/lib/pages/onboarding/find_device/found_devices.dart(5 hunks)app/lib/providers/capture_provider.dart(1 hunks)app/lib/services/devices/device_connection.dart(1 hunks)app/lib/services/devices/omi_connection.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/services/devices/device_connection.dart
- app/lib/providers/capture_provider.dart
🔇 Additional comments (8)
app/lib/pages/home/device.dart (3)
25-25: LGTM!The nullable String state field is appropriate for storing the optional custom device name.
38-52: LGTM!The two-stage initialization strategy is well-designed: reading from SharedPreferences provides immediate UI feedback, while the post-frame callback ensures device info is fetched before querying the BLE device name.
191-191: LGTM!The fallback chain (
_customDeviceName ?? provider.pairedDevice?.name ?? 'Unknown Device') is correctly implemented and consistently applied. The custom name takes precedence when available, meeting the PR requirement to display the saved name instead of the default.Also applies to: 370-371
app/lib/services/devices/omi_connection.dart (2)
2-2: LGTM!The
dart:convertimport is correctly added to support UTF-8 encoding/decoding for device names.
18-18: LGTM!The UUID constant follows the established naming convention and sequential pattern of other settings characteristics.
app/lib/pages/onboarding/find_device/found_devices.dart (3)
4-4: LGTM!Import is necessary for the new device naming functionality.
42-48: LGTM!The helper correctly implements the display name logic: it prefers per-device custom names from storage, falls back to the provided device name, and only appends the short ID for the default 'Omi' name. This aligns with the PR objective that custom names should not show serial numbers.
203-203: LGTM!Both call sites correctly use
_getDisplayNamewith appropriate device ID and name parameters. Once the storage inconsistency in_completeAppleWatchOnboardingis resolved, these will properly display custom device names.Also applies to: 283-283
|
mark as draft turn it ready once it's ready for review |
|
@beastoin It's ready for review |
|
mark as draft if WIP @krushnarout |
It's ready for review |
closes #2824
Summary by CodeRabbit
New Features
Behavioral