RadioLib icon indicating copy to clipboard operation
RadioLib copied to clipboard

Minor issue in startReceiveDutyCycleAuto

Open tve opened this issue 1 year ago • 2 comments

The calculation of sleepSymbols and the check whether to sleep at all are not fully in agreement:

https://github.com/jgromes/RadioLib/blob/23dcc4b8b7e29344c3692c4bab8b5db579bdf4cb/src/modules/SX126x/SX126x.cpp#L636

Specifically, if preamble length == 2*minSymbols then the sleepSymbols=0 but the test whether to sleep fall through. It should test for >= instead of >. It's most likely harmless, just a bit confusing. (Meshtastic actually falls exactly into the == case...)

tve avatar May 15 '24 15:05 tve

Thank you for the report! The proposed fix does make sense and it's quite easy to fix, however, have you tested this actually has the expected impact? If Meshtastic really is that corner case, then it would be better to test this before applying the change.

jgromes avatar May 15 '24 16:05 jgromes

I have not tested, I know Meshtastic uses the startReceiveDutyCycleAuto (https://github.com/meshtastic/firmware/blob/master/src/mesh/SX126xInterface.cpp#L266) and I was trying to understand the power savings, but then discovered that there's no sleep time and thus no power savings. @GUVWAF confirmed in discord "Yes, I think indeed it doesn’t sleep. However, quite some time ago I tried to change it to the normal startReceive(), but that lead to issues. Not sure if it’s still the case." I simply thought to flag the minor issue. As far as I can tell, it will still simply call startReceive at https://github.com/jgromes/RadioLib/blob/master/src/modules/SX126x/SX126x.cpp#L662 since sleepPeriod will be zero.

tve avatar May 15 '24 17:05 tve

If this hasn't been tested, then I would much rather keep this unchanged (even if there is an off-by-one error) - especially because as per your quote:

tried to change it to the normal startReceive(), but that lead to issues

We need more information about the impact of this and we need to know whether this is an actual problem that needs solving.

jgromes avatar May 26 '24 12:05 jgromes