Skip to content

Enhancement/preset offgrid repeat toggle#275

Merged
446564 merged 4 commits into
zjs81:devfrom
just-stuff-tm:enhancement/preset-offgrid-repeat-toggle
Apr 8, 2026
Merged

Enhancement/preset offgrid repeat toggle#275
446564 merged 4 commits into
zjs81:devfrom
just-stuff-tm:enhancement/preset-offgrid-repeat-toggle

Conversation

@just-stuff-tm

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

Copy link
Copy Markdown
Contributor

@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

zjs81 commented Mar 14, 2026

Copy link
Copy Markdown
Owner

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

just-stuff-tm commented Mar 15, 2026

Copy link
Copy Markdown
Contributor Author

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread lib/screens/settings_screen.dart

_clientRepeat = widget.connector.clientRepeat ?? false;
_selectedPresetIndex = _findMatchingPresetIndex();
_lastNonRepeatSnapshot = _currentSnapshot();

@446564 446564 Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree this can be removed

Comment thread lib/screens/settings_screen.dart
Comment on lines 1380 to +1381
Navigator.pop(context);
_logRadioSettingsState('Radio settings saved successfully');

@446564 446564 Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't see this error in debug logs while testing but it can't hurt to move it up.

@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 #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
@446564

446564 commented Apr 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

translation conflicts to be addressed was issue on main not dev

@446564 446564 changed the base branch from main to dev April 8, 2026 16:09
@446564 446564 self-assigned this Apr 8, 2026
446564
446564 approved these changes Apr 8, 2026
View reviewed changes

@446564 446564 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Small, almost optional, changes but otherwise good.

Tested a few times to double check and all functioning well.

Comment thread lib/screens/settings_screen.dart

_clientRepeat = widget.connector.clientRepeat ?? false;
_selectedPresetIndex = _findMatchingPresetIndex();
_lastNonRepeatSnapshot = _currentSnapshot();

@446564 446564 Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree this can be removed

Comment thread lib/screens/settings_screen.dart
Comment on lines 1380 to +1381
Navigator.pop(context);
_logRadioSettingsState('Radio settings saved successfully');

@446564 446564 Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't see this error in debug logs while testing but it can't hurt to move it up.

@446564 446564 merged commit 9436c2d into zjs81:dev Apr 8, 2026
6 checks passed
446564 added a commit that referenced this pull request Apr 8, 2026
@446564
small clean up from PR #275
9d20be1 
just removes extraneous assignment to _lastNonRepeatSnapshot and moves
the Navigator pop to after all uses of the context in _RadioSettingsDialog
446564 added a commit that referenced this pull request Apr 8, 2026
@446564
Merge pull request #358 from zjs81/followup-#275
5ea044a 
small clean up from PR #275
@just-stuff-tm just-stuff-tm deleted the enhancement/preset-offgrid-repeat-toggle branch April 9, 2026 16:38
446564 added a commit that referenced this pull request Apr 9, 2026
@446564
small clean up from PR #275
69433b6 
just removes extraneous assignment to _lastNonRepeatSnapshot and moves
the Navigator pop to after all uses of the context in _RadioSettingsDialog
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
@446564 446564 446564 approved these changes

Assignees

@446564 446564

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 @446564