Fix the unsigned underflow in calcMaxPacketMillis()'s airtime fallback - #12
Open
mmmorks wants to merge 1 commit into
Open
Fix the unsigned underflow in calcMaxPacketMillis()'s airtime fallback#12mmmorks wants to merge 1 commit into
calcMaxPacketMillis()'s airtime fallback#12mmmorks wants to merge 1 commit into
Conversation
The fallback said "4 secs" and computed 4000 - preamble_us, i.e. 4 ms minus
a preamble that exceeds it at every setting there is. The subtraction is
unsigned, so the result is not a mis-sized deadline but the absence of one:
at SF7/BW250 preamble_us is 22656, 4000 - 22656 wraps to 4294948640 us, and
the later (payload_us * 8) / cr overflows again, leaving a payload watchdog
of roughly 49 days. CustomSX1262::isReceiving() uses that deadline to clear
a latched HEADER_VALID, so a header IRQ that never completes into a packet
would hold the channel "busy" for the life of the node.
Make the fallback a flat 4 s of payload (MAX_PACKET_FALLBACK_PAYLOAD_US,
overridable per platform), with no subtraction left to underflow.
This hardens a latent path rather than fixing an observed failure: the
branch is only reached when getTimeOnAir(MAX_TRANS_UNIT) <= preamble_us,
which happens on an unconfigured modem, and every caller in the tree
(Custom{SX1262,SX1268,LLCC68,LR1110,LR2021,STM32WLx}Wrapper::setParams)
configures freq/bw/sf/cr first. It is one setParams ordering change away
from being live, and the failure mode is silent and permanent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
getTimeOnAir(MAX_TRANS_UNIT)returns nothing usable,calcMaxPacketMillis()falls back to what the comment calls "4 secs at worst case" — but computes4000 - preamble_us, i.e. 4 ms minus a preamble that exceeds it at every setting there is. The subtraction is unsigned.At SF7/BW250,
preamble_usis 22656, so4000 - 22656wraps to4294948640µs, and the following(payload_us * 8) / croverflows again. The result is not a mis-sized deadline but the absence of one: a payload watchdog of roughly 49 days.CustomSX1262::isReceiving()uses that deadline to clear a latchedHEADER_VALID, so a header IRQ that never completes into a packet would hold the channel "busy" for the life of the node.The fallback becomes a flat 4 s of payload —
MAX_PACKET_FALLBACK_PAYLOAD_US, overridable per platform — with no subtraction left to underflow. Long on purpose: this deadline exists to break a stuck header IRQ, and one that expires early would clear the flags of a packet still arriving.Scope
This hardens a latent path rather than fixing an observed failure, and I would rather say so than overstate it. The branch is only reached when
getTimeOnAir(MAX_TRANS_UNIT) <= preamble_us, which happens on an unconfigured modem, and every caller in the tree (Custom{SX1262,SX1268,LLCC68,LR1110,LR2021,STM32WLx}Wrapper::setParams) configures freq/bw/sf/cr first. It is onesetParamsordering change away from being live, and the failure mode is silent and permanent.How it was tested
pio test -e native: all suites pass.meshnology_w12_repeater(ESP32-S3).Dependencies
Independent. Applies to
dev.