Fix: Move global variable definitions from headers to source files to prevent multiple definition errors

[nerdCopter]

Fix: Move global variable definitions from headers to source files to prevent multiple definition errors

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

• Changed global variable definitions in cms.h and ledstrip.h to extern declarations.
• Added the actual variable definitions to their respective source files (cms.c and ledstrip.c).
• The variable specialColors in ledstrip remains a by-value struct, matching the pattern used in Betaflight and other professional firmware projects.
• This approach follows proper C linkage rules and eliminates multiple definition errors.

Note on Betaflight

Betaflight, the upstream project, uses the same pattern: global variables are declared as extern in headers and defined in a single source file. For ledstrip, Betaflight also keeps specialColors as a by-value struct, not a pointer, for consistency and simplicity.

Impact

• Unit tests and firmware now build without linker errors related to multiple definitions.
• No functional changes to runtime behavior; this is a build/linkage correctness fix.
• Matches Betaflight and industry best practices for global variable linkage.

Closes

• BUG: Global Variables Defined in Headers Without extern #1096 (FIRMWARE BUG: Global Variables Defined in Headers Without extern)

---

Summary by CodeRabbit

• Refactor
  • Reduced public/global surface by internalizing LED color configuration and removing several externally exposed color variables.
  • Converted the display reference to a non-duplicated declaration to avoid multiple definitions.
  • Introduced internal lifecycle flags to better control LED strip initialization and enabled state.

[coderabbitai]

📝 Walkthrough

Walkthrough

Header-only symbol definitions were converted to extern declarations and corresponding symbols were defined/initialized in implementation files: pCurrentDisplay in the CMS header was changed to extern; LED strip color pointers were moved/initialized in ledstrip.c and their header declarations were removed; LED-strip lifecycle flags were added in ledstrip.c.

Changes

Cohort / File(s)	Summary
CMS header change src/main/cms/cms.h	Changed displayPort_t *pCurrentDisplay from a definition to an extern declaration.
LED strip implementation additions src/main/io/ledstrip.c	Added global pointers hsvColor_t *colors, const modeColorIndexes_t *modeColors, specialColorIndexes_t specialColors and static flags ledStripInitialised (false) and ledStripEnabled (true); ledStripInit assigns configuration pointers and sets initialization flag.
LED strip header cleanup src/main/io/ledstrip.h	Removed public/global declarations for colors, modeColors, and specialColors.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant ledstrip.c as LedStrip
    Note over LedStrip: Global pointers & flags now live in implementation
    Caller->>LedStrip: ledStripInit()
    alt populate config
        LedStrip-->>LedStrip: colors = ledStripConfigMutable()->colors
        LedStrip-->>LedStrip: modeColors = ledStripConfig()->modeColors
        LedStrip-->>LedStrip: specialColors = ledStripConfig()->specialColors
        LedStrip-->>LedStrip: ledStripInitialised = false
        LedStrip-->>LedStrip: ledStripEnabled = true
    end
    Caller->>LedStrip: read color data (uses internal pointers)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Fix: Move global variable definitions from headers to source files to prevent multiple definition errors" directly and specifically describes the main objective of the changeset. It captures the core fix—converting global variable definitions in headers to extern declarations and moving definitions to source files—and explains the problem being solved (multiple definition errors). The title is concise, clear, and specific enough that a teammate reviewing commit history would immediately understand the primary change without needing to read the description.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b0f9d1 and f516506.

📒 Files selected for processing (2)

• src/main/io/ledstrip.c (1 hunks)
• src/main/io/ledstrip.h (0 hunks)

💤 Files with no reviewable changes (1)

• src/main/io/ledstrip.h

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/io/ledstrip.c

⏰ 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-rest)
• GitHub Check: build (targets-group-2)
• GitHub Check: build (targets-group-3)
• GitHub Check: build (targets-group-1)

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nerdCopter]

@coderabbitai, re-verify that this is how Betaflight does it and that I may merge this PR as-is without further testing nor flight-testing.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Clone Betaflight repository to compare the pattern
cd /tmp
git clone --depth 1 https://github.com/betaflight/betaflight.git
cd betaflight

