Conversation
|
It needs to be combined with the new variables added in this commit for it to compile successfully. |
|
Exciting! |
|
Made sure heltec tracker v2 can still build: Quency-D#1 |
|
This PR moves SX126x FEM control from |
Thank you. Only a very small number of samples of the Heltec Tracker v2 board were released before, and it has now been redesigned. I think it's appropriate to wait until the new design is available before making any modifications. Therefore, this submission will not be merged for the time being. |
Yes, your concerns are valid. However, currently USE_GC1109_PA is only used for heltec-v4 and heltec tracker v2. The heltec-v4 part has been modified in this submission. As for heltec tracker v2, since only a small number of samples were previously released and it has been redesigned, we plan to modify and submit it again after the new design is completed. |
Should we remove the Heltec Wireless Tracker v2 from Meshtastic firmware altogether? If we merge this PR the heltec wireless tracker v2 build will fail. That's what my PR tried to address. :) |
I tried using this pr to compile the Heltec Wireless Tracker v2, and it compiled successfully. What error did you get during compilation? Could you post it so I can see? |
The build itself compiles, but the issue is at runtime — the PR removes all USE_GC1109_PA guarded code and replaces it with HAS_LORA_FEM guards, but only heltec_v4 defines HAS_LORA_FEM=1. Since the tracker v2 doesn't define it, the GC1109 FEM would never be initialized, TX/RX mode switching wouldn't happen, and deep sleep RTC hold logic would be skipped. It compiles fine but the FEM would be completely uncontrolled at runtime. Essentially it breaks Heltec Wireless Tracker v2. It's better to remove it then, or merge the PR suggestion I made. |
Let me think about it. Could you please explain how you perform the format checking? What tools do you use? |
|
The repo uses Trunk for CI lint/format checks — it runs automatically on every PR. For C/C++ files it uses clang-format (config is in You can run it locally with: |
Thank you for your answer. I fixed the FEM control in Heltec Tracker v2. I think this macro approach is also simpler and easier to understand. What do you think? |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the Heltec WiFi LoRa 32 V4.3 board, which uses a different FEM (KCT8103L) compared to V4.2 (GC1109), and centralizes FEM control logic into a new LoRaFEMInterface class. The V4.3 hardware is auto-detected at runtime by reading the default pull state of GPIO2. Additionally, a new LNA toggle menu is added to the screen UI for boards that support LNA control (KCT8103L).
Changes:
- New
LoRaFEMInterfaceclass (src/mesh/LoRaFEMInterface.h/.cpp) unifies FEM control for all supported boards (GC1109 and KCT8103L), replacing board-specificUSE_GC1109_PAconditionals throughout the codebase - Renamed FEM pin macros from generic (
LORA_PA_EN,LORA_PA_TX_EN) to chip-specific (LORA_GC1109_PA_EN,LORA_GC1109_PA_TX_EN), and added KCT8103L pin definitions toheltec_v4/variant.h - Added a new LoRa FEM LNA toggle UI menu in
MenuHandler.h/.cppcontrolled by a newfem_lna_modeconfig field
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/mesh/LoRaFEMInterface.h |
New class declaration for unified FEM control interface |
src/mesh/LoRaFEMInterface.cpp |
New implementation with runtime FEM detection and per-chip control logic |
src/mesh/SX126xInterface.cpp |
Delegates FEM init/sleep/TX/RX to LoRaFEMInterface |
src/mesh/RadioInterface.h |
Conditionally includes new LoRaFEMInterface.h |
src/mesh/RadioInterface.cpp |
Uses loraFEMInterface.powerConversion() when HAS_LORA_FEM=1 |
src/sleep.cpp |
Uses loraFEMInterface.setRxModeEnableWhenMCUSleep() instead of inline GC1109 code |
src/graphics/draw/MenuHandler.h |
Adds new LoraFemLnaToggleMenu enum value and method declaration |
src/graphics/draw/MenuHandler.cpp |
Adds LNA toggle menu for boards with controllable LNA |
src/configuration.h |
Adds default definition HAS_LORA_FEM 0 |
variants/esp32s3/heltec_v4/variant.h |
Adds KCT8103L pin definitions alongside renamed GC1109 macros |
variants/esp32s3/heltec_v4/platformio.ini |
Adds -D HAS_LORA_FEM=1 |
variants/esp32s3/heltec_wireless_tracker_v2/variant.h |
Renames FEM pin macros to chip-specific names |
variants/esp32s3/heltec_wireless_tracker_v2/platformio.ini |
Adds -D HAS_LORA_FEM=1 |
| power = loraFEMInterface.powerConversion(power); | ||
| } | ||
| #else | ||
| // todo:All entries containing "lora fem" are grouped together above. |
There was a problem hiding this comment.
There is a leftover in-code TODO comment on line 928. This style of embedded todo comment should not appear in production code. Either the comment should be removed entirely or the suggested cleanup should be done as part of this PR.
| // todo:All entries containing "lora fem" are grouped together above. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| #ifdef USE_GC1109_PA | ||
| // Power Amps are often non-linear, so we can use an array of values for the power curve | ||
| #define NUM_PA_POINTS 22 | ||
| #define TX_GAIN_LORA 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 11, 10, 10, 9, 9, 8, 7 |
There was a problem hiding this comment.
It looks like this PR would lose this define, which is probably not intentional. Or if intentional, let's remove it.
There was a problem hiding this comment.
This section was also used in the Heltec Tracker v2, so it should be retained.
🤝 Attestations