Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor issue in startReceiveDutyCycleAuto #1100

Closed
tve opened this issue May 15, 2024 · 3 comments
Closed

Minor issue in startReceiveDutyCycleAuto #1100

tve opened this issue May 15, 2024 · 3 comments
Labels
more information needed Further investigation is needed, not necessarily by the author

Comments

@tve
Copy link

tve commented May 15, 2024

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

if(2 * minSymbols > senderPreambleLength) {

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...)

@jgromes
Copy link
Owner

jgromes commented May 15, 2024

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.

@tve
Copy link
Author

tve commented May 15, 2024

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.

@jgromes
Copy link
Owner

jgromes commented May 26, 2024

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 jgromes closed this as not planned Won't fix, can't repro, duplicate, stale May 26, 2024
@jgromes jgromes added the more information needed Further investigation is needed, not necessarily by the author label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more information needed Further investigation is needed, not necessarily by the author
Projects
None yet
Development

No branches or pull requests

2 participants