fix: align channel frequency display with firmware#2088
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesLoRa channel calculation
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e9e90ee to
57ca94e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Meshtastic/Helpers/LoRaChannelCalculator.swift (1)
221-323: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet the narrow-region spacing explicitly
Meshtastic/Helpers/LoRaChannelCalculator.swift:229-306—spacingstill defaults to0in everyRegionInfo(regionCode:)case, but the firmware’s narrow profile uses0.0104 MHzspacing. That makes the slot/frequency math for that region diverge from firmware; ifspacingis only a placeholder, drop it instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Meshtastic/Helpers/LoRaChannelCalculator.swift` around lines 221 - 323, Update the RegionInfo(regionCode:) narrow-region cases to use the firmware’s 0.0104 MHz spacing by passing it through the private RegionInfo initializer, especially the EU narrow profile; ensure slot/frequency calculations consume this value instead of the default zero. If spacing is not intended to be configurable, remove the unused spacing property and initializer parameter rather than retaining a zero placeholder.
🧹 Nitpick comments (1)
MeshtasticTests/LoraDeviceEnumTests.swift (1)
843-923: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test for
overrideFrequencycombined withfrequencyOffset. The newradioFrequencyMHzbranch that returnsoverrideFrequency + frequencyOffsetis untested; all five added tests useoverrideFrequency: 0.
MeshtasticTests/LoraDeviceEnumTests.swift#L843-L923: add a@TestconstructingLoRaChannelCalculatorwith a non-zerooverrideFrequencyand a non-zerofrequencyOffset, asserting the sum is returned regardless ofslot.Meshtastic/Helpers/LoRaChannelCalculator.swift#L90-L99: no code change needed here; this is the logic the new test should cover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MeshtasticTests/LoraDeviceEnumTests.swift` around lines 843 - 923, Add a `@Test` alongside LoRaChannelCalculatorTests that constructs LoRaChannelCalculator with non-zero overrideFrequency and frequencyOffset, then asserts radioFrequencyMHz returns their sum for an arbitrary slot. Update MeshtasticTests/LoraDeviceEnumTests.swift lines 843-923; Meshtastic/Helpers/LoRaChannelCalculator.swift lines 90-99 requires no direct change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Meshtastic/Helpers/LoRaChannelCalculator.swift`:
- Around line 221-323: Update the RegionInfo(regionCode:) narrow-region cases to
use the firmware’s 0.0104 MHz spacing by passing it through the private
RegionInfo initializer, especially the EU narrow profile; ensure slot/frequency
calculations consume this value instead of the default zero. If spacing is not
intended to be configurable, remove the unused spacing property and initializer
parameter rather than retaining a zero placeholder.
---
Nitpick comments:
In `@MeshtasticTests/LoraDeviceEnumTests.swift`:
- Around line 843-923: Add a `@Test` alongside LoRaChannelCalculatorTests that
constructs LoRaChannelCalculator with non-zero overrideFrequency and
frequencyOffset, then asserts radioFrequencyMHz returns their sum for an
arbitrary slot. Update MeshtasticTests/LoraDeviceEnumTests.swift lines 843-923;
Meshtastic/Helpers/LoRaChannelCalculator.swift lines 90-99 requires no direct
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20172ada-5e88-4b30-a991-478cb9310b62
📒 Files selected for processing (2)
Meshtastic/Helpers/LoRaChannelCalculator.swiftMeshtasticTests/LoraDeviceEnumTests.swift
garthvh
left a comment
garthvh
left a comment
There was a problem hiding this comment.
Verified this end-to-end before merging:
- Firmware fidelity: checked every formula against the cited
RadioInterface.cpp@5b1c1a9— slot width (spacing + 2·padding + bw),round()-based slot count, first-channel centering with padding, offset-applied-last (including on override frequency), the 15.6 kHz Tiny bandwidth, and all the ham default slots / band bounds in theRDEFtable. All match exactly. - Regression safety: ran the existing suites that depend on this calculator —
ModemPresetFrequencySlot,BeaconAddVsSwitch,DiscoveryScanEngine(31 tests) — plus the new suite. All pass.
Two notes for the record:
- The
floor → roundslot-count change also (correctly) fixes five non-ham regions where iOS disagreed with firmware:UA_433/PH_433/KZ_433(6.8 → 7 slots),ANZ_433(6.96 → 7), andLORA_24(102.8 → 103). Users in those regions will see their hash-derived slot move to match their radios — right thing, just noting it since the PR body only mentions US LongFast. - Heads-up on the test command in the description:
-only-testing:MeshtasticTests/LoraDeviceEnumTestsdoesn't run the new suite — the struct isLoRaChannelCalculatorTests, so that filter executes 0 of the new tests. Worth knowing for anyone re-verifying.
Two small follow-ups from the same review are filed separately and assigned your way (EU_866 band alignment, and a Mesh Beacons default-slot interaction).
Nice work tracing this through the firmware — the captured serial-output evidence made it easy to trust. How's class going, by the way?
2 participants
Summary
Fixes the iOS Channels frequency summary for firmware-managed amateur-band channel plans.
Fixes #2087.
channel_numat its default.frequency_offsetafter calculating an automatic or override frequency.Root cause
The iOS calculator used a simplified
freqStart + bandwidth / 2 + (slot - 1) * bandwidthmodel. For ITU Region 2 Amateur 2 m / TinyFast, that meant:Firmware defines TinyFast as 15.6 kHz RF bandwidth and turns it into 20 kHz slot spacing with 2.2 kHz padding on each side. ITU Region 2 Amateur 2 m also has an explicit default slot of 51. Firmware then applies
frequency_offsetto the final result.Relevant firmware evidence:
Captured evidence
The supplied captures show the original mismatch:
ITU Region 2 / Amateur 2m.ITU2_2M TinyFast, 145.010 MHz.channel_num: 51and 145.009995 MHz.Tests
xcodebuild test -quiet -project Meshtastic.xcodeproj -scheme Meshtastic -destination 'platform=iOS Simulator,name=iPhone 17 Pro' -only-testing:MeshtasticTests/LoraDeviceEnumTestsThe new focused cases verify:
+0.010 MHzfrequency offset is applied after the slot calculation.Summary by CodeRabbit
Bug Fixes
Tests