feat(firmware): add a special function to trigger key lock.#7565
Open
philmoz wants to merge 2 commits into
Open
feat(firmware): add a special function to trigger key lock.#7565philmoz wants to merge 2 commits into
philmoz wants to merge 2 commits into
Conversation
philmoz
added
enhancement ✨
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 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 |
📝 WalkthroughWalkthroughAdds the conditional ChangesDisable Keys function integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CustomFunction
participant evalUIFunctions
participant KeyLock
participant Popup
CustomFunction->>evalUIFunctions: Activate FUNC_DISABLE_KEYS
evalUIFunctions->>KeyLock: setKeyLockedState(active)
KeyLock-->>Popup: Emit toggle event
Popup->>Popup: POPUP_WAIT(message, unlockInfo)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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.
Inline comments:
In `@radio/src/functions.cpp`:
- Around line 198-209: Update the aggregate active-function processing around
CFN_FUNC(cfn) and setKeyLockedState: compute the previous and new aggregate
states once across global and model functions, then synchronize the lock from
that transition even when a No Keys row is disabled, cleared, or replaced.
Remove per-row comparisons that depend on iteration order, while preserving
newActiveFunctions updates for FUNCTION_DISABLE_KEYS.
In `@radio/src/gui/common/stdlcd/popups_common.cpp`:
- Around line 253-257: Update POPUP_WAIT to reset warningInfoFlags when
installing new wait-popup information, alongside warningInfoText and
warningInfoLength, so the popup does not inherit styling from the previous
popup.
In `@radio/src/keys.cpp`:
- Around line 202-207: Synchronize cross-task access to s_keys_locked and
s_keys_lock_notify between setKeyLockedState() and consumeKeysLockToggleEvent().
Use the project’s existing synchronization mechanism consistently for both reads
and writes, or marshal setKeyLockedState() updates onto the key task, ensuring
key-lock toggle notifications cannot be lost.
In `@radio/src/translations/i18n/fi.h`:
- Line 244: Translate the new function label and key-lock unlock message in all
affected locale files: radio/src/translations/i18n/fi.h at 244-244 and 576-576,
radio/src/translations/i18n/fr.h at 247-247 and 580-580, and
radio/src/translations/i18n/he.h at 247-247 and 580-580. Use Finnish, French,
and Hebrew text respectively, and verify RTL rendering of the unlock-message
placeholders in he.h.
In `@radio/src/translations/i18n/it.h`:
- Line 244: Localize the key-lock UI strings in all affected catalog entries: in
radio/src/translations/i18n/it.h lines 244 and 577, provide Italian text for
TR_SF_DISABLE_KEYS and both popup variants; in radio/src/translations/i18n/jp.h
lines 244 and 576, provide Japanese text for TR_SF_DISABLE_KEYS and both popup
variants. Replace only the remaining English strings while preserving the
existing translation keys and formatting.
In `@radio/src/translations/i18n/pt.h`:
- Line 245: Translate the new key-lock function label and lock popup strings in
radio/src/translations/i18n/pt.h at lines 245 and 577,
radio/src/translations/i18n/ru.h at lines 247 and 579, and
radio/src/translations/i18n/se.h at lines 247 and 579. Replace the English text
with accurate Portuguese, Russian, and Swedish translations while preserving all
%s+%s placeholders and the existing TR_BW_COL(...) layout.
In `@tools/cfn_sorter.cpp`:
- Line 131: Update the FUNC_DISABLE_KEYS guard entry in the cfn sorter
definitions to include CFN_ONLY alongside the existing hardware-key condition,
then regenerate radio/src/cfn_sort.cpp so CFN-only builds emit the same function
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56437c16-5f46-49f7-b90d-8b671b8cb5d8
📒 Files selected for processing (56)
companion/src/firmwares/customfunctiondata.cppcompanion/src/firmwares/customfunctiondata.hcompanion/src/firmwares/edgetx/yaml_customfunctiondata.cppradio/src/cfn_sort.cppradio/src/dataconstants.hradio/src/edgetx.hradio/src/functions.cppradio/src/gui/colorlcd/model/special_functions.cppradio/src/gui/colorlcd/model/special_functions.hradio/src/gui/common/stdlcd/popups_common.cppradio/src/gui/common/stdlcd/popups_common.hradio/src/gui/gui_common.cppradio/src/keys.cppradio/src/keys.hradio/src/lua/api_general.cppradio/src/main.cppradio/src/storage/yaml/CMakeLists.txtradio/src/storage/yaml/yaml_datastructs_128x64.cppradio/src/storage/yaml/yaml_datastructs_c14.cppradio/src/storage/yaml/yaml_datastructs_f16.cppradio/src/storage/yaml/yaml_datastructs_gx12.cppradio/src/storage/yaml/yaml_datastructs_st16.cppradio/src/storage/yaml/yaml_datastructs_t15.cppradio/src/storage/yaml/yaml_datastructs_t15pro.cppradio/src/storage/yaml/yaml_datastructs_t20.cppradio/src/storage/yaml/yaml_datastructs_t22.cppradio/src/storage/yaml/yaml_datastructs_tpro.cppradio/src/storage/yaml/yaml_datastructs_tx15.cppradio/src/storage/yaml/yaml_datastructs_tx16smk3.cppradio/src/storage/yaml/yaml_datastructs_x10.cppradio/src/storage/yaml/yaml_datastructs_x9d.cppradio/src/storage/yaml/yaml_datastructs_x9dp2019.cppradio/src/storage/yaml/yaml_datastructs_x9e.cppradio/src/targets/simu/CMakeLists.txtradio/src/translations/i18n/cn.hradio/src/translations/i18n/cz.hradio/src/translations/i18n/da.hradio/src/translations/i18n/de.hradio/src/translations/i18n/en.hradio/src/translations/i18n/es.hradio/src/translations/i18n/fi.hradio/src/translations/i18n/fr.hradio/src/translations/i18n/he.hradio/src/translations/i18n/it.hradio/src/translations/i18n/jp.hradio/src/translations/i18n/ko.hradio/src/translations/i18n/nl.hradio/src/translations/i18n/pl.hradio/src/translations/i18n/pt.hradio/src/translations/i18n/ru.hradio/src/translations/i18n/se.hradio/src/translations/i18n/tw.hradio/src/translations/i18n/ua.hradio/src/translations/sim_string_list.hradio/src/translations/string_list.htools/cfn_sorter.cpp
💤 Files with no reviewable changes (1)
- radio/src/gui/colorlcd/model/special_functions.h
Comment on lines
+198
to
+209
| if (CFN_ACTIVE(cfn) == 0) { | ||
| active = false; | ||
| } else { | ||
| #if defined(KEYS_LOCK_KEY1) && defined(KEYS_LOCK_KEY2) | ||
| // 'No Keys' function checks both switch states | ||
| if (CFN_FUNC(cfn) == FUNC_DISABLE_KEYS) { | ||
| bool locked = isFunctionActive(FUNCTION_DISABLE_KEYS); | ||
| if (active != locked) | ||
| setKeyLockedState(active); | ||
| if (active) | ||
| newActiveFunctions |= (1u << FUNCTION_DISABLE_KEYS); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Synchronize the lock from the aggregate function transition.
This block is never reached when an active “No Keys” row is disabled, cleared, or changed to another function, leaving s_keys_locked enabled after newActiveFunctions clears. Multiple rows are also order-dependent because each compares against the previous aggregate state. Compute the previous/new aggregate state once, including global and model functions, and call setKeyLockedState() only for that transition.
🤖 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/functions.cpp` around lines 198 - 209, Update the aggregate
active-function processing around CFN_FUNC(cfn) and setKeyLockedState: compute
the previous and new aggregate states once across global and model functions,
then synchronize the lock from that transition even when a No Keys row is
disabled, cleared, or replaced. Remove per-row comparisons that depend on
iteration order, while preserving newActiveFunctions updates for
FUNCTION_DISABLE_KEYS.
Comment on lines
+202
to
+207
| void setKeyLockedState(bool state) | ||
| { | ||
| if (g_eeGeneral.keyLockEnabled && s_keys_locked != state) { | ||
| s_keys_locked = state; | ||
| s_keys_lock_notify = true; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline radio/src/keys.cpp --view expanded || true
printf '\n== Relevant symbols ==\n'
rg -n "setKeyLockedState|consumeKeysLockToggleEvent|s_keys_locked|s_keys_lock_notify|evalFunctions|keyLockEnabled" radio/src/keys.cpp radio/src -S
printf '\n== keys.cpp excerpt ==\n'
sed -n '1,320p' radio/src/keys.cpp
printf '\n== threading / task context search ==\n'
rg -n "mixer thread|Mixer|threaded|key poll|polling|consumeKeysLockToggleEvent|evalFunctions\(" radio/src -SRepository: EdgeTX/edgetx
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== functions.cpp around evalFunctions ==\n'
sed -n '150,240p' radio/src/functions.cpp
printf '\n== main.cpp around consumeKeysLockToggleEvent ==\n'
sed -n '260,320p' radio/src/main.cpp
printf '\n== mixer task scheduling ==\n'
sed -n '100,230p' radio/src/tasks/mixer_task.cpp
printf '\n== keys.cpp lock state area ==\n'
sed -n '180,220p' radio/src/keys.cpp
printf '\n== any synchronization around key lock state ==\n'
rg -n "critical|mutex|lock|atomic|s_keys_lock|consumeKeysLockToggleEvent|setKeyLockedState" radio/src -S | sed -n '1,200p'Repository: EdgeTX/edgetx
Length of output: 27066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== keys.cpp around keysPollingCycle and lock toggling ==\n'
sed -n '500,575p' radio/src/keys.cpp
printf '\n== call sites of keysPollingCycle ==\n'
rg -n "keysPollingCycle\(" radio/src -S
printf '\n== call sites of checkKeysLock ==\n'
rg -n "checkKeysLock\(" radio/src -SRepository: EdgeTX/edgetx
Length of output: 3258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== edgetx.cpp around the main loop ==\n'
sed -n '180,260p' radio/src/edgetx.cpp
printf '\n== task definitions / scheduler references ==\n'
rg -n "task_running\(|createTask|xTask|Thread|mixerTask\(|mainTask|guiTask|checkKeysLock\(" radio/src -S
printf '\n== mixer_task.cpp top and task startup refs ==\n'
sed -n '1,120p' radio/src/tasks/mixer_task.cppRepository: EdgeTX/edgetx
Length of output: 47386
Avoid sharing key-lock flags across tasks unsafely. setKeyLockedState() runs from the mixer task, while consumeKeysLockToggleEvent() is read on the main task. Protect s_keys_locked and s_keys_lock_notify with the same synchronization or marshal the update onto the key task to avoid lost toggles.
🤖 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/keys.cpp` around lines 202 - 207, Synchronize cross-task access to
s_keys_locked and s_keys_lock_notify between setKeyLockedState() and
consumeKeysLockToggleEvent(). Use the project’s existing synchronization
mechanism consistently for both reads and writes, or marshal setKeyLockedState()
updates onto the key task, ensuring key-lock toggle notifications cannot be
lost.
| #define TR_SF_SCREENSHOT "Screenshot" | ||
| #define TR_SF_RACING_MODE "RacingMode" | ||
| #define TR_SF_DISABLE_TOUCH "No Touch" | ||
| #define TR_SF_DISABLE_KEYS "No Keys" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new feature strings in every affected locale.
The new function label and key-lock popup remain English across all three locale files, leaving users with untranslated UI.
radio/src/translations/i18n/fi.h#L244-L244,576-576: add Finnish translations for the function label and unlock message.radio/src/translations/i18n/fr.h#L247-L247,580-580: add French translations for the function label and unlock message.radio/src/translations/i18n/he.h#L247-L247,580-580: add Hebrew translations and verify RTL rendering of the unlock placeholders.
📍 Affects 3 files
radio/src/translations/i18n/fi.h#L244-L244(this comment)radio/src/translations/i18n/fr.h#L247-L247radio/src/translations/i18n/he.h#L247-L247
🤖 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/translations/i18n/fi.h` at line 244, Translate the new function
label and key-lock unlock message in all affected locale files:
radio/src/translations/i18n/fi.h at 244-244 and 576-576,
radio/src/translations/i18n/fr.h at 247-247 and 580-580, and
radio/src/translations/i18n/he.h at 247-247 and 580-580. Use Finnish, French,
and Hebrew text respectively, and verify RTL rendering of the unlock-message
placeholders in he.h.
| #define TR_SF_SCREENSHOT "Screenshot" | ||
| #define TR_SF_RACING_MODE "Modo Racing" | ||
| #define TR_SF_DISABLE_TOUCH "Disab. touch" | ||
| #define TR_SF_DISABLE_KEYS "No Keys" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the new key-lock strings in each catalog.
The new function label and popup wording remain English in both locale files, leaving Italian and Japanese users with untranslated UI text.
radio/src/translations/i18n/it.h#L244-L244: provide an Italian translation forTR_SF_DISABLE_KEYS.radio/src/translations/i18n/it.h#L577-L577: translate both Italian key-lock popup variants.radio/src/translations/i18n/jp.h#L244-L244: provide a Japanese translation forTR_SF_DISABLE_KEYS.radio/src/translations/i18n/jp.h#L576-L576: translate both Japanese key-lock popup variants.
📍 Affects 2 files
radio/src/translations/i18n/it.h#L244-L244(this comment)radio/src/translations/i18n/it.h#L577-L577radio/src/translations/i18n/jp.h#L244-L244radio/src/translations/i18n/jp.h#L576-L576
🤖 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/translations/i18n/it.h` at line 244, Localize the key-lock UI
strings in all affected catalog entries: in radio/src/translations/i18n/it.h
lines 244 and 577, provide Italian text for TR_SF_DISABLE_KEYS and both popup
variants; in radio/src/translations/i18n/jp.h lines 244 and 576, provide
Japanese text for TR_SF_DISABLE_KEYS and both popup variants. Replace only the
remaining English strings while preserving the existing translation keys and
formatting.
| #define TR_SF_SCREENSHOT "Capt. Tela" | ||
| #define TR_SF_RACING_MODE "ModCorrida" | ||
| #define TR_SF_DISABLE_TOUCH "No Touch" | ||
| #define TR_SF_DISABLE_KEYS "No Keys" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new key-lock strings in every affected locale.
The new function label and lock popup remain English in the Portuguese, Russian, and Swedish locales.
radio/src/translations/i18n/pt.h#L245-L245,577-577: add Portuguese translations.radio/src/translations/i18n/ru.h#L247-L247,579-579: add Russian translations.radio/src/translations/i18n/se.h#L247-L247,579-579: add Swedish translations.
Preserve the %s+%s placeholders and TR_BW_COL(...) layout.
📍 Affects 3 files
radio/src/translations/i18n/pt.h#L245-L245(this comment)radio/src/translations/i18n/ru.h#L247-L247radio/src/translations/i18n/se.h#L247-L247
🤖 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/translations/i18n/pt.h` at line 245, Translate the new key-lock
function label and lock popup strings in radio/src/translations/i18n/pt.h at
lines 245 and 577, radio/src/translations/i18n/ru.h at lines 247 and 579, and
radio/src/translations/i18n/se.h at lines 247 and 579. Replace the English text
with accurate Portuguese, Russian, and Swedish translations while preserving all
%s+%s placeholders and the existing TR_BW_COL(...) layout.
| { TR_SF_RGBLEDS, "FUNC_RGB_LED", FUNC_RGB_LED, "" }, | ||
| { TR_SF_LCD_TO_VIDEO, "FUNC_LCD_TO_VIDEO", FUNC_LCD_TO_VIDEO, "defined(VIDEO_SWITCH)" }, | ||
| { TR_SF_PUSH_CUST_SWITCH, "FUNC_PUSH_CUST_SWITCH", FUNC_PUSH_CUST_SWITCH, "defined(FUNCTION_SWITCHES)" }, | ||
| { TR_SF_DISABLE_KEYS, "FUNC_DISABLE_KEYS", FUNC_DISABLE_KEYS, "defined(KEYS_LOCK_KEY1) && defined(KEYS_LOCK_KEY2)" }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'FUNC_DISABLE_KEYS|CFN_ONLY|KEYS_LOCK_KEY[12]' \
tools/cfn_sorter.cpp radio/src/dataconstants.h radio/src/cfn_sort.cppRepository: EdgeTX/edgetx
Length of output: 8679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generator logic and nearby table entries.
sed -n '1,220p' tools/cfn_sorter.cpp
printf '\n--- dataconstants excerpt ---\n'
sed -n '720,750p' radio/src/dataconstants.h
printf '\n--- generated sorter excerpt ---\n'
sed -n '120,140p' radio/src/cfn_sort.cppRepository: EdgeTX/edgetx
Length of output: 6868
Add CFN_ONLY to this guard
radio/src/dataconstants.h already includes FUNC_DISABLE_KEYS for CFN_ONLY, but tools/cfn_sorter.cpp still emits it only under the hardware-key condition. That leaves the generated sorter out of sync for CFN-only builds; add || defined(CFN_ONLY) here and regenerate radio/src/cfn_sort.cpp.
🤖 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 `@tools/cfn_sorter.cpp` at line 131, Update the FUNC_DISABLE_KEYS guard entry
in the cfn sorter definitions to include CFN_ONLY alongside the existing
hardware-key condition, then regenerate radio/src/cfn_sort.cpp so CFN-only
builds emit the same function consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@radio/src/functions.cpp`:
- Around line 486-505: Remove the per-row locked-state comparison and
setKeyLockedState call from evalUIFunctions in radio/src/functions.cpp at lines
486-505, retaining only the aggregate newActiveFunctions update. In
radio/src/edgetx.cpp at lines 220-224, after both evalUIFunctions calls
complete, synchronize the lock once using
isFunctionActive(FUNCTION_DISABLE_KEYS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8644be62-616b-46df-b65c-ae1e5b26e1c8
📒 Files selected for processing (15)
radio/src/cfn_sort.cppradio/src/dataconstants.hradio/src/edgetx.cppradio/src/edgetx.hradio/src/functions.cppradio/src/gui/common/stdlcd/popups_common.cppradio/src/keys.hradio/src/storage/yaml/yaml_datastructs_nb4p.cppradio/src/storage/yaml/yaml_datastructs_nv14.cppradio/src/storage/yaml/yaml_datastructs_pa01.cppradio/src/storage/yaml/yaml_datastructs_pl18.cppradio/src/storage/yaml/yaml_datastructs_pl18u.cppradio/src/storage/yaml/yaml_datastructs_xlite.cppradio/src/storage/yaml/yaml_datastructs_xlites.cpptools/cfn_sorter.sh
💤 Files with no reviewable changes (1)
- radio/src/dataconstants.h
🚧 Files skipped from review as they are similar to previous changes (2)
- radio/src/keys.h
- radio/src/cfn_sort.cpp
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Use case : I want to automatically lock the radio keys when I arm the model to prevent accidental key presses
The key lock feature is a fantastic enhancement (thanks @3djc); but still requires the user to manually activate the key combo.
This PR adds a new special/global function to control the key lock feature.
The 'No Keys' special function works like this:
The key combo is always active so it is not possible to lock oneself out of the radio.
If the keys are unlocked after being locked by the special function, the special function will not re-lock them until its state changes.
This PR also make the popup messages more meaningful and fit better on B&W screens.
Summary by CodeRabbit