chore: use settings from radio JSON files instead of hal.h#7411
chore: use settings from radio JSON files instead of hal.h#7411philmoz wants to merge 4 commits into
Conversation
philmoz
added
compilation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds JSON schema/models and Jinja template to generate hal_settings.h from board JSON. Refactors CMake to centralize generation and dependencies. Migrates numerous board JSONs to new lcd/backlight/leds/timers schema. Updates companion parsing, headers, IRQ macro names, drivers, Lua/YAML/Simu wiring, and GUI gating. ChangesHardware definition generation and integration
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/CMake
participant Schema as hwdef_schema.json
participant Gen as generator.py
participant Tmpl as hal_settings.jinja
participant Build as Targets (board, stm32_drivers)
participant Code as Firmware/Headers
Dev->>Build: Configure AddHWGenTarget(${FLAVOUR}.json → hal_settings.h)
Dev->>Schema: Validate board JSON
Build->>Gen: Run generator with validated JSON
Gen->>Tmpl: Render with display/leds/backlight/timers
Tmpl-->>Build: hal_settings.h generated
Build->>Build: add_dependencies(... hal_settings)
Build->>Code: Compile with new macros/IRQ names
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
radio/util/hw_defs/hal_settings.jinja (1)
10-45: 🏗️ Heavy liftBoolean HAL feature macros expand to
True/False, but current usage isdefined(...)-gated
hal_settings.jinjaemits#define OLED_SCREEN True/False-style values for boolean HAL fields, and the current codebase referencesOLED_SCREEN,HAS_BACKLIGHT_COLOR,STATUS_LEDS,LCD_HORIZONTAL_INVERT, andLCD_VERTICAL_INVERTvia#if defined(...)/#if !defined(...)(so theTrue/Falsenumeric-expression risk doesn’t show up in current consumers).
For extra robustness/future-proofing, consider switching to value-less#definefortrue(skipfalse) or emit1/0instead of PythonTrue/False.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/util/hw_defs/hal_settings.jinja` around lines 10 - 45, The template currently emits Python True/False text for boolean HAL fields which can break consumers using `#if` defined(...) checks; update the hal_settings.jinja generation so booleans are robust: either (preferred) emit value-less defines only when true (e.g., if val is True then output "`#define` OLED_SCREEN" and skip when False) or consistently emit "1"/"0" integers (e.g., "`#define` OLED_SCREEN 1" or "0"); apply this change to the boolean symbols referenced in the codebase such as OLED_SCREEN, HAS_BACKLIGHT_COLOR, STATUS_LEDS, LCD_HORIZONTAL_INVERT, and LCD_VERTICAL_INVERT by testing val's truthiness and emitting the appropriate form.
🤖 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 1229-1235: The loader currently populates display fields but omits
the new lcd_vertical_invert flag; add parsing for
o.value("lcd_vertical_invert").toBool() and assign it to the display struct
(e.g. set display.vertical_invert or display.lcd_vertical_invert depending on
the existing field name in the Display struct) alongside the other assignments
(display.w, display.h, display.phys_w, display.phys_h, display.depth,
display.color, display.oled) so the companion stays in sync with the migrated
board schema.
- Around line 1247-1250: The assignment to cfs_led_strip_length is wrong: it's
storing a boolean (toInt() > 0) instead of the actual length, causing cfs.groups
to be computed as 0; change the line that defines cfs_led_strip_length to
capture the integer length (int cfs_led_strip_length =
o.value("cfs_led_strip_length").toInt();) and keep the existing logic that
computes cfs.groups using cfs_led_strip_length and cfs_leds_per_switch, ensuring
cfs.rgb_led is set from cfs.groups as before.
In `@radio/src/boards/hw_defs/pa01.json`:
- Line 306: The led_strip_gpio value uses an octal-style integer literal; change
the pin argument in the JSON from "GPIO_PIN(GPIOF, 06)" to use a decimal literal
(e.g., "GPIO_PIN(GPIOF, 6)") so GPIO_PIN/ GPIOF evaluation is consistent with
other entries and avoids octal parsing issues; update the entry named
led_strip_gpio accordingly.
In `@radio/src/boards/hw_defs/tx15.json`:
- Around line 337-340: The backlight timer is TIM1 so the backlight_timer_freq
must use the APB2 timer domain instead of the APB1 expression; update the value
of backlight_timer_freq (currently "(PERI1_FREQUENCY * TIMER_MULT_APB1)") to use
the APB2 clock symbols (e.g. "PERI2_FREQUENCY * TIMER_MULT_APB2") so TIM1
references the correct APB2 timer frequency.
In `@radio/src/boards/hw_defs/tx16smk3.json`:
- Around line 451-454: The backlight timer TIM1 is an APB2 peripheral but the
JSON uses the APB1 clock expression; update the backlight_timer_freq value to
use the APB2 timer clock expression instead of "(PERI1_FREQUENCY *
TIMER_MULT_APB1)". Replace that string with the equivalent APB2 expression (e.g.
use PERI2_FREQUENCY and TIMER_MULT_APB2) so backlight_timer = "TIM1" has
backlight_timer_freq matching APB2.
In `@radio/src/boards/hw_defs/x9d.json`:
- Around line 353-358: The hardware definition for X9D is missing the
has_int_module_support flag which causes generated settings to omit built-in RF
module support; add "has_int_module_support": true to the "hardware" object in
x9d.json (alongside has_ext_module_support, sport_max_baudrate, cpu, cpu_type)
so the target correctly recognizes internal-module support.
In `@radio/src/boards/hw_defs/x9d`+.json:
- Around line 367-372: The hardware block for X9D+ removed the internal-module
flag which alters generated capabilities; restore the internal module support by
adding "has_int_module_support": true back into the same "hardware" object (next
to "has_ext_module_support") so X9D+ retains its built-in RF/internal module
paths.
In `@radio/src/boards/hw_defs/x9d`+2019.json:
- Around line 361-366: The hardware definition for the X9D+ 2019 JSON removed
the internal module flag; restore the "has_int_module_support" property in the
"hardware" object of x9d+2019.json (add "has_int_module_support": true) so the
generated capabilities include internal RF support for the X9D+ 2019 target.
In `@radio/src/boards/hw_defs/x9e.json`:
- Around line 559-564: The hardware block for X9E omitted the internal-module
flag, which breaks generated capabilities; update the "hardware" object in
x9e.json (where "has_ext_module_support", "sport_max_baudrate", "cpu", and
"cpu_type" are defined) to include "has_int_module_support": true so the target
correctly advertises its built-in RF hardware and internal-module functionality
is preserved.
---
Nitpick comments:
In `@radio/util/hw_defs/hal_settings.jinja`:
- Around line 10-45: The template currently emits Python True/False text for
boolean HAL fields which can break consumers using `#if` defined(...) checks;
update the hal_settings.jinja generation so booleans are robust: either
(preferred) emit value-less defines only when true (e.g., if val is True then
output "`#define` OLED_SCREEN" and skip when False) or consistently emit "1"/"0"
integers (e.g., "`#define` OLED_SCREEN 1" or "0"); apply this change to the
boolean symbols referenced in the codebase such as OLED_SCREEN,
HAS_BACKLIGHT_COLOR, STATUS_LEDS, LCD_HORIZONTAL_INVERT, and LCD_VERTICAL_INVERT
by testing val's truthiness and emitting the appropriate form.
🪄 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: f8c01218-433e-443b-a2e7-e38db07e594c
📒 Files selected for processing (82)
cmake/Macros.cmakecompanion/src/firmwares/boardjson.cppradio/src/CMakeLists.txtradio/src/boards/generic_stm32/CMakeLists.txtradio/src/boards/generic_stm32/rgb_leds.cppradio/src/boards/hw_defs/boxer.jsonradio/src/boards/hw_defs/bumblebee.jsonradio/src/boards/hw_defs/commando8.jsonradio/src/boards/hw_defs/el18.jsonradio/src/boards/hw_defs/f16.jsonradio/src/boards/hw_defs/gx12.jsonradio/src/boards/hw_defs/lr3pro.jsonradio/src/boards/hw_defs/mt12.jsonradio/src/boards/hw_defs/nb4p.jsonradio/src/boards/hw_defs/nv14.jsonradio/src/boards/hw_defs/pa01.jsonradio/src/boards/hw_defs/pl18.jsonradio/src/boards/hw_defs/pl18ev.jsonradio/src/boards/hw_defs/pl18u.jsonradio/src/boards/hw_defs/pocket.jsonradio/src/boards/hw_defs/st16.jsonradio/src/boards/hw_defs/t12.jsonradio/src/boards/hw_defs/t12max.jsonradio/src/boards/hw_defs/t14.jsonradio/src/boards/hw_defs/t15.jsonradio/src/boards/hw_defs/t15pro.jsonradio/src/boards/hw_defs/t16.jsonradio/src/boards/hw_defs/t18.jsonradio/src/boards/hw_defs/t20.jsonradio/src/boards/hw_defs/t20v2.jsonradio/src/boards/hw_defs/t8.jsonradio/src/boards/hw_defs/tlite.jsonradio/src/boards/hw_defs/tpro.jsonradio/src/boards/hw_defs/tpros.jsonradio/src/boards/hw_defs/tprov2.jsonradio/src/boards/hw_defs/tx12.jsonradio/src/boards/hw_defs/tx12mk2.jsonradio/src/boards/hw_defs/tx15.jsonradio/src/boards/hw_defs/tx16s.jsonradio/src/boards/hw_defs/tx16smk3.jsonradio/src/boards/hw_defs/v12.jsonradio/src/boards/hw_defs/v14.jsonradio/src/boards/hw_defs/v16.jsonradio/src/boards/hw_defs/x10.jsonradio/src/boards/hw_defs/x10express.jsonradio/src/boards/hw_defs/x12s.jsonradio/src/boards/hw_defs/x7.jsonradio/src/boards/hw_defs/x7access.jsonradio/src/boards/hw_defs/x9d+.jsonradio/src/boards/hw_defs/x9d+2019.jsonradio/src/boards/hw_defs/x9d.jsonradio/src/boards/hw_defs/x9e.jsonradio/src/boards/hw_defs/x9lite.jsonradio/src/boards/hw_defs/x9lites.jsonradio/src/boards/hw_defs/xlite.jsonradio/src/boards/hw_defs/xlites.jsonradio/src/boards/hw_defs/zorro.jsonradio/src/bootloader/CMakeLists.txtradio/src/gui/212x64/radio_setup.cppradio/src/gui/common/stdlcd/feature_control.hradio/src/hal/CMakeLists.txtradio/src/lua/CMakeLists.txtradio/src/lua/api_general.cppradio/src/storage/yaml/CMakeLists.txtradio/src/targets/common/arm/stm32/CMakeLists.txtradio/src/targets/common/arm/stm32/mixer_scheduler_driver.cppradio/src/targets/common/arm/stm32/timers_driver.cppradio/src/targets/horus/hal.hradio/src/targets/pa01/hal.hradio/src/targets/pl18/CMakeLists.txtradio/src/targets/pl18/hal.hradio/src/targets/simu/CMakeLists.txtradio/src/targets/st16/hal.hradio/src/targets/stm32h7s78-dk/hal.hradio/src/targets/t15pro/hal.hradio/src/targets/taranis/CMakeLists.txtradio/src/targets/taranis/hal.hradio/src/targets/tx15/hal.hradio/src/targets/tx16smk3/hal.hradio/util/hw_defs/generator.pyradio/util/hw_defs/hal_settings.jinjaradio/util/hw_defs/models.py
💤 Files with no reviewable changes (7)
- radio/src/hal/CMakeLists.txt
- radio/src/targets/pl18/CMakeLists.txt
- radio/src/gui/common/stdlcd/feature_control.h
- radio/src/targets/taranis/CMakeLists.txt
- radio/src/lua/CMakeLists.txt
- radio/src/storage/yaml/CMakeLists.txt
- radio/src/bootloader/CMakeLists.txt
| display.w = o.value("lcd_w").toInt(); | ||
| display.h = o.value("lcd_h").toInt(); | ||
| display.phys_w = o.value("lcd_phys_w").toInt(); | ||
| display.phys_h = o.value("lcd_phys_h").toInt(); | ||
| display.depth = o.value("lcd_depth").toInt(); | ||
| display.color = display.depth == 16 ? 1 : 0; | ||
| display.oled = o.value("oled_screen").toBool(); |
There was a problem hiding this comment.
Parse lcd_vertical_invert with the rest of the display schema.
zorro.json now defines this flag, but this loader still drops it. That leaves companion out of sync with the migrated board schema for vertically inverted LCDs.
🤖 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/boardjson.cpp` around lines 1229 - 1235, The loader
currently populates display fields but omits the new lcd_vertical_invert flag;
add parsing for o.value("lcd_vertical_invert").toBool() and assign it to the
display struct (e.g. set display.vertical_invert or display.lcd_vertical_invert
depending on the existing field name in the Display struct) alongside the other
assignments (display.w, display.h, display.phys_w, display.phys_h,
display.depth, display.color, display.oled) so the companion stays in sync with
the migrated board schema.
There was a problem hiding this comment.
Irrelevant. Not used by companion.
There was a problem hiding this comment.
@philmoz, understood — since lcd_vertical_invert has no use in the companion, there's no need to parse it. Thanks for the clarification.
| "backlight_timer": "TIM1", | ||
| "backlight_timer_channel": "LL_TIM_CHANNEL_CH3", | ||
| "backlight_gpio_af": "GPIO_AF1", | ||
| "backlight_timer_freq": "(PERI1_FREQUENCY * TIMER_MULT_APB1)" |
There was a problem hiding this comment.
Use the APB2 clock expression for the TIM1 backlight timer.
backlight_timer is TIM1, so backlight_timer_freq should follow the APB2 timer domain, not APB1. This happens to work today because both domains are equal here, but it bakes in the wrong dependency.
Proposed fix
"backlight": {
"backlight_gpio": "GPIO_PIN(GPIOA, 10)",
"backlight_timer": "TIM1",
"backlight_timer_channel": "LL_TIM_CHANNEL_CH3",
"backlight_gpio_af": "GPIO_AF1",
- "backlight_timer_freq": "(PERI1_FREQUENCY * TIMER_MULT_APB1)"
+ "backlight_timer_freq": "(PERI2_FREQUENCY * TIMER_MULT_APB2)"
},🤖 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/hw_defs/tx15.json` around lines 337 - 340, The backlight
timer is TIM1 so the backlight_timer_freq must use the APB2 timer domain instead
of the APB1 expression; update the value of backlight_timer_freq (currently
"(PERI1_FREQUENCY * TIMER_MULT_APB1)") to use the APB2 clock symbols (e.g.
"PERI2_FREQUENCY * TIMER_MULT_APB2") so TIM1 references the correct APB2 timer
frequency.
There was a problem hiding this comment.
Will not change. Copied existing value from hal.h.
There was a problem hiding this comment.
Understood — preserving the value from hal.h is reasonable for this migration. Noted that on this board both APB1 and APB2 run at the same frequency, so there's no functional difference.
| "backlight_timer": "TIM1", | ||
| "backlight_timer_channel": "LL_TIM_CHANNEL_CH3", | ||
| "backlight_gpio_af": "GPIO_AF1", | ||
| "backlight_timer_freq": "(PERI1_FREQUENCY * TIMER_MULT_APB1)" |
There was a problem hiding this comment.
Use the APB2 clock expression for the TIM1 backlight timer.
TIM1 should be driven from the APB2 timer clock. Keeping the APB1 expression here makes this board definition drift-prone if those clock settings ever stop matching.
Proposed fix
"backlight": {
"backlight_gpio": "GPIO_PIN(GPIOA, 10)",
"backlight_timer": "TIM1",
"backlight_timer_channel": "LL_TIM_CHANNEL_CH3",
"backlight_gpio_af": "GPIO_AF1",
- "backlight_timer_freq": "(PERI1_FREQUENCY * TIMER_MULT_APB1)"
+ "backlight_timer_freq": "(PERI2_FREQUENCY * TIMER_MULT_APB2)"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "backlight_timer": "TIM1", | |
| "backlight_timer_channel": "LL_TIM_CHANNEL_CH3", | |
| "backlight_gpio_af": "GPIO_AF1", | |
| "backlight_timer_freq": "(PERI1_FREQUENCY * TIMER_MULT_APB1)" | |
| "backlight_timer": "TIM1", | |
| "backlight_timer_channel": "LL_TIM_CHANNEL_CH3", | |
| "backlight_gpio_af": "GPIO_AF1", | |
| "backlight_timer_freq": "(PERI2_FREQUENCY * TIMER_MULT_APB2)" |
🤖 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/hw_defs/tx16smk3.json` around lines 451 - 454, The backlight
timer TIM1 is an APB2 peripheral but the JSON uses the APB1 clock expression;
update the backlight_timer_freq value to use the APB2 timer clock expression
instead of "(PERI1_FREQUENCY * TIMER_MULT_APB1)". Replace that string with the
equivalent APB2 expression (e.g. use PERI2_FREQUENCY and TIMER_MULT_APB2) so
backlight_timer = "TIM1" has backlight_timer_freq matching APB2.
There was a problem hiding this comment.
Will not change. Copied existing value from hal.h.
There was a problem hiding this comment.
@philmoz, understood — preserving the existing hal.h value is reasonable. On this board PERI1_FREQUENCY * TIMER_MULT_APB1 and PERI2_FREQUENCY * TIMER_MULT_APB2 evaluate identically (both 240 MHz), so there is no functional difference. I'll drop the suggestion.
| "hardware": { | ||
| "has_audio_mute": 0, | ||
| "has_bling_leds": 0, | ||
| "has_ext_module_support": 1, | ||
| "has_int_module_support": 0, | ||
| "has_ext_module_support": true, | ||
| "sport_max_baudrate": 400000, | ||
| "surface": 0, | ||
| "cpu": "STM32F205xE", | ||
| "cpu_type": "STM32F2" | ||
| }, |
There was a problem hiding this comment.
Add back has_int_module_support for X9D.
X9D is also an internal-module radio. Leaving this flag out changes the generated settings for the target and can disable built-in RF module support.
Suggested fix
"hardware": {
"has_ext_module_support": true,
+ "has_int_module_support": true,
"sport_max_baudrate": 400000,
"cpu": "STM32F205xE",
"cpu_type": "STM32F2"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "hardware": { | |
| "has_audio_mute": 0, | |
| "has_bling_leds": 0, | |
| "has_ext_module_support": 1, | |
| "has_int_module_support": 0, | |
| "has_ext_module_support": true, | |
| "sport_max_baudrate": 400000, | |
| "surface": 0, | |
| "cpu": "STM32F205xE", | |
| "cpu_type": "STM32F2" | |
| }, | |
| "hardware": { | |
| "has_ext_module_support": true, | |
| "has_int_module_support": true, | |
| "sport_max_baudrate": 400000, | |
| "cpu": "STM32F205xE", | |
| "cpu_type": "STM32F2" | |
| }, |
🤖 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/hw_defs/x9d.json` around lines 353 - 358, The hardware
definition for X9D is missing the has_int_module_support flag which causes
generated settings to omit built-in RF module support; add
"has_int_module_support": true to the "hardware" object in x9d.json (alongside
has_ext_module_support, sport_max_baudrate, cpu, cpu_type) so the target
correctly recognizes internal-module support.
There was a problem hiding this comment.
Incorrect. Only used in companion to control UI.
There was a problem hiding this comment.
@philmoz, understood — has_int_module_support is a companion UI control flag and has no effect on firmware RF module support. My concern was incorrect.
✏️ Learnings added
Learnt from: philmoz
Repo: EdgeTX/edgetx PR: 7411
File: radio/src/boards/hw_defs/x9d.json:353-358
Timestamp: 2026-05-30T01:34:50.840Z
Learning: In EdgeTX (repository: EdgeTX/edgetx), the `has_int_module_support` flag in board JSON hardware definitions (e.g., `radio/src/boards/hw_defs/x9d.json`) is only used by the companion application to control UI display. It does not affect firmware behavior or built-in RF module support. Omitting it from a board's JSON is valid and does not disable any firmware functionality.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "hardware": { | ||
| "has_audio_mute": 0, | ||
| "has_bling_leds": 0, | ||
| "has_ext_module_support": 1, | ||
| "has_int_module_support": 0, | ||
| "has_ext_module_support": true, | ||
| "sport_max_baudrate": 400000, | ||
| "surface": 0, | ||
| "cpu": "STM32F205xE", | ||
| "cpu_type": "STM32F2" | ||
| }, |
There was a problem hiding this comment.
Keep has_int_module_support for X9D+.
This target has built-in RF hardware. Dropping has_int_module_support from the migrated hardware block changes the generated capability set and can disable internal module paths for this radio.
Suggested fix
"hardware": {
"has_ext_module_support": true,
+ "has_int_module_support": true,
"sport_max_baudrate": 400000,
"cpu": "STM32F205xE",
"cpu_type": "STM32F2"
},🤖 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/hw_defs/x9d`+.json around lines 367 - 372, The hardware
block for X9D+ removed the internal-module flag which alters generated
capabilities; restore the internal module support by adding
"has_int_module_support": true back into the same "hardware" object (next to
"has_ext_module_support") so X9D+ retains its built-in RF/internal module paths.
There was a problem hiding this comment.
Incorrect. Only used in companion to control UI.
There was a problem hiding this comment.
Understood, thanks for the correction — has_int_module_support only drives companion UI and has no impact on firmware capability generation. My concern was invalid.
✏️ Learnings added
Learnt from: philmoz
Repo: EdgeTX/edgetx PR: 7411
File: radio/src/boards/hw_defs/x9d+.json:367-372
Timestamp: 2026-05-30T01:34:56.654Z
Learning: In the EdgeTX codebase (`radio/src/boards/hw_defs/*.json`), the `has_int_module_support` field in the `hardware` object of board JSON definitions is only used by the companion application to control UI display. It does not affect firmware build-time capability generation or internal module paths in the firmware itself.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "hardware": { | ||
| "has_audio_mute": 0, | ||
| "has_bling_leds": 0, | ||
| "has_ext_module_support": 1, | ||
| "has_int_module_support": 0, | ||
| "has_ext_module_support": true, | ||
| "sport_max_baudrate": 400000, | ||
| "surface": 0, | ||
| "cpu": "STM32F407xE", | ||
| "cpu_type": "STM32F4" | ||
| }, |
There was a problem hiding this comment.
Restore has_int_module_support for X9D+ 2019.
This board has a built-in internal module. Omitting has_int_module_support here changes the generated hardware capabilities and can break internal RF support on the 2019 target.
Suggested fix
"hardware": {
"has_ext_module_support": true,
+ "has_int_module_support": true,
"sport_max_baudrate": 400000,
"cpu": "STM32F407xE",
"cpu_type": "STM32F4"
},🤖 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/hw_defs/x9d`+2019.json around lines 361 - 366, The hardware
definition for the X9D+ 2019 JSON removed the internal module flag; restore the
"has_int_module_support" property in the "hardware" object of x9d+2019.json (add
"has_int_module_support": true) so the generated capabilities include internal
RF support for the X9D+ 2019 target.
There was a problem hiding this comment.
Incorrect. Only used in companion to control UI.
There was a problem hiding this comment.
@philmoz, understood — has_int_module_support is only consumed by companion for UI purposes, not by the firmware build. My concern was incorrect. Thanks for the clarification.
✏️ Learnings added
Learnt from: philmoz
Repo: EdgeTX/edgetx PR: 7411
File: radio/src/boards/hw_defs/x9d+2019.json:361-366
Timestamp: 2026-05-30T01:35:03.736Z
Learning: In the EdgeTX codebase (radio/src/boards/hw_defs/*.json), the `has_int_module_support` field in the `hardware` section of board JSON files is only used by the companion application to control UI display. It does not affect firmware build capabilities or internal RF module support at the firmware level.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| "hardware": { | ||
| "has_audio_mute": 0, | ||
| "has_bling_leds": 0, | ||
| "has_ext_module_support": 1, | ||
| "has_int_module_support": 0, | ||
| "has_ext_module_support": true, | ||
| "sport_max_baudrate": 400000, | ||
| "surface": 0, | ||
| "cpu": "STM32F407xE", | ||
| "cpu_type": "STM32F4" | ||
| }, |
There was a problem hiding this comment.
Preserve has_int_module_support on X9E.
X9E has built-in RF hardware as well. Omitting the internal-module flag here changes the generated capability set and can break internal module functionality for this target.
Suggested fix
"hardware": {
"has_ext_module_support": true,
+ "has_int_module_support": true,
"sport_max_baudrate": 400000,
"cpu": "STM32F407xE",
"cpu_type": "STM32F4"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "hardware": { | |
| "has_audio_mute": 0, | |
| "has_bling_leds": 0, | |
| "has_ext_module_support": 1, | |
| "has_int_module_support": 0, | |
| "has_ext_module_support": true, | |
| "sport_max_baudrate": 400000, | |
| "surface": 0, | |
| "cpu": "STM32F407xE", | |
| "cpu_type": "STM32F4" | |
| }, | |
| "hardware": { | |
| "has_ext_module_support": true, | |
| "has_int_module_support": true, | |
| "sport_max_baudrate": 400000, | |
| "cpu": "STM32F407xE", | |
| "cpu_type": "STM32F4" | |
| }, |
🤖 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/hw_defs/x9e.json` around lines 559 - 564, The hardware block
for X9E omitted the internal-module flag, which breaks generated capabilities;
update the "hardware" object in x9e.json (where "has_ext_module_support",
"sport_max_baudrate", "cpu", and "cpu_type" are defined) to include
"has_int_module_support": true so the target correctly advertises its built-in
RF hardware and internal-module functionality is preserved.
There was a problem hiding this comment.
Incorrect. Only used in companion to control UI.
There was a problem hiding this comment.
@philmoz, understood — has_int_module_support is companion-side only and doesn't affect firmware behavior. My comment was incorrect. I already have this recorded in my knowledge base from an earlier note on this PR, so I won't flag it again.
🧠 Learnings used
Learnt from: philmoz
Repo: EdgeTX/edgetx PR: 7411
File: radio/src/boards/hw_defs/x9d.json:353-358
Timestamp: 2026-05-30T01:34:50.840Z
Learning: In this repo’s EdgeTX board hardware JSON definitions (e.g., files under radio/src/boards/hw_defs/), the `has_int_module_support` flag is used only by the companion application to decide UI display. It does not change firmware behavior or whether any built-in RF module support is available. During code review, it’s valid for a board JSON to omit `has_int_module_support`; do not treat its absence as disabling firmware functionality.
3djc
left a comment
3djc
left a comment
There was a problem hiding this comment.
Overall I'm for it. The only drawback is that it starts to make the addition of new radio very complex, since we have no heleprs or validation of those .json. Before I could add it to 2.12, and use the generated json as a reference point, few additions where needed. This starts to make the task very complex
2 participants
Start to move away from hal.h files and use the radio JSON files for build settings.
Changes:
Known concerns:
Summary by CodeRabbit
Release Notes