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

LR1110 anomalously high SNR #1161

Closed
thebentern opened this issue Jul 13, 2024 · 15 comments
Closed

LR1110 anomalously high SNR #1161

thebentern opened this issue Jul 13, 2024 · 15 comments
Labels
bug Something isn't working resolved Issue was resolved (e.g. bug fixed, or feature implemented)

Comments

@thebentern
Copy link

thebentern commented Jul 13, 2024

Hi Jan,

We just added support in Meshtastic for some of the Seeed studio boards featuring the LR111X series of radios thanks to your work in adding support within RadioLib. 😄

One of things we've noted since then, is that the SNR values seem oddly high
meshtastic/firmware#4284

I did a cursory dig into the codebase to see if anything obvious popped up and the only thing I found in comparison to the SX126X family is that there is no if(snrPkt < 128) logical analogue in the LR111X family, but I wasn't entirely sure if that had more to do with the uint8 boxing in contrast to the amply sized float buffer we have on the LR111X. Anyway, I may be barking up the wrong tree, but I thought I'd point it out just in case it lead to anything. 😅
https://github.com/jgromes/RadioLib/blob/d1bfccd6128f8fc9e35aa29a0d0c21bb077d7af0/src/modules/LR11x0/LR11x0.cpp#L2377C1-L2378C1

if(snrPkt < 128) {

@jgromes
Copy link
Owner

jgromes commented Jul 13, 2024

It is probably a bug, I was not entuirely sure how to do this; though the LR1110 User manual is not 100% clear on this point. Could you try to change the calculation to reflect the one for SX126x, or use the equation shown in the user manual (below)?

Screenshot_99

@jgromes jgromes added the bug Something isn't working label Jul 13, 2024
@fifieldt
Copy link

fifieldt commented Jul 14, 2024

Idea: since it's two's complement, and SNR is negative, do we need to undo that?

Logic: Real SNR is -8dBm, but we're currently getting 56 from RadioLib

# Undo RadioLib /4.0f: 
56*4 = 224

# 224 in binary
11100000

# invert bits
00011111

# add 1 (=32 decimal)
100000

# divide by 4
32/4 = 8

@fifieldt
Copy link

fifieldt commented Jul 14, 2024

As a data point, getting larger SNRs from RadioLib for worse reception, so it does seem like the value may be inverted.

@jgromes
Copy link
Owner

jgromes commented Jul 14, 2024

@fifieldt PRs are welcome of course ;)

@lyusupov
Copy link
Contributor

@fifieldt
Copy link

Thanks @lyusupov . Giving that a test

@fifieldt
Copy link

I tried:
if(snrPkt) { *snrPkt = (float)((buff[1] + 2) >> 2); }

and compared the results to a co-located non-LR1110 device. This table shows the RSSI/SNR for the same nodes for the two different devices:

Node non-LR1110 LR1110
Xc -56/11.0 -64/10.0
Xa42 -85/12.8 -71/10.0
XYS1 -100/9.0 -112/44.0

@GUVWAF
Copy link
Contributor

GUVWAF commented Jul 15, 2024

buff is a uint8_t array, which is not two’s complement.
I believe you’ll have to cast it to an int8_t before casting it to a float:

if(snrPkt) { *snrPkt = (float)((int8_t)buff[1]) / 4.0f; }

(Don’t have the hardware to confirm yet.)

@fifieldt
Copy link

fifieldt commented Jul 15, 2024

OK, I think @GUVWAF is on to something :) Here are the test values using the code above:

Node non-LR1110 LR1110
Xc -56/11.0 -76/9.3
Xa42 -85/12.8 -71/10.3
XYS1 -100/9.0 -112/-8.5

The 44 SNR became -8.5, with the rest more or less the same.

If that last value was positive rather than negative it'd be the same as a non-LR1110 radio.

@GUVWAF
Copy link
Contributor

GUVWAF commented Jul 15, 2024

Hmm, only 2dB difference in SNR for 44dB difference in RSSI as reported by the non-LR1110 device looks strange. The 17.8dB difference in SNR reported now by the LR1110 for a difference of 36dB RSSI sounds more reasonable to me.

Are you using Meshtastic for the RSSI/SNR reports? It can be a bit tricky because SNR is stored persistently on the device, while you only get RSSI from a new incoming packet. If there are nodes with firmware <2.3 the RSSI/SNR can also be from the last relayer instead of the actual transmitter. Best to check the serial logs to be sure to get the RSSI and SNR from the same packet.

@jgromes
Copy link
Owner

jgromes commented Jul 15, 2024

@GUVWAF

I believe you’ll have to cast it to an int8_t before casting it to a float

Most likely yes, I think I probably didn't notice this issue in testing as my devices are right next to each other, so it's quite rare for me to get SNR < 0 on my test setup.

@fifieldt

What is the "non-LR1110" device? I agree with @GUVWAF that SNR of 9 dB at -100 dBm looks a bit suspicious.

@fifieldt
Copy link

fifieldt commented Jul 15, 2024

Thanks for taking a look, @GUVWAF and @jgromes .

No caching or firmware issues here. The test device is freshly flashed via SWDIO (i.e full erase) release pre2.4.0, before each test. The others devices are 2.3.15.

Xc (Heltec Wireless Tracker 1.1, 3.5dBi omni) and Xa42 (wio-tracker-wm1110, 3.5dBi omni) are in the same room as the test device (wio-sdk-wm1110, terrible stub antenna).

XYS (RAK4631, 5.8dBI omni) is on a 6m mast 3 stories up from the test site.

@fifieldt
Copy link

Sorry, the non-LR1110 Device is a Tbeam, also with a terrible stub antenna.

@jgromes
Copy link
Owner

jgromes commented Jul 16, 2024

I did a little test of my own, here are the results. As the transmitter, I used SX1262 with output power set to 0 dBm and -9 dBm so that I can get lower RSSI/SNR values. I tried couple different possible calculations, as well as the one from the reference driver.

Result for 0 dBm transmission:

18:33:42.406 -> RLB_DBG: RSSI
18:33:42.406 -> RLB_DBG: (float)buff[0] / -2.0f = -110.00
18:33:42.406 -> RLB_DBG: (float)((int8_t)buff[0]) / -2.0f = 18.00
18:33:42.406 -> RLB_DBG: -( int8_t )( buff[0] >> 1 ) = -110
18:33:42.406 -> RLB_DBG: SNR
18:33:42.406 -> RLB_DBG: (float)buff[1] / 4.0f = 5.00
18:33:42.406 -> RLB_DBG: (float)((int8_t)buff[1]) / 4.0f = 5.00
18:33:42.406 -> RLB_DBG: (float)((int8_t)buff[1]) / -4.0f = -5.00
18:33:42.406 -> RLB_DBG: ( ( ( int8_t ) buff[1] ) + 2 ) >> 2 = 5

Result for -9 dBm transmission:

18:34:01.071 -> RLB_DBG: RSSI
18:34:01.071 -> RLB_DBG: (float)buff[0] / -2.0f = -115.00
18:34:01.071 -> RLB_DBG: (float)((int8_t)buff[0]) / -2.0f = 13.00
18:34:01.071 -> RLB_DBG: -( int8_t )( buff[0] >> 1 ) = -115
18:34:01.071 -> RLB_DBG: SNR
18:34:01.071 -> RLB_DBG: (float)buff[1] / 4.0f = 61.75
18:34:01.071 -> RLB_DBG: (float)((int8_t)buff[1]) / 4.0f = -2.25
18:34:01.071 -> RLB_DBG: (float)((int8_t)buff[1]) / -4.0f = 2.25
18:34:01.071 -> RLB_DBG: ( ( ( int8_t ) buff[1] ) + 2 ) >> 2 = -2

So it looks like casting to int8_t as proposed by @GUVWAF produces the value that matches the reference for both positive and negative SNRs, I'll push that as the fix for this issue. The RSSI calculation looks OK as it is.

jgromes added a commit that referenced this issue Jul 16, 2024
@jgromes
Copy link
Owner

jgromes commented Jul 16, 2024

Fixed in the latest commit ;)

@jgromes jgromes closed this as completed Jul 16, 2024
@jgromes jgromes added the resolved Issue was resolved (e.g. bug fixed, or feature implemented) label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved Issue was resolved (e.g. bug fixed, or feature implemented)
Projects
None yet
Development

No branches or pull requests

5 participants