Just set LED_BUILTIN universally to -1, as we don't use it. #8830
Conversation
jp-bennett
force-pushed
the
kill-led-builtin
branch
from
02c9734 to
e500ae6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 76 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -I variants/esp32/tbeam | ||
| -DBOARD_HAS_PSRAM | ||
| -mfix-esp32-psram-cache-issue | ||
| -ULED_BUILTIN |
There was a problem hiding this comment.
Using -ULED_BUILTIN here conflicts with the global definition -DLED_BUILTIN=-1 set in the main platformio.ini (line 58). This creates inconsistent behavior:
- The global config defines LED_BUILTIN=-1
- This variant config undefines it
- The result is that LED_BUILTIN is undefined for this variant, which differs from other variants
Since the global definition already sets LED_BUILTIN=-1, consider removing the -ULED_BUILTIN flag. If there's a specific reason this variant needs LED_BUILTIN undefined (e.g., the Arduino core for this board defines it), document that reasoning or handle it consistently across all affected variants.
| -ULED_BUILTIN |
|
|
||
| #define LED_PIN LED_BUILTIN | ||
|
|
||
| #define BATTERY_PIN 26 |
There was a problem hiding this comment.
The removal of #define LED_PIN LED_BUILTIN leaves LED_PIN undefined for this variant. Other similar RP2040 variants define LED_PIN as PIN_LED (e.g., variants/rp2040/rak11310/variant.h, variants/rp2040/rpipico/variant.h). Consider adding #define LED_PIN PIN_LED to maintain consistency with other variants and ensure LED_PIN has a valid definition.
There was a problem hiding this comment.
This one is probably legit feedback. Will look.
| ${esp32s3_base.build_flags} | ||
| -D HELTEC_V3 | ||
| -I variants/esp32s3/heltec_v3 | ||
| -ULED_BUILTIN |
There was a problem hiding this comment.
Using -ULED_BUILTIN here conflicts with the global definition -DLED_BUILTIN=-1 set in the main platformio.ini (line 58). This creates inconsistent behavior:
- The global config defines LED_BUILTIN=-1
- This variant config undefines it
- The result is that LED_BUILTIN is undefined for this variant, which differs from other variants
Since the global definition already sets LED_BUILTIN=-1, consider removing the -ULED_BUILTIN flag. If there's a specific reason this variant needs LED_BUILTIN undefined (e.g., the Arduino core for this board defines it), document that reasoning or handle it consistently across all affected variants.
| -ULED_BUILTIN |
| -I variants/esp32c6/tlora_c6 | ||
| -DARDUINO_USB_CDC_ON_BOOT=1 | ||
| -DARDUINO_USB_MODE=1 | ||
| -ULED_BUILTIN |
There was a problem hiding this comment.
Using -ULED_BUILTIN here conflicts with the global definition -DLED_BUILTIN=-1 set in the main platformio.ini (line 58). This creates inconsistent behavior:
- The global config defines LED_BUILTIN=-1
- This variant config undefines it
- The result is that LED_BUILTIN is undefined for this variant, which differs from other variants
Since the global definition already sets LED_BUILTIN=-1, consider removing the -ULED_BUILTIN flag. If there's a specific reason this variant needs LED_BUILTIN undefined (e.g., the Arduino core for this board defines it), document that reasoning or handle it consistently across all affected variants.
| -ULED_BUILTIN |
The Arduino framework really wants this set, but then does nothing with it, that I can tell. Exploratory PR to drop it universally.