fix(pl18): Sound Playing will Sometimes Sluggish - v2.11#7472
fix(pl18): Sound Playing will Sometimes Sluggish - v2.11#7472richardclli wants to merge 2 commits into
Conversation
Restore DAC DMA underrun interrupt handler (DMAUDRIE1) removed in 6d6c449. When audio buffers starve during flash erase, the underrun ISR stops DMA within ~31us instead of letting circular DMA replay stale data indefinitely. Add RTOS yield to flash_wait_for_not_busy() so the audio task gets CPU time during long flash erases (45-400ms), reducing starvation at the source.
📝 WalkthroughWalkthroughEdgeTX 2.11 "Jolly Mon" release preparation: version bumped to revision 6, dev containers and CI images pinned to 2.11. Four new radio boards added (FlySky PA01, PL18U, ST16, HelloRadioSky V14) across companion and firmware. Companion storage layer mutated to preserve hardware calibration on write. New automated release-drafter workflow and companion CI orchestrator added. Multiple model-editing behavior fixes. Release infrastructure and CI overhaul
New hardware support: FlySky PA01/PL18U/ST16 and HelloRadioSky V14
Companion storage layer refactor
Companion app model-editing and UI improvements
Sequence Diagram(s)sequenceDiagram
participant Companion
participant helpers_cpp as helpers.cpp startSimulation
participant QTemporaryDir
participant QProcess
participant SimulatorBinary
Companion->>helpers_cpp: startSimulation()
helpers_cpp->>QTemporaryDir: create temp dir
helpers_cpp->>QTemporaryDir: write RadioData (settings + models)
helpers_cpp->>helpers_cpp: build CLI args (--profile, --flags, tempdir)
helpers_cpp->>QProcess: execute(simulatorBinary, args)
QProcess->>SimulatorBinary: launch
SimulatorBinary-->>QProcess: exit code
QProcess-->>helpers_cpp: return code
helpers_cpp->>Companion: show error dialog on failure / cleanup simuData
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
richardclli
changed the title
spi_flash.cpp is compiled for both bootloader and main firmware. The bootloader does not link FreeRTOS, so RTOS_WAIT_MS(1) causes undefined reference to vTaskDelay. Guard with BOOT and use bare-metal delay_ms(1) for the bootloader instead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/src/modeledit/customfunctions.cpp (1)
434-440:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp Inc/Dec GV values before displaying them.
The constant branch clamps
cfn.param, but the Inc/Dec branch only clamps theQDoubleSpinBoxdisplay range. If a loaded model has an out-of-range increment after a GV range change, the UI shows a clipped value while the underlyingcfn.paramremains invalid.Proposed fix
if (cfn.adjustMode == FUNC_ADJUST_GVAR_INCDEC) { - double rng = abs(model->gvarData[gvidx].getMax() - model->gvarData[gvidx].getMin()); - rng *= model->gvarData[gvidx].multiplierGet(); + const int rawRange = abs(model->gvarData[gvidx].getMax() - model->gvarData[gvidx].getMin()); + double rng = rawRange * model->gvarData[gvidx].multiplierGet(); fswtchParam[i]->setMinimum(-rng); fswtchParam[i]->setMaximum(rng); + if (cfn.param < -rawRange) { + cfn.param = -rawRange; + emit modified(); + } else if (cfn.param > rawRange) { + cfn.param = rawRange; + emit modified(); + } fswtchParam[i]->setValue(cfn.param * model->gvarData[gvidx].multiplierGet()); }🤖 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 `@companion/src/modeledit/customfunctions.cpp` around lines 434 - 440, In the FUNC_ADJUST_GVAR_INCDEC branch, clamp cfn.param to the valid range (between -rng and rng) before multiplying it by the multiplier and setting it in fswtchParam[i]. Currently, the code only clamps the display range via setMinimum and setMaximum, but does not clamp the underlying cfn.param value itself. Use std::clamp or similar logic to ensure cfn.param is clamped to the range before it is multiplied by the multiplier in the setValue call.
🟠 Major comments (22)
.github/workflows/nightly.yml-20-22 (1)
20-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNew targets can produce
.uf2, but nightly packaging still omits them.With added targets (Line 20/Line 21/Line 40), nightly artifacts may miss firmware files because upload still only includes
*.bin, whiletools/build-gh.shemits.uf2for some boards.Suggested fix
- name: Package firmware ${{ matrix.target }} uses: actions/upload-artifact@v4 with: name: edgetx-firmware-nightly-${{ matrix.target }} path: | fw.json LICENSE *.bin + *.uf2Also applies to: 40-40
🤖 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 @.github/workflows/nightly.yml around lines 20 - 22, The nightly workflow is configured to upload only `.bin` artifacts, but the newly added targets (pl18, pl18ev, pl18u, pa01, t12, t12max) produce `.uf2` firmware files as well. Update the artifact upload configuration in the nightly workflow to include both `*.bin` and `*.uf2` file patterns so that firmware files generated by all targets are properly packaged in nightly releases..github/workflows/win_cpn-64.yml-56-56 (1)
56-56:⚠️ Potential issue | 🟠 MajorPin all GitHub Actions to immutable commit SHAs.
Lines 27, 56, 61, and 83 reference mutable action tags. Pin all actions to full commit SHAs (e.g.,
actions/checkout@abc123def...) to enforce deterministic and trusted workflow execution, preventing potential supply chain attacks.🤖 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 @.github/workflows/win_cpn-64.yml at line 56, Replace all mutable GitHub Actions version tags with immutable full commit SHAs in the workflow file. On lines 27, 56, 61, and 83, change all action references (such as actions/checkout@v4) from using version tags to using the complete commit SHA format (e.g., actions/checkout@<full-commit-sha>). This ensures deterministic and secure workflow execution by preventing any changes to action behavior through tag mutations. Verify each action reference is updated to use a pinned commit hash rather than a mutable tag like `@v4`.Source: Linters/SAST tools
.github/workflows/companion.yml-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorUse glob-compatible branch patterns instead of regex syntax.
Lines 7 and 21 contain
[0-9]+.[0-9]+, but GitHub Actions branch filters use glob patterns, not regex. In glob syntax,+and.are literal characters, so this pattern won't match version branches like2.11. Change to[0-9]*.[0-9]*to match zero or more digits on each side of the literal dot.Suggested patch
branches: - 'main' - - '[0-9]+.[0-9]+' + - '[0-9]*.[0-9]*' @@ branches: - 'main' - - '[0-9]+.[0-9]+' + - '[0-9]*.[0-9]*'Also applies to: 21-21
🤖 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 @.github/workflows/companion.yml at line 7, The branch filter patterns at lines 7 and 21 use regex syntax instead of GitHub Actions glob syntax. In glob patterns, the `+` quantifier and `.` metacharacter are treated as literal characters, so the pattern `[0-9]+.[0-9]+` will not match version branches like 2.11. Replace both occurrences of the pattern `[0-9]+.[0-9]+` with `[0-9]*.[0-9]*` to correctly match zero or more digits on each side of the literal dot using glob-compatible syntax..github/workflows/release-drafter.yml-22-22 (1)
22-22:⚠️ Potential issue | 🟠 MajorPin action references to immutable commit SHAs.
Lines 22 and 162 use mutable version tags (
@v4and@v2). Pin to full commit SHAs instead for supply-chain safety and policy compliance.🤖 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 @.github/workflows/release-drafter.yml at line 22, The action references in the release-drafter workflow file are using mutable version tags (`@v4` on line 22 and `@v2` on line 162) which pose supply-chain security risks. Replace both mutable version tag references with their full immutable commit SHAs. For the actions/checkout@v4 reference on line 22, resolve the current v4 tag to its corresponding commit SHA. Similarly, update line 162 to use the full commit SHA instead of `@v2`. This ensures that the workflow always runs the exact same version of each action regardless of future tag movements.Source: Linters/SAST tools
.github/workflows/macosx_cpn.yml-26-29 (1)
26-29:⚠️ Potential issue | 🟠 MajorSet
persist-credentials: falseon the checkout action to prevent credential leakage.The workflow archives artifacts (line 74) after checking out the repository. With the default
persist-credentials: true, the authentication token is written to.git/configand could be included in archived artifacts, creating a security exposure.Suggested patch
- name: Check out the repo uses: actions/checkout@v4 with: + persist-credentials: false submodules: recursive🤖 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 @.github/workflows/macosx_cpn.yml around lines 26 - 29, The checkout action in the "Check out the repo" step uses the default persist-credentials setting, which writes the authentication token to .git/config and could expose it in archived artifacts. Add the `persist-credentials: false` parameter to the actions/checkout@v4 action to prevent this security exposure. This will ensure credentials are not retained in the repository configuration that might be included in artifact archives.Source: Linters/SAST tools
.github/workflows/release-drafter.yml-64-71 (1)
64-71:⚠️ Potential issue | 🟠 MajorPin
git-cliffversion and verify checksums.This step pulls
latestat runtime and executes it without integrity verification, making releases non-reproducible and increasing supply-chain risk. Git-cliff provides SHA512 checksums on its release pages that should be used to verify the downloaded tarball.Suggested patch (pattern)
- - name: Install git-cliff + - name: Install git-cliff + env: + GIT_CLIFF_VERSION: "2.13.1" run: | - VERSION=$(wget -qO- https://api.github.com/repos/orhun/git-cliff/releases/latest | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/' | sed 's/^v//') - FILENAME="git-cliff-${VERSION}-x86_64-unknown-linux-gnu.tar.gz" - wget -O git-cliff.tar.gz "https://github.com/orhun/git-cliff/releases/download/v${VERSION}/${FILENAME}" + FILENAME="git-cliff-${GIT_CLIFF_VERSION}-x86_64-unknown-linux-gnu.tar.gz" + wget -O git-cliff.tar.gz "https://github.com/orhun/git-cliff/releases/download/v${GIT_CLIFF_VERSION}/${FILENAME}" + wget -O git-cliff.tar.gz.sha512 "https://github.com/orhun/git-cliff/releases/download/v${GIT_CLIFF_VERSION}/git-cliff-${GIT_CLIFF_VERSION}-x86_64-unknown-linux-gnu.tar.gz.sha512" + sha512sum -c git-cliff.tar.gz.sha512 tar -xzvf git-cliff.tar.gz - mv git-cliff-${VERSION}/git-cliff /usr/local/bin/ + mv git-cliff-${GIT_CLIFF_VERSION}/git-cliff /usr/local/bin/🤖 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 @.github/workflows/release-drafter.yml around lines 64 - 71, The "Install git-cliff" step dynamically fetches the latest version without pinning a specific version and does not verify the integrity of the downloaded tarball, creating reproducibility and supply-chain security issues. Replace the dynamic latest version fetch with a hardcoded pinned version number, then add a checksum verification step that downloads the SHA512 checksums from the git-cliff release page and validates the downloaded tarball before extracting and installing it. This ensures deterministic builds and prevents execution of potentially tampered binaries..github/workflows/companion.yml-1-4 (1)
1-4:⚠️ Potential issue | 🟠 MajorAdd explicit least-privilege workflow permissions.
This orchestrator workflow lacks an explicit
permissions:block and inherits broader default permissions than necessary. Following the principle of least privilege, add a permissions declaration at the workflow level.Suggested patch
name: Companion on: push: +permissions: + contents: read + jobs:Also applies to: 37-51
🤖 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 @.github/workflows/companion.yml around lines 1 - 4, The Companion workflow lacks an explicit permissions block at the workflow level, which causes it to inherit broader default permissions than necessary. Add a permissions declaration immediately after the on trigger section in the workflow file to follow the principle of least privilege. Define only the specific permissions required for this orchestrator workflow to function, explicitly setting all others to the minimum required access level or denying them as appropriate.Source: Linters/SAST tools
.github/workflows/win_cpn-64.yml-55-58 (1)
55-58:⚠️ Potential issue | 🟠 MajorSet
persist-credentials: falseon checkout.This artifact-producing workflow doesn't require authenticated Git operations after checkout. Disabling credential persistence follows the principle of least privilege and prevents the token from being available in subsequent steps.
Suggested patch
- name: Check out the repo uses: actions/checkout@v4 with: + persist-credentials: false submodules: recursive🤖 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 @.github/workflows/win_cpn-64.yml around lines 55 - 58, The actions/checkout@v4 action in the workflow is missing the persist-credentials configuration option. Add persist-credentials: false to the with section of the checkout action to follow the principle of least privilege and prevent Git credentials from persisting in subsequent workflow steps, since this artifact-producing workflow doesn't require authenticated Git operations after the initial checkout.Source: Linters/SAST tools
companion/src/modeledit/inputs.cpp-671-673 (1)
671-673:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix leaked temporary buffer and enforce null-termination when swapping input names.
The current swap leaks
QByteArrayand can leaveinputNames[*]unterminated for max-length names.Proposed fix
- QByteArray *tname = new QByteArray(model->inputNames[idx2]); - strncpy(model->inputNames[idx2], model->inputNames[idx1], sizeof(model->inputNames[idx2]) - 1); - strncpy(model->inputNames[idx1], tname->data(), sizeof(model->inputNames[idx1]) - 1); + QByteArray tname(model->inputNames[idx2], sizeof(model->inputNames[idx2])); + strncpy(model->inputNames[idx2], model->inputNames[idx1], sizeof(model->inputNames[idx2]) - 1); + model->inputNames[idx2][sizeof(model->inputNames[idx2]) - 1] = '\0'; + strncpy(model->inputNames[idx1], tname.constData(), sizeof(model->inputNames[idx1]) - 1); + model->inputNames[idx1][sizeof(model->inputNames[idx1]) - 1] = '\0';🤖 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 `@companion/src/modeledit/inputs.cpp` around lines 671 - 673, The swap operation for input names has two issues: the temporary QByteArray object allocated with new is never deleted, causing a memory leak, and strncpy may not null-terminate the strings if the source name uses the full buffer size. To fix this, replace the dynamically allocated QByteArray pointer with a stack-allocated QByteArray temporary (remove the new keyword), and after each strncpy call in the swap sequence, explicitly set the final byte to null character to ensure proper null-termination of model->inputNames[idx2] and model->inputNames[idx1].companion/src/CMakeLists.txt-365-366 (1)
365-366:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the ST16 flavour variable typo.
Line 366 sets
FLAVOR, but the surrounding logic usesFLAVOUR. This prevents ST16 from flowing through the same packaging/variant path as other targets.Suggested fix
elseif(PCB STREQUAL ST16) - set(FLAVOR st16) + set(FLAVOUR st16)🤖 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 `@companion/src/CMakeLists.txt` around lines 365 - 366, In the CMakeLists.txt file, the ST16 configuration block contains a spelling inconsistency where the variable is set as `FLAVOR` instead of `FLAVOUR`. This mismatch prevents ST16 from following the same packaging and variant configuration path as other targets. Change the variable name in the `set(FLAVOR st16)` statement to `set(FLAVOUR st16)` to align with the spelling convention used throughout the rest of the CMake configuration logic for other PCB types.companion/src/firmwares/opentx/opentxinterface.cpp-354-356 (1)
354-356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd PA01/ST16 to
HasIntModuleFlySkyboard checks.
HasIntModuleFlySkycurrently excludesBOARD_FLYSKY_PA01andBOARD_FLYSKY_ST16, butBoards::getDefaultInternalModules()maps both toMODULE_TYPE_FLYSKY_AFHDS3. This mismatch can disable FlySky-module behavior on those targets.Suggested fix
case HasIntModuleFlySky: return id.contains("afhds2a") || id.contains("afhds3") || - IS_FLYSKY_NV14(board) || IS_FLYSKY_EL18(board) || IS_FAMILY_PL18(board); + IS_FLYSKY_NV14(board) || IS_FLYSKY_EL18(board) || IS_FAMILY_PL18(board) || + IS_FLYSKY_PA01(board) || IS_FLYSKY_ST16(board);🤖 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 `@companion/src/firmwares/opentx/opentxinterface.cpp` around lines 354 - 356, The HasIntModuleFlySky function is missing board checks for BOARD_FLYSKY_PA01 and BOARD_FLYSKY_ST16, which should be included in the return statement alongside the existing board checks (IS_FLYSKY_NV14, IS_FLYSKY_EL18, IS_FAMILY_PL18). Add conditional checks for these two missing boards in the same manner as the existing board macros to ensure they are properly recognized as having internal FlySky modules, matching the mappings in Boards::getDefaultInternalModules().companion/src/firmwares/moduledata.cpp-88-90 (1)
88-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclude the full PL18 family in the XJT blocks.
Line 88 and Line 90 exclude only
IS_FLYSKY_PL18(board), but Line 101 treatsIS_FAMILY_PL18(board)as the shared family. This can leave PL18U/PL18EV incorrectly eligible for XJT options.Suggested fix
- return !IS_ACCESS_RADIO(board, id) && !IS_FAMILY_T16(board) && !IS_FAMILY_T12(board) && !IS_FLYSKY_NV14(board) && !IS_FLYSKY_EL18(board) && !IS_FLYSKY_PL18(board) && !IS_FLYSKY_ST16(board); + return !IS_ACCESS_RADIO(board, id) && !IS_FAMILY_T16(board) && !IS_FAMILY_T12(board) && !IS_FLYSKY_NV14(board) && !IS_FLYSKY_EL18(board) && !IS_FAMILY_PL18(board) && !IS_FLYSKY_ST16(board); ... - return !(IS_ACCESS_RADIO(board, id) || id.contains("eu")) && !IS_FAMILY_T16(board) && !IS_FAMILY_T12(board) && !IS_FLYSKY_NV14(board) && !IS_FLYSKY_EL18(board) && !IS_FLYSKY_PL18(board) && !IS_FLYSKY_ST16(board); + return !(IS_ACCESS_RADIO(board, id) || id.contains("eu")) && !IS_FAMILY_T16(board) && !IS_FAMILY_T12(board) && !IS_FLYSKY_NV14(board) && !IS_FLYSKY_EL18(board) && !IS_FAMILY_PL18(board) && !IS_FLYSKY_ST16(board);Also applies to: 101-101
🤖 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 `@companion/src/firmwares/moduledata.cpp` around lines 88 - 90, The XJT pulse blocks (PULSES_PXX_XJT and PULSES_PXX_XJT_D8) currently exclude only IS_FLYSKY_PL18(board), but this leaves PL18 variants like PL18U and PL18EV incorrectly eligible for XJT options since line 101 uses IS_FAMILY_PL18(board) as the family-level check. Replace all occurrences of IS_FLYSKY_PL18(board) with IS_FAMILY_PL18(board) in both the PULSES_PXX_XJT case and PULSES_PXX_XJT_D8 case to properly exclude the entire PL18 family from XJT support.cmake/FetchMiniz.cmake-19-20 (1)
19-20:⚠️ Potential issue | 🟠 MajorGuard the warning-suppression flag by compiler.
-Wno-unused-functionis not a portable option for all compilers; applying it unconditionally tominizwill cause build failures on Windows/MSVC builds, particularly if WARNINGS_AS_ERRORS is enabled. Use a CMake generator expression to apply the flag only to GCC/Clang.Suggested fix
-# suppress compiler warnings for this add-in only -target_compile_options(miniz PRIVATE -Wno-unused-function) +# suppress compiler warnings for this add-in only +target_compile_options(miniz PRIVATE + $<$<COMPILE_LANG_AND_ID:C,GNU,Clang,AppleClang>:-Wno-unused-function> +)🤖 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 `@cmake/FetchMiniz.cmake` around lines 19 - 20, The target_compile_options call for the miniz target applies the -Wno-unused-function flag unconditionally, which causes build failures on MSVC builds when WARNINGS_AS_ERRORS is enabled since this flag is not supported by MSVC. Wrap the compiler flag using a CMake generator expression with $<CXX_COMPILER_ID:...> to conditionally apply the -Wno-unused-function flag only when the compiler is GCC or Clang, excluding MSVC from receiving this unsupported option.companion/src/firmwares/rawsource.cpp-107-107 (1)
107-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize and bounds-check the GVAR index before indexing.
Line 107 uses
index - 1directly. Negative source indices are supported elsewhere viaabs(index), so a negative GVAR source can index beforegvarData.Proposed fix
- GVarData gv = model->gvarData[index - 1]; + const int gvIndex = abs(index) - 1; + if (!model || gvIndex < 0 || gvIndex >= CPN_MAX_GVARS) + break; + + GVarData gv = model->gvarData[gvIndex];🤖 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 `@companion/src/firmwares/rawsource.cpp` at line 107, The code at line 107 in the gvarData access uses index - 1 directly without normalizing negative indices, which can cause out-of-bounds access when index is negative or zero. Normalize the index using abs(index) before subtracting 1 to get the array position, similar to how negative source indices are handled elsewhere in the codebase. Additionally, add bounds-checking to ensure the resulting index is within the valid range of the gvarData array before accessing it with the expression model->gvarData[index - 1].companion/src/firmwares/rawsource.cpp-122-125 (1)
122-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve zero in absolute GVAR ranges that cross zero.
For a GVAR range like
-100..100, Lines 123-124 produce100..100, so the UI can no longer select valid absolute thresholds such as0..99.Proposed fix
if (flags & RANGE_ABS_FUNCTION) { - result.min = (flags & RANGE_DELTA_FUNCTION) ? 0 : abs(result.min); - result.max = abs(result.max); + const double min = result.min; + const double max = result.max; + result.min = (min <= 0 && max >= 0) ? 0 : std::min(abs(min), abs(max)); + result.max = std::max(abs(min), abs(max)); }🤖 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 `@companion/src/firmwares/rawsource.cpp` around lines 122 - 125, The absolute GVAR range handling in the block where flags & RANGE_ABS_FUNCTION is checked does not properly preserve zero for ranges that cross zero. When a range spans from negative to positive values (like -100..100), the current logic taking abs(result.min) loses the ability to select zero as a valid threshold. Modify the logic in this block to check if the original range crosses zero by examining whether result.min is negative and result.max is positive, then set result.min to 0 for such ranges instead of taking its absolute value, ensuring that valid thresholds like 0..99 remain selectable in the UI.companion/src/firmwares/customisation_data.h-36-40 (1)
36-40:⚠️ Potential issue | 🟠 MajorUpdate YAML serializer for NV14 to match new buffer size.
YAML serializers mostly align with the new sizes, but
yaml_datastructs_nv14.cppat line 790 defineswidgetNamewith size 12 instead of 20. Update it to matchWIDGET_NAME_LEN=20to prevent widget names from being truncated when read by the firmware.Mismatch found
yaml_datastructs_nv14.cpp:790: YAML_STRING("widgetName", 12), // Should be: YAML_STRING("widgetName", 20),Other YAML files correctly specify the sizes for
stringValue=12,widgetName=20, andoptionsarray with count10.🤖 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 `@companion/src/firmwares/customisation_data.h` around lines 36 - 40, The YAML serializer in yaml_datastructs_nv14.cpp has a size mismatch for the widgetName field. In yaml_datastructs_nv14.cpp, locate the YAML_STRING macro definition for "widgetName" (currently specifying size 12) and update the size parameter from 12 to 20 to match the WIDGET_NAME_LEN constant defined in customisation_data.h. This will ensure widget names are not truncated when read by the firmware.companion/src/storage/storage.h-54-54 (1)
54-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
load(GeneralSettings&)is effectively non-functional for current concrete formatsLine 54 now defaults to
return false;, butStorage::load(GeneralSettings&)dispatches to this virtual path. In this cohort,YamlFormat,LabelsStorageFormat(and thusEtxFormat/SdcardFormat) don’t overrideload(GeneralSettings&), so the call path always fails.🔧 Proposed fix (safe fallback in base class)
- virtual bool load(GeneralSettings & generalSettings) { return false; } + virtual bool load(GeneralSettings & generalSettings) + { + RadioData radioData; + if (!load(radioData)) + return false; + generalSettings = radioData.generalSettings; + return true; + }🤖 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 `@companion/src/storage/storage.h` at line 54, The virtual method `load(GeneralSettings&)` in the base Storage class currently returns false by default, but concrete implementations like YamlFormat, LabelsStorageFormat, EtxFormat, and SdcardFormat do not override this method, causing all calls to fail. Either implement the `load(GeneralSettings&)` method in each concrete format class to handle loading general settings appropriately for that format, or provide a safe fallback implementation in the base Storage class that properly handles the GeneralSettings parameter instead of unconditionally returning false.companion/src/storage/labeled.cpp-219-219 (1)
219-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain the cleanup regex to intended filenames.
Line 219’s
[0-9s]+also matches names likemodel1s.ymlandmodelss.yml, so cleanup can delete files outside the intendedmodelNN.yml/ legacymodels.ymlset.Proposed fix
- const std::regex yml_regex("MODELS/(model([0-9s]+)\\.yml)", std::regex_constants::icase); + const std::regex yml_regex("MODELS/((model[0-9]+|models)\\.yml)", std::regex_constants::icase);🤖 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 `@companion/src/storage/labeled.cpp` at line 219, The regex pattern in the yml_regex declaration at line 219 uses the character class [0-9s]+, which incorrectly matches both digits and the letter 's' in any combination, allowing filenames like model1s.yml and modelss.yml to match unintentionally. Fix the regex to constrain it to only match the intended filenames: numbered models like modelNN.yml (where NN contains only digits) and the legacy models.yml form. Replace the overly permissive [0-9s]+ pattern with a more specific pattern that matches either a sequence of digits only, or uses alternation to separately handle the models.yml case, ensuring the cleanup operation only targets the correct set of files.companion/src/storage/storage.cpp-142-149 (1)
142-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a
GeneralSettingsload override for labeled formats.Line 143 dispatches to
StorageFormat::load(GeneralSettings&), but the provided format declarations only show the base overload returningfalse. Add an override that delegates toloadRadioSettings(...), otherwiseStorage::load(generalSettings)cannot succeed for ETX/SD-card storage.Proposed fix
--- a/companion/src/storage/labeled.h +++ b/companion/src/storage/labeled.h @@ - virtual bool load(RadioData & radioData); + virtual bool load(RadioData & radioData); + virtual bool load(GeneralSettings & generalSettings) override; virtual bool write(RadioData & radioData);--- a/companion/src/storage/labeled.cpp +++ b/companion/src/storage/labeled.cpp @@ bool LabelsStorageFormat::write(RadioData & radioData) { @@ } +bool LabelsStorageFormat::load(GeneralSettings & generalSettings) +{ + return loadRadioSettings(generalSettings); +} + bool LabelsStorageFormat::loadRadioSettings(GeneralSettings & generalSettings) {🤖 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 `@companion/src/storage/storage.cpp` around lines 142 - 149, The StorageFormat class lacks an override for the load method that accepts GeneralSettings as a parameter, causing load operations to fail for labeled formats like ETX/SD-card storage. Add a load(GeneralSettings&) method override to the StorageFormat class (or the relevant derived format classes) that properly delegates to the loadRadioSettings(...) method instead of relying on the base implementation that returns false. This will allow the load call at line 143 to succeed when format->load(generalSettings) is invoked.companion/src/storage/labeled.cpp-187-193 (1)
187-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStage calibration preservation outside the caller object.
Line 187 mutates
radioDatabefore laterwriteFile,deleteFile, and model writes can fail. If the write returnsfalse, the open document can still retain target-radio calibration; serialize a local copy or restore on failure.Proposed fix
bool LabelsStorageFormat::write(RadioData & radioData) { + RadioData dataToWrite = radioData; + // TODO @@ if (loadRadioSettings(gsCur)) { - GeneralSettings & gsNew = radioData.generalSettings; + GeneralSettings & gsNew = dataToWrite.generalSettings; gsNew.txCurrentCalibration = gsCur.txCurrentCalibration; gsNew.txVoltageCalibration = gsCur.txVoltageCalibration; @@ QByteArray radioSettingsBuffer; - if (!writeRadioSettingsToYaml(radioData.generalSettings, radioSettingsBuffer)) + if (!writeRadioSettingsToYaml(dataToWrite.generalSettings, radioSettingsBuffer)) return false; @@ - for (const auto& model : radioData.models) { + for (const auto& model : dataToWrite.models) { @@ if (hasLabels) { QByteArray labelsListBuffer; - if (!writeLabelsListToYaml(radioData, labelsListBuffer) || !writeFile(labelsListBuffer, "MODELS/labels.yml")) + if (!writeLabelsListToYaml(dataToWrite, labelsListBuffer) || !writeFile(labelsListBuffer, "MODELS/labels.yml")) return false; }🤖 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 `@companion/src/storage/labeled.cpp` around lines 187 - 193, The code mutates radioData.generalSettings through the gsNew reference by copying calibration values (txCurrentCalibration, txVoltageCalibration, and inputConfig calib values) before the subsequent write operations that could fail. If those writes return false, the document will still contain the unwanted calibration modifications. Either create a local copy of the calibration values before mutation and restore them if any subsequent write operation fails, or defer the mutation of radioData until after all write and model operations have succeeded and confirmed their success.companion/src/simulator.cpp-235-244 (1)
235-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit
--flags 0instead of treating it as standalone.The non-Apple launcher passes
--flags 0for normal Companion launches, but line 379 maps zero toSIMULATOR_FLAGS_STANDALONE. Default to standalone only when--flagsis absent, then pass the parsed value through unchanged.Proposed fix
- int flags = 0; - + simOptions->flags = SIMULATOR_FLAGS_STANDALONE; if (cliOptions.isSet(optFlags)) { bool chk; - flags = cliOptions.value(optFlags).toInt(&chk); - if (!chk) - flags = 0; + int flags = cliOptions.value(optFlags).toInt(&chk); + if (!chk) { + showHelp(cliOptions, QApplication::translate("SimulatorMain", "Invalid flags value: %1").arg(cliOptions.value(optFlags))); + return CommandLineExitErr; + } + simOptions->flags = flags; + cliOptsFound = true; } - - simOptions->flags = flags;- SimulatorMainWindow * mainWindow = new SimulatorMainWindow(nullptr, simOptions.simulatorId, (simOptions.flags ? simOptions.flags : SIMULATOR_FLAGS_STANDALONE)); + SimulatorMainWindow * mainWindow = new SimulatorMainWindow(nullptr, simOptions.simulatorId, simOptions.flags);Also applies to: 379-379
🤖 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 `@companion/src/simulator.cpp` around lines 235 - 244, The current code treats an explicitly provided `--flags 0` the same as when the `--flags` option is not provided at all, but you need to preserve the explicit zero value. Instead of always defaulting flags to zero and letting the downstream logic at line 379 map zero to SIMULATOR_FLAGS_STANDALONE, you should only apply the standalone default when `--flags` is completely absent (when cliOptions.isSet(optFlags) is false). When the flag is explicitly set, pass the parsed integer value through unchanged to simOptions->flags, even if that value is zero.companion/src/simulation/simulator.h-135-135 (1)
135-135:⚠️ Potential issue | 🟠 MajorFix serialization: read
flagsin v4 and initialize the field.Line 158 incorrectly reads
simulatorIdagain instead offlags. Additionally,flagsis uninitialized and never deserialized, breaking stream alignment betweenoperator<<andoperator>>.Fixes
- quint8 flags; // Flags passed from Companion + quint8 flags = 0; // Flags passed from Companionif (o.m_version >= 3) in >> o.simulatorId; if (o.m_version >= 4) - in >> o.simulatorId; + in >> o.flags; + else + o.flags = 0;🤖 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 `@companion/src/simulation/simulator.h` at line 135, The deserialization operator>> is incorrectly reading simulatorId twice instead of reading the flags field. Locate the operator>> method that deserializes the simulator data, find where simulatorId is being read a second time (around line 158), and change that line to read flags instead. This ensures the flags field is properly deserialized and maintains correct stream alignment with the operator<< serialization method.
🟡 Minor comments (10)
.github/workflows/release-drafter.yml-21-25 (1)
21-25:⚠️ Potential issue | 🟡 MinorAdd
persist-credentials: falseto checkout for defense-in-depth security.While this workflow doesn't critically depend on persisted credentials (no authenticated Git pushes, and GITHUB_TOKEN is explicitly managed), setting
persist-credentials: falsefollows security best practices by avoiding unnecessary token storage in git config.Suggested patch
- name: Check out the repo uses: actions/checkout@v4 with: ref: ${{ inputs.tag }} fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/release-drafter.yml around lines 21 - 25, Add the `persist-credentials: false` parameter to the `actions/checkout@v4` action configuration in the release-drafter.yml workflow. This should be added as a new line under the `with` section that already contains `ref` and `fetch-depth`, following GitHub Actions security best practices to prevent unnecessary token storage in git configuration.Source: Linters/SAST tools
.github/ISSUE_TEMPLATE/bug-report.yml-89-89 (1)
89-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude PL18U in the transmitter option label.
Line 89 omits PL18U even though this PR adds PL18U board support elsewhere, so reports for that radio will be misclassified.
Suggested fix
- - Flysky PL18/PL18EV + - Flysky PL18/PL18EV/PL18U🤖 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 @.github/ISSUE_TEMPLATE/bug-report.yml at line 89, The transmitter option label "Flysky PL18/PL18EV" on line 89 is missing PL18U even though this PR adds PL18U board support elsewhere. Update this option label to include PL18U alongside PL18 and PL18EV so that users with the PL18U transmitter can accurately select their device when filing bug reports.companion/src/firmwares/rawsource.cpp-35-38 (1)
35-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
getValueconsistent with the new conversion path.Line 37 still ignores
offset, so existing callers ofgetValue()will display different values fromtoDisplay()whenever a range has a non-zero offset.Proposed fix
float RawSourceRange::getValue(int value) { - return float(value) * step; // TODO + offset ?? + return float(toDisplay(value)); }🤖 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 `@companion/src/firmwares/rawsource.cpp` around lines 35 - 38, The getValue method in the RawSourceRange class is not applying the offset parameter in its calculation, causing inconsistency with the toDisplay() method when ranges have non-zero offsets. Modify the getValue method to include the offset in the calculation along with the step multiplication, ensuring that both getValue and toDisplay produce consistent results for the same input value.companion/src/modeledit/logicalswitches.cpp-45-47 (1)
45-47:⚠️ Potential issue | 🟡 MinorHeaders should match field visibility logic.
The Duration/Delay columns are shown in table headers unconditionally (lines 45–47), but their visibility in the UI is conditional based on switch function family (line 433 disables DELAY for EDGE family, lines 465–466 gate visibility via masks). Additionally, the print output gate at
modelprinter.cpp:641checksLogicalSwitchesExt(which always returnstruein the only implementation), making that check redundant. Either make headers conditional to match field visibility, or verify that unconditional headers are the intended design.🤖 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 `@companion/src/modeledit/logicalswitches.cpp` around lines 45 - 47, The headerLabels construction unconditionally includes "Duration" and "Delay" column headers, but the visibility of these columns is conditionally controlled elsewhere based on switch function family (lines 433 disable DELAY for EDGE family, lines 465-466 gate visibility via masks). Either make the headerLabels construction conditional to match the same visibility logic that gates the actual field visibility in the table, or verify that having these headers unconditionally while columns are conditionally hidden is the intended design. Review the visibility mask logic and function family checks to ensure the headers align with what columns are actually shown to the user.companion/src/storage/storage.cpp-91-96 (1)
91-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve backend write errors before deleting the format.
Storage::writeandStorage::writeModelreturnfalsewithout copyingformat->error(), so detailed errors set byEtxFormat,SdcardFormat, orYamlFormatare lost.Proposed fix
bool Storage::write(RadioData & radioData) { bool ret = false; StorageFormat *format = getStorageFormat(); if (format) { ret = format->write(radioData); + if (!ret) + setError(format->error()); delete format; } return ret; } bool Storage::writeModel(const RadioData & radioData, const int modelIndex) { bool ret = false; StorageFormat *format = getStorageFormat(); if (format) { ret = format->writeModel(radioData, modelIndex); + if (!ret) + setError(format->error()); delete format; } return ret; }Also applies to: 104-109
🤖 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 `@companion/src/storage/storage.cpp` around lines 91 - 96, The issue is that when `format->write(radioData)` completes and the format object is deleted, the error details from `format->error()` are lost before they can be preserved. To fix this in the Storage::write method (and similarly in Storage::writeModel), capture the error message from format->error() before calling delete format, store it in an appropriate member variable or error tracking mechanism, and then proceed with the deletion and return statement. This ensures that error information from EtxFormat, SdcardFormat, or YamlFormat implementations is preserved and accessible to the caller.companion/src/storage/labeled.cpp-196-199 (1)
196-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the detailed
RADIO/radio.ymlerror.
loadRadioSettings()already sets the path and YAML exception details; Line 197 replaces that with a generic string.Proposed fix
} else { - setError("Error reading current settings from radio"); return false; }🤖 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 `@companion/src/storage/labeled.cpp` around lines 196 - 199, The setError call on line 197 is replacing the detailed error information that was already set by loadRadioSettings() with a generic string. Remove the setError call in the else block (the one that sets "Error reading current settings from radio") to preserve the detailed error context including the RADIO/radio.yml path and YAML exception details that loadRadioSettings() has already captured.companion/src/helpers.cpp-399-416 (1)
399-416:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up
simuDataand theQProcesson the non-Apple path.The temporary-directory/storage error paths return before deleting
simuData, and the heap-allocatedQProcessis never deleted afterexecute().Proposed cleanup
else { QString resultMsg = QCoreApplication::translate("Companion", "Error creating temporary directory for models and settings."); QMessageBox::critical(NULL, QCoreApplication::translate("Companion", "Simulator Error"), resultMsg); + delete simuData; return; } @@ if (!storage.write(*simuData)) { QString resultMsg = QCoreApplication::translate("Companion", "Error writing models and settings to temporary directory."); QMessageBox::critical(NULL, QCoreApplication::translate("Companion", "Simulator Error"), resultMsg); + delete simuData; return; } @@ if (result != 0) QMessageBox::critical(NULL, QCoreApplication::translate("Companion", "Simulator Error"), resultMsg); - if (simuData) - delete simuData; + delete simu; + delete simuData;Also applies to: 434-454
🤖 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 `@companion/src/helpers.cpp` around lines 399 - 416, The code has memory leaks in the error handling paths. In the tmpDir.isValid() check failure and the storage.write() failure sections, the function returns without deleting the heap-allocated simuData object. Additionally, the QProcess object created and executed in the code section around lines 434-454 is never deleted after execute() completes. Add cleanup code to delete simuData before each error return statement in these validation and write error paths, and delete the QProcess object after the execute() call completes to prevent memory leaks.companion/src/generaledit/generalsetup.ui-1475-1481 (1)
1475-1481:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace stale copy-pasted help text.
The switch delay help text describes LCD contrast, and the beeper mode help still describes numeric volume levels that no longer match the combo items.
Proposed text cleanup
- <string><!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"> -<html><head><meta name="qrichtext" content="1" /><style type="text/css"> -p, li { white-space: pre-wrap; } -</style></head><body style=" font-family:'MS Shell Dlg 2'; font-size:8.25pt; font-weight:400; font-style:normal;"> -<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;"><span style=" font-size:8pt;">LCD Screen Contrast</span></p> -<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;"><span style=" font-size:8pt;">Values can be 20-45</span></p></body></html></string> + <string>Delay before playing the switch mid-position sound, in milliseconds.</string>- <string>Beeper volume - -0 - Quiet. No beeps at all. -1 - No Keys. Normal beeps but menu keys do not beep. -2 - Normal. -3 - Loud. -4 - Extra loud.</string> + <string>Beeper mode. + +Quiet - No beeps. +Alarms Only - Alarm beeps only. +No Keys - Normal beeps, but menu keys do not beep. +All - All beeps enabled.</string>Also applies to: 2488-2495
🤖 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 `@companion/src/generaledit/generalsetup.ui` around lines 1475 - 1481, The whatsThis property contains stale help text describing LCD Screen Contrast with values 20-45, but this text is actually for a switch delay control and does not match what the control does. Replace this help text with appropriate description of switch delay functionality. Additionally, fix similar stale help text in the beeper mode section (around lines 2488-2495) where the help describes numeric volume levels that no longer match the actual combo box items available. Replace these whatsThis properties with accurate descriptions that correspond to what each control actually controls.companion/src/generaledit/generalsetup.ui-2540-2544 (1)
2540-2544:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stray
>from the new label.This will render as
>One log per dayin the settings panel.Proposed fix
- <string>>One log per day</string> + <string>One log per day</string>🤖 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 `@companion/src/generaledit/generalsetup.ui` around lines 2540 - 2544, The label text property in the QLabel widget named label_3 has a stray `>` character at the beginning of the string. Remove this character from the text property so it reads "One log per day" instead of ">One log per day".companion/src/helpers.cpp-447-448 (1)
447-448:⚠️ Potential issue | 🟡 MinorUse
QString::number(result)to format the exit code as decimal text.
QString(result)passes the integer to the constructor as a character code, not a numeric value. The string concatenation at line 448 requiresQString::number()to properly convert the exit code to its decimal representation.Proposed fix
- resultMsg = QCoreApplication::translate("Companion", "Exited with result code:") % QString(result); + resultMsg = QCoreApplication::translate("Companion", "Exited with result code:") % QString::number(result);🤖 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 `@companion/src/helpers.cpp` around lines 447 - 448, In the else if block where the resultMsg variable is being assigned, replace the QString(result) constructor call with QString::number(result) to properly convert the integer exit code to its decimal text representation. The QString constructor with an integer argument treats the value as a character code rather than a numeric value that needs to be formatted as decimal text.
🧹 Nitpick comments (2)
cmake/FetchYamlCpp.cmake (1)
8-8: Add yaml-cpp version reference to the pinned commit.Line 8 pins a raw commit SHA, which is reproducible but difficult to audit. Add an inline comment documenting the release version or PR reference (e.g.,
# yaml-cpp 0.9.0).🤖 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 `@cmake/FetchYamlCpp.cmake` at line 8, The GIT_TAG line pinning the yaml-cpp commit is not documented with version information, making it difficult to audit. Add an inline comment to the GIT_TAG variable that documents the corresponding release version or PR reference (for example, indicating this is yaml-cpp 0.9.0 or a specific PR) to improve code maintainability and auditability while preserving the reproducible commit SHA.companion/src/modelprinter.cpp (1)
571-571: ⚡ Quick winUse
RawSourceRange::toDisplay()for printer formatting.Line 571 duplicates the new conversion formula and keeps a stale TODO; using the helper keeps UI and printed output on the same path.
Proposed refactor
- result += QString::number((double)(range.step * ls.val2 /*TODO+ source.getRawOffset(model)*/) + range.offset, 'f', range.decimals); + result += QString::number(range.toDisplay(ls.val2), 'f', range.decimals);🤖 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 `@companion/src/modelprinter.cpp` at line 571, The code on line 571 manually duplicates the conversion formula for formatting range values instead of using the helper method RawSourceRange::toDisplay(). Replace the manual calculation (which multiplies range.step by ls.val2, adds range.offset, and formats with decimals) with a call to the RawSourceRange::toDisplay() helper method to keep the printer formatting consistent with the UI output path and remove the stale TODO comment about source.getRawOffset().
| TAG="${{ inputs.tag }}" | ||
| if gh release view "$TAG" --json isDraft --jq '.isDraft' 2>/dev/null | grep -q true; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/release-drafter.yml | head -150Repository: EdgeTX/edgetx
Length of output: 6573
Pass workflow inputs via env and validate tag format before use.
Direct interpolation of ${{ inputs.tag }} into shell scripts creates injection risk. Use environment variables and validate the tag format with a case statement pattern.
Suggested patch (pattern)
- - name: Clean up any previous drafts for this version
+ - name: Clean up any previous drafts for this version
+ env:
+ INPUT_TAG: ${{ inputs.tag }}
run: |
- TAG="${{ inputs.tag }}"
+ TAG="$INPUT_TAG"
+ case "$TAG" in
+ v[0-9]*.[0-9]*.[0-9]*|v[0-9]*.[0-9]*.[0-9]*-rc*|v[0-9]*.[0-9]*.[0-9]*-beta*|v[0-9]*.[0-9]*.[0-9]*-alpha*) ;;
+ *) echo "::error::Invalid tag format: $TAG"; exit 1 ;;
+ esac
if gh release view "$TAG" --json isDraft --jq '.isDraft' 2>/dev/null | grep -q true; then
echo "Deleting existing draft release for tag: $TAG"
gh release delete "$TAG" --yes
fiApply to lines 29, 41, 76, 80, 81, and 110 (line 84 and 111–112 use previously set variables).
📝 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.
| TAG="${{ inputs.tag }}" | |
| if gh release view "$TAG" --json isDraft --jq '.isDraft' 2>/dev/null | grep -q true; then | |
| env: | |
| INPUT_TAG: ${{ inputs.tag }} | |
| run: | | |
| TAG="$INPUT_TAG" | |
| case "$TAG" in | |
| v[0-9]*.[0-9]*.[0-9]*|v[0-9]*.[0-9]*.[0-9]*-rc*|v[0-9]*.[0-9]*.[0-9]*-beta*|v[0-9]*.[0-9]*.[0-9]*-alpha*) ;; | |
| *) echo "::error::Invalid tag format: $TAG"; exit 1 ;; | |
| esac | |
| if gh release view "$TAG" --json isDraft --jq '.isDraft' 2>/dev/null | grep -q true; then | |
| echo "Deleting existing draft release for tag: $TAG" | |
| gh release delete "$TAG" --yes | |
| fi |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 29-29: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/release-drafter.yml around lines 29 - 30, The workflow
uses inputs.tag directly in shell commands without validation, creating a
potential injection risk. After setting the TAG environment variable on line 29,
add validation before using it in any shell command by implementing a case
statement pattern to verify the tag matches expected format. Apply this same
validation pattern to all other locations where the tag input is referenced
(lines 41, 76, 80, 81, and 110), ensuring the tag is sanitized and validated
before being passed to the gh release commands and other shell operations.
Source: Linters/SAST tools
| ExpoData sortedExpoData[CPN_MAX_EXPOS]; | ||
| int destidx = 0; | ||
|
|
||
| QMap<int, int>::const_iterator i; | ||
| for (i = map.constBegin(); i != map.constEnd(); ++i) { | ||
| memcpy(&sortedExpoData[destidx], &expoData[i.value()], sizeof(ExpoData)); | ||
| destidx++; | ||
| } | ||
|
|
||
| memcpy(&expoData[0], &sortedExpoData[0], CPN_MAX_EXPOS * sizeof(ExpoData)); | ||
| } |
There was a problem hiding this comment.
Initialize sortedExpoData before full-buffer copy.
Line 1461 allocates sortedExpoData without initialization, and Line 1470 copies the entire array back. When not all slots are populated, uninitialized stack data is written into expoData, which can corrupt model entries.
🛠️ Suggested fix
- ExpoData sortedExpoData[CPN_MAX_EXPOS];
+ ExpoData sortedExpoData[CPN_MAX_EXPOS] = {};
int destidx = 0;📝 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.
| ExpoData sortedExpoData[CPN_MAX_EXPOS]; | |
| int destidx = 0; | |
| QMap<int, int>::const_iterator i; | |
| for (i = map.constBegin(); i != map.constEnd(); ++i) { | |
| memcpy(&sortedExpoData[destidx], &expoData[i.value()], sizeof(ExpoData)); | |
| destidx++; | |
| } | |
| memcpy(&expoData[0], &sortedExpoData[0], CPN_MAX_EXPOS * sizeof(ExpoData)); | |
| } | |
| ExpoData sortedExpoData[CPN_MAX_EXPOS] = {}; | |
| int destidx = 0; | |
| QMap<int, int>::const_iterator i; | |
| for (i = map.constBegin(); i != map.constEnd(); ++i) { | |
| memcpy(&sortedExpoData[destidx], &expoData[i.value()], sizeof(ExpoData)); | |
| destidx++; | |
| } | |
| memcpy(&expoData[0], &sortedExpoData[0], CPN_MAX_EXPOS * sizeof(ExpoData)); | |
| } |
🤖 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 `@companion/src/firmwares/modeldata.cpp` around lines 1461 - 1471, The
sortedExpoData array is declared but not initialized, and when the loop only
partially fills it (copying from expoData using memcpy), the remaining
uninitialized stack memory persists and gets copied back into expoData on the
final memcpy call, corrupting model entries. Initialize the entire
sortedExpoData array immediately after declaration using memset to zero out all
CPN_MAX_EXPOS slots before the QMap iteration loop begins populating individual
entries.
| #elif defined(AUDIO_SPI) | ||
| #define AUDIO_BUFFER_COUNT (2) // smaller than Taranis since there is also a buffer on the ADC chip | ||
| #elif defined(PCBPL18) | ||
| #define AUDIO_BUFFER_COUNT (10) // PL18 need more buffer for smooth audio | ||
| #elif defined(STORAGE_USE_SPI_FLASH) | ||
| #define AUDIO_BUFFER_COUNT (10) // SPI Flash need more buffer for smooth audio |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== audio.h selection block =="
nl -ba radio/src/audio.h | sed -n '84,112p'
echo
echo "== where AUDIO_SPI / STORAGE_USE_SPI_FLASH are defined =="
rg -n -C2 '\b(AUDIO_SPI|STORAGE_USE_SPI_FLASH)\b' --iglob '*CMakeLists.txt' --iglob '*.cmake' --iglob '*.h'
echo
echo "== board compile-definition context for new FlySky targets =="
rg -n -C3 'PL18U|PL18EV|PL18|ST16|PA01|target_compile_definitions|add_definitions' --iglob '*CMakeLists.txt'Repository: EdgeTX/edgetx
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== audio.h selection block (lines 84-112) =="
cat -n radio/src/audio.h | sed -n '84,112p'
echo
echo "== where AUDIO_SPI / STORAGE_USE_SPI_FLASH are defined =="
rg -n '\b(AUDIO_SPI|STORAGE_USE_SPI_FLASH)\b' -C2 radio/src/
echo
echo "== CMake board definitions for FlySky targets and SPI settings =="
rg -n 'PL18U|PL18EV|PL18|ST16|PA01|AUDIO_SPI|STORAGE_USE_SPI_FLASH' -C2 --iglob '*CMakeLists.txt' --iglob '*.cmake'Repository: EdgeTX/edgetx
Length of output: 16360
Macro precedence bug: SPI-flash targets incorrectly assigned smaller audio buffer.
The #elif chain evaluates conditions in order. PL18 and ST16 targets define both AUDIO_SPI and STORAGE_USE_SPI_FLASH in their hal.h files. Line 93's AUDIO_SPI condition matches first, assigning AUDIO_BUFFER_COUNT = 2, bypassing line 95's buffer-10 assignment intended for SPI-flash storage. This causes SPI-flash boards to use an undersized buffer despite the explicit comment that "SPI Flash need more buffer for smooth audio."
Reorder the conditions so the more specific SPI-flash requirement takes precedence, or use a combined condition that accounts for boards defining both.
🤖 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 `@radio/src/audio.h` around lines 93 - 96, The preprocessor conditions for
AUDIO_BUFFER_COUNT have a precedence issue where targets defining both AUDIO_SPI
and STORAGE_USE_SPI_FLASH match the AUDIO_SPI condition first (line 93) with
value 2, bypassing the intended STORAGE_USE_SPI_FLASH condition (line 95) with
value 10. Reorder these conditions so the STORAGE_USE_SPI_FLASH check comes
before the AUDIO_SPI check, since SPI-flash storage is the more specific
requirement that needs the larger buffer. This ensures boards like PL18 and ST16
that define both macros will correctly receive the buffer-10 assignment.
|
This PR looks to have been created against the wrong branch - main instead of 2.11. The branch in GH is based on 2.11 so the PR should be as well. |
|
Yeah, it is just a placeholder. I need sometime to check if this PR works or not. This is why the status is Draft. |
richardclli
changed the title
2 participants
Branch 2.10 is much better, so some change is backport the handling of dma buffer underrun from 2.10 branch
Summary by CodeRabbit
Release Notes
New Features
Improvements