fix: move global variable definitions from headers to source files to prevent multiple definition errors#1097
fix: move global variable definitions from headers to source files to prevent multiple definition errors#1097nerdCopter wants to merge 0 commit intoemuflight:masterfrom
Conversation
📝 WalkthroughWalkthroughFix global variable linkage issues by converting header-defined variables to extern declarations. Variables are moved to source files to prevent multiple-definition linker errors during compilation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes apply a consistent, well-established C linkage pattern across multiple files. Review focuses on verifying proper coordination between header declarations and source file definitions, with low logic density and highly repetitive patterns. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/cms/cms.h(1 hunks)src/main/io/ledstrip.c(1 hunks)src/main/io/ledstrip.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (targets-group-3)
- GitHub Check: build (targets-group-2)
- GitHub Check: build (targets-group-1)
- GitHub Check: build (targets-group-rest)
🔇 Additional comments (3)
src/main/cms/cms.h (1)
31-31: Multiple-definition risk resolved; extern paired correctly with single definitionVerification confirms exactly one non-extern definition exists at
cms.c:84(displayPort_t *pCurrentDisplay;), properly paired with the extern declaration atcms.h:31. No tentative definitions or duplicates detected. The fix is correct and eliminates the multiple-definition risk.src/main/io/ledstrip.c (1)
1055-1059: Remove the unusedspecialColorsglobal variable; no staleness issue existsThe global
specialColorsvariable is never actually used anywhere in the codebase—all accesses go throughledStripConfig()->specialColorsinstead. The initialization assigns it by copy, but since nothing reads the global afterward, there's no staleness problem.Additionally, the proposed change alone wouldn't compile: it assigns a pointer to a struct-typed variable, and would require also changing the extern declaration from
specialColorIndexes_t specialColors;tospecialColorIndexes_t *specialColors;. But this is unnecessary since the variable isn't used.Either remove the unused global entirely, or update the review comment to reflect that the variable is unused rather than stale.
Likely an incorrect or invalid review comment.
src/main/io/ledstrip.h (1)
163-165: Externs are dead code; the staleness concern is invalidThe review's concern about "future config changes won't be reflected" is incorrect. The extern
specialColorsglobal is assigned once during initialization and never accessed externally—all client code (msp.c, cli.c) uses theledStripConfig()API instead. The same pattern applies to thecolorsandmodeColorsexterns, which are also unused externally.While an inconsistency exists (specialColors is value-based while colors and modeColors are pointer-based), this inconsistency has no functional impact since the extern is not accessed after initialization. The real issue is that all three externs appear to be dead code or internal scaffolding that should either be removed or clearly marked as internal. Changing specialColors to a pointer would be cosmetic refactoring, not a functional fix.
Likely an incorrect or invalid review comment.
| hsvColor_t *colors; | ||
| const modeColorIndexes_t *modeColors; | ||
| specialColorIndexes_t specialColors; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid by-value global; define specialColors as pointer-to-const
Define specialColors as a pointer to the config struct for live updates and consistency with the other externs.
-hsvColor_t *colors;
-const modeColorIndexes_t *modeColors;
-specialColorIndexes_t specialColors;
+hsvColor_t *colors;
+const modeColorIndexes_t *modeColors;
+const specialColorIndexes_t *specialColors;📝 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.
| hsvColor_t *colors; | |
| const modeColorIndexes_t *modeColors; | |
| specialColorIndexes_t specialColors; | |
| hsvColor_t *colors; | |
| const modeColorIndexes_t *modeColors; | |
| const specialColorIndexes_t *specialColors; | |
🤖 Prompt for AI Agents
In src/main/io/ledstrip.c around lines 80 to 83, specialColors is currently
defined by-value which prevents live updates and is inconsistent with the other
externs; change its definition to a pointer-to-const matching the pattern used
for modeColors (e.g., const specialColorIndexes_t *specialColors) and update any
corresponding extern/declaration sites and usages to dereference the pointer
where needed so callers can read the config through the pointer without copying
or mutating it.
There was a problem hiding this comment.
@coderabbitai, Betaflight does it like this. Can you confirm? https://github.com/betaflight/betaflight
If this PR does it properly like Betaflight, then approve it as is. If it is missing something then report such.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Clone Betaflight repository and check how they implement the ledstrip globals
git clone --depth 1 https://github.com/betaflight/betaflight.git /tmp/betaflight 2>&1
# Check ledstrip.h for extern declarations
echo "=== Betaflight ledstrip.h extern declarations ==="
grep -A 5 -B 5 "extern.*colors\|extern.*modeColors\|extern.*specialColors" /tmp/betaflight/src/main/io/ledstrip.h
echo ""
echo "=== Betaflight ledstrip.c variable definitions ==="
# Check ledstrip.c for actual definitions (look around line 80-100)
grep -n "^hsvColor_t\|^const modeColorIndexes_t\|^specialColorIndexes_t\|^const specialColorIndexes_t" /tmp/betaflight/src/main/io/ledstrip.c | head -20
Length of output: 280
🏁 Script executed:
#!/bin/bash
# Let's look at the actual content of Betaflight's ledstrip.h and ledstrip.c more carefully
echo "=== Betaflight ledstrip.h - checking for extern declarations ==="
cat /tmp/betaflight/src/main/io/ledstrip.h | grep -A 2 -B 2 "colors\|modeColors\|specialColors" | head -30
echo ""
echo "=== Betaflight ledstrip.c - variable definitions (lines 170-185) ==="
sed -n '170,185p' /tmp/betaflight/src/main/io/ledstrip.c
echo ""
echo "=== Betaflight ledstrip.c - ledStripInit function ==="
grep -A 10 "void ledStripInit" /tmp/betaflight/src/main/io/ledstrip.c
Length of output: 1584
🏁 Script executed:
#!/bin/bash
# Check if EmuFlight has extern declarations in ledstrip.h and how current PR handles this
echo "=== EmuFlight PR ledstrip.h (current state) ==="
cat src/main/io/ledstrip.h | grep -A 3 -B 3 "colors\|modeColors\|specialColors" | head -40
echo ""
echo "=== Check Betaflight ledstrip.h for any extern declarations ==="
grep -n "^extern" /tmp/betaflight/src/main/io/ledstrip.h | head -10
Length of output: 1087
@nerdCopter, you're absolutely right! I've confirmed against Betaflight's implementation, and your PR correctly implements specialColors as a by-value variable, matching Betaflight exactly. My previous suggestion to make it a pointer was incorrect, and I withdraw that recommendation.
Comparison with Betaflight:
✅ Correctly matches Betaflight:
specialColorIndexes_t specialColors;is defined by-value (not as pointer)const modeColorIndexes_t *modeColors;matches
Architectural differences from Betaflight:
-
Public API exposure: EmuFlight adds
externdeclarations inledstrip.h, making these variables part of the public API. Betaflight keeps these variables internal toledstrip.cwith no extern declarations in the header. -
Minor const difference: Betaflight uses
const hsvColor_t *colors;while EmuFlight useshsvColor_t *colors;(non-const). This appears intentional given your code usesledStripConfigMutable()->colors.
Verdict: The PR correctly follows Betaflight's pattern for the variable types. The differences (extern visibility and const qualifier) appear to be intentional design choices for EmuFlight's architecture. The linkage fix itself is properly implemented.
The PR resolves the multiple-definition linker errors as intended. ✅
🧬 Code Graph Analysis Results
<src/main/io/ledstrip.c> - No direct snippet provided in the relevant files for this exact file. However, the following snippets are potentially relevant to understanding interactions and behavior used by ledstrip.c (e.g., battery indicators, failsafe, RC mode gating, and RSSI):
-
File: src/main/flight/failsafe.c
-
Lines 112-114:
bool failsafeIsActive(void) {
return failsafeState.active;
} -
Rationale: ledStrip uses failsafeIsActive() to determine certain LED behavior (e.g., warning layer). This helps understand how failsafe state affects LED layers.
-
-
File: src/main/fc/rc_modes.c
-
Lines 55-57:
bool IS_RC_MODE_ACTIVE(boxId_e boxId) {
return bitArrayGet(&rcModeActivationMask, boxId);
} -
Rationale: RC mode gating may influence LED layers or indicators; this provides context on how RC mode boxes are checked.
-
-
File: src/test/unit/telemetry_crsf_msp_unittest.cc
-
Lines 268-270:
uint8_t calculateBatteryPercentageRemaining(void) {
return 67;
} -
Rationale: Provides a test-facing implementation for battery percentage used by battery-related LED logic.
-
-
File: src/test/unit/ledstrip_unittest.cc
-
Lines 326-329:
void getLedHsv(uint16_t index, hsvColor_t *color) {
UNUSED(index);
UNUSED(color);
} -
Lines 345-345:
void setStripColor(const hsvColor_t *color) {
UNUSED(color);
} -
Rationale: These show test stubs for LED color accessors that are part of LED strip state management.
-
-
File: src/main/rx/crsf.c
-
Lines 230-256:
STATIC_UNIT_TESTED uint8_t crsfFrameStatus(rxRuntimeConfig_t *rxRuntimeConfig) {
...
if (crsfFrameDone) {
crsfFrameDone = false;
...
return RX_FRAME_COMPLETE;
}
return RX_FRAME_PENDING;
} -
Rationale: Context for frame status handling; LED strip may react to RX state indirectly through battery/RSSI/health indicators.
-
-
File: src/main/rx/rx.c
-
Lines 311-313:
bool rxIsReceivingSignal(void) {
return rxSignalReceived;
} -
Rationale: Used by some LED layers (e.g., indicator or overlay) to reflect reception activity.
-
-
File: src/main/rx/rx.c
-
Lines 625-627:
uint8_t getRssiPercent(void) {
return scaleRange(getRssi(), 0, RSSI_MAX_VALUE, 0, 100);
} -
Rationale: Used by LED layers that show RSSI-related indicators.
-
-
File: src/main/legacy/??? (not provided) — but usage context:
- In ledstrip.c, color assets and color mapping refer to HSV[] and HSV() macro. See general usage in ledstrip.c vs. snippet below.
-
File: src/main/io/ledstrip.h
-
Lines 175-177:
static inline uint8_t ledGetX(const ledConfig_t *lcfg) {
return ((*lcfg >> (LED_POS_OFFSET + LED_X_BIT_OFFSET)) & LED_XY_MASK);
} -
Lines 178-180:
static inline uint8_t ledGetY(const ledConfig_t *lcfg) {
return ((*lcfg >> (LED_POS_OFFSET + LED_Y_BIT_OFFSET)) & LED_XY_MASK);
} -
Lines 181-183:
static inline uint8_t ledGetFunction(const ledConfig_t *lcfg) {
return ((*lcfg >> LED_FUNCTION_OFFSET) & LED_BIT_MASK(LED_FUNCTION_BITCNT));
} -
Lines 187-189:
static inline uint8_t ledGetColor(const ledConfig_t *lcfg) {
return ((*lcfg >> LED_COLOR_OFFSET) & LED_BIT_MASK(LED_COLOR_BITCNT));
} -
Lines 190-192:
static inline uint8_t ledGetDirection(const ledConfig_t *lcfg) {
return ((*lcfg >> LED_DIRECTION_OFFSET) & LED_BIT_MASK(LED_DIRECTION_BITCNT));
} -
Lines 197-199:
static inline bool ledGetOverlayBit(const ledConfig_t *lcfg, int id) {
return ((ledGetOverlay(lcfg) >> id) & 1);
} -
Lines 200-202:
static inline bool ledGetDirectionBit(const ledConfig_t *lcfg, int id) {
return ((ledGetDirection(lcfg) >> id) & 1);
} -
Rationale: These accessors define how LED strip configuration is encoded and read, which is essential to understanding ledStrip.c behavior.
-
Notes:
- If you want me to extract and include any of these snippets exactly as shown (or summarized), tell me which ones you want in full or summarized form with explicit file and line references.
|
1 participant
Fix: Move Global Variable Definitions from Headers to Source Files
Summary:
This PR resolves a firmware bug where global variables were defined directly in header files (cms.h and ledstrip.h) instead of being declared as
extern. This caused multiple definition linker errors when the headers were included in multiple translation units, such as during unit testing.Details:
externdeclarations.Impact:
Closes:
Summary by CodeRabbit