Skip to content

Enhancement/preset offgrid repeat toggle#275

Open
just-stuff-tm wants to merge 5 commits intozjs81:mainfrom
just-stuff-tm:enhancement/preset-offgrid-repeat-toggle
Open

Enhancement/preset offgrid repeat toggle#275
just-stuff-tm wants to merge 5 commits intozjs81:mainfrom
just-stuff-tm:enhancement/preset-offgrid-repeat-toggle

Conversation

@just-stuff-tm
Copy link
Copy Markdown
Contributor

@just-stuff-tm just-stuff-tm commented Mar 9, 2026 •
edited
Loading

@just-stuff-tm just-stuff-tm marked this pull request as ready for review March 11, 2026 03:05
@just-stuff-tm just-stuff-tm force-pushed the enhancement/preset-offgrid-repeat-toggle branch from 8d10adc to f233bdb Compare March 11, 2026 03:16
@zjs81
Copy link
Copy Markdown
Owner

zjs81 commented Mar 14, 2026

Code Review Findings

Must Fix

  1. Floating-point frequency comparison is fragile — settings_screen.dart
    _findMatchingPresetIndexForSnapshot and _inferNonRepeatSnapshotForRepeatEnabled compare frequencies with epsilon = 0.001, but values go through double.tryParse, toStringAsFixed(3), and freqHz / 1000.0 which can accumulate rounding errors beyond that threshold. Compare integer Hz values instead.

  2. initialValue is not a valid parameter on DropdownButtonFormField — settings_screen.dart
    The correct parameter is value. If this compiles, it's being silently ignored and the dropdown has no initial selection. If it doesn't compile, this is a build break. Needs to be changed to value: _selectedPresetIndex.

  3. PR bundles two unrelated features
    Linux BLE pairing and preset/off-grid repeat toggle are independent concerns. This makes the diff harder to review and increases revert blast radius. Consider splitting into two PRs.

Should Fix

  1. Busy-wait polling loop in pairAndTrust — linux_ble_pairing_service.dart
    The 200ms polling loop runs for up to 45 seconds. Use a Completer signaled by handleChunk instead — it's more responsive and idiomatic for async Dart.

  2. Recursive retry without explicit depth bound — linux_ble_pairing_service.dart
    pairAndTrust calls itself recursively via removeRetryUsed and proactivePinRetryUsed flags. The boolean guards theoretically limit depth but the interaction is hard to verify. Use a single explicit retry counter instead.

  3. Two snapshot classes for the same concept
    MeshCoreRadioStateSnapshot (connector, Hz/raw ints) and _RadioSettingsSnapshot (settings screen, MHz/enums) represent nearly identical data. The conversion between them is error-prone. Unify or add a factory/conversion method.

  4. _toUiCodingRate / _toDeviceCodingRate round-trip fidelity
    These must be perfect inverses or the coding rate silently shifts on restore. Add an assertion or test verifying the round-trip.

Nice to Have

  • Extract hardcoded delay values (700ms, 1200ms, 6s timeout) as named constants with comments
  • _logRadioSettingsState is very verbose for production — consider gating behind debug flag
  • _FakeProcess in tests should use broadcast stream controllers to avoid single-listener issues
  • Off-grid frequency band mapping is hardcoded — consider deriving from the preset list

@just-stuff-tm
Copy link
Copy Markdown
Contributor Author

just-stuff-tm commented Mar 15, 2026

I rebased will be pushing with the enhancements shortly

Copilot AI review requested due to automatic review settings March 15, 2026 20:15
@just-stuff-tm just-stuff-tm force-pushed the enhancement/preset-offgrid-repeat-toggle branch from 166cd72 to fb11882 Compare March 15, 2026 20:15
Copilot AI reviewed Mar 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the off-grid repeat toggle in the radio settings dialog so that toggling repeat mode automatically adjusts the frequency to the appropriate off-grid frequency for the selected region/band (and restores it when disabled). It also tracks and remembers the non-repeat radio state across the session, filters off-grid presets from the visible dropdown, and adds debug logging for radio settings state changes.

Changes:

  • Automatically switch frequency to the band-appropriate off-grid frequency when the repeat toggle is enabled, and restore the original preset frequency when disabled.
  • Introduce _RadioSettingsSnapshot class and MeshCoreRadioStateSnapshot to capture and remember radio state, enabling round-trip persistence of non-repeat settings.
  • Add debug-mode logging throughout radio settings interactions and refactor preset selection to track and sync with manual setting changes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lib/connector/meshcore_connector.dart Adds MeshCoreRadioStateSnapshot data class and in-memory storage for remembered non-repeat radio state on the connector.
lib/screens/settings_screen.dart Major refactor of radio settings dialog: adds snapshot tracking, off-grid frequency auto-switching, preset filtering, sync logic, and debug logging. Moves coding rate helpers to top-level.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/screens/settings_screen.dart

_clientRepeat = widget.connector.clientRepeat ?? false;
_selectedPresetIndex = _findMatchingPresetIndex();
_lastNonRepeatSnapshot = _currentSnapshot();
lib/screens/settings_screen.dart
Comment on lines 1380 to +1381
Navigator.pop(context);
_logRadioSettingsState('Radio settings saved successfully');
@wel97459
Refactor contact handling and other improvments (zjs81#317)
d0e3767 
* Refactor contact filtering and improve localization strings; enhance path trace handling

* Add localization for new CLI commands and update existing strings

* Enhance contact handling and UI updates across multiple screens
add unfiltered contact access and improve last seen resolution

* Add polling interval configuration and improve contact handling

* Reorder command constants for better organization and clarity

* Refactor contact handling by removing unnecessary mapping and improving clarity across multiple screens

* Moved RadioStatsIconButton in chat screen for improved UI consistency

* Added indicators to AppBar for channels

* Ignore contacts with self public key in contact handling

* Simplify path removal logic and clean up unused imports in path management dialog

* Enhance path hop resolution by adding distance checks to improve candidate selection accuracy

* Remove unnecessary reset of radio stats poll reference count in polling interval setter
@just-stuff-tm just-stuff-tm force-pushed the enhancement/preset-offgrid-repeat-toggle branch 2 times, most recently from d0e3767 to 6375710 Compare March 27, 2026 15:48
@just-stuff-tm
Reapply "Fixed Preset on offgrid repeat toggle enhancemet zjs81#183"
6b6d9ca 
This reverts commit 758619b.
@just-stuff-tm
fix(settings): preserve preset across off-grid repeat
c9145c9
@just-stuff-tm
fix(settings): scope repeat preset memory to saved state
36697c6
@just-stuff-tm
fix(settings): use integer Hz comparison, unify snapshot conversion, …
1e9508d 
…gate debug logging

- Replace floating-point epsilon frequency comparison with integer Hz
- Add frequencyHz getter and fromMeshCoreSnapshot/toMeshCoreSnapshot
  conversion methods on _RadioSettingsSnapshot
- Move _toUiCodingRate/_toDeviceCodingRate to documented top-level functions
- Gate _logRadioSettingsState behind kDebugMode
- Use integer Hz in == and hashCode for _RadioSettingsSnapshot

Addresses code review findings on preset/off-grid repeat toggle PR.
@just-stuff-tm just-stuff-tm force-pushed the enhancement/preset-offgrid-repeat-toggle branch from 6375710 to 1e9508d Compare March 27, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@just-stuff-tm @zjs81 @wel97459