jgromes / RadioLib

Universal wireless communication library for embedded devices
https://jgromes.github.io/RadioLib/
MIT License
1.48k stars 373 forks source link

LR1110 anomalously high SNR #1161

Closed thebentern closed 1 month ago

thebentern commented 1 month ago

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 https://github.com/meshtastic/firmware/issues/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 https://github.com/jgromes/RadioLib/blob/d1bfccd6128f8fc9e35aa29a0d0c21bb077d7af0/src/modules/SX126x/SX126x.cpp#L1357

jgromes commented 1 month ago

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

fifieldt commented 1 month ago

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 commented 1 month ago

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

jgromes commented 1 month ago

@fifieldt PRs are welcome of course ;)

lyusupov commented 1 month ago

an excerpt from reference Lora-net driver -

https://github.com/Lora-net/lr1110_driver/blob/master/src/lr1110_radio.c#L290-L295

fifieldt commented 1 month ago

Thanks @lyusupov . Giving that a test

fifieldt commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

jgromes commented 1 month ago

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 commented 1 month ago

Fixed in the latest commit ;)