You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Mesh improvements, launcher sorting and WiFi buffers increased - #727
The mesh API and service now manage presets and radio and network settings per backend. MeshCore adds regional profiles, scoped flood transport, and safer channel-file handling. Meshtastic adds modem presets and preset-linked primary channel names. The settings interface and channel flows reflect these changes. The update also changes launcher sorting to ignore case and sets a Wi-Fi RX buffer count for devices with SPIRAM.
Priority: Normal
Merge Risk: Moderate · up to 9b666
Resolve radio validation and migration issues before merging: invalid settings can disable mesh communication, and upgrades or rollbacks can change channel identity. Region changes can also remain applied when preset selection fails.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title accurately identifies the three main change areas: mesh improvements, launcher sorting, and increased WiFi buffers. It is concise and specific enough for the changeset.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 27.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 72 functions across 22 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
Commit to this branch
Create a new PR
🧪 Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
Note
Follow-up findings are listed below so you can review them separately from inline comments.
Follow-ups (4)
🟡 Minor (4)
Minor · Use tt::std::vector for the retained radio setting values. · MeshSettings.cpp:79-80
Tactility/Source/app/meshsettings/MeshSettings.cpp:79-80 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low value
Use tt::std::vector for the retained radio setting values.
radioSettingValues is a std::vector in Context, which lives for the whole app session. As per coding guidelines, "Containers that live long use the tt::std replacements from TactilityKernel: tt::std::vector, tt::std::deque, tt::std::map and tt::std::string... They prefer external RAM." The file already uses std::vector for its other Context containers, so this change repeats an existing deviation. Switching the type needs an API check first.
Minor · Do not label every ERROR on a LoRa-backed radio as a region problem. · MeshSettings.cpp:145-147
Do not label every ERROR on a LoRa-backed radio as a region problem.
getStatus() now returns "Settings not allowed in " for every MESH_STATE_ERROR when lora is true. The mesh_service.cpp source sets MESH_STATE_ERROR for backend creation failure (line 686), for a region or preset rejection (line 695), for a failed profile apply (line 705), and when the radio reports an error in tick() (line 921). Only the line 695 case matches the region text. Show a region-specific state for that case, or keep the generic error text for the others.
Minor · Stop preset selection when saving the required region fails. · MeshSettings.cpp:586-589
Stop preset selection when saving the required region fails.
When settings::radio::saveRegion() returns false, onPresetSelected() still calls mesh_set_preset(). The preset needs the other region, so the radio rejects its frequency and the mesh stays in MESH_STATE_ERROR. Return before selecting the preset, and log the failure.
🐛 Suggested fix
if (mesh_get_preset_region(ctx->presetBackend, name, ®ion) == ERROR_NONE && region != RADIO_REGION_UNSET && region != settings::radio::loadRegion()) {
- settings::radio::saveRegion(region);+ if (!settings::radio::saveRegion(region)) {+ LOG_E(TAG, "Failed to save the region for the selected preset");+ return;+ }
}
Minor · Reject unsupported Default regions before saving the preset. · meshcore_backend.cpp:458-459
Reject unsupported Default regions before saving the preset.
getPresetRegion returns success for PRESET_DEFAULT without checking the default profile table. onPresetSelected then calls mesh_set_preset even when mesh_get_preset_region fails. Since mesh_set_preset persists the selection, update both the callback and its caller.
This failure predates the PR.
Suggested fix
- if (mesh_get_preset_region(ctx->presetBackend, name, ®ion) == ERROR_NONE && region != RADIO_REGION_UNSET && region != settings::radio::loadRegion()) {+ if (mesh_get_preset_region(ctx->presetBackend, name, ®ion) != ERROR_NONE) {+ return;+ }+ if (region != RADIO_REGION_UNSET && region != settings::radio::loadRegion()) {
settings::radio::saveRegion(region);
}
mesh_set_preset(ctx->presetBackend, name);
*region = current;
- if (preset == PRESET_DEFAULT || preset > PROFILES.size()) {+ if (preset == PRESET_DEFAULT) {+ const auto* match = std::find_if(DEFAULT_PROFILES.begin(), DEFAULT_PROFILES.end(), [current](const DefaultProfile& profile) {+ return profile.region == current;+ });+ return match == DEFAULT_PROFILES.end() ? ERROR_NOT_SUPPORTED : ERROR_NONE;+ }+ if (preset > PROFILES.size()) {
return ERROR_NONE;
}
Note: this behaviour predates this PR (already present at merge base f79c5c4c1438); the change keeps it reachable.
The subcase checks mesh_get_network_settings, which returns stored settings. It does not read host->network. Capture the MeshHost* passed to loopbackCreate, then call mesh_host_get_network and assert path_hash_size == 3. If configure stops updating host->network, the test must fail.
The new test checks only preset regions. A wrong frequency that remains within the region, spreading factor, or coding rate can pass. Existing service tests use a synthetic backend and do not cover MeshCore named presets. Add representative get_radio_profile assertions for the changed named presets.
191-202: Trivial · Data Integrity & Integration · Heavy lift
Add an emitted-frame regression test for scoped transmission.
The helper tests do not exercise send_text through applyNetwork. A regression could emit the wrong route, transport code, or path-hash size while these tests remain green. Capture mesh_host_transmit, parse the emitted frame, and assert those fields with configured network settings.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 7ce58012-5cbf-4df1-9983-67c8af05e485
Commits
Reviewing files that changed from the base of the PR and between f79c5c4 and 70f9c24.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Reject presets the current region cannot fit before committing them.
mesh_set_preset() commits ShortTurbo on an EU-868 radio. Its 500 kHz bandwidth exceeds the 250 kHz region span. Reconfiguration then fails, the mesh enters MESH_STATE_ERROR, and the prior preset is not restored. Check the preset against the current region before commitBackendSettings(), and reject when getRadioProfile() does not return ERROR_NONE.
Reference: existing check in mesh_set_radio_settings
mesh_set_radio_settings() already rejects an incompatible channel with ERROR_OUT_OF_RANGE. fitsRegion() is defined after mesh_set_preset(), so it needs a forward declaration or a move before use.
🟡 Minor (2)
Minor · Break ties in the launcher sort with the app ID. · AppsMode.cpp:114
strcasecmp(...) < 0 returns false in both directions for names that differ only in case, such as Chat and chat. The previous < gave them a fixed order. Now their relative order depends on the unstable sort and the input order, so tile positions can swap between repopulations. Use the app ID as a tie-breaker. IDs are unique.
Suggested fix
::std::ranges::sort(items, [](const TileGridItem& a, const TileGridItem& b) {
if (a.highlighted != b.highlighted) {
return a.highlighted;
}
- return strcasecmp(a.name.c_str(), b.name.c_str()) < 0;+ const int nameOrder = strcasecmp(a.name.c_str(), b.name.c_str());+ if (nameOrder != 0) {+ return nameOrder < 0;+ }+ return a.id < b.id;
});
Minor · Reject out-of-range path_hash_size in mesh_set_network_settings. · mesh_service.cpp:1663-1666
Reject out-of-range path_hash_size in mesh_set_network_settings.
The setter stores any path_hash_size value. getNetwork returns a nonzero stored value unchanged, so mesh_get_network_parameters can report a value such as 4. applyNetwork clamps flood packets to 1–3, so those packets use 3. The reported value and the transmitted value then differ. Reject nonzero values outside 1–3 in the setter. Keep 0, which selects the preset value.
Documenting the clamp in mesh.h does not fix this. The getter is documented to return the parameters a backend runs with, and it would still report the unclamped value.
Add a MeshCore test for the channel removal callback.
The loopback assertion at service_test.cpp:679 expects ERROR_NOT_SUPPORTED. The loopback backend has no remove_channel callback, so mesh_remove_channel() returns that error before any backend call. A regression that returns ERROR_NOT_SUPPORTED for channel 0 before dispatch would still pass this test.
No test calls the MeshCore removeChannel callback (meshcore_backend.cpp:691, registered at :722). Add a test that calls mesh_remove_channel() with the MeshCore backend active and asserts the expected success result. This exercises the service dispatch and the backend callback together.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 05f92c06-59ca-4dc2-92f9-4347d0889e15
Commits
Reviewing files that changed from the base of the PR and between 70f9c24 and 913b6da.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · Rolled-back firmware drops saved channels, and channel edits… · meshtastic_backend.cpp:246-251
Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp:246-251 🗄️ Data Integrity & Integration | 🟡 Minor | ⚖️ Poor tradeoff
Rolled-back firmware drops saved channels, and channel edits there overwrite channels.bin.
The new saveChannels writes the MTCH marker before the channel records. The merge-base loadChannels starts its record loop at offset 0 with a 66-byte record size, so it cannot skip the marker. Each record is read 4 bytes early. The key-length byte then lands on name byte 28, which is 0 for typical names, so every record is rejected. The loader falls back to the default LongFast channel with the default PSK, and the saved primary and custom channels are not restored. A name byte of 16 or 32 at that offset would load a wrong key instead. That case is rare.
Running base firmware does not rewrite the file at boot. saveChannels runs only from setChannel and removeChannel. If a user edits channels under base firmware, the base in-memory list (default channel only, plus the new entry) is written in the legacy layout. The original keys are then lost, even after returning to current firmware. Reflashing current firmware before any channel edit restores the saved channels.
A separate versioned file alone is not a safe correction. A later base-firmware edit would write channels.bin, while current firmware would keep reading the stale versioned file and discard that edit. A fix needs a rule for which file owns the data. No localized diff is established, so none is proposed.
Note
Follow-up findings are listed below so you can review them separately from inline comments.
Follow-up (1)
🟡 Minor (1)
Minor · Restore the previous region when the preset save fails. · MeshSettings.cpp:588-591
Restore the previous region when the preset save fails.
onPresetSelected() saves the new region before it calls mesh_set_preset(). Saving a region applies it to the radio at once. If mesh_set_preset() then fails, the radio keeps the new region, but the previous preset stays selected. The mesh service reconfigures the radio with that preset when the region changes, and this can put the mesh in the error state. The return value of mesh_set_preset() is not checked.
Capture the previous region before the save. If mesh_set_preset() fails, restore that region. Log the error when the restore also fails.
loadRegion() returns RADIO_REGION_UNSET when no region is stored. Restoring that value is accepted by saveRegion().
🐛 Suggested fix
RadioRegion region = RADIO_REGION_UNSET;
if (mesh_get_preset_region(ctx->presetBackend, name, ®ion) == ERROR_NONE && region != RADIO_REGION_UNSET && region != settings::radio::loadRegion()) {
- if (!settings::radio::saveRegion(region)) {- LOG_E(TAG, "Failed to save the region, so the preset isn't selected");- return;- }+ const RadioRegion previousRegion = settings::radio::loadRegion();+ if (!settings::radio::saveRegion(region)) {+ LOG_E(TAG, "Failed to save the region, so the preset isn't selected");+ return;+ }+ if (mesh_set_preset(ctx->presetBackend, name) != ERROR_NONE &&+ !settings::radio::saveRegion(previousRegion)) {+ LOG_E(TAG, "Failed to restore the previous region");+ }+ return;
}
mesh_set_preset(ctx->presetBackend, name);
Cover a failed first migration followed by a service lookup.
The new test calls mesh_settings_migrate_legacy() directly and never reaches getBackendSettings(). The retry gate runs only on a service lookup, so no test in the inspected file exercises it. Nothing checks that a failed migration leaves the flag unset or that a later lookup retries. A change that sets legacySettingsMigrated without checking the result, or that skips the retry, would not fail the tests that were inspected.
Add a service-level test. Make the first migration fail, perform a later lookup through a mesh_* call that reaches getBackendSettings(), and check the owner's settings. fakeDataApi has no failure option, so the test also needs a way to make the settings file open fail.
The new region loop skips every preset whose get_radio_profile call does not return ERROR_NONE. A refused LongFast lookup for US 915 therefore skips the fit check without failing. No other test asserts a successful LongFast profile lookup for RADIO_REGION_US_915. A change that refuses LongFast in that region can pass the suite.
Add a test that asserts ERROR_NONE for LongFast in US 915, following the ShortTurbo assertion pattern. If the preset lookup is changed, confirm that the preset is present before using its index.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 79ace386-aaf8-4393-926f-7153f3623ba3
Commits
Reviewing files that changed from the base of the PR and between 913b6da and 5fe5fd4.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
Files skipped from review as they are similar to previous changes: 2
Modules/mesh-module/private/mesh/mesh_settings.h
Modules/mesh-module/source/mesh_settings.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
set -e
echo"=== BASE addChannel 150-170 ==="
sed -n '150,170p' /tmp/base_backend.cpp
echo"=== BASE setChannel 486-512 ==="
sed -n '486,512p' /tmp/base_backend.cpp
echo"=== HEAD addChannel 182-196 ==="
sed -n '182,196p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp
echo"=== HEAD setChannel 569-600 ==="
sed -n '569,600p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp
echo"=== mesh_command inspect_load_channel_names 271-290 ==="
sed -n '271,290p' Modules/mesh-module/source/mesh_command.cpp
echo"=== UI/other callers that set a channel name ==="
rg -n -F -e '.name = ' -e 'set_channel' -e 'MeshChannel' Modules/mesh-module/source Modules/mesh-meshtastic-module/source ||test"$?" -eq 1
Repository: TactilityProject/Tactility
Length of output: 8649
Keep an explicitly named LongFast primary channel from being renamed on upgrade.
In a legacy file without channels.preset, a primary channel named LongFast with the default key is marked followsPreset. Choosing another preset then renames it, which changes its name, hash, and frequency slot. At the merge base, this record kept LongFast. Decide how legacy LongFast records map to the preset-following default so that explicit names stay fixed.
The reason will be displayed to describe this comment to others. Learn more.
Major · Data Integrity & Integration · Heavy lift
Supported by static analysis
Script executed:
sed -n '244,266p;184,194p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp; git show main:Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp | sed -n '165,260p'
set -u
echo"== meshtastic_channel_hash =="; sed -n '145,175p' Modules/mesh-meshtastic-module/source/meshtastic_protocol.cpp
echo"== head receive/transmit match =="; sed -n '484,500p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp
echo"== head tx channel selection =="; sed -n '526,545p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp
echo"== head loader fallback callers of saveChannels/followsPreset at 405-415 =="; sed -n '400,420p' Modules/mesh-meshtastic-module/source/meshtastic_backend.cpp
Repository: TactilityProject/Tactility
Length of output: 3880
Keep the default primary channel name in channels.bin for rollback compatibility.
With this change, a default primary channel is saved with an empty name. A rolled-back build that predates channels.preset loads that name as-is. Its channel hash then becomes the XOR of the key bytes only, which does not match the LongFast hash that peers use. The rolled-back device therefore drops LongFast traffic and sends packets that peers reject. The format comment says older versions can read the file back, but they do not get the right channel identity.
Fix the save path or the record format so an older loader gets the LongFast name. A localized change is not established. Writing DEFAULT_CHANNEL_NAME would conflict with the channels.preset marker logic in loadChannels, so the fix needs a design choice.
If the channels.preset write fails after channels.bin is saved, the next load treats a custom LongFast primary channel as the legacy default. After a restart, selecting another preset renames that channel and changes its hash and frequency slot. Report the failure to setChannel() and removeChannel().
The reason will be displayed to describe this comment to others. Learn more.
Minor · Functional Correctness · Quick win
Supported by static analysis
Script executed:
sed -n '189,220p;690,735p' Modules/mesh-module/source/mesh_service.cpp; sed -n '135,210p' Modules/mesh-module/source/mesh_settings.cpp
Repository: TactilityProject/Tactility
Length of output: 6584
Script executed:
rg -n -F -e 'getBackendEntry' -e 'getBackendSettings' -e 'mesh_settings_migrate_legacy' -e 'legacySettingsMigrated' -e 'mesh_settings_load_backend' -- Modules/ ||test"$?" -eq 1
echo'--- base getBackendSettings region ---'
git show f79c5c4c1438ffbc40ca66f6d6b1fb8a62b69ca8:Modules/mesh-module/source/mesh_service.cpp | rg -n -F -e 'getBackendSettings' -e 'legacySettingsMigrated' -e 'mesh_settings_migrate_legacy' -e 'mesh_settings_load_backend'||test"$?" -eq 1
echo'--- head mesh_service.cpp lines 180-230 ---'
sed -n '180,230p' Modules/mesh-module/source/mesh_service.cpp
Repository: TactilityProject/Tactility
Length of output: 5917
Retry legacy migration when a backend entry is already cached.
If the settings file cannot be opened on the first lookup, getBackendEntry() skips the legacy migration for that backend. The cached-entry branch only retries mesh_settings_load_backend(), so the migration never runs for that entry. Later lookups read the backend-qualified keys, find none, and configure uses default preset and radio settings. Run the migration on lookups while legacySettingsMigrated is false, and reset the cached loaded flags after it succeeds.
sed -n '120,145p' Drivers/sx126x-module/private/drivers/sx1262_radio.cpp;echo'--- cc1101 80-100'; sed -n '80,100p' Drivers/cc1101-module/private/drivers/cc1101_radio.cpp;echo'--- radio.cpp 475-520'; sed -n '475,520p' TactilityKernel/source/drivers/radio.cpp
Repository: TactilityProject/Tactility
Length of output: 3411
Reject non-positive bandwidth in the regional check.
A negative bandwidth passes the findBand check, because reversed bounds still contain an in-band frequency. The setter then persists it. The SX1262 and CC1101 drivers reject it with ERROR_OUT_OF_RANGE during reconfiguration, so applyProfile fails and the mesh enters MESH_STATE_ERROR.
🐛 Suggested fix
- if (band == nullptr || settings.radio_settings.power > band->power_max) {+ if (profile.bandwidth <= 0 || band == nullptr || settings.radio_settings.power > band->power_max) {
Committable suggestion
IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const RadioRegionBand* band = findBand(region, profile.frequency, profile.bandwidth);
if (profile.bandwidth <= 0 || band == nullptr || settings.radio_settings.power > band->power_max) {
returnERROR_OUT_OF_RANGE;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
KenVanHoeylandt
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Documentation