Bug | SNR Value

[jlpoltrack]

Was looking at my EdgeTX logs and the TSNR value seems to show odd values. When I believe that it should be a negative number it doesn't seem to be the converted correctly, example:

I looked through the code and I can't seem to pinpoint where this issue is. The driver should be returning signed values: https://github.com/olliw42/sx12xx-lib/blob/main/src/sx128x.h#L105

Looks okay here too: https://github.com/olliw42/mLRS/blob/main/mLRS/Common/common_stats.h#L131

[olliw42]

I can't speak for EdgeTx, but for OpenTx it does display it positive. It's actually because CRSF uses it as positive. Watch out for the crsf_cvt_rssi() function. The driver returns it as -68, crsf as 68.

You should see that "wrong" sign with the other rssi values too.

I think this is correct.

[jlpoltrack]

I'm okay with the positive RSSI, but I think that the expected SNR values should be something like -15 to 15. Here's the deduplicated values across the logs (I think 63 should be -1, 62 > -2, etc, offset by 64):

[olliw42]

?? I don't understand I believe it just returns the snr values which it gets from the driver...

[jlpoltrack]

So I think I found the issue, but let me start with why I was confused. Per the datasheet SNR is a signed 8-bit value multiplied by 4:

So the SNR range should be -127 / 4 to +127 / 4 or -31 to +31 (note that values of 53 to 63 should be impossible).

This line in the module divides by 4 first then does the conversion to signed 8-bit. It should do the conversion first then divide by 4. https://github.com/olliw42/sx12xx-lib/blob/3e0512f6d11339bb5fcfa4c087dd0d6d2ce9ccbe/src/sx126x.cpp#L329

When I update the line to this, I can see a negative SNR using stats (very helpful :) )

Hope this makes sense

[olliw42]

interesting

a quick google gave in another driver ( status[1] < 128 ) ? ( packetStatus->LoRa.SnrPkt = status[1] / 4 ) : ( packetStatus->LoRa.SnrPkt = ( ( status[1] - 256 ) /4 ) );

[jlpoltrack]

That method returns the same values as my code but seems a bit complex. Here's some comparisons with the original, my code and the version from another driver:

[olliw42]

excellent

is there a similar issues in the other drivers too?

I'm still pondering about this corrected thing mybe we introduce a function GetStatusRaw, or GetRssiSnr, or...

[OptimusTi]

Make sure to actually measure/know your noise floor and compare values with SNR and RSSI. You may find something very interesting.

[jlpoltrack]

I believe this issue exists currently in the SX128x and SX126x drivers. The SX127x appears to do it correctly (but haven't validated this on hardware):

https://github.com/olliw42/sx12xx-lib/blob/main/src/sx127x.cpp#L322

[olliw42]

@OptimusTi many thx for your comment and sharing your insight. It would be great however if you just could tell us ... going by solving riddles is not always the most efficient approach :D

[OptimusTi]

"Make sure to actually measure" is not a riddle. It's just a recommendation. It's the only way to make sure that the values are correct.

[olliw42]

:) and what does the "You may find something very interesting" refer too, if I may ask?

I am unfortunately not in a position to actually measure such things, sadly.

[jlpoltrack]

Somewhat related, I don't think that RSSI for 1RSS needs to be inverted here: https://github.com/olliw42/mLRS/blob/main/mLRS/Common/common_types.cpp#L168

If I remove the inversion, EdgeTX handles 1RSS correctly (see also this note from ExpressLRS: https://github.com/ExpressLRS/ExpressLRS/blob/master/src/src/tx_main.cpp#L152)

Edit: Alright, this overflows when RSSI is 0 (very close + high power) due to constrain here: https://github.com/olliw42/mLRS/blob/main/mLRS/Common/common_types.h#L89

[jlpoltrack]

Addressed with: https://github.com/olliw42/sx12xx-lib/pull/4