Added RSNR to the OSD

[TonyBlit]

On protocols using LoRa (Crossfire, ExpressLRS, etc) there's a signal quality measure (RSNR) that was not properly shown on the OSD. There was an ugly hack for showing RSNR instead of RSSI DBM, but especially on ExpressLRS this is not desirable as the RSSI sensitivity changes depending on the RF mode so you want to see them both. This PR adds SNR as a proper element to the OSD and surprisingly doesn't need extra flash space as a consequence of removing the use_snr hack. It would be nice having customized symbols for RSSI (dbm) and RSSI (snr), but for the time being SYM_RSSI is used. I've noticed a possibly outdated comment here: https://github.com/betaflight/betaflight/blob/5d41f9830bf40cd288662f386812dd050b4a53c4/src/main/osd/osd.h#L169

[KarateBrot]

Very cool, I can directly get a use out of it now that I'm using ELRS.

[mikeller]

Not really sure about this - this is adding a lot of extra code / firmware size to expand something that is already supported in the current version, which will probably only make a difference / be actively used by a very small fraction of users. Have you considered any alternatives (like fixing the 'ugly hack') that avoids adding this much extra code?

[TonyBlit]

I was reluctant to do this for a long time as I also expected the firmware size to grow, but when testing ELRS I decided to give it a go and I was surprised by the numbers.

Master (5d41f9830): text data bss dec hex filename 505983 7632 110932 624547 987a3 ./obj/main/betaflight_STM32F405.elf

snr_osd_element: text data bss dec hex filename 505959 7644 110940 624543 9879f ./obj/main/betaflight_STM32F405.elf

[etracer65]

Minor point, but could you please fix the PR title. You have a typo calling it "RNSR" instead of "RSNR".

[etracer65]

Also, could you please limit the changes to only the purpose of this PR? It looks like you made many minor code formatting changes to unrelated files and in particular the STM libraries. If these things need to be corrected they should be in another PR. And probably not worth fixing minor coding style issues in the STM libraries as they'll just come back if/when the libraries are updated and adds work for us to identify any "true" bug fixes we might have applied to the libraries in the past.

[TonyBlit]

@etracer65 is it acceptable to correct the minor format problems on the files this PR is touching? or is it better to leave them out of this PR as well

[etracer65]

is it acceptable to correct the minor format problems on the files this PR is touching? or is it better to leave them out of this PR as well

Minor fixes in the affected files are fine as long as they're obvious and don't distract from the actual changes. Otherwise reviewing code changes becomes difficult.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

[ctzsnooze]

Sorry guys need to hold this until 4.4

[JyeSmith]

Bump.

Any chance of a rebase so it ready for some testing.

[TonyBlit]

@JyeSmith done!

[TonyBlit]

First rebased onto 4.3 release (in case someone finds it useful), and second time onto master head. Checked flash size on F411: it takes 304 bytes on 4.3 release and 256 bytes on the latest version.

[JyeSmith]

Thanks @TonyBlit

I'll get it flashed this week for some testing.

[blckmn]

AUTOMERGE: (FAIL)

• github identifies PR as mergeable -> FAIL
• assigned to a milestone -> PASS
• cooling off period lapsed -> PASS
• commit count less or equal to three -> FAIL
• Don't merge label NOT found -> PASS
• at least one RN: label found -> PASS
• Tested label found -> FAIL
• assigned to an approver -> FAIL
• approver count at least three -> PASS

[hdavid]

Would it make sense to also implement this for spi based receivers (as proposed in https://github.com/betaflight/betaflight/issues/11711)?

I tried to have a go at it:

index 93815353e6..00a8ab407c 100644
--- a/src/main/rx/expresslrs.c
+++ b/src/main/rx/expresslrs.c
@@ -1012,6 +1012,9 @@ static void handleLinkStatsUpdate(const uint32_t timeStampMs)
 #ifdef USE_RX_RSSI_DBM
             setRssiDbm(receiver.rssiFiltered, RSSI_SOURCE_RX_PROTOCOL);
 #endif
+#ifdef USE_RX_RSNR
+            setRsnr(receiver.snr);
+#endif
 #ifdef USE_RX_LINK_QUALITY_INFO
             setLinkQualityDirect(receiver.uplinkLQ);
 #endif
@@ -1023,6 +1026,9 @@ static void handleLinkStatsUpdate(const uint32_t timeStampMs)
 #ifdef USE_RX_RSSI_DBM
             setRssiDbmDirect(-130, RSSI_SOURCE_RX_PROTOCOL);
 #endif
+#ifdef USE_RX_RSNR
+            setRsnr(-30);
+#endif
 #ifdef USE_RX_LINK_QUALITY_INFO
             setLinkQualityDirect(0);
 #endif

Happy to send a PR if needed.

[dopplet]

Has this one died on the vine? I was looking forward to having both SNR and dBm RSSI on the OSD at the same time. I've been bitten by one or the other being good, and the other bad.

[hdavid]

as the expresslrs 3.0 spi PR has been merged, here is the updated (and fixed the missing "/4") diff for making this work on spi elrs receivers too:

diff --git a/src/main/rx/expresslrs.c b/src/main/rx/expresslrs.c

index 96db5b325..6e70cb422 100644
--- a/src/main/rx/expresslrs.c
+++ b/src/main/rx/expresslrs.c
@@ -949,6 +949,9 @@ static void handleConnectionStateUpdate(const uint32_t timeStampMs)
         receiver.lastSyncPacketMs = timeStampMs;         // reset this variable to stop rf mode switching and add extra time
         receiver.rfModeCycledAtMs = timeStampMs;         // reset this variable to stop rf mode switching and add extra time
         setRssiDirect(0, RSSI_SOURCE_RX_PROTOCOL);
+#ifdef USE_RX_RSNR
+        setRsnr(-30);
+#endif
 #ifdef USE_RX_RSSI_DBM
         setRssiDbmDirect(-130, RSSI_SOURCE_RX_PROTOCOL);
 #endif
@@ -1016,6 +1019,9 @@ static void handleLinkStatsUpdate(const uint32_t timeStampMs)
             receiver.rssiFiltered = simpleLPFilterUpdate(&rssiFilter, receiver.rssi);
             uint16_t rssiScaled = scaleRange(constrain(receiver.rssiFiltered, receiver.rfPerfParams->sensitivity, -50), receiver.rfPerfParams->sensitivity, -50, 0, 1023);
             setRssi(rssiScaled, RSSI_SOURCE_RX_PROTOCOL);
+#ifdef USE_RX_RSNR
+            setRsnr(receiver.snr/4);
+#endif
 #ifdef USE_RX_RSSI_DBM
             setRssiDbm(receiver.rssiFiltered, RSSI_SOURCE_RX_PROTOCOL);
 #endif
@@ -1027,6 +1033,9 @@ static void handleLinkStatsUpdate(const uint32_t timeStampMs)
 #endif
         } else {
             setRssiDirect(0, RSSI_SOURCE_RX_PROTOCOL);
+#ifdef USE_RX_RSNR
+            setRsnr(-30);
+#endif
 #ifdef USE_RX_RSSI_DBM
             setRssiDbmDirect(-130, RSSI_SOURCE_RX_PROTOCOL);
 #endif

[TonyBlit]

thanks @hdavid I'm on sick leave atm, will merge in a few weeks once recovered

[dopplet]

I'm a bit of a git noob.... I am currently building master on a linux host, how do I manually merge this PR into master and test it? (if it's an easy set of steps).

[hdavid]

I'm a bit of a git noob.... I am currently building master on a linux host, how do I manually merge this PR into master and test it? (if it's an easy set of steps).

i did that using github desktop client:

• i forked betaflight repo (not sure if this step is needed)
• cloned using github client
• from the branch menu/tab, you have a "pr" sub tab. pick this PR
• this will create a branch locally from this pr.
• then merge master into this branch or vice versa.

[haslinghuis]

@TonyBlit please rebase

[TonyBlit]

Not fully recovered yet, but just to unblock the PR I've rebased and quickly merged (not tested) @hdavid changes, some notes though:

• setRsnrDirect used when appropriate
• setRssi is filtering the signal although rx.c is filtering it again, looks odd. In any case setRsnr should be filtering the signal for consistency as well. I've left this pending to be done based on feedback

[github-actions[bot]]

Do you want to test this code? Here you have an automated build: Assets WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

[ctzsnooze]

We seem to have a lot of Rx link monitoring data values. What are the units and ranges for each of them - LQ, RSSI, RSSI_DBM, and RX_SNR?

It seems each of these are smoothed with a moving average filter.

There is an alternative PT1 frameErrFilter set by rssi_src_frame_lpf_period where the period defaults to 30 * 0.1s or 3s (very slow). This is applied to RSSI as an alternative to the 16 value median filter, if the period is not zero.

Since it defaults to 30, the median filter is not used for RSSI by default.

RSSI_DBM uses the 16 point median filter. It does not have a PT1 alternative.

A 16 point moving average has a very different smoothing effect at 500hz vs 50hz.

I think that if we add a separate element then some thought should be given to either:

a) making the PT1 alternative available for each of RSSI, RSSI_DBM, and RX_SNR, and retaining the 16 point moving average alternative

b) making the PT1 alternative available for each of RSSI, RSSI_DBM, and RX_SNR, and removing the 16 point moving average alternative to the PT1. Each could have its own independent value, or the cutoff value could be shared. The latter would give equivalent smoothing on all estimates at once.

c) making the moving average 'frequency responsive' and allowing the user to set how strongly it filters (somehow).

Personally I would be in favour of option 2, with one single cutoff.

[TonyBlit]

The frameErrFilter does seem to be used only by setRssi (non dbm) when there are frame errors, which are only triggered by the sbus implementation. Otherwise, the moving average is always used at rx level, which btw I've never been happy with -16 samples is too little to remove jumpiness on crossfire- and worse in elrs for sure, that's why the extra pt1 might be there. I'd just replace the moving average at rx.c for a pt1 properly initialized depending on rx rate, but that's outside of the scope of this PR.

[KarateBrot]

@TonyBlit A PT1 will be better at rejecting high freq noise than a simple average. If a PT1 still doesn't cut it, you can try a biquad lowpass with a Q value of 1/sqrt(3) with a cutoff off approx. sample rate / 16. That will give you a harder cutoff (for better noise rejection) with a very predictable delay. This will be a lot better than a 16 point average - guaranteed. You can then increase the cutoff to sample rate / 8, sample rate / 4, ... if the filter works "too well".

[TonyBlit]

btw, it may be desirable to have a different osd symbol for rssi, rssi_dbm and rssi_snr as it happens on inav. Are there still some free characters on the osd font for this?

[KarateBrot]

They are all occupied afaik. Maybe it makes sense to rearrange and make some room in the near future. We have another feature that would benefit from new OSD characters.

[ctzsnooze]

@TonyBlit let's see if this PR can be resolved for 4.4. Feature freeze is coming up.

[TonyBlit]

Improved the filtering and tested at 50 and 150Hz. Surprisingly, 150Hz doesn't jump 3 times as much on my end, so higher rates might not jump as much as may look like.