[sx126x][sx127x] Fixes from code review

- Without LoRa, require the SX127x radio to have a sync word or preamble
  detection, as listen before talk cannot see a packet arriving otherwise
- Warn once when a send finds the radio failed

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
clydebarrow
2026-10-20 09:35:06 +11:00
co-authored by Claude Opus 5.5
parent 69422272a4
commit 6449084613
5 changed files with 62 additions and 5 deletions
@@ -32,6 +32,13 @@ bool RadioLinkBase::skip_empty_() const {
return true;
}
void RadioLinkBase::radio_not_ready_(bool failed) {
if (!failed || this->failed_warned_)
return;
this->failed_warned_ = true;
ESP_LOGW(TAG, "%s: The radio failed to set up, so nothing can be sent", this->name_);
}
void RadioLinkBase::radio_free_() {
this->listen_before_talk_.end_busy();
this->disable_loop();
@@ -20,11 +20,14 @@ class RadioLinkBase : public PacketInterface {
bool may_send_(bool busy);
/// Returns true for an empty packet, which the radios cannot send, so there is nothing to retry.
bool skip_empty_() const;
/// Warns once when a send finds the radio failed; until its setup has run, sends are refused quietly.
void radio_not_ready_(bool failed);
/// Ends the busy period when loop() finds the radio free.
void radio_free_();
void receive_(const std::vector<uint8_t> &packet, float rssi, float snr);
ListenBeforeTalk listen_before_talk_;
bool failed_warned_{false};
};
/**
@@ -54,8 +57,10 @@ class RadioLink : public RadioLinkBase, public Listener {
bool send_to_interface(const PacketBuffer &data, const PacketMetaData &meta_data) override {
// The radio is set up after the layers above, which may send from their own setup
if (!this->radio_->is_ready())
if (!this->radio_->is_ready()) {
this->radio_not_ready_(this->radio_->is_failed());
return false;
}
size_t size = data.size();
if (size == 0)
return this->skip_empty_();
@@ -12,8 +12,10 @@ from .. import (
CONF_MODULATION,
CONF_PACKET_MODE,
CONF_PAYLOAD_LENGTH,
CONF_PREAMBLE_DETECT,
CONF_RX_START,
CONF_SX127X_ID,
CONF_SYNC_VALUE,
DOMAIN,
SX127x,
SX127xListener,
@@ -34,7 +36,10 @@ CONFIG_SCHEMA = radio_link_schema(SX127xPacketInterface, CONF_SX127X_ID, SX127x)
def _check_fsk(config: ConfigType, radio: ConfigType) -> None:
"""Without LoRa, the radio must send packets, not a continuous bit stream, and holds fewer bytes."""
"""Without LoRa, the radio must send packets, not a continuous bit stream, and holds fewer bytes.
It can tell that a packet is arriving only from a detected preamble or sync word, which listen before talk needs.
"""
if radio[CONF_MODULATION] == "LORA":
return
if not radio.get(CONF_PACKET_MODE, False):
@@ -42,6 +47,12 @@ def _check_fsk(config: ConfigType, radio: ConfigType) -> None:
f"Without LoRa modulation the radio's '{CONF_PACKET_MODE}' must be true, as a link sends packets",
path=[CONF_SX127X_ID],
)
if not radio.get(CONF_SYNC_VALUE) and not radio.get(CONF_PREAMBLE_DETECT):
raise cv.Invalid(
f"Without LoRa modulation the radio needs a '{CONF_SYNC_VALUE}' or '{CONF_PREAMBLE_DETECT}', so that "
"the link can tell when a packet is arriving and not send over it",
path=[CONF_SX127X_ID],
)
if config[CONF_MAX_PACKET_SIZE] > FSK_MAX_PACKET_SIZE:
raise cv.Invalid(
f"Without LoRa modulation the radio sends at most {FSK_MAX_PACKET_SIZE} bytes",
@@ -22,6 +22,7 @@ class FakeRadio : public Component {
public:
bool is_receiving() { return this->receiving; }
void register_listener(FakeListener *listener) { this->listener = listener; }
void fail() { this->mark_failed(); }
FakeError transmit_packet(const uint8_t *data, size_t length) {
this->sent.emplace_back(data, data + length);
return FakeError::NONE;
@@ -97,6 +98,13 @@ TEST_F(RadioLinkTest, EmptyPacketCountsAsSentWithoutTransmitting) {
EXPECT_TRUE(this->radio_.sent.empty());
}
TEST_F(RadioLinkTest, FailedRadioRefusesSends) {
this->radio_.fail();
EXPECT_FALSE(this->send_());
EXPECT_FALSE(this->send_());
EXPECT_TRUE(this->radio_.sent.empty());
}
TEST_F(RadioLinkTest, ReceivedPacketCarriesSignal) {
Packet received;
PacketMetaData meta;
+29 -3
View File
@@ -106,7 +106,12 @@ class _FullConfig(dict):
(
sx127x_packet_interface,
"sx127x",
{"modulation": "FSK", "packet_mode": False, "payload_length": 0},
{
"modulation": "FSK",
"packet_mode": False,
"payload_length": 0,
"sync_value": [0x12],
},
{},
"packet_mode",
{"packet_mode": True},
@@ -114,11 +119,30 @@ class _FullConfig(dict):
(
sx127x_packet_interface,
"sx127x",
{"modulation": "OOK", "packet_mode": True, "payload_length": 0},
{
"modulation": "OOK",
"packet_mode": True,
"payload_length": 0,
"sync_value": [0x12],
},
{CONF_MAX_PACKET_SIZE: 65},
"at most 64",
None,
),
(
sx127x_packet_interface,
"sx127x",
{
"modulation": "FSK",
"packet_mode": True,
"payload_length": 0,
"sync_value": [],
"preamble_detect": 0,
},
{},
"sync_value",
{"preamble_detect": 2},
),
],
)
def test_radio_settings_checked(
@@ -129,7 +153,9 @@ def test_radio_settings_checked(
match: str,
fix: dict | None,
) -> None:
"""Packets vary in length, and FSK or OOK on an SX127x needs packet mode and holds at most 64 bytes."""
"""Packets vary in length; FSK or OOK on an SX127x needs packet mode, a sync word or preamble detection, and
holds at most 64 bytes.
"""
config = {f"{domain}_id": "radio", CONF_MAX_PACKET_SIZE: 64, **link}
token = fv.full_config.set(_FullConfig(domain, radio))
try: