Repository navigation
feat(hrs-v12): Support for HelloRadioSky V12 - v2.12 - #7113
Merged
Merged
Conversation
Member
Author
|
@philmoz Please review the external antenna GUI changes, not sure what is the best way to generalize this. |
Collaborator
Difficult to review without simulator support or a radio to test. |
pfeerick
added
hardware support
needs: rebase
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a new HelloRadioSky V12 (H750) radio target with its full board/driver stack, build/tooling integration, and Companion firmware registration. It also migrates ChangesV12 Target Board Bring-up
Estimated code review effort: 4 (Complex) | ~75 minutes Antenna-Mode Migration and External-Antenna UI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
radio/src/storage/yaml/yaml_datastructs_st16.cpp (1)
667-673:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAntenna mode appears dropped from ST16 module-data serialization.
After replacing the old 2-bit field with padding (Line 672),
struct_ModuleDatain this file does not define anantennaModefield, so module antenna selection won’t round-trip through YAML.💡 Proposed schema fix
static const struct YamlNode struct_ModuleData[] = { YAML_IDX, YAML_UNSIGNED_CUST( "type", 8, r_moduleType, w_moduleType ), YAML_CUSTOM("subType",r_modSubtype,w_modSubtype), YAML_UNSIGNED( "channelsStart", 8 ), YAML_SIGNED_CUST( "channelsCount", 8, r_channelsCount, w_channelsCount ), YAML_ENUM("failsafeMode", 4, enum_FailsafeModes, NULL), - YAML_PADDING( 4 ), + YAML_ENUM("antennaMode", 2, enum_AntennaModes, NULL), + YAML_PADDING( 2 ), YAML_UNION("mod", 200, union_anonymous_4_elmts, select_mod_type), YAML_END };🤖 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/storage/yaml/yaml_datastructs_st16.cpp` around lines 667 - 673, The ST16 module serialization dropped the 2-bit antennaMode field (now YAML_PADDING) causing module antenna selection to be lost; in the YAML schema (struct_anonymous_6 used by struct_ModuleData) restore the antennaMode entry by replacing the YAML_PADDING(2) with YAML_UNSIGNED("antennaMode", 2) (and ensure struct_ModuleData’s field list/mapping includes antennaMode accordingly so the field round-trips through serialization/deserialization).radio/src/storage/yaml/yaml_datastructs_nb4p.cpp (1)
635-640:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop
antennaModefrom the schema during the move.This hunk removes the old
pxx-scoped bits, but this generated file never reintroduces the replacementModuleData::antennaModefield. As written, YAML round-trips for nb4p will silently lose the per-model antenna selection instead of migrating it. Please update the generator so the field moves rather than disappears, then regenerate this target.🤖 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/storage/yaml/yaml_datastructs_nb4p.cpp` around lines 635 - 640, The generated YAML schema dropped the per-model antenna selection by removing the old pxx-scoped bits without adding the new ModuleData::antennaMode field; update the generator that builds struct_anonymous_6 so it maps/moves those pxx bits into a YAML_UNSIGNED (or appropriate enum/string) entry named "antennaMode" (matching ModuleData::antennaMode) in the node array, ensure padding/order matches adjacent fields (power, receiverTelemetryOff, receiverHigherChannels), and then regenerate yaml_datastructs_nb4p.cpp so YAML round-trips preserve the antennaMode value for each model.radio/src/translations/i18n/tw.h (1)
1051-1056:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMode 4 and mode 5 are still indistinguishable in Traditional Chinese.
This block maps both
TR_ANTENNA_MODES_4andTR_ANTENNA_MODES_5to外置天線, so the UI can't distinguish the combined mode from external-only. Mode 4 should use a distinct label such as the equivalent of “internal + external”.🤖 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/tw.h` around lines 1051 - 1056, TR_ANTENNA_MODES_4 and TR_ANTENNA_MODES_5 are both set to "外置天線" so the UI cannot distinguish the combined mode; change TR_ANTENNA_MODES_4 to a distinct Traditional Chinese label such as "內置+外置天線" (or equivalent) to represent internal+external, and then update TR_ANTENNA_SELECT to include the corresponding three entries (內置天線, 內置+外置天線, 外置天線) so the select list matches the mode macros (refer to TR_ANTENNA_MODES_4, TR_ANTENNA_MODES_5, and TR_ANTENNA_SELECT).companion/src/firmwares/boards.cpp (1)
837-859:⚠️ Potential issue | 🟠 Major | ⚡ Quick winV12's default internal module is not in the supported-module list.
getDefaultInternalModules()now returnsMODULE_TYPE_CROSSFIREforBOARD_HELLORADIOSKY_V12, butgetSupportedInternalModules()never adds V12 to any branch and still falls back toMODULE_TYPE_NONEonly. That gives Companion an impossible default and can leave the Internal RF selector out of sync with the stored 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/boards.cpp` around lines 837 - 859, getDefaultInternalModules() returns MODULE_TYPE_CROSSFIRE for BOARD_HELLORADIOSKY_V12 but getSupportedInternalModules() never lists V12, causing an impossible default; update getSupportedInternalModules() to include BOARD_HELLORADIOSKY_V12 in the same branch that returns MODULE_TYPE_CROSSFIRE (or otherwise add it to the branch that builds the supported modules list) so the supported set contains CROSSFIRE for that board and the Internal RF selector stays in sync with the stored default.radio/src/gui/gui_common.cpp (1)
1070-1074:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate AFHDS3 availability check.
Lines 1071-1074 duplicate the exact same check that already exists at lines 1061-1064. This appears to be an accidental duplication.
🐛 Suggested fix
`#if` !defined(AFHDS2) if (moduleType == MODULE_TYPE_FLYSKY_AFHDS2A) return false; `#endif` - -#if !defined(AFHDS3) - if (moduleType == MODULE_TYPE_FLYSKY_AFHDS3) - return false; -#endif 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 `@radio/src/gui/gui_common.cpp` around lines 1070 - 1074, Remove the duplicated AFHDS3 availability check block so the logic only tests once; locate the repeated conditional that uses moduleType == MODULE_TYPE_FLYSKY_AFHDS3 (the `#if` !defined(AFHDS3) / if (moduleType == MODULE_TYPE_FLYSKY_AFHDS3) return false; block) and delete the second occurrence (the one at lines reported in the review) leaving the original check intact to avoid redundant early returns.
🧹 Nitpick comments (14)
radio/src/targets/common/arm/stm32/usb_driver.cpp (1)
132-143: ⚡ Quick winDecouple
USB_GPIO_SELinit fromUSB_GPIO_VBUSguard
USB_GPIO_SELsetup is currently gated byUSB_GPIO_VBUS. If a target defines selector GPIO without VBUS sensing, Line 139-143 won’t run.Suggested refactor
-#if defined(USB_GPIO_VBUS) -#if defined(USB_GPIO_VBUS_OPEN_DRAIN) - gpio_init(USB_GPIO_VBUS, GPIO_IN_PU, GPIO_PIN_SPEED_LOW); -#else - gpio_init(USB_GPIO_VBUS, GPIO_IN, GPIO_PIN_SPEED_LOW); -#endif - `#if` defined(USB_GPIO_SEL) gpio_init(USB_GPIO_SEL, GPIO_OUT, GPIO_PIN_SPEED_LOW); gpio_set(USB_GPIO_SEL); - //gpio_clear(USB_GPIO_SEL); `#endif` + +#if defined(USB_GPIO_VBUS) +#if defined(USB_GPIO_VBUS_OPEN_DRAIN) + gpio_init(USB_GPIO_VBUS, GPIO_IN_PU, GPIO_PIN_SPEED_LOW); +#else + gpio_init(USB_GPIO_VBUS, GPIO_IN, GPIO_PIN_SPEED_LOW); +#endif // prime debounce state... usbPlugged(); `#endif`🤖 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/targets/common/arm/stm32/usb_driver.cpp` around lines 132 - 143, The USB_GPIO_SEL initialization is incorrectly nested inside the USB_GPIO_VBUS guard so targets that define USB_GPIO_SEL but not USB_GPIO_VBUS never run gpio_init/gpio_set; extract the USB_GPIO_SEL block out of the USB_GPIO_VBUS `#if` and create a separate conditional block: wrap the gpio_init(USB_GPIO_SEL,...), gpio_set(USB_GPIO_SEL) (and optional gpio_clear) with its own `#if` defined(USB_GPIO_SEL) ... `#endif` at the same level as the USB_GPIO_VBUS handling so USB_GPIO_SEL is initialized regardless of USB_GPIO_VBUS being defined.radio/src/targets/common/arm/stm32/stm32_i2c_driver.h (1)
30-31: 💤 Low valueStale comment: update to reflect 4 I2C bus instances.
The comment says "Maxmimum 2 I2C bus instances" but the enum now supports 4 buses. Also has a typo ("Maxmimum").
Suggested fix
-// Maxmimum 2 I2C bus instances +// Maximum 4 I2C bus instances enum { I2C_Bus_1 = 0, I2C_Bus_2,I2C_Bus_3,I2C_Bus_4 };🤖 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/targets/common/arm/stm32/stm32_i2c_driver.h` around lines 30 - 31, Update the stale comment above the enum that declares I2C_Bus_1, I2C_Bus_2, I2C_Bus_3, I2C_Bus_4: fix the typo "Maxmimum" and change the text to reflect that there are four I2C bus instances (e.g., "Maximum 4 I2C bus instances" or similar) so the comment matches the enum values.radio/src/boards/helloradio-h750/lcd_driver.h (2)
42-43: ⚡ Quick winTypo in typedef name:
lcdSpiInitFucPtr.The typedef name appears to have a typo -
Fucinstead ofFunc.✏️ Suggested fix
-typedef void (*lcdSpiInitFucPtr)(void); -typedef unsigned int LcdReadIDFucPtr( void ); +typedef void (*lcdSpiInitFuncPtr)(void); +typedef unsigned int LcdReadIDFuncPtr( void );Note: This will require updating the extern declarations at lines 45-47 and any usages in the implementation.
🤖 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/boards/helloradio-h750/lcd_driver.h` around lines 42 - 43, The typedef names contain typos: rename lcdSpiInitFucPtr to lcdSpiInitFuncPtr and LcdReadIDFucPtr to LcdReadIDFuncPtr, then update all corresponding extern declarations and any usage sites (function pointer variables, casts, assignments) to use the corrected identifiers (look for lcdSpiInitFucPtr and LcdReadIDFucPtr occurrences, plus the externs declared nearby) so signatures remain the same but the names are fixed consistently across header and implementation.
25-32: 💤 Low valueDuplicate timing constants defined.
The same timing values are defined twice with different naming conventions (e.g.,
HBPvsHORIZONTAL_BACK_PORCH). Consider consolidating to a single set to avoid maintenance issues if values need to change.Also applies to: 70-75
🤖 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/boards/helloradio-h750/lcd_driver.h` around lines 25 - 32, The header currently duplicates timing constants (e.g., HBP, VBP, HSW, VSH, HFP, VFP) alongside alternate names like HORIZONTAL_BACK_PORCH etc.; remove the duplicates by choosing one canonical naming scheme and replacing all uses to reference that single set (for example keep HORIZONTAL_BACK_PORCH or HBP consistently), delete the redundant defines, and update any references in the lcd driver code to use the chosen symbols (check uses of HBP, HORIZONTAL_BACK_PORCH, HSW, HORIZONTAL_SYNC_WIDTH, VBP, VERTICAL_BACK_PORCH, etc.) so there is exactly one definition per timing constant.radio/src/boards/helloradio-h750/touch_driver.h (1)
58-68: ⚡ Quick winMisleading Doxygen comment references wrong typedef name.
The comment at line 58-60 references
ft6x06_handle_TypeDefbut the actual typedef istc_handle_TypeDef. This appears to be copy-paste from another driver.✏️ Suggested fix
-/** `@typedef` ft6x06_handle_TypeDef - * ft6x06 Handle definition. +/** `@typedef` tc_handle_TypeDef + * Touch controller Handle definition. */ typedef struct {🤖 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/boards/helloradio-h750/touch_driver.h` around lines 58 - 68, The Doxygen comment above the typedef is incorrect: replace the misleading reference to ft6x06_handle_TypeDef with the correct typedef name tc_handle_TypeDef (or remove the typedef name from the comment) so the doc matches the actual struct; update the comment block that documents tc_handle_TypeDef (the struct containing currActiveTouchNb and currActiveTouchIdx) to use the correct identifier.radio/src/gui/colorlcd/radio/hw_intmodule.cpp (1)
72-89: 💤 Low valueMissing padding and inconsistent call ordering.
Two minor issues in this new UI block:
Missing
antline->padLeft(PAD_SMALL)that exists in the similar block at line 47, causing potential visual inconsistency.The order of
SET_DIRTY()andcheckExternalAntenna()in the else branch (lines 84-86) differs from the analogous code at lines 63-65. While functionally equivalent, consistency aids maintainability.🔧 Suggested consistency fix
`#if` defined(INTMODULE_ANTSEL_GPIO) && defined(EXTERNAL_ANTENNA) auto antline = parent->newLine(grid); + antline->padLeft(PAD_SMALL); new StaticText(antline, rect_t{}, STR_ANTENNA); new Choice(antline, rect_t{}, STR_ANTENNA_SELECT, 0, ANTENNA_MODE_EXTERNAL, GET_DEFAULT(g_eeGeneral.antennaMode), [=](int8_t antenna) { if (!isExternalAntennaEnabled() && (antenna == ANTENNA_MODE_EXTERNAL)) { if (confirmationDialog(STR_ANTENNACONFIRM1, STR_ANTENNACONFIRM2)) { g_eeGeneral.antennaMode = antenna; SET_DIRTY(); } } else { g_eeGeneral.antennaMode = antenna; - SET_DIRTY(); checkExternalAntenna(); + SET_DIRTY(); } }); `#endif`🤖 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/gui/colorlcd/radio/hw_intmodule.cpp` around lines 72 - 89, Add the same left padding and reorder calls to match the analogous block: call antline->padLeft(PAD_SMALL) after creating antline, and in the Choice lambda's else branch set g_eeGeneral.antennaMode = antenna; then call SET_DIRTY() before calling checkExternalAntenna(); keep the existing confirmationDialog/isExternalAntennaEnabled logic unchanged so the symbols to edit are antline, padLeft(PAD_SMALL), the Choice lambda, g_eeGeneral.antennaMode, SET_DIRTY(), and checkExternalAntenna().radio/src/gui/colorlcd/module/crossfire_settings.cpp (1)
92-92: 💤 Low valueRemove self-assignment (no-op).
moduleIdx = moduleIdx;has no effect. This appears to be leftover from refactoring or a copy-paste artifact.- moduleIdx = moduleIdx;🤖 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/gui/colorlcd/module/crossfire_settings.cpp` at line 92, The line "moduleIdx = moduleIdx;" is a self-assignment no-op in crossfire_settings.cpp; remove this statement or replace it with the intended assignment to the member variable (e.g., this->moduleIdx = moduleIdx or assign to the actual member name) inside the constructor or setter where it appears (look for function/constructor names referencing moduleIdx in crossfire_settings.cpp); ensure the correct target (member vs parameter) is used so the parameter value is stored.tools/generate-yaml.sh (1)
14-14: 💤 Low valueQuote the default assignment to prevent potential globbing.
The static analysis tool flagged this default assignment. While the current value doesn't contain active glob characters, quoting the RHS is defensive and idiomatic:
-: ${FLAVOR:="t15;tx16s;pl18;nv14;pl18u;nb4p;v12;x9d;x9dp2019;x9e;xlite;xlites;x7;tpro;t20;f16;gx12;st16;pa01;tx15;t15pro;tx16smk3"} +: "${FLAVOR:=t15;tx16s;pl18;nv14;pl18u;nb4p;v12;x9d;x9dp2019;x9e;xlite;xlites;x7;tpro;t20;f16;gx12;st16;pa01;tx15;t15pro;tx16smk3}"🤖 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/generate-yaml.sh` at line 14, The default assignment for FLAVOR in tools/generate-yaml.sh uses an unquoted RHS which can trigger pathname expansion; update the assignment for the FLAVOR variable (the line containing : ${FLAVOR:=...}) to quote the right-hand side string so the default value is quoted (e.g., change the RHS to a quoted string) to prevent potential globbing and follow shell best practices.radio/src/targets/v12/CMakeLists.txt (1)
100-108: 💤 Low valueRedundant
FUNCTION_SWITCHESdefinition block.Lines 106-108 add
-DFUNCTION_SWITCHESagain, but this is already done at line 102 whenFUNCTION_SWITCHES_WITH_RGBis enabled (which setsFUNCTION_SWITCHES=YESat line 101). The second block is unreachable whenFUNCTION_SWITCHES_WITH_RGBisYES.♻️ Suggested cleanup
if(FUNCTION_SWITCHES_WITH_RGB) set(FUNCTION_SWITCHES YES) add_definitions(-DFUNCTION_SWITCHES) add_definitions(-DFUNCTION_SWITCHES_RGB_LEDS) endif() - -if(FUNCTION_SWITCHES) -add_definitions(-DFUNCTION_SWITCHES) -endif()🤖 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/targets/v12/CMakeLists.txt` around lines 100 - 108, The CMakeLists contains a redundant block that re-adds -DFUNCTION_SWITCHES: remove the second conditional that tests FUNCTION_SWITCHES (the block with add_definitions(-DFUNCTION_SWITCHES) at the end) because FUNCTION_SWITCHES is already set and defined when FUNCTION_SWITCHES_WITH_RGB is true (the block that sets FUNCTION_SWITCHES YES and calls add_definitions(-DFUNCTION_SWITCHES) and add_definitions(-DFUNCTION_SWITCHES_RGB_LEDS)); simply delete the trailing if(FUNCTION_SWITCHES)...endif() block so -DFUNCTION_SWITCHES is only added once via FUNCTION_SWITCHES_WITH_RGB or via any other explicit upstream logic.radio/src/targets/v12/hal.h (1)
120-120: 💤 Low valueStale comment on switch GPIO definition.
Line 120 comment says "PE.05" but the actual pin is
GPIOHpin 8 (PH.08). Update the comment to match.🤖 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/targets/v12/hal.h` at line 120, Update the stale inline comment for the SWITCHES_GPIO_PIN_A macro: in the definition of SWITCHES_GPIO_PIN_A (currently defined as LL_GPIO_PIN_8) change the trailing comment from "PE.05" to the correct port/pin "PH.08" so the comment accurately reflects GPIOH pin 8.radio/src/boards/helloradio-h750/backlight_driver.cpp (1)
60-77: 💤 Low valueUnused variable
lastDutyCycle.The variable
lastDutyCycleis declared but only referenced in commented-out code. Either remove it or uncomment the LCD power management logic when ready.🤖 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/boards/helloradio-h750/backlight_driver.cpp` around lines 60 - 77, The variable lastDutyCycle is unused because the LCD power-management block inside backlightEnable(uint8_t dutyCycle) is commented out; either remove the global lastDutyCycle declaration and any related commented references, or restore the intended logic: re-enable the if(!dutyCycle)/else if(!lastDutyCycle) branch inside backlightEnable and update lastDutyCycle = dutyCycle so transitions call lcdOffFunction / lcdOnFunction / lcdInit as appropriate (references: lastDutyCycle, backlightEnable, lcdOffFunction, lcdOnFunction, lcdInit).radio/src/boards/helloradio-h750/lcd_driver.cpp (2)
326-411: 💤 Low valueRemove or document commented-out initialization code.
This large block of commented-out LCD initialization code adds visual noise. Either remove it if it's no longer needed, or add a brief comment explaining why it's preserved (e.g., for alternative LCD controller support).
🤖 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/boards/helloradio-h750/lcd_driver.cpp` around lines 326 - 411, The large commented-out LCD init sequence (calls to lcdWriteCommand and lcdWriteData for commands like 0x11, 0xB2, 0xE0/E1, etc.) is visual noise; either delete this dead block or add a one-line comment above it stating why it’s preserved (e.g., "Alternative init sequence for XYZ controller kept for reference") and include the controller/version and why it differs; locate the block around the lcd initialization code (references: lcdWriteCommand, lcdWriteData, commands 0x11/0xB2/0xE0/0xE1) and apply the change so the file no longer contains an unexplained multi-line commented init sequence.
106-123: ⚖️ Poor tradeoffBusy-wait on FMARK has no timeout protection.
The
while (!gpio_read(LCD_FMARK));loop at line 112 will hang indefinitely if the LCD FMARK signal never asserts (e.g., due to hardware fault or LCD not initialized). The TODO comment acknowledges this, but consider adding a timeout to prevent system hangs.🤖 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/boards/helloradio-h750/lcd_driver.cpp` around lines 106 - 123, In startLcdRefresh, replace the unbounded busy-wait on gpio_read(LCD_FMARK) with a bounded wait using a timeout: capture a start timestamp (or tick count), loop checking gpio_read(LCD_FMARK) and break when asserted or when elapsed time exceeds a defined timeout constant, and on timeout abort the refresh path (skip set_column_addr/set_row_addr/memory_write) and report the failure (e.g., return an error code or call an error-logging routine). Make the timeout value configurable via a constant and update any callers of startLcdRefresh if you change its return type to indicate failure; reference the startLcdRefresh function, gpio_read, LCD_FMARK, set_column_addr, set_row_addr, and memory_write when applying the change.radio/src/drivers/icm42627.cpp (1)
22-25: 💤 Low valueClarify comment about device compatibility.
The comment mentions ICM42607C but the code validates WHO_AM_I == 0x20 at line 84. If this driver specifically targets ICM42627 (as the filename suggests), consider updating the comment to match, or document which devices are actually supported.
🤖 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/drivers/icm42627.cpp` around lines 22 - 25, Top-of-file comment refers to ICM42607C but the driver filename and runtime check expect a different device (the WHO_AM_I check compares to 0x20 in icm42627.cpp), so update the comment to accurately list the supported device(s) or explicitly document compatibility; specifically, edit the header comment in icm42627.cpp to state that this driver targets ICM42627 (or the exact device(s) whose WHO_AM_I == 0x20), and ensure any constants or the probe/init routine that checks WHO_AM_I (the WHO_AM_I check comparing to 0x20) and related identifiers are consistent with that documented device list.
🤖 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 `@companion/src/firmwares/boardjson.cpp`:
- Around line 92-94: The injected TILT_X/TILT_Y for BOARD_RADIOMASTER_TX16SMK3,
BOARD_FLYSKY_PA01, and BOARD_HELLORADIOSKY_V12 is not reflected in the HasIMU
capability, causing GyroAxes>0 while HasIMU==0; update the
Boards::getCapability(board, HasIMU) logic to include these three board
constants (BOARD_RADIOMASTER_TX16SMK3, BOARD_FLYSKY_PA01,
BOARD_HELLORADIOSKY_V12) or alternatively make HasIMU compute from gyro axis
presence (e.g., GyroAxes>0 or presence of TILT_X/TILT_Y) so IMU-gated flows
remain enabled when the gyro fixups at the TILT_X/TILT_Y injection point apply.
In `@companion/src/firmwares/boards.cpp`:
- Around line 952-958: The V12 EXTMODSIZE_SMALL branch is never reached because
it's inside the getCapability(board, HasColorLcd) guard while
IS_HELLORADIOSKY_V12 is excluded from HasColorLcd; move the V12 check out of
that guard (or add a direct board check) so the override runs regardless of
HasColorLcd. Concretely, change the conditional sequence around
getCapability/IS_HELLORADIOSKY_V12 so IS_HELLORADIOSKY_V12 returns
EXTMODSIZE_SMALL before or separately from the HasColorLcd branch (symbols:
getCapability, HasColorLcd, IS_HELLORADIOSKY_V12, EXTMODSIZE_SMALL). Ensure
behavior for IS_FLYSKY_EL18 remains the same.
In `@radio/src/boards/helloradio-h750/backlight_driver.cpp`:
- Around line 89-94: The global flag boardBacklightOn is never updated, so
isBacklightEnabled() always returns false; update boardBacklightOn inside
backlightEnable() to reflect the actual duty state (set true when duty > 0 and
false when duty == 0) and ensure any early returns or error paths still keep the
flag consistent; reference the boardBacklightOn variable and the
backlightEnable() and isBacklightEnabled() functions when making this change.
In `@radio/src/boards/helloradio-h750/board.h`:
- Around line 77-85: IS_INTERNAL_MODULE_ON() is incorrectly hardcoded to false
while INTERNAL_MODULE_ON()/INTERNAL_MODULE_OFF() actually drive
INTMODULE_PWR_GPIO; change the macro to read the real GPIO state (e.g. define
IS_INTERNAL_MODULE_ON() as (gpio_read(INTMODULE_PWR_GPIO) ? 1 : 0) or
equivalent), ensuring it matches the GPIO used in board.cpp so callers correctly
detect the internal-module power state.
In `@radio/src/boards/helloradio-h750/extflash_driver.cpp`:
- Around line 52-60: The loop currently always programs up to QSPI_NOR_PAGE_SIZE
bytes which can cross a page boundary when address is not page-aligned; change
the sizing logic so the first write is limited to the remaining bytes in the
current NOR page and subsequent writes use full page sizes. In the loop around
stm32_qspi_nor_program use a page_offset = address % QSPI_NOR_PAGE_SIZE, set
size = min(len, QSPI_NOR_PAGE_SIZE - page_offset) for the first iteration (then
size = min(len, QSPI_NOR_PAGE_SIZE) thereafter), keep the existing ret check
after stm32_qspi_nor_program, and update len/address/data the same way; this
ensures no single program call spans two NOR pages.
In `@radio/src/boards/helloradio-h750/sdram_driver.cpp`:
- Around line 109-205: SDRAM_InitSequence declares timeout but never uses it
because all __FMC_SDRAM_GET_FLAG(..., FMC_SDRAM_FLAG_BUSY) busy-wait loops were
commented out; either remove the unused timeout (and the related SDRAM_TIMEOUT
symbol) to silence warnings, or restore proper readiness checks between commands
(re-enable the while loops using timeout and __FMC_SDRAM_GET_FLAG with
FMC_Bank5_6_R and FMC_SDRAM_FLAG_BUSY) before each
FMC_SDRAM_SendCommand/FMC_SDRAM_SetAutoRefreshNumber/FMC_SDRAM_ProgramRefreshRate
call so the controller is polled for readiness rather than relying solely on
delay_ms; update the code in SDRAM_InitSequence accordingly (refer to the
timeout variable, SDRAM_TIMEOUT, __FMC_SDRAM_GET_FLAG, FMC_SDRAM_FLAG_BUSY, and
the
FMC_SDRAM_SendCommand/FMC_SDRAM_SetAutoRefreshNumber/FMC_SDRAM_ProgramRefreshRate
locations).
In `@radio/src/boards/helloradio-h750/system_clock.h`:
- Line 22: The declaration for SystemClock_Config is a non-prototype C
declaration; update the function prototype in system_clock.h by changing the
declaration of SystemClock_Config to use an explicit void parameter list (i.e.,
SystemClock_Config(void)) so it enforces argument checking and matches the
implementation in system_clock.c.
In `@radio/src/boards/helloradio-h750/touch_driver.cpp`:
- Around line 84-103: TS_IO_Read and TS_IO_ReadMultiple currently convert I2C
failures into garbage by returning uninitialized data or a fixed success value;
update TS_IO_Read to initialize result (e.g., 0 or a defined error sentinel)
before the retry loop and only return a valid byte if touch_i2c_read succeeds,
otherwise return the error sentinel; update TS_IO_ReadMultiple to zero-fill or
otherwise initialize the provided buffer before retries, and return the actual
number of bytes read (or 0 on failure) instead of always returning 1; reference
touch_i2c_read and initTouchI2C in your changes so retries still re-init the bus
but failures do not produce bogus touch events.
In `@radio/src/gui/128x64/model_setup.cpp`:
- Around line 2196-2203: The code references
g_model.moduleData[INTERNAL_MODULE].antennaMode which will fail if ModuleData
lacks an antennaMode member; update the ModuleData definition to include the
antennaMode field (matching the type used by
reusableBuffer.moduleSetup.antennaMode) or change all accesses in
model_setup.cpp (the antenna switching block that calls POPUP_CONFIRMATION,
SET_WARNING_INFO, onModelAntennaSwitchConfirm and checkExternalAntenna) to the
correct existing field name; ensure the internal moduleData type and any
serialization/load/save code are updated consistently so
g_model.moduleData[INTERNAL_MODULE].antennaMode compiles and behaves as
intended.
In `@radio/src/gui/128x64/view_main.cpp`:
- Around line 79-82: The external-antenna indicator is currently gated by
isModuleXJT(INTERNAL_MODULE) which hides the "E" for non-XJT internal modules;
remove that runtime check so the indicator is shown whenever external antenna
support is compiled in and isExternalAntennaEnabled() returns true. Replace the
if condition that currently combines isModuleXJT(INTERNAL_MODULE) &&
isExternalAntennaEnabled() with a single isExternalAntennaEnabled() check (leave
the surrounding `#if` defined(EXTERNAL_ANTENNA) and the lcdDrawText(VBATT_X - 1,
VBATT_Y + 8, "E", TINSIZE) call unchanged).
In `@radio/src/gui/colorlcd/module/pxx1_settings.cpp`:
- Around line 40-57: The code references md->antennaMode and calls
checkExternalAntenna() but ModuleData no longer has antennaMode and
checkExternalAntenna() is undeclared; fix by either restoring the field or
updating usages: if the model now nests pxx, change all md->antennaMode accesses
in pxx1_settings.cpp (the Choice lambda and the initial conditional) to
md->pxx.antennaMode and ensure GET_DEFAULT(md->pxx.antennaMode) is used, and
also add or include a proper declaration for checkExternalAntenna() (or include
the header that declares it); alternatively, if the intended change was to add
antennaMode to ModuleData, add that field in ModuleData’s definition and ensure
checkExternalAntenna() is declared/visible to this translation unit.
In `@radio/src/targets/v12/hal.h`:
- Around line 191-192: The ADC_GPIOA_PINS macro is using commas (which invoke
the comma operator) instead of bitwise OR, causing only the final pin to be
kept; update the ADC_GPIOA_PINS definition to combine all ADC_GPIO_PIN_* symbols
with the bitwise OR operator (|) just like ADC_GPIOC_PINS does so the macro
yields the combined bitmask; check ADC_GPIOC_PINS for consistency and ensure all
pin tokens (ADC_GPIO_PIN_STICK_LH, ADC_GPIO_PIN_STICK_LV, ADC_GPIO_PIN_STICK_RH,
ADC_GPIO_PIN_STICK_RV) are included and OR-ed together.
In `@radio/src/translations/i18n/he.h`:
- Line 1061: Update the TR_ANTENNA_SELECT macro so the two option labels are
translated to Hebrew (replace "Internal","External" with the Hebrew equivalents)
while preserving the existing macro format and comma-separated quoted strings;
specifically edit the TR_ANTENNA_SELECT definition to use the Hebrew words for
"Internal" and "External" (e.g., "פנימי","חיצוני") and ensure the file remains
UTF-8 encoded so the characters render correctly.
In `@radio/src/translations/i18n/it.h`:
- Line 1058: TR_ANTENNA_SELECT uses mismatched gender forms
("Interna","Esterno"); update the second option to the feminine form to match
the first by changing the string "Esterno" to "Esterna" so the pair reads
"Interna","Esterna" (ensure you update the TR_ANTENNA_SELECT definition
accordingly).
In `@radio/src/translations/i18n/ua.h`:
- Line 1059: Update the TR_ANTENNA_SELECT macro to use feminine adjective forms
that match the antenna noun: replace "Внутрішній","Зовнішній" with
"Внутрішня","Зовнішня" in the `#define` TR_ANTENNA_SELECT so the UI label uses
correct grammatical gender.
---
Outside diff comments:
In `@companion/src/firmwares/boards.cpp`:
- Around line 837-859: getDefaultInternalModules() returns MODULE_TYPE_CROSSFIRE
for BOARD_HELLORADIOSKY_V12 but getSupportedInternalModules() never lists V12,
causing an impossible default; update getSupportedInternalModules() to include
BOARD_HELLORADIOSKY_V12 in the same branch that returns MODULE_TYPE_CROSSFIRE
(or otherwise add it to the branch that builds the supported modules list) so
the supported set contains CROSSFIRE for that board and the Internal RF selector
stays in sync with the stored default.
In `@radio/src/gui/gui_common.cpp`:
- Around line 1070-1074: Remove the duplicated AFHDS3 availability check block
so the logic only tests once; locate the repeated conditional that uses
moduleType == MODULE_TYPE_FLYSKY_AFHDS3 (the `#if` !defined(AFHDS3) / if
(moduleType == MODULE_TYPE_FLYSKY_AFHDS3) return false; block) and delete the
second occurrence (the one at lines reported in the review) leaving the original
check intact to avoid redundant early returns.
In `@radio/src/storage/yaml/yaml_datastructs_nb4p.cpp`:
- Around line 635-640: The generated YAML schema dropped the per-model antenna
selection by removing the old pxx-scoped bits without adding the new
ModuleData::antennaMode field; update the generator that builds
struct_anonymous_6 so it maps/moves those pxx bits into a YAML_UNSIGNED (or
appropriate enum/string) entry named "antennaMode" (matching
ModuleData::antennaMode) in the node array, ensure padding/order matches
adjacent fields (power, receiverTelemetryOff, receiverHigherChannels), and then
regenerate yaml_datastructs_nb4p.cpp so YAML round-trips preserve the
antennaMode value for each model.
In `@radio/src/storage/yaml/yaml_datastructs_st16.cpp`:
- Around line 667-673: The ST16 module serialization dropped the 2-bit
antennaMode field (now YAML_PADDING) causing module antenna selection to be
lost; in the YAML schema (struct_anonymous_6 used by struct_ModuleData) restore
the antennaMode entry by replacing the YAML_PADDING(2) with
YAML_UNSIGNED("antennaMode", 2) (and ensure struct_ModuleData’s field
list/mapping includes antennaMode accordingly so the field round-trips through
serialization/deserialization).
In `@radio/src/translations/i18n/tw.h`:
- Around line 1051-1056: TR_ANTENNA_MODES_4 and TR_ANTENNA_MODES_5 are both set
to "外置天線" so the UI cannot distinguish the combined mode; change
TR_ANTENNA_MODES_4 to a distinct Traditional Chinese label such as "內置+外置天線" (or
equivalent) to represent internal+external, and then update TR_ANTENNA_SELECT to
include the corresponding three entries (內置天線, 內置+外置天線, 外置天線) so the select list
matches the mode macros (refer to TR_ANTENNA_MODES_4, TR_ANTENNA_MODES_5, and
TR_ANTENNA_SELECT).
---
Nitpick comments:
In `@radio/src/boards/helloradio-h750/backlight_driver.cpp`:
- Around line 60-77: The variable lastDutyCycle is unused because the LCD
power-management block inside backlightEnable(uint8_t dutyCycle) is commented
out; either remove the global lastDutyCycle declaration and any related
commented references, or restore the intended logic: re-enable the
if(!dutyCycle)/else if(!lastDutyCycle) branch inside backlightEnable and update
lastDutyCycle = dutyCycle so transitions call lcdOffFunction / lcdOnFunction /
lcdInit as appropriate (references: lastDutyCycle, backlightEnable,
lcdOffFunction, lcdOnFunction, lcdInit).
In `@radio/src/boards/helloradio-h750/lcd_driver.cpp`:
- Around line 326-411: The large commented-out LCD init sequence (calls to
lcdWriteCommand and lcdWriteData for commands like 0x11, 0xB2, 0xE0/E1, etc.) is
visual noise; either delete this dead block or add a one-line comment above it
stating why it’s preserved (e.g., "Alternative init sequence for XYZ controller
kept for reference") and include the controller/version and why it differs;
locate the block around the lcd initialization code (references:
lcdWriteCommand, lcdWriteData, commands 0x11/0xB2/0xE0/0xE1) and apply the
change so the file no longer contains an unexplained multi-line commented init
sequence.
- Around line 106-123: In startLcdRefresh, replace the unbounded busy-wait on
gpio_read(LCD_FMARK) with a bounded wait using a timeout: capture a start
timestamp (or tick count), loop checking gpio_read(LCD_FMARK) and break when
asserted or when elapsed time exceeds a defined timeout constant, and on timeout
abort the refresh path (skip set_column_addr/set_row_addr/memory_write) and
report the failure (e.g., return an error code or call an error-logging
routine). Make the timeout value configurable via a constant and update any
callers of startLcdRefresh if you change its return type to indicate failure;
reference the startLcdRefresh function, gpio_read, LCD_FMARK, set_column_addr,
set_row_addr, and memory_write when applying the change.
In `@radio/src/boards/helloradio-h750/lcd_driver.h`:
- Around line 42-43: The typedef names contain typos: rename lcdSpiInitFucPtr to
lcdSpiInitFuncPtr and LcdReadIDFucPtr to LcdReadIDFuncPtr, then update all
corresponding extern declarations and any usage sites (function pointer
variables, casts, assignments) to use the corrected identifiers (look for
lcdSpiInitFucPtr and LcdReadIDFucPtr occurrences, plus the externs declared
nearby) so signatures remain the same but the names are fixed consistently
across header and implementation.
- Around line 25-32: The header currently duplicates timing constants (e.g.,
HBP, VBP, HSW, VSH, HFP, VFP) alongside alternate names like
HORIZONTAL_BACK_PORCH etc.; remove the duplicates by choosing one canonical
naming scheme and replacing all uses to reference that single set (for example
keep HORIZONTAL_BACK_PORCH or HBP consistently), delete the redundant defines,
and update any references in the lcd driver code to use the chosen symbols
(check uses of HBP, HORIZONTAL_BACK_PORCH, HSW, HORIZONTAL_SYNC_WIDTH, VBP,
VERTICAL_BACK_PORCH, etc.) so there is exactly one definition per timing
constant.
In `@radio/src/boards/helloradio-h750/touch_driver.h`:
- Around line 58-68: The Doxygen comment above the typedef is incorrect: replace
the misleading reference to ft6x06_handle_TypeDef with the correct typedef name
tc_handle_TypeDef (or remove the typedef name from the comment) so the doc
matches the actual struct; update the comment block that documents
tc_handle_TypeDef (the struct containing currActiveTouchNb and
currActiveTouchIdx) to use the correct identifier.
In `@radio/src/drivers/icm42627.cpp`:
- Around line 22-25: Top-of-file comment refers to ICM42607C but the driver
filename and runtime check expect a different device (the WHO_AM_I check
compares to 0x20 in icm42627.cpp), so update the comment to accurately list the
supported device(s) or explicitly document compatibility; specifically, edit the
header comment in icm42627.cpp to state that this driver targets ICM42627 (or
the exact device(s) whose WHO_AM_I == 0x20), and ensure any constants or the
probe/init routine that checks WHO_AM_I (the WHO_AM_I check comparing to 0x20)
and related identifiers are consistent with that documented device list.
In `@radio/src/gui/colorlcd/module/crossfire_settings.cpp`:
- Line 92: The line "moduleIdx = moduleIdx;" is a self-assignment no-op in
crossfire_settings.cpp; remove this statement or replace it with the intended
assignment to the member variable (e.g., this->moduleIdx = moduleIdx or assign
to the actual member name) inside the constructor or setter where it appears
(look for function/constructor names referencing moduleIdx in
crossfire_settings.cpp); ensure the correct target (member vs parameter) is used
so the parameter value is stored.
In `@radio/src/gui/colorlcd/radio/hw_intmodule.cpp`:
- Around line 72-89: Add the same left padding and reorder calls to match the
analogous block: call antline->padLeft(PAD_SMALL) after creating antline, and in
the Choice lambda's else branch set g_eeGeneral.antennaMode = antenna; then call
SET_DIRTY() before calling checkExternalAntenna(); keep the existing
confirmationDialog/isExternalAntennaEnabled logic unchanged so the symbols to
edit are antline, padLeft(PAD_SMALL), the Choice lambda,
g_eeGeneral.antennaMode, SET_DIRTY(), and checkExternalAntenna().
In `@radio/src/targets/common/arm/stm32/stm32_i2c_driver.h`:
- Around line 30-31: Update the stale comment above the enum that declares
I2C_Bus_1, I2C_Bus_2, I2C_Bus_3, I2C_Bus_4: fix the typo "Maxmimum" and change
the text to reflect that there are four I2C bus instances (e.g., "Maximum 4 I2C
bus instances" or similar) so the comment matches the enum values.
In `@radio/src/targets/common/arm/stm32/usb_driver.cpp`:
- Around line 132-143: The USB_GPIO_SEL initialization is incorrectly nested
inside the USB_GPIO_VBUS guard so targets that define USB_GPIO_SEL but not
USB_GPIO_VBUS never run gpio_init/gpio_set; extract the USB_GPIO_SEL block out
of the USB_GPIO_VBUS `#if` and create a separate conditional block: wrap the
gpio_init(USB_GPIO_SEL,...), gpio_set(USB_GPIO_SEL) (and optional gpio_clear)
with its own `#if` defined(USB_GPIO_SEL) ... `#endif` at the same level as the
USB_GPIO_VBUS handling so USB_GPIO_SEL is initialized regardless of
USB_GPIO_VBUS being defined.
In `@radio/src/targets/v12/CMakeLists.txt`:
- Around line 100-108: The CMakeLists contains a redundant block that re-adds
-DFUNCTION_SWITCHES: remove the second conditional that tests FUNCTION_SWITCHES
(the block with add_definitions(-DFUNCTION_SWITCHES) at the end) because
FUNCTION_SWITCHES is already set and defined when FUNCTION_SWITCHES_WITH_RGB is
true (the block that sets FUNCTION_SWITCHES YES and calls
add_definitions(-DFUNCTION_SWITCHES) and
add_definitions(-DFUNCTION_SWITCHES_RGB_LEDS)); simply delete the trailing
if(FUNCTION_SWITCHES)...endif() block so -DFUNCTION_SWITCHES is only added once
via FUNCTION_SWITCHES_WITH_RGB or via any other explicit upstream logic.
In `@radio/src/targets/v12/hal.h`:
- Line 120: Update the stale inline comment for the SWITCHES_GPIO_PIN_A macro:
in the definition of SWITCHES_GPIO_PIN_A (currently defined as LL_GPIO_PIN_8)
change the trailing comment from "PE.05" to the correct port/pin "PH.08" so the
comment accurately reflects GPIOH pin 8.
In `@tools/generate-yaml.sh`:
- Line 14: The default assignment for FLAVOR in tools/generate-yaml.sh uses an
unquoted RHS which can trigger pathname expansion; update the assignment for the
FLAVOR variable (the line containing : ${FLAVOR:=...}) to quote the right-hand
side string so the default value is quoted (e.g., change the RHS to a quoted
string) to prevent potential globbing and follow shell best practices.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fc29d28-d398-48d6-8c67-1d48dfe6fdcf
📒 Files selected for processing (109)
companion/src/firmwares/boardjson.cppcompanion/src/firmwares/boards.cppcompanion/src/firmwares/boards.hcompanion/src/firmwares/edgetx/yaml_moduledata.cppcompanion/src/firmwares/generalsettings.cppcompanion/src/firmwares/moduledata.hcompanion/src/firmwares/opentx/opentxinterface.cppcompanion/src/generaledit/hardware.cppcompanion/src/modeledit/setup_module.cppradio/src/CMakeLists.txtradio/src/boards/generic_stm32/i2c_bus.cppradio/src/boards/generic_stm32/rgb_6pos.cppradio/src/boards/helloradio-h750/backlight_driver.cppradio/src/boards/helloradio-h750/board.cppradio/src/boards/helloradio-h750/board.hradio/src/boards/helloradio-h750/extflash_driver.cppradio/src/boards/helloradio-h750/extflash_driver.hradio/src/boards/helloradio-h750/haptic_driver.cppradio/src/boards/helloradio-h750/key_driver.cppradio/src/boards/helloradio-h750/lcd_driver.cppradio/src/boards/helloradio-h750/lcd_driver.hradio/src/boards/helloradio-h750/sdram_driver.cppradio/src/boards/helloradio-h750/system_clock.cradio/src/boards/helloradio-h750/system_clock.hradio/src/boards/helloradio-h750/touch_driver.cppradio/src/boards/helloradio-h750/touch_driver.hradio/src/boards/helloradio-h750/usb_charger_driver.cppradio/src/datastructs.hradio/src/datastructs_private.hradio/src/drivers/icm42627.cppradio/src/drivers/icm42627.hradio/src/edgetx.cppradio/src/gui/128x64/model_setup.cppradio/src/gui/128x64/view_main.cppradio/src/gui/colorlcd/bitmaps.cppradio/src/gui/colorlcd/bitmaps.hradio/src/gui/colorlcd/module/crossfire_settings.cppradio/src/gui/colorlcd/module/pxx1_settings.cppradio/src/gui/colorlcd/radio/hw_intmodule.cppradio/src/gui/colorlcd/widgets/radio_info.cppradio/src/gui/common/stdlcd/radio_hardware.cppradio/src/gui/gui_common.cppradio/src/gui/gui_common.hradio/src/hal/rotary_encoder.hradio/src/pulses/modules_helpers.cppradio/src/storage/yaml/CMakeLists.txtradio/src/storage/yaml/yaml_datastructs.cppradio/src/storage/yaml/yaml_datastructs_128x64.cppradio/src/storage/yaml/yaml_datastructs_f16.cppradio/src/storage/yaml/yaml_datastructs_gx12.cppradio/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_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_tpro.cppradio/src/storage/yaml/yaml_datastructs_tx15.cppradio/src/storage/yaml/yaml_datastructs_tx16smk3.cppradio/src/storage/yaml/yaml_datastructs_v12.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/storage/yaml/yaml_datastructs_xlite.cppradio/src/storage/yaml/yaml_datastructs_xlites.cppradio/src/targets/common/arm/stm32/pwr_driver.cppradio/src/targets/common/arm/stm32/stm32_i2c_driver.cppradio/src/targets/common/arm/stm32/stm32_i2c_driver.hradio/src/targets/common/arm/stm32/usb_driver.cppradio/src/targets/horus/hal.hradio/src/targets/taranis/CMakeLists.txtradio/src/targets/taranis/board.hradio/src/targets/taranis/hal.hradio/src/targets/taranis/lcd_driver_spi.cppradio/src/targets/taranis/usb_descriptor.hradio/src/targets/v12/CMakeLists.txtradio/src/targets/v12/hal.hradio/src/targets/v12/usb_descriptor.hradio/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.hradio/util/build-firmware.pyradio/util/hw_defs/legacy_names.pyradio/util/hw_defs/switch_config.pytools/boards.pytools/build-common.shtools/generate-yaml.sh
💤 Files with no reviewable changes (3)
- radio/src/targets/taranis/usb_descriptor.h
- radio/src/targets/taranis/CMakeLists.txt
- radio/util/build-firmware.py
pfeerick
removed
the
needs: rebase
pfeerick
force-pushed
the
hrs-v12-2.12
branch
4 times, most recently
from
June 17, 2026 08:00
98c9f9c to
c2258ef
Compare
pfeerick
force-pushed
the
hrs-v12-2.12
branch
4 times, most recently
from
July 16, 2026 04:29
08ce7a4 to
7914010
Compare
The original preliminary HelloRadioSky V12/V14 support targeted a shared Taranis-family board (PCB=X7 PCBREV=V12, STM32F407) using the common monochrome SPI LCD driver. V12 shipped as its own STM32H750 colorlcd board with a dedicated targets/v12 tree instead, leaving this placeholder config and its RADIO_V12 conditionals in the shared taranis/ files dead. Co-authored-by: Richard Li <richard.li@ces.hk>
Squashed bring-up history for the HRS V12/V12p target: bootloader, LCD, keys/trims, USB, firmware boot, UI, charging/power indication, external antenna RF, and function switches support. Co-authored-by: Richard Li <richard.li@ces.hk> Co-authored-by: radiosky <839034809@qq.com> Co-authored-by: helloradiosky <839034809@qq.com>
Member
|
@coderabbitai review again |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
radio/src/edgetx.cpp (1)
1796-1796: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the typo in the comment.
The comment refers to
V15, but the macro check is forRADIO_V12.♻️ Proposed refactor
- // SYS+MDL are also used as power combo on V15. Prevent their menu BREAK + // SYS+MDL are also used as power combo on V12. Prevent their menu BREAK🤖 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/edgetx.cpp` at line 1796, Correct the version reference in the comment adjacent to the SYS+MDL power-combo handling so it says V12 instead of V15, matching the RADIO_V12 macro check. Do not alter the surrounding logic.radio/src/drivers/icm42670.cpp (1)
127-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap the entire bitwise expression in the cast.
Due to C operator precedence, the cast only applies to the shifted upper byte before the bitwise OR:
((int16_t)(buf[0] << 8)) | buf[1]. While this mathematically works out due to integer promotion and two's complement sign extension, it is non-idiomatic and relies on signed bitwise operations. Wrap the full expression in the cast to ensure the bitwise operations happen on unsigned values before being cast toint16_t.♻️ Proposed refactor
- data->gyro_x = -((int16_t)(buf[0] << 8) | buf[1]); - data->gyro_y = -((int16_t)(buf[2] << 8) | buf[3]); - data->gyro_z = ((int16_t)(buf[4] << 8) | buf[5]); + data->gyro_x = -(int16_t)((buf[0] << 8) | buf[1]); + data->gyro_y = -(int16_t)((buf[2] << 8) | buf[3]); + data->gyro_z = (int16_t)((buf[4] << 8) | buf[5]); if (i2c_read(s_i2c_bus, s_i2c_addr, ACCEL_DATA_X1_REG, 1, buf, 6) < 0) { TRACE("ICM42670 ERROR: accel read error"); return -1; } - data->accel_x = ((int16_t)(buf[0] << 8) | buf[1]); - data->accel_y = ((int16_t)(buf[2] << 8) | buf[3]); - data->accel_z = ((int16_t)(buf[4] << 8) | buf[5]); + data->accel_x = (int16_t)((buf[0] << 8) | buf[1]); + data->accel_y = (int16_t)((buf[2] << 8) | buf[3]); + data->accel_z = (int16_t)((buf[4] << 8) | buf[5]);🤖 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/drivers/icm42670.cpp` around lines 127 - 138, Update the gyro and accelerometer assignments in the ICM42670 data-read path so the complete 16-bit byte-combination expression is formed with unsigned operands and then cast once to int16_t. Apply this consistently to gyro_x, gyro_y, gyro_z, accel_x, accel_y, and accel_z without changing their existing sign inversions or axis mappings.
🤖 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.
Nitpick comments:
In `@radio/src/drivers/icm42670.cpp`:
- Around line 127-138: Update the gyro and accelerometer assignments in the
ICM42670 data-read path so the complete 16-bit byte-combination expression is
formed with unsigned operands and then cast once to int16_t. Apply this
consistently to gyro_x, gyro_y, gyro_z, accel_x, accel_y, and accel_z without
changing their existing sign inversions or axis mappings.
In `@radio/src/edgetx.cpp`:
- Line 1796: Correct the version reference in the comment adjacent to the
SYS+MDL power-combo handling so it says V12 instead of V15, matching the
RADIO_V12 macro check. Do not alter the surrounding logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85a4e69b-a826-4e95-933b-3c52b186e2e5
📒 Files selected for processing (47)
companion/src/firmwares/boardjson.cppcompanion/src/firmwares/boards.cppcompanion/src/firmwares/boards.hcompanion/src/firmwares/edgetx/yaml_moduledata.cppcompanion/src/firmwares/generalsettings.cppcompanion/src/firmwares/moduledata.hcompanion/src/firmwares/opentx/opentxinterface.cppcompanion/src/generaledit/hardware.cppcompanion/src/modeledit/setup_module.cppradio/src/CMakeLists.txtradio/src/boards/generic_stm32/i2c_bus.cppradio/src/boards/generic_stm32/rgb_6pos.cppradio/src/boards/helloradio-h750/backlight_driver.cppradio/src/boards/helloradio-h750/board.cppradio/src/boards/helloradio-h750/board.hradio/src/boards/helloradio-h750/extflash_driver.cppradio/src/boards/helloradio-h750/extflash_driver.hradio/src/boards/helloradio-h750/haptic_driver.cppradio/src/boards/helloradio-h750/key_driver.cppradio/src/boards/helloradio-h750/lcd_driver.cppradio/src/boards/helloradio-h750/lcd_driver.hradio/src/boards/helloradio-h750/sdram_driver.cppradio/src/boards/helloradio-h750/system_clock.cradio/src/boards/helloradio-h750/system_clock.hradio/src/boards/helloradio-h750/touch_driver.cppradio/src/boards/helloradio-h750/touch_driver.hradio/src/boards/helloradio-h750/usb_charger_driver.cppradio/src/datastructs.hradio/src/datastructs_private.hradio/src/drivers/icm42670.cppradio/src/drivers/icm42670.hradio/src/edgetx.cppradio/src/gui/128x64/model_setup.cppradio/src/gui/128x64/view_main.cppradio/src/gui/colorlcd/CMakeLists.txtradio/src/gui/colorlcd/bitmaps.cppradio/src/gui/colorlcd/bitmaps.hradio/src/gui/colorlcd/module/crossfire_settings.cppradio/src/gui/colorlcd/module/ext_antenna_settings.cppradio/src/gui/colorlcd/module/ext_antenna_settings.hradio/src/gui/colorlcd/module/module_setup.cppradio/src/gui/colorlcd/radio/hw_intmodule.cppradio/src/gui/colorlcd/widgets/radio_info.cppradio/src/gui/common/stdlcd/radio_hardware.cppradio/src/gui/gui_common.cppradio/src/gui/gui_common.hradio/src/hal/rotary_encoder.h
🚧 Files skipped from review as they are similar to previous changes (31)
- companion/src/firmwares/moduledata.h
- radio/src/boards/helloradio-h750/usb_charger_driver.cpp
- companion/src/firmwares/boardjson.cpp
- radio/src/boards/helloradio-h750/touch_driver.h
- radio/src/boards/helloradio-h750/system_clock.h
- radio/src/gui/colorlcd/bitmaps.h
- radio/src/boards/helloradio-h750/system_clock.c
- radio/src/boards/helloradio-h750/haptic_driver.cpp
- radio/src/boards/helloradio-h750/key_driver.cpp
- radio/src/boards/helloradio-h750/extflash_driver.h
- radio/src/hal/rotary_encoder.h
- radio/src/CMakeLists.txt
- radio/src/gui/128x64/view_main.cpp
- radio/src/gui/colorlcd/bitmaps.cpp
- companion/src/generaledit/hardware.cpp
- radio/src/gui/colorlcd/widgets/radio_info.cpp
- radio/src/boards/helloradio-h750/lcd_driver.cpp
- radio/src/boards/helloradio-h750/extflash_driver.cpp
- companion/src/firmwares/edgetx/yaml_moduledata.cpp
- radio/src/datastructs_private.h
- radio/src/boards/helloradio-h750/backlight_driver.cpp
- companion/src/firmwares/opentx/opentxinterface.cpp
- radio/src/boards/helloradio-h750/lcd_driver.h
- radio/src/boards/helloradio-h750/sdram_driver.cpp
- radio/src/boards/helloradio-h750/touch_driver.cpp
- companion/src/firmwares/boards.h
- radio/src/boards/helloradio-h750/board.h
- radio/src/gui/128x64/model_setup.cpp
- companion/src/firmwares/boards.cpp
- companion/src/modeledit/setup_module.cpp
- radio/src/boards/helloradio-h750/board.cpp
Wire IMU and 6POS ADC to post-rebase APIs, route SA-SD through the GPIO switch driver, and fix switch/pot/legacy-name settings for multipos support.
Add companion support and rework external antenna handling to be generic instead of PXX1-specific: clean out PXX1/XJT coupling, refactor antennaMode into a shared per-model setting, fix ADC/GPIO pin definitions, add native translations, and consolidate the external antenna mode UI into a shared helper.
…ypass it pwrPressed() is a generic "is a power button held" signal used across boot/charging loops and switch-state code, not the panic/force-shutdown gesture (that's pwrForcePressed()). Collapsing pwrPressed() to the dual-button check broke delay timing instead of gating the start condition, and also caused PWR_PRESS_SHUTDOWN_DELAY()'s existing pwrForcePressed() instant-off special case to fire on every normal press, collapsing the configured pwrOffSpeed delay to zero. Restore pwrPressed() to the plain either-button check, and instead add pwrDelayHoldActive() which requires both buttons on PWR_BUTTON_DUAL radios to start/continue the on/off delay countdown. Drop the instant-off special case for PWR_BUTTON_DUAL boards, since pressing both buttons is the normal way to start the shutdown delay there, not a separate panic chord layered on top of a single-button press. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(imu): ICM42670 was planned
…utton shutdown method (#7474)
The resistor ladder on PC.05 (ADC channel 8) drives function switches SE–SJ via sixPosUpdateFromAdc() and is not a user-facing pot. Renaming POT3 → RAW1 removes it from the flex input list so it no longer appears in the hardware config menu, calibration wizard, Lua mixer sources, or YAML legacy inputs, while keeping SIXPOS_SWITCH_INDEX = 6 valid and the ADC DMA reading unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Was hardcoded to false; now mirrors IS_EXTERNAL_MODULE_ON() by reading INTMODULE_PWR_GPIO, which INTERNAL_MODULE_ON/OFF() drive via gpio_set/clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sixPosUpdateFromAdc() only committed a new decoded ADC position when a button was actually pressed (current != 0), so releases were never recorded and the last-pressed switch kept reading DOWN forever. Since evalFunctionSwitches() only reacts to state edges, re-pressing that switch produced no edge and did nothing until a different button on the shared 6POS ladder was pressed, incidentally clearing the stuck bit. Now the switch-state bits always reflect the true debounced position, including "nothing pressed", while the position-indicator value keeps its old sticky-last-position behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pfeerick
added a commit
that referenced
this pull request
Jul 22, 2026
…ranis files The original preliminary HelloRadioSky V12/V14 support targeted a shared Taranis-family board (PCB=X7 PCBREV=V12, STM32F407) using the common monochrome SPI LCD driver. V12 shipped as its own STM32H750 colorlcd board with a dedicated targets/v12 tree instead (see following commits), leaving this placeholder's RADIO_V12 conditionals in the shared taranis/ hal.h and lcd_driver_spi.cpp dead - main never had the equivalent cleanup that 2.12's PR #7113 applied (commit 4d85c03). Remove them now, mirroring that commit. The CMakeLists.txt/board.h/usb_descriptor.h side of this removal already happened when this branch's stale monochrome PCBREV=V12 placeholder was replaced by the real board's own targets/v12/ tree. Co-authored-by: Richard Li <richard.li@ces.hk>
pfeerick
added a commit
that referenced
this pull request
Jul 22, 2026
…n, disable IMU Ports the real HelloRadioSky V12 (STM32H750, 320x240 16bpp color LCD) board bring-up from the 2.12 branch (where PR #7113 landed) onto main's JSON-driven hardware description pattern (PR #7411 "chore: use settings from radio JSON files instead of hal.h"), replacing the stale monochrome F407 placeholder removed in the previous commit. - Author boards/hw_defs/v12.json (real 320x240 16bpp STM32H750 color board), modeled on t22.json (closest sibling HRS H750 board) with V12's actual pin/DMA facts from 2.12's targets/v12/hal.h and the helloradiosky reference branch. - Trim targets/v12/hal.h down to facts not covered by the generated hal_settings.h (USB, telemetry, LCD SPI+DMA, QSPI, I2C, touch, intmodule/extmodule, trainer port), following the pl18/t22 pattern. - Add targets/v12/CMakeLists.txt (stm32_rgbleds.cpp, pca95xx.cpp, HARDWARE_TOUCH) and targets/v12/usb_descriptor.h, and set IMU OFF: 2.12's own v12/CMakeLists.txt marks IMU "not populated in final" and ships the icm42670 driver unused; this port follows that and drops the old-PR-branch icm42627.cpp wiring, which collided with main's unrelated V16 icm42627 driver (#7482). - Add the full boards/helloradio-h750/ driver set (LCD, touch, backlight, haptic, key, SDRAM, system clock, USB charger, ext flash), ported off the retired stm32_ws2812 API onto the current boards/generic_stm32/ rgb_leds.h API (rgbSetLedColor/rgbLedClearAll/rgbLedColorApply). - sixPosUpdateFromAdc(): 2.12 replaced the old getSixPosAnalogValue() mechanism with a weak-symbol hook in rgb_6pos.cpp overridden by board.cpp, driving the sticky 6POS-to-function-switch RGB mapping ("Fixed function switch" in 2.12's history) - ported verbatim. - v12.json: reclassify the third ADC input (6POS raw analog reading) as RAW instead of FLEX/MULTIPOS, hiding it from the pot UI (V12 only has 2 real pots), and correct switch defaults to match 2.12's final tuning (SA 2POS, SD TOGGLE, SE-SJ 2POS as custom switches). - legacy_names.py: split V12 out of the shared {v12, v14, v14lcd} group into its own entry (2 pots, not 3), matching the pot-count correction. - Wire I2C_B4 (V12's touch/IMU bus) into the shared i2c_bus.cpp and stm32_i2c_driver, restore INTMODULE_ANTSEL_GPIO/USB_GPIO_SEL init in pwr_driver.cpp/usb_driver.cpp, and the RADIO_V12 dual-switch pwrPressed() branch. - Guard MIXSRC_TILT_X/Y in yaml_datastructs_v12.cpp behind defined(IMU), and add CHKSIZE(ModelData, 7640) for V12's actual struct layout. - Register the v12 flavour in radio/src/CMakeLists.txt, tools/boards.py, radio/util/build-firmware.py, tools/build-common.sh, tools/generate-yaml.sh, and radio/src/storage/yaml/CMakeLists.txt so yaml_datastructs_v12.cpp is actually compiled in. - Remove leftover duplicate antenna-selection code the stale PR branch had bolted on next to the generalized paths in several more places (edgetx.cpp double-calling checkExternalAntenna(), a third hand-rolled antenna Choice widget in crossfire_settings.cpp, duplicate icon guards in bitmaps.cpp/.h), and restore two unrelated main improvements the stale branch had silently reverted by predating them (rgb_6pos.cpp weak-symbol pattern already covered above; keysPollingCycle()'s KEY_ACTIVITY_KEYS/KEY_ACTIVITY_TRIMS bitmask API had regressed to a plain bool). Firmware now builds cleanly for -DPCB=V12 and produces a real 320x240/16bpp color firmware.bin/uf2 (STM32H750), not the old monochrome image. Co-authored-by: radiosky <839034809@qq.com> Co-authored-by: helloradiosky <839034809@qq.com> Co-authored-by: Richard Li <richard.li@ces.hk>
pfeerick
added a commit
that referenced
this pull request
Jul 22, 2026
Port PR #7113's antenna-selection generalization from the 2.12 branch onto main, replacing the mock-up that the stale hrs-v12-main PR branch had bolted on with #if defined(RADIO_V12) special cases next to the existing #if defined(INTERNAL_MODULE_PXX1) code path (creating, on V12, either dead code or duplicate antenna Choice widgets depending on the call site). - datastructs_private.h: move ModuleData::antennaMode out of the pxx union sub-struct to the ModuleData top level, guarded by defined(EXTERNAL_ANTENNA) instead of being PXX1-only. Same bit layout (subType shrinks 4->2 bits to make room), so no struct-size change for boards without EXTERNAL_ANTENNA. - gui_common.cpp/.h: single generalized checkExternalAntenna() driven by defined(INTMODULE_ANTSEL_GPIO) (V12's hardware antenna-select GPIO) vs the existing non-GPIO confirmation-popup path, plus a new shared setAntennaModeWithConfirm() helper used by every antenna-mode Choice widget (radio-level and per-model) so the GPIO-apply step can't be missed at just one call site. - Rename pxx1_settings.{cpp,h} -> ext_antenna_settings.{cpp,h} (PXX1AntennaSettings -> ExtAntennaSettings), and gate its CMake compilation on `INTERNAL_MODULE_PXX1 OR EXTERNAL_ANTENNA` (main doesn't expose EXTERNAL_ANTENNA as a CMake variable outside V12, so dropping the PXX1 half of that condition, as 2.12 does, would have silently stopped compiling this file for every existing PXX1 board). - Port the same generalization through module_setup.cpp, hw_intmodule.cpp, radio_info.cpp (colorlcd), radio_hardware.cpp (stdlcd/128x64), 128x64/model_setup.cpp, 128x64/view_main.cpp, and modules_helpers.cpp's isExternalAntennaEnabled(). - Regenerate yaml_datastructs_*.cpp for every board via tools/generate-yaml.sh (full FLAVOR list, not just v12) to confirm the antennaMode field move is bit-layout-neutral everywhere (verified x9d/x10/v12 diffs: non-EXTERNAL_ANTENNA boards get a same-size padding field, EXTERNAL_ANTENNA boards get the field moved with identical total size). Firmware for -DPCB=V12 still builds cleanly after this port.
pfeerick
added a commit
that referenced
this pull request
Jul 22, 2026
…n, disable IMU Ports the real HelloRadioSky V12 (STM32H750, 320x240 16bpp color LCD) board bring-up from the 2.12 branch (where PR #7113 landed) onto main's JSON-driven hardware description pattern (PR #7411 "chore: use settings from radio JSON files instead of hal.h"), replacing the stale monochrome F407 placeholder removed in the previous commit. - Author boards/hw_defs/v12.json (real 320x240 16bpp STM32H750 color board), modeled on t22.json (closest sibling HRS H750 board) with V12's actual pin/DMA facts from 2.12's targets/v12/hal.h and the helloradiosky reference branch. - Trim targets/v12/hal.h down to facts not covered by the generated hal_settings.h (USB, telemetry, LCD SPI+DMA, QSPI, I2C, touch, intmodule/extmodule, trainer port), following the pl18/t22 pattern. - Add targets/v12/CMakeLists.txt (stm32_rgbleds.cpp, pca95xx.cpp, HARDWARE_TOUCH) and targets/v12/usb_descriptor.h, and set IMU OFF: 2.12's own v12/CMakeLists.txt marks IMU "not populated in final" and ships the icm42670 driver unused; this port follows that and drops the old-PR-branch icm42627.cpp wiring, which collided with main's unrelated V16 icm42627 driver (#7482). - Add the full boards/helloradio-h750/ driver set (LCD, touch, backlight, haptic, key, SDRAM, system clock, USB charger, ext flash), ported off the retired stm32_ws2812 API onto the current boards/generic_stm32/ rgb_leds.h API (rgbSetLedColor/rgbLedClearAll/rgbLedColorApply). - sixPosUpdateFromAdc(): 2.12 replaced the old getSixPosAnalogValue() mechanism with a weak-symbol hook in rgb_6pos.cpp overridden by board.cpp, driving the sticky 6POS-to-function-switch RGB mapping ("Fixed function switch" in 2.12's history) - ported verbatim. - v12.json: reclassify the third ADC input (6POS raw analog reading) as RAW instead of FLEX/MULTIPOS, hiding it from the pot UI (V12 only has 2 real pots), and correct switch defaults to match 2.12's final tuning (SA 2POS, SD TOGGLE, SE-SJ 2POS as custom switches). - legacy_names.py: split V12 out of the shared {v12, v14, v14lcd} group into its own entry (2 pots, not 3), matching the pot-count correction. - Wire I2C_B4 (V12's touch/IMU bus) into the shared i2c_bus.cpp and stm32_i2c_driver, restore INTMODULE_ANTSEL_GPIO/USB_GPIO_SEL init in pwr_driver.cpp/usb_driver.cpp, and the RADIO_V12 dual-switch pwrPressed() branch. - Guard MIXSRC_TILT_X/Y in yaml_datastructs_v12.cpp behind defined(IMU), and add CHKSIZE(ModelData, 7640) for V12's actual struct layout. - Register the v12 flavour in radio/src/CMakeLists.txt, tools/boards.py, radio/util/build-firmware.py, tools/build-common.sh, tools/generate-yaml.sh, and radio/src/storage/yaml/CMakeLists.txt so yaml_datastructs_v12.cpp is actually compiled in. - Remove leftover duplicate antenna-selection code the stale PR branch had bolted on next to the generalized paths in several more places (edgetx.cpp double-calling checkExternalAntenna(), a third hand-rolled antenna Choice widget in crossfire_settings.cpp, duplicate icon guards in bitmaps.cpp/.h), and restore two unrelated main improvements the stale branch had silently reverted by predating them (rgb_6pos.cpp weak-symbol pattern already covered above; keysPollingCycle()'s KEY_ACTIVITY_KEYS/KEY_ACTIVITY_TRIMS bitmask API had regressed to a plain bool). Firmware now builds cleanly for -DPCB=V12 and produces a real 320x240/16bpp color firmware.bin/uf2 (STM32H750), not the old monochrome image. Co-authored-by: radiosky <839034809@qq.com> Co-authored-by: helloradiosky <839034809@qq.com> Co-authored-by: Richard Li <richard.li@ces.hk>
pfeerick
added a commit
that referenced
this pull request
Jul 22, 2026
Port PR #7113's antenna-selection generalization from the 2.12 branch onto main, replacing the mock-up that the stale hrs-v12-main PR branch had bolted on with #if defined(RADIO_V12) special cases next to the existing #if defined(INTERNAL_MODULE_PXX1) code path (creating, on V12, either dead code or duplicate antenna Choice widgets depending on the call site). - datastructs_private.h: move ModuleData::antennaMode out of the pxx union sub-struct to the ModuleData top level, guarded by defined(EXTERNAL_ANTENNA) instead of being PXX1-only. Same bit layout (subType shrinks 4->2 bits to make room), so no struct-size change for boards without EXTERNAL_ANTENNA. - gui_common.cpp/.h: single generalized checkExternalAntenna() driven by defined(INTMODULE_ANTSEL_GPIO) (V12's hardware antenna-select GPIO) vs the existing non-GPIO confirmation-popup path, plus a new shared setAntennaModeWithConfirm() helper used by every antenna-mode Choice widget (radio-level and per-model) so the GPIO-apply step can't be missed at just one call site. - Rename pxx1_settings.{cpp,h} -> ext_antenna_settings.{cpp,h} (PXX1AntennaSettings -> ExtAntennaSettings), and gate its CMake compilation on `INTERNAL_MODULE_PXX1 OR EXTERNAL_ANTENNA` (main doesn't expose EXTERNAL_ANTENNA as a CMake variable outside V12, so dropping the PXX1 half of that condition, as 2.12 does, would have silently stopped compiling this file for every existing PXX1 board). - Port the same generalization through module_setup.cpp, hw_intmodule.cpp, radio_info.cpp (colorlcd), radio_hardware.cpp (stdlcd/128x64), 128x64/model_setup.cpp, 128x64/view_main.cpp, and modules_helpers.cpp's isExternalAntennaEnabled(). - Regenerate yaml_datastructs_*.cpp for every board via tools/generate-yaml.sh (full FLAVOR list, not just v12) to confirm the antennaMode field move is bit-layout-neutral everywhere (verified x9d/x10/v12 diffs: non-EXTERNAL_ANTENNA boards get a same-size padding field, EXTERNAL_ANTENNA boards get the field moved with identical total size). Firmware for -DPCB=V12 still builds cleanly after this port.
12 tasks done
pfeerick
added a commit
that referenced
this pull request
Aug 21, 2026
) Upstream regenerated t15pro and tx15, the two descriptors the leak had corrupted on main. On 2.12 a different file is affected, so it is regenerated here in the same commit: yaml_datastructs_x10.cpp had YAML_PADDING(4) where ModuleData.antennaMode belongs. That field was lost by 9506ec7 ("feat(v12): support for HelloRadioSky V12" (#7113)), which regenerated the descriptors through the shared build tree this commit replaces - v12's cached options leaked into x10's generation and dropped the field. main never saw it: its own v12 support (#7114) regenerated cleanly and has carried antennaMode throughout. yaml_datastructs_x10.cpp is used by x10, x10express, tx16s, v16 and x12s, so on all of those the per-module antenna selection was silently neither written to nor read from model files, reverting to the default on every load. Padding accounting is unchanged - PADDING(4) becomes PADDING(2) plus a 2-bit antennaMode - so no following field shifts and existing model files stay readable. Files written before this fix carry no antennaMode key and load with the default, which is what they did already. Regenerating all 25 targets with the fixed script leaves the other 24 descriptors byte-identical, confirming x10 was the only casualty on 2.12. The tools/generate-yaml.sh resolution keeps 2.12's FLAVOR list, which includes gx15 and omits tx16s, and takes main's per-target BUILD_DIR restructure. The requirements.txt hunk is dropped - asciitree is already installed by tools/setup_buildenv_ubuntu24.04.sh on 2.12. Co-authored-by: philmoz <phil.a.mitchell@gmail.com> Co-authored-by: Peter Feerick <5500713+pfeerick@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 5c64f0a)
pfeerick
added a commit
that referenced
this pull request
Aug 23, 2026
) Upstream regenerated t15pro and tx15, the two descriptors the leak had corrupted on main. On 2.12 a different file is affected, so it is regenerated here in the same commit: yaml_datastructs_x10.cpp had YAML_PADDING(4) where ModuleData.antennaMode belongs. That field was lost by 9506ec7 ("feat(v12): support for HelloRadioSky V12" (#7113)), which regenerated the descriptors through the shared build tree this commit replaces - v12's cached options leaked into x10's generation and dropped the field. main never saw it: its own v12 support (#7114) regenerated cleanly and has carried antennaMode throughout. yaml_datastructs_x10.cpp is used by x10, x10express, tx16s, v16 and x12s, so on all of those the per-module antenna selection was silently neither written to nor read from model files, reverting to the default on every load. Padding accounting is unchanged - PADDING(4) becomes PADDING(2) plus a 2-bit antennaMode - so no following field shifts and existing model files stay readable. Files written before this fix carry no antennaMode key and load with the default, which is what they did already. Regenerating all 25 targets with the fixed script leaves the other 24 descriptors byte-identical, confirming x10 was the only casualty on 2.12. The tools/generate-yaml.sh resolution keeps 2.12's FLAVOR list, which includes gx15 and omits tx16s, and takes main's per-target BUILD_DIR restructure. The requirements.txt hunk is dropped - asciitree is already installed by tools/setup_buildenv_ubuntu24.04.sh on 2.12. Co-authored-by: philmoz <phil.a.mitchell@gmail.com> Co-authored-by: Peter Feerick <5500713+pfeerick@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 5c64f0a)
pfeerick
added a commit
that referenced
this pull request
Aug 28, 2026
) Upstream regenerated t15pro and tx15, the two descriptors the leak had corrupted on main. On 2.12 a different file is affected, so it is regenerated here in the same commit: yaml_datastructs_x10.cpp had YAML_PADDING(4) where ModuleData.antennaMode belongs. That field was lost by 9506ec7 ("feat(v12): support for HelloRadioSky V12" (#7113)), which regenerated the descriptors through the shared build tree this commit replaces - v12's cached options leaked into x10's generation and dropped the field. main never saw it: its own v12 support (#7114) regenerated cleanly and has carried antennaMode throughout. yaml_datastructs_x10.cpp is used by x10, x10express, tx16s, v16 and x12s, so on all of those the per-module antenna selection was silently neither written to nor read from model files, reverting to the default on every load. Padding accounting is unchanged - PADDING(4) becomes PADDING(2) plus a 2-bit antennaMode - so no following field shifts and existing model files stay readable. Files written before this fix carry no antennaMode key and load with the default, which is what they did already. Regenerating all 25 targets with the fixed script leaves the other 24 descriptors byte-identical, confirming x10 was the only casualty on 2.12. The tools/generate-yaml.sh resolution keeps 2.12's FLAVOR list, which includes gx15 and omits tx16s, and takes main's per-target BUILD_DIR restructure. The requirements.txt hunk is dropped - asciitree is already installed by tools/setup_buildenv_ubuntu24.04.sh on 2.12. Co-authored-by: philmoz <phil.a.mitchell@gmail.com> Co-authored-by: Peter Feerick <5500713+pfeerick@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 5c64f0a)
This was referenced Aug 31, 2026
Merged
This was referenced Sep 4, 2026
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
4 participants
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.
Summary of changes:
Note: Due to the external antenna support refactor, models on X10/X12 (the only other radios that had external antenna support) that were configured as internal or external will default to ask after upgrading to this firmware. If Companion is used as part of migration from a older firmware version, it will carry over the settings.
Summary by CodeRabbit
New Features
Bug Fixes