echo "=== Checking cms.h for pCurrentDisplay ==="
if [ -f "src/main/cms/cms.h" ]; then
    grep -n "pCurrentDisplay" src/main/cms/cms.h
else
    echo "cms.h not found"
fi

echo ""
echo "=== Checking cms.c for pCurrentDisplay ==="
if [ -f "src/main/cms/cms.c" ]; then
    grep -n "pCurrentDisplay" src/main/cms/cms.c | head -5
else
    echo "cms.c not found"
fi

echo ""
echo "=== Checking ledstrip.h for colors, modeColors, specialColors ==="
if [ -f "src/main/io/ledstrip.h" ]; then
    grep -n -E "(colors|modeColors|specialColors)" src/main/io/ledstrip.h | grep -v "//"
else
    echo "ledstrip.h not found"
fi

echo ""
echo "=== Checking ledstrip.c for colors, modeColors, specialColors ==="
if [ -f "src/main/io/ledstrip.c" ]; then
    grep -n -E "^(hsvColor_t \*colors|const modeColorIndexes_t \*modeColors|specialColorIndexes_t specialColors)" src/main/io/ledstrip.c
else
    echo "ledstrip.c not found"
fi

Length of output: 1200

---

🏁 Script executed:

#!/bin/bash
cd /tmp/betaflight

echo "=== Full context around ledstrip.c global variables ==="
grep -n -B2 -A2 "^hsvColor_t \*colors" src/main/io/ledstrip.c
grep -n -B2 -A2 "^const modeColorIndexes_t \*modeColors" src/main/io/ledstrip.c
grep -n -B2 -A2 "^specialColorIndexes_t specialColors" src/main/io/ledstrip.c

echo ""
echo "=== Looking for extern declarations in ledstrip.h ==="
grep -n -B2 -A2 "extern.*colors" src/main/io/ledstrip.h
grep -n -B2 -A2 "extern.*modeColors" src/main/io/ledstrip.h
grep -n -B2 -A2 "extern.*specialColors" src/main/io/ledstrip.h

echo ""
echo "=== Check if colors/modeColors/specialColors appear as standalone globals in ledstrip.h ==="
cat src/main/io/ledstrip.h | grep -n -E "^(extern |)(hsvColor_t \*colors|const modeColorIndexes_t \*modeColors|specialColorIndexes_t specialColors)"

Length of output: 488

---

🏁 Script executed:

#!/bin/bash
cd /tmp/betaflight

echo "=== Full ledstrip.h content - look for any extern variable declarations ==="
cat src/main/io/ledstrip.h | grep -n "extern" | head -20

echo ""
echo "=== Check the actual definitions in ledstrip.c - wider context ==="
sed -n '170,185p' src/main/io/ledstrip.c

echo ""
echo "=== Now check EmuFlight's current structure ==="
cd /root/repo

echo ""
echo "=== Current ledstrip.h in EmuFlight PR ==="
cat src/main/io/ledstrip.h | grep -n -E "extern.*(colors|modeColors|specialColors)"

echo ""
echo "=== Current ledstrip.c in EmuFlight PR ==="
cat src/main/io/ledstrip.c | grep -n -E "^(hsvColor_t|const modeColorIndexes_t|specialColorIndexes_t).*(colors|modeColors|specialColors)" | head -10

Length of output: 869

---

🏁 Script executed:

#!/bin/bash
# Check the EmuFlight repository files
cd /workspace

echo "=== EmuFlight cms.h ==="
grep -n "pCurrentDisplay" src/main/cms/cms.h

echo ""
echo "=== EmuFlight cms.c ==="
grep -n "^displayPort_t \*pCurrentDisplay" src/main/cms/cms.c

echo ""
echo "=== EmuFlight ledstrip.h - check for extern declarations ==="
grep -n -E "extern.*(colors|modeColors|specialColors)" src/main/io/ledstrip.h

echo ""
echo "=== EmuFlight ledstrip.c - check definitions ==="
sed -n '170,180p' src/main/io/ledstrip.c

echo ""
echo "=== Compare const qualifiers ==="
echo "Betaflight ledstrip.c has:"
echo "  const hsvColor_t *colors;"
echo "  const modeColorIndexes_t *modeColors;"
echo "  specialColorIndexes_t specialColors;"
echo ""
echo "EmuFlight ledstrip.c has:"
grep -E "^(hsvColor_t|const modeColorIndexes_t|specialColorIndexes_t).*(colors|modeColors|specialColors)" src/main/io/ledstrip.c

Length of output: 1414

---

🏁 Script executed:

#!/bin/bash

echo "=== Checking if Betaflight has extern declarations for ledstrip variables anywhere ==="
cd /tmp/betaflight
grep -r "extern.*\*colors" --include="*.h" src/main/

echo ""
echo "=== Check if these variables are static or file-local in Betaflight ==="
grep -B2 -A2 "const hsvColor_t \*colors" src/main/io/ledstrip.c

echo ""
echo "=== Check if there are any other files that use these variables in Betaflight ==="
rg -n --type c --type cpp "\bcolors\b" src/main/ | grep -v "ledstrip" | grep -v "colors\[" | head -20

echo ""
echo "=== Summary check: Are these variables used outside ledstrip.c in Betaflight? ==="
rg -n "modeColors" src/main/ --type c | grep -v "ledstrip.c" | grep -v "ledstrip.h" | head -10

Length of output: 1848

---

🏁 Script executed:

#!/bin/bash

echo "=== Check if colors/modeColors/specialColors are accessed outside ledstrip.c in EmuFlight ==="
rg -n "\bcolors\b" src/main/ --type c | grep -v "ledstrip.c" | grep -v "ledstrip.h" | grep -v "colors\[" | head -20

echo ""
echo "=== Check modeColors usage in EmuFlight ==="
rg -n "\bmodeColors\b" src/main/ --type c | grep -v "ledstrip.c" | grep -v "ledstrip.h" | head -20

echo ""
echo "=== Check specialColors usage in EmuFlight ==="
rg -n "\bspecialColors\b" src/main/ --type c | grep -v "ledstrip.c" | grep -v "ledstrip.h" | head -20

echo ""
echo "=== Check if these are truly needed as extern in EmuFlight ==="
echo "Looking for direct references to the global pointers colors, modeColors, specialColors"
echo "vs accessing through config functions like ledStripConfig()->colors"

Length of output: 1859

---

🏁 Script executed:

#!/bin/bash

echo "=== Check the original state before this PR (master branch) ==="
git fetch origin master
git show origin/master:src/main/io/ledstrip.h | grep -n -A1 -B1 -E "(colors|modeColors|specialColors)" | head -20

echo ""
echo "=== Check if Betaflight's variables have static keyword ==="
cd /tmp/betaflight
grep -n "static.*colors" src/main/io/ledstrip.c | head -5
grep -n "^const hsvColor_t" src/main/io/ledstrip.c
grep -n "^const modeColorIndexes_t" src/main/io/ledstrip.c
grep -n "^specialColorIndexes_t" src/main/io/ledstrip.c

echo ""
echo "=== Summary: Check if these have external or internal linkage in Betaflight ==="
echo "In C, a global variable declared with 'const' at file scope has internal linkage by default"
echo "A pointer declared as 'const type *ptr' means ptr has external linkage but points to const data"
echo ""
echo "Betaflight: const hsvColor_t *colors - external linkage (can be accessed from other TUs)"
echo "Betaflight: const modeColorIndexes_t *modeColors - external linkage"
echo "Betaflight: specialColorIndexes_t specialColors - external linkage"

Length of output: 1524

---

@nerdCopter, I've thoroughly compared this PR with Betaflight's approach. Here's what I found:

CMS Variables (cms.h / cms.c)

✅ Matches Betaflight exactly:

• Betaflight: extern displayPort_t *pCurrentDisplay; in cms.h, displayPort_t *pCurrentDisplay; in cms.c
• EmuFlight PR: Identical pattern

LED Strip Variables (ledstrip.h / ledstrip.c)

⚠️ Differs from Betaflight's approach:

Betaflight:

• Does NOT declare colors, modeColors, or specialColors in ledstrip.h at all
• Keeps them as file-scope variables in ledstrip.c only:

  const hsvColor_t *colors;
  const modeColorIndexes_t *modeColors;
  specialColorIndexes_t specialColors;

EmuFlight PR:

• Adds extern declarations in ledstrip.h:

  extern hsvColor_t *colors;
  extern const modeColorIndexes_t *modeColors;
  extern specialColorIndexes_t specialColors;

• Defines them in ledstrip.c

Key Finding: In both EmuFlight and Betaflight, all external usage accesses these through ledStripConfig()->colors rather than the global pointers directly. Betaflight doesn't expose these globals in the header at all.

Merge Assessment

Can you merge as-is? Yes, with caveats:

1. ✅ Fixes the stated issue (BUG: Global Variables Defined in Headers Without extern #1096): Resolves multiple definition linker errors
2. ✅ No functional changes: Pure linkage fix, no runtime behavior altered
3. ⚠️ Doesn't exactly match Betaflight: For LED strip variables, Betaflight keeps them file-local rather than exporting via header
4. Minor const difference: Betaflight uses const hsvColor_t *colors (pointer to const), you have hsvColor_t *colors (pointer to mutable)

Recommendation: Safe to merge for fixing the immediate build issue. If you want to match Betaflight exactly in the future, you could consider making these LED strip variables static in ledstrip.c since they're not directly accessed from other files.

[nerdCopter]

@coderabbitai, does commit f516506 fix your number 3 ? how important is number 4? This is an OLD Betaflight/Butterflight fork -- as far as i know LEDSTRIP has been working.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check commit f5165063f to see what it changes
git show f5165063f --stat

echo ""
echo "=== Detailed changes in commit f5165063f ==="
git show f5165063f

Length of output: 2444

---

@nerdCopter, excellent follow-up! Let me address your questions:

Does commit f516506 fix issue #3?

✅ YES, completely! The commit perfectly addresses issue #3 by:

• Removing the extern declarations from ledstrip.h
• Making the variables static in ledstrip.c:

  static hsvColor_t *colors;
  static const modeColorIndexes_t *modeColors;
  static specialColorIndexes_t specialColors;

This now exactly matches Betaflight's pattern of keeping these as file-local variables that aren't exposed in the header.

How important is issue #4 (const qualifier)?

Not important for functionality. Issue #4 is purely a defensive programming detail:

• Betaflight: const hsvColor_t *colors (pointer to const data)
• EmuFlight: hsvColor_t *colors (pointer to mutable data)

Why it doesn't matter:

1. Since colors is assigned from ledStripConfigMutable()->colors, the data can be modified through that configuration path regardless
2. It's a compile-time safety feature to prevent accidental writes through this specific pointer
3. Your point is valid: if LEDSTRIP has been working in your fork, this const difference has no runtime impact

Final Recommendation

✅ Safe to merge as-is. With commit f516506 included:

• Both CMS and LED strip patterns now match Betaflight's approach
• The multiple definition issue (BUG: Global Variables Defined in Headers Without extern #1096) is fixed
• No functional changes, pure linkage/build correctness
• The minor const difference is a code style detail that doesn't affect your working LEDSTRIP implementation

You can proceed with merging without additional testing or flight-testing for this linkage fix.

[nerdCopter]

@coderabbitai, , in the future use, for example, either "Number 3" or use Alphabet (e.g. A.) in the initial recommendations as we do not want to reference old pull-requests like #3

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@nerdCopter, understood! I'll use either spelled-out numbering (e.g., "Number 3") or alphabetic notation (e.g., "A.", "B.", "C.") in my recommendations to avoid accidental GitHub issue/PR references with the # syntax. Thank you for the guidance!

---

✏️ Learnings added

Learnt from: nerdCopter
URL: https://github.com/emuflight/EmuFlight/pull/1098

Timestamp: 2025-10-22T16:50:06.042Z
Learning: When providing numbered lists or recommendations in GitHub comments, use either spelled-out numbers (e.g., "Number 1", "Number 2", "Number 3") or alphabetic notation (e.g., "A.", "B.", "C.") instead of plain numeric notation (e.g., "1.", "2.", "3.") to avoid accidentally referencing GitHub issues or pull requests with the `#` syntax.

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.