Skip to content

Station G3: Expose, persist, and apply FEM gain preferences - #3137

Open
agessaman wants to merge 2 commits into
meshcore-dev:devfrom
agessaman:feat/station-g3-fem-prefs
Open

Station G3: Expose, persist, and apply FEM gain preferences#3137
agessaman wants to merge 2 commits into
meshcore-dev:devfrom
agessaman:feat/station-g3-fem-prefs

Conversation

@agessaman

Copy link
Copy Markdown
Contributor

Adds radio.fem.txgain so the Station G3's FEM transmit gain can be set from the CLI and persisted, plus two fixes I hit on the way.

What's here

  • set/get radio.fem.txgain on|off drives PA PL1, which picks between power levels 1/2 and 3/4 (the PA PL2 jumper decides which pair). Saved as fem_txgain. Other boards hit the existing canControlLoRaFemPaGain() default and report unsupported, so nothing changes for them.
  • fem_rxgain was being serialized from rx_boosted_gain instead of radio_fem_rxgain, so set radio.fem.rxgain never survived a reboot. Fixed.
  • The PA level is now applied at the start of a transmit rather than when the CLI write happens. PL1 moves the PA's DC-DC rail rather than selecting a logic-level gain, and the CLI runs on every loop pass whether or not a packet is in flight, so a set could drop the rail mid-transmit with the SX1262 still driving the PA. setPAGainEnable() just records the value now, and the pin gets written from setTxModeEnable(), which runs before startTransmit(). The level only matters during TX so waiting costs nothing. I couldn't reproduce the resulting reboot on demand (it needs the write to land inside a transmit), but the sequencing is wrong either way.

Also moves the Station G3 FEM pin handling into LoRaFEMControl like the Heltec variants, and documents both commands.

Testing

Bench-tested on a Station G3 (ESP32-S3 + BQ35LORA900V1M). I hung an INA219 on the input rail and temporarily logged idle vs peak power per transmit, to confirm the pin really does change the PA level and isn't just wiggling a GPIO. With fem_txgain=on: 0.38 W idle, 8.14 W TX peak (14.62 V / 0.56 A), which lines up with the 6.330 W the Neil quotes for power level 1. That test isn't in the PR, it came back out once it had done its job.

Repeater firmware builds clean, and the setting round-trips and survives a reboot.

For reviewers

The fem_rxgain fix changes what an existing key means on upgrade, for the boards with FEM LNA control (heltec_t096, heltec_v4, heltec_tracker_v2). All three build with SX126X_RX_BOOSTED_GAIN=1, so the value that migrates in is the default anyway. Only someone who had turned radio.rxgain off would end up with a 0 and want to flip it back.

PA PL1 re-targets the PA's DC-DC supply rail rather than selecting a
logic-level gain, and the serial CLI is serviced on every main-loop pass
regardless of whether a transmit is in flight. A `set radio.fem.txgain`
write could therefore move the rail mid-transmit, while the SX1262 was
still driving the PA at full input power.

Record the requested level in setPAGainEnable() and drive the pin from
setTxModeEnable(), which runs from onBeforeTransmit() ahead of
startTransmit(). The level only matters while transmitting, so deferring
costs nothing.

Document that the pref is saved immediately but applied at the next
transmit, so `get radio.fem.txgain` can lead the hardware until then.
@agessaman

Copy link
Copy Markdown
Contributor Author

This addresses #3123.

@cwichura

cwichura commented Aug 9, 2026

Copy link
Copy Markdown

@ripplebiz This also contains a small bugfix for the new config serializer. (The second bullet point listed.)

@IoTThinks

Copy link
Copy Markdown
Contributor
  • fem_rxgain was being serialized from rx_boosted_gain instead of radio_fem_rxgain, so set radio.fem.rxgain never survived a reboot. Fixed.

I will take note of this PR during my build.
This should be a valid fix for Heltec v4.3 and T096 too.

@ACETyr

ACETyr commented Aug 10, 2026

Copy link
Copy Markdown

Added a host-side regression test for the fem_rxgain binding and ran it against this branch.

Result

tree result
dev @ f6c25e6a, unmodified 2/2 fail
dev + only def("fem_rxgain", _parent->radio_fem_rxgain) 2/2 pass
this PR @ c58c9b2f 2/2 pass
this PR, full pio test -e native 27/27 pass

Host: g++ 15.2.0, env:native.

The test drives the real NodePrefs from helpers/CommonCLI.h rather than a copy, so it tracks the shipping binding. It asserts both directions with opposite values (rx_boosted_gain=0, radio_fem_rxgain=1) so a binding that aliases one field onto the other cannot pass by coincidence.

Two things were needed to compile the header on the host, both contained in the test file:

  • a FILESYSTEM stub with mkdirIdentityStore.h only defines FILESYSTEM for Arduino targets, and one inline method calls it;
  • numeric print() overrides on the capture stream — the mock Print in test/mocks/Stream.h stubs every numeric overload to return 0 without emitting, so integer fields serialise blank and an assertion cannot distinguish 0 from 1.

Use it if it is helpful — happy to open it as a PR against your branch, or you can paste it in.

test/test_node_prefs_fem/test_node_prefs_fem.cpp
// Regression test for the FEM RX-gain preference binding in NodePrefs.
//
// `radio.fem.rxgain` (the external FEM LNA) and `rxgain` (the SX1262's own
// boosted-RX register) are two different settings, but RadioPrefs::structure()
// binds BOTH JSON keys to `rx_boosted_gain`:
//
//     def("rxgain",     _parent->rx_boosted_gain);
//     def("fem_rxgain", _parent->rx_boosted_gain);   // <-- should be radio_fem_rxgain
//
// so `radio_fem_rxgain` is never serialised at all and `set radio.fem.rxgain`
// does not survive a reboot, even though the CLI handler does call savePrefs().
// Reported in meshcore-dev/MeshCore#3145, fix proposed in #3137.
//
// This is a pure serialisation defect, so it is provable on any host — no
// Heltec V4 hardware required. The test is written against the real NodePrefs,
// not a copy, so it tracks the shipping binding.

#include <gtest/gtest.h>
#include <cstdio>

// IdentityStore.h only defines FILESYSTEM for the Arduino targets. NodePrefs
// itself never touches the filesystem — these tests drive saveSerial/loadSerial
// against streams — but a couple of inline methods in the headers it pulls in
// do, so the host build needs a type with those members present.
struct FILESYSTEM {
  bool mkdir(const char*) { return true; }
};

#include "helpers/CommonCLI.h"

// A stream that captures whatever the serializer writes.
//
// The mock Print in test/mocks/Stream.h stubs every numeric print() to return 0
// without emitting anything, so integer fields would come out blank and the
// assertions below could not tell 0 from 1. These overrides render them.
class CaptureStream : public Stream {
    int len = 0;
    char _buf[2048];
    size_t emit(long long v) {
        char tmp[24];
        int n = snprintf(tmp, sizeof(tmp), "%lld", v);
        for (int i = 0; i < n; i++) write((uint8_t)tmp[i]);
        return (size_t)n;
    }
public:
    size_t write(uint8_t b) override {
        if (len < (int)sizeof(_buf) - 1) { _buf[len++] = (char)b; _buf[len] = 0; return 1; }
        return 0;
    }
    size_t print(unsigned char v, int r = DEC) override { return emit(v); }
    size_t print(int v, int r = DEC) override { return emit(v); }
    size_t print(unsigned int v, int r = DEC) override { return emit(v); }
    size_t print(long v, int r = DEC) override { return emit(v); }
    size_t print(unsigned long v, int r = DEC) override { return emit((long long)v); }
    size_t print(long long v, int r = DEC) override { return emit(v); }
    size_t print(unsigned long long v, int r = DEC) override { return emit((long long)v); }

    const char* text() { _buf[len] = 0; return _buf; }
};

// A stream that replays a canned config document.
class ReplayStream : public Stream {
    const char* _t; int pos, len;
public:
    ReplayStream(const char* t) : _t(t) { pos = 0; len = (int)strlen(t); }
    int available() override { return len - pos; }
    int read() override { return pos < len ? _t[pos++] : -1; }
    int peek() override { return pos < len ? _t[pos] : -1; }
};

// The two settings must round-trip independently. Give them opposite values so
// a binding that aliases one onto the other cannot pass by coincidence.
TEST(NodePrefsFem, FemRxGainIsSerialisedFromItsOwnField) {
    NodePrefs prefs;
    prefs.rx_boosted_gain  = 0;   // SX1262 internal boosted RX: off
    prefs.radio_fem_rxgain = 1;   // external FEM LNA: on

    CaptureStream out;
    ASSERT_TRUE(prefs.saveSerial(out));

    const char* doc = out.text();
    EXPECT_NE(nullptr, strstr(doc, "rxgain:0"))
        << "expected the SX1262 boosted-gain key to emit 0; got: " << doc;
    EXPECT_NE(nullptr, strstr(doc, "fem_rxgain:1"))
        << "fem_rxgain emitted the wrong field — it is bound to rx_boosted_gain, "
           "so radio_fem_rxgain is never stored. Full document: " << doc;
}

TEST(NodePrefsFem, FemRxGainSurvivesALoad) {
    NodePrefs prefs;
    prefs.rx_boosted_gain  = 1;
    prefs.radio_fem_rxgain = 1;

    // What a node should read back after the operator ran
    // `set radio.fem.rxgain off` and rebooted.
    ReplayStream in("{radio:{rxgain:1,fem_rxgain:0}}");
    ASSERT_TRUE(prefs.loadSerial(in));

    EXPECT_EQ(0, (int)prefs.radio_fem_rxgain)
        << "radio_fem_rxgain did not take the stored fem_rxgain value — "
           "the FEM LNA reverts to its compiled default on every boot";
    EXPECT_EQ(1, (int)prefs.rx_boosted_gain)
        << "rx_boosted_gain was clobbered by the fem_rxgain key";
}

// ── main ───────────────────────────────────────────────────────

int main(int argc, char** argv) {
    ::testing::InitGoogleTest(&argc, argv);
    return RUN_ALL_TESTS();
}

@ACETyr

ACETyr commented Aug 10, 2026

Copy link
Copy Markdown

Separate observation from grepping this branch, almost certainly out of scope for this PR — flagging only in case it is unintended.

examples/companion_radio/ has its own NodePrefs, and it still carries the two-keys-one-field shape:

examples/companion_radio/NodePrefs.h:52:      def("rxgain", _parent->rx_boosted_gain);
examples/companion_radio/NodePrefs.h:53:      def("fem_rxgain", _parent->rx_boosted_gain);

Verified on c58c9b2f:

  • that class declares rx_boosted_gain and no FEM field — radio_fem_rxgain occurs 0 times anywhere under examples/companion_radio/;
  • setLoRaFemLnaEnabled and canControlLoRaFemLna also occur 0 times under examples/companion_radio/. setLoRaFemLnaEnabled is called once each in simple_repeater/MyMesh.cpp, simple_room_server/MyMesh.cpp and simple_sensor/SensorMesh.cpp;
  • helpers/CommonCLI.h is included by simple_repeater, simple_room_server and simple_sensor only, not by companion_radio.

So after this PR the key fem_rxgain is bound to radio_fem_rxgain in the CommonCLI NodePrefs and to rx_boosted_gain in the companion NodePrefs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants