AP_OSD: Add CRSF link stats display fields

[rmaia3d]

As described here, knowing both RSSI and LQ is important to know the health of your radio signal. Knowing the current RSSI in dbm allows comparing to receiver sensitivity limit and knowing when getting there, or close.

The CRSF protocol (used by both Crossfire and ELRS radio systems) provides a link stats frame with detailed statistics that include this info and a few other items, but there currently is no way to put them on the OSD display of Ardupilot based systems. This PR adds those fields.

I have used as a base the work already done by @shellixyz in implementing these fields for Arducustom and made some changes and improvements.

Main improvement is detecting when BTFL fonts are being used and automatically replace the non-existent icons in the DJI fontset with direct letter representations (in the LQ and Tx power (uplink) fields). It uses more character space, but since the HD grid is larger, it shouldn't be too much of a problem.

Here's how it looks in BTFL fonts mode:

And in analog mode:

The added fields (RSSI in dBm, Tx uplink power, LQ in % of packets received, uplink SNR in dB and active rx antenna) are highlighted by the red ellipses in the images above.

The fields are individually configurable (no need to use all 5 at once, the user can choose which one they want to use) and also I have added configurable blinking alert levels for the RSSI, SNR and LQ fields.

[Hwurzburg]

Will review in detail later, but the flash cost for 1M boards is pretty big....we might have to make some this a build option and not default...

[rmaia3d]

Will review in detail later, but the flash cost for 1M boards is pretty big....we might have to make some this a build option and not default...

If it helps in that regard, I did all the testing of these changes on two MatekF405 boards (1M flash), both plane and copter (with the copter also running bdshot and IMU batch sampling). :-)

[rmaia3d]

Build Summary comparison between current master branch and the PR branch:

Copter:

- master (1d0fc4e)

Build directory: .../ardupilot/build/MatekF405
Target          Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
----------------------------------------------------------------------------------------------------------
bin/arducopter    877840      2388   128776                880228          102812  Not Applicable

- ap_osd_crsf_lnk_stats (515717c)

Build directory: .../ardupilot/build/MatekF405
Target          Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
----------------------------------------------------------------------------------------------------------
bin/arducopter    879076      2388   128780                881464          101572  Not Applicable

Plane:

- master (1d0fc4e)

Build directory: .../ardupilot/build/MatekF405
Target         Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
---------------------------------------------------------------------------------------------------------
bin/arduplane    920808      2296   128856                923104           59924  Not Applicable

- ap_osd_crsf_lnk_stats (515717c)

Build directory: .../ardupilot/build/MatekF405
Target         Text (B)  Data (B)  BSS (B)  Total Flash Used (B)  Free Flash (B)  External Flash Used (B)
---------------------------------------------------------------------------------------------------------
bin/arduplane    922428      2296   128860                924724           58300  Not Applicable

From these, the total flash impact is from 1.2 to 1.6kB.

[Hwurzburg]

its in the CI test results....1.6KB is considerable...will need to be discussed in dev call

[rmaia3d]

You have a point there, I didn't pay attention to this fact...

[andyp1per]

For analog I don't think this is an improvement - far too much text on the screen. In general I think we are looking for abbreviations that don't take up too much real estate. Also, as Henry says, 1.6Kb is way too much for such a feature.

[andyp1per]

Also the CRSF specific stuff needs to be hidden behind the CRSF define

[rmaia3d]

For analog I don't think this is an improvement - far too much text on the screen. In general I think we are looking for abbreviations that don't take up too much real estate. Also, as Henry says, 1.6Kb is way too much for such a feature.

It is not necessary to put all fields on the screen at once. I only did it to show, in a single image, how they all look. They are individually selectable and configurable.

For analog (and other HD systems that allow custom fonts to be uploaded) @shellixyz has included an extra set of icons in the Arducustom fonts. This greatly reduces the screen real state while retaining full functionality.

I chose to not include these extra icons in this PR to avoid opening another can of worms. I'm not sure how open the core team would be to expanding that set of files, so I settled for the text based compromise without expanding the icons.

For DJI systems (BTFL fonts enabled) this is moot, because that font/icon set is not customizable outside of DJI itself.

[rmaia3d]

Also the CRSF specific stuff needs to be hidden behind the CRSF define

Do you believe this is a feature that could possibly be accepted into the core Ardupilot code? Even as a non-default build option, like @Hwurzburg suggested a few comments back? I understand flash usage is a concern, but I feel there's many people who would like this data on their OSD and don't have the programming expertise to implement it like I did, thus my intention behind this contribution.

I ask this because if there's openness to including, I can rework it under the CRSF define, and see if I can improve the flash footprint as well. But if the core team feels this is really not worth being included, then there's no point in me putting in that time and effort.

[Hwurzburg]

I think that adding these panels as a build option would be fine...those that really want these CRSF RF extensions could get a version off the server...easy change to this PR..would need crsf dependency in build option,among the other obvious ones...

[rmaia3d]

That's great! I believe that a build option is a very good compromise. No unnecessary flash usage but, with just a little bit of extra effort, the feature will be there for those who want it. It is fine by me!

Is this the macro that the CRSF specific code should be reworked under?

#if AP_RCPROTOCOL_CRSF_ENABLED

Also, any other macros/defines I should use to separate out the code of this PR?

[Hwurzburg]

you need to ifdef all the OSD panel additions with a #if AP_OSD_CRSF_EXTENSIONS_ENABLED wrapper...and in the header do an #ifndef to disable it, so its only in when specified in the build server like here

ardupilot/libraries/AP_OSD/AP_OSD_Screen.cpp

Line 352 in 6fb99d6

#if HAL_WITH_ESC_TELEM

• add this opton in build_options.py for the custom build server under Other and make it require CRSF rc option
• add to extract_features.py based on a symbol only present when this feature is compiled in
• probably should ifdef out the added variables also in CRSF RC

[rmaia3d]

you need to ifdef all the OSD panel additions with a #if AP_OSD_CRSF_EXTENSIONS_ENABLED wrapper...and in the header do an #ifndef to disable it, so its only in when specified in the build server like here

Ok, thanks a lot for the pointers! I will make these changes and report back as soon as they are done!

[rmaia3d]

Just added the commits putting the code under the wrappers, so if this isn't chosen amongst the build options, the flash impact is zero. The default is disabled.

Please check and let me know if I have added correctly to the python build scripts (commit f557423).

[andyp1per]

For analog I don't think this is an improvement - far too much text on the screen. In general I think we are looking for abbreviations that don't take up too much real estate. Also, as Henry says, 1.6Kb is way too much for such a feature.

It is not necessary to put all fields on the screen at once. I only did it to show, in a single image, how they all look. They are individually selectable and configurable.

For analog (and other HD systems that allow custom fonts to be uploaded) @shellixyz has included an extra set of icons in the Arducustom fonts. This greatly reduces the screen real state while retaining full functionality.

I chose to not include these extra icons in this PR to avoid opening another can of worms. I'm not sure how open the core team would be to expanding that set of files, so I settled for the text based compromise without expanding the icons.

For DJI systems (BTFL fonts enabled) this is moot, because that font/icon set is not customizable outside of DJI itself.

This is a much better solution - I think you should include the commits from ArduCustom here to support this.

[andyp1per]

Also the CRSF specific stuff needs to be hidden behind the CRSF define

Do you believe this is a feature that could possibly be accepted into the core Ardupilot code? Even as a non-default build option, like @Hwurzburg suggested a few comments back? I understand flash usage is a concern, but I feel there's many people who would like this data on their OSD and don't have the programming expertise to implement it like I did, thus my intention behind this contribution.

I ask this because if there's openness to including, I can rework it under the CRSF define, and see if I can improve the flash footprint as well. But if the core team feels this is really not worth being included, then there's no point in me putting in that time and effort.

I think what you need to do is build some support for this in the forums. I think if 10 people say this is great then that's a very different calculation to just you. I let some stuff of this nature go into CRSF in the past and was never very happy with the outcome - most people found the output spammy and indeed the utility is quite subjective. Obviously having it completely compiled out by default makes it less intrusive, but I would rather we got this right from the get go than having to live with something sub-optimal. I do know that people like having both RSSI db and LQ in CRSF, so I am receptive to the overall idea.

[rmaia3d]

This is a much better solution - I think you should include the commits from ArduCustom here to support this.

Yes, I agree it is best solution so far. I have copied over the fonts from Arducustom, including all of the added symbols there. There's an extra 16 symbols, not all of them being used by this PR, but it does leave room for even more improvements and customizations in the future.

The new font files and the code additions to support them are in commits e9e75cc and cc58599.

However, these additions and the ELRS RFMD tables expansion (in commit 4c4f167) are not under the CRSF_EXTENSIONS or any other define, they would be global to all builds. Combined, including the new font files, this results in a 200 byte increase in flash usage.

Here's how the updated icons look in analog OSD (second image shows with RFMD prefixing the LQ value, added via commit d2ca7ee):

[rmaia3d]

I think what you need to do is build some support for this in the forums. I think if 10 people say this is great then that's a very different calculation to just you. I let some stuff of this nature go into CRSF in the past and was never very happy with the outcome - most people found the output spammy and indeed the utility is quite subjective. Obviously having it completely compiled out by default makes it less intrusive, but I would rather we got this right from the get go than having to live with something sub-optimal. I do know that people like having both RSSI db and LQ in CRSF, so I am receptive to the overall idea.

Ok, that sounds fair! Initially, the idea was indeed to just implement the LQ and RSSI dbm panels, however I realized the work involved in implementing all of them wasn't that much, so it was a "why not" kind of decision.

Meanwhile, I have added further commits with more improvements (described in the previous comment) and also some code reorganizations for improved readability and organization.

I have separated out the methods for drawing the new panels under BTFL fonts mode, where there aren't convenient symbols for labelling the panels. So there's the "normal" method which uses the custom symbols (available in analog and HD systems like WS and HDZero) and there's the _btfl method which uses direct letter representations without any custom symbols (which only DJI could implement). The choice between one method or the other is automatically done based on the BTFL fonts OSD setting being enabled or not.

Since the HD screen has a lot more pixel real-state, I believe the space used by the extra characters (instead of the single character icons) is not that bad and is a good compromise. The extra methods increased a little the flash footprint, but in my opinion the improved organization and tidiness is worth it.

[andyp1per]

Looking better! Some comments added

[Hwurzburg]

Also commits need to be squashed and Tools/gittools/got-subsystems-split used

[rmaia3d]

Also commits need to be squashed and Tools/gittools/got-subsystems-split used

Do you mean squash everything into a single commit and then run the git-subsystems-split tool? Is there any reference for usage of this script?

[Hwurzburg]

Also commits need to be squashed and Tools/gittools/got-subsystems-split used

Do you mean squash everything into a single commit and then run the git-subsystems-split tool? Is there any reference for usage of this script?

yes....its not is the wiki, but I will add it to the contributing code section....I dont speed a lot of time on the dev wiki since all my time is consumed on making the user side more friendly....and it could use some attention...

[rmaia3d]

Ok, so I created a local test branch to experiment on and this is the result I got after squashing all the 22 commits in this PR to a single one and then running the git-subsystems-split script:

After that, git log --oneline outputs this:

Is this the expected outcome?

[Hwurzburg]

yep...but you need to fix the merge conflict in extract features, and do the squash and split again

[rmaia3d]

yep...but you need to fix the merge conflict in extract features, and do the squash and split again

Ok, thanks! I have rebased my branch to the current state of master (as of now), fixed the conflicts, squashed and split. :-)

[andyp1per]

Looking good! A few small things to fix

[lida2003]

@rmaia3d If I wanna try on tag 4.4.4, what steps should I do?

a) cherry-pick this commit
b) define AP_OSD_LINK_STATS_EXTENSIONS_ENABLED in hwdef, is there any other macro needed?
c) build a custom binary for update

am i right?

[rmaia3d]

@rmaia3d If I wanna try on tag 4.4.4, what steps should I do?

a) cherry-pick this commit b) define AP_OSD_LINK_STATS_EXTENSIONS_ENABLED in hwdef, is there any other macro needed? c) build a custom binary for update

am i right?

Yes, cherry-picking would be the easier way. Cherry-pick all 4 commits and it should do it. You may encounter some merge conflicts in the process, but easily resolved.

For item b) adding the define to the hwdef file works, it is one way to do it. Or you can also change in AP_OSD_Config.h the define value from 0 to 1. This will make the extra OSD panels available for all boards when building the custom binary, without the need to change each hwdef file.

For c), yes, you will need to build locally a custom binary and upload it to the board. If you don't already have a local build environment set up, this docs page has detailed instructions.

addressed

PEterB's requests addressed

[peterbarker]

Suggested change

	#if AP_OSD_EXTENDED_LNK_STATS
	#if AP_OSD_EXTENDED_LNK_STATS_ENABLED

We'll make this change in a future PR....

[rmaia3d]

Do you mean you think this is a typo or you'd rather have the define renamed?

[rmaia3d]

There's a user request here to backport this to 4.5.x.

Should I create another PR backporting this to the 4.5.x branch or this will be handled by the dev team without the need of another PR from me?

[rmackay9]

@rmaia3d,

I've added it to the 4.5 project so it will be considered for backporting. If it can be backported to the 4.5 without merge conflicts then no need for a new PR. I'll usually ask for help if it can't be backported. Txs!

[rmackay9]

I did not add this to 4.5.2-beta1 due to a merge conflict

[rmaia3d]

I did not add this to 4.5.2-beta1 due to a merge conflict

Ok, I tried adding these changes to the current Plane-4.5 branch and indeed some merge conflicts showed up in two files (AP_RCProtocol_config.h and extract_features.py). All of them related to features that are present in the 4.6 codebase, but not in the 4.5. I simply removed the "offending" lines and was able to merge and build successfully.

Here's the branch with the merged changes, on my account. Would you like me to open a PR into Plane-4.5 from this branch?

https://github.com/rmaia3d/ardupilot/tree/pr/4.5/osd_lnk_stats

[andyp1per]

Yes open a PR I think

[rmaia3d]

Yes open a PR I think

Ok!! Done, #26912

[lida2003]

I did not add this to 4.5.2-beta1 due to a merge conflict

Hi @rmaia3d @andyp1per @rmackay9 Currently Copter-4.5.2 doesn't have this feature right? Cause I didn't find those macros:AP_OSD_EXTENDED_LNK_STATS AP_OSD_EXTENDED_LNK_STATS_ENABLED

[rmaia3d]

I did not add this to 4.5.2-beta1 due to a merge conflict

Hi @rmaia3d @andyp1per @rmackay9 Currently Copter-4.5.2 doesn't have this feature right? Cause I didn't find those macros:AP_OSD_EXTENDED_LNK_STATS AP_OSD_EXTENDED_LNK_STATS_ENABLED

Yes, you are correct. It hasn't been included in 4.5.2 but should be in 4.5.3.

[andyp1per]

@rmaia3d there appears to be a problem with the WS font which is part of this PR - the armed and disarmed symbols appear to be the wrong way round. When disarmed I get a little flying prop symbol and when armed, nothing.

[Hwurzburg]

Thats the normal way of showing armed/disarmed....same with AP OSDs....additional symbol following mode when disarmed goes away when armed

[rmaia3d]

@rmaia3d there appears to be a problem with the WS font which is part of this PR - the armed and disarmed symbols appear to be the wrong way round. When disarmed I get a little flying prop symbol and when armed, nothing.

As @Hwurzburg has said, it looks like this is indeed the normal and expected behavior.

Looking through the code, the SYM_ARMED icon is 0x00, which both on the analog and Walksnail fonts maps to a blank space (also in BTFL fonts mode). When disarmed (SYM_DISARMED), the symbol changes, becoming a "framed" D in the analog fonts and the little prop with a red line going over it in Walksnail (in BTFL mode, for the DJI O3 system, it is mapped to a character that resembles an asterisk).

This DISarmed icon in Walksnail from the fonts included in this PR is this one:

[lida2003]

@rmaia3d

I tried RC_LQ and LINK_Q, and didn't see LQ 4:100 in my 4.5.4 + Backport OSD extended link stats to 4.5.x #26912 custom build.

Anyway to get 4:100?

[rmaia3d]

Anyway to get 4:100?

@lida2003 Yes! You need to go into the full parameter list, under the OSD_OPTIONS parameter and enable this option (which is bit 7):

Then the RC_LQ panel will display as X:Y where X is the RF Mode number and Y is the actual LQ %age level. For example, 4:100.