iNavFlight / OpenTX-Telemetry-Widget

INAV Lua Telemetry with support for EdgeTX
https://luatelemetry.readthedocs.io/en/latest/
GNU General Public License v3.0
156 stars 32 forks source link

Add check for 2RSS to improve functionality with ELRS true diversity receivers #78

Closed rmaia3d closed 1 year ago

rmaia3d commented 1 year ago

ELRS True diversity receivers only have one "side" active at a time (they are actually two Rx's with their own individual antennas on the same PCB, and switch which "side" is active based on signal strength) , so they report either 1RSS or 2RSS, with the other sometimes being 0, specially at startup.

If, during startup, only 2RSS is active and reported, the widget doesn't recognize the Rx connection and considers as "No telemetry active" since it only checks for a non-zero value in 1RSS (which, being inactive, is reported as 0). When, in fact, there is telemetry active and being sent normally, it is just coming via the 2RSS "side".

In cases like this, depending on the Rx environment, it can take a while until the 1RSS "side" is activated (it will usually only be once the RF link strength on the 2RSS "side" falls significantly). But since the 2RSS "side" is active and working properly, the widget behavior can be weird or misleading. This is the behavior I experienced with two distinct Happymodel EP1 Dual TCXO's ELRS RX units.

This change aims to fix that, by checking that both 1RSS and 2RSS are zero before considering that there is no telemetry present. If either of them is non-zero (i.e. active), it considers as a normal connection.

I have tested with both EP1 Duals and some other non-diversity ELRS Rx's and it seems to work properly, but further testing is more than welcome. Specially testing with TBS Crossfire RX's, which I don't own any and can't test to see if my changes to the code break any functionality with them.

rmaia3d commented 1 year ago

Thanks. Can you normalise the test so both 1RSS and 2RSS check for 0 or nil ? Just for consistency.

You are welcome!! Just added another commit with the normalisation, let me know if that is what you meant.

stronnag commented 1 year ago

Apologies, I wasn't very clear as to what I was thinking, rather than testing for 0 and nil, we should do both test in the same way (so either check for 0 or nil, probably 0 as we're checking the data value).

Then I thought a bit more:

For the first two bullets, something like:

if getValue(data.rssi_id) == 0 or getValue(data.rssi_id) == nil then
        --[[ Also check for RSSI2, in case RSSI2 is active with RSSI1 being inactive
             (Sometimes happens with True Diversity ELRS receivers on startup) ]]
        local rssi2 = getTelemetryId("2RSS")
        if rssi2 > -1 and  getValue(rssi2) == 0 then
            data.rssi = 0
            data.telem = false
            return 0
        end
    end

Then (for the final bullet), consider (as you have the hardware, I don't)

               ...
               if rssi2 > -1 and  getValue(rssi2) == 0 then
            data.rssi = 0
            data.telem = false
            return 0
                else
                      -- prefer Ant. 2
                      data.rssi_id = rssi2
        end
rmaia3d commented 1 year ago

Sorry for not replying earlier!

For the first two bullet points, yes, maybe the check for nil is really not necessary. The logic behind it was that, if it is a non-diversity receiver which reports only (and always) 1RSS, I'm not sure if the getValue() function will return nil. So, for that case, I put the nil check there. However, if even with the sensor not present, getValue() will return 0 instead of nil, then that is totally redundant.

For the third bullet point, I don't think it's a good idea to switch to 2RSS "permanently" because the Rx keeps changing between 1RSS and 2RSS according to which side has a better a reception. So, even though it may startup in 2RSS, as soon as 1RSS gets a better signal, it will switch. And again, if on 1RSS and suddenly 2RSS starts getting a better signal, it will deactivate 1RSS and activate 2RSS. And so on.

With all that said, I made a similar suggestion to the "official" ELRS widget and Bryan Mayland came up with an even better idea. Instead of checking both RSS parameters (with all the above "implications"), this check could be made just on the TPWR parameter. This way, it doesn't matter if it's a single Rx, diversity Rx or even a dual Rx. All of them report just a single TPWR value. It should work perfectly fine with all ELRS Rx's. My only doubt with this idea is in the case of Crossfire receivers, since I don't have any of them to test, I don't know if this would work with them as well. If their behavior is the same (report TPWR only when powered up), then this could be the solution!

The code would then look like this:


if getValue(data.tpwr_id) == 0 then
    data.rssi = 0
    data.telem = false
    return 0
end`
rmaia3d commented 1 year ago

Pushed a commit with the check on TPWR instead. Ran some bench tests and it seems to work just fine with the ELRS receivers I have. Still remains to be tested on Crossfire receivers, though.

Junnicutt commented 1 year ago

Pushed a commit with the check on TPWR instead. Ran some bench tests and it seems to work just fine with the ELRS receivers I have. Still remains to be tested on Crossfire receivers, though.

For ELRS TPWR is only transmitted when using Wide switch mode. If Hybrid mode is used then TPWR isn't transmitted. This may be the case as well for the full resolution channel modes (aka the Full res 100HZ and 333HZ modes), but I can't say for certain.

I don't think many people use the hybrid mode anymore but it's worth at least thinking about. I think a lot of people will use the full res modes though so that would just need to be checked.

Great job with this PR, these full diversity ELRS rx's are great so this is definitely something that would have affected a lot of people.

rmaia3d commented 1 year ago

Pushed a commit with the check on TPWR instead. Ran some bench tests and it seems to work just fine with the ELRS receivers I have. Still remains to be tested on Crossfire receivers, though.

For ELRS TPWR is only transmitted when using Wide switch mode. If Hybrid mode is used then TPWR isn't transmitted. This may be the case as well for the full resolution channel modes (aka the Full res 100HZ and 333HZ modes), but I can't say for certain.

I don't think many people use the hybrid mode anymore but it's worth at least thinking about. I think a lot of people will use the full res modes though so that would just need to be checked.

Great job with this PR, these full diversity ELRS rx's are great so this is definitely something that would have affected a lot of people.

Good catch!! I had only tested with wide switch mode, and didn't test any of the full resolution modes.

So, I went to test them. And all of them, including hybrid switch and full resolution modes do report TPWR as a telemetry sensor. So the code as is in the pull request, does work for all ELRS modes. At least on my tests.

I will see if I can get any of the ELRS devs to chime in here with a more accurate assessment.

stronnag commented 1 year ago

@rmaia3d let me know when you're done / content and the PR will be merged.

cbf123 commented 1 year ago

So is the documentation at https://www.expresslrs.org/software/dynamic-transmit-power/ wrong, or are we looking at TPWR as reported by the TX module to the TX (i.e. the receiver is not really involved)?

If the latter, how does checking tpwr_id give any info about whether we're getting telemetry from the receiver?

Junnicutt commented 1 year ago

So is the documentation at https://www.expresslrs.org/software/dynamic-transmit-power/ wrong, or are we looking at TPWR as reported by the TX module to the TX (i.e. the receiver is not really involved)?

If the latter, how does checking tpwr_id give any info about whether we're getting telemetry from the receiver?

You won't see any of the telemetry data until a connection to the receiver is made. I've never understood why exactly. It seems like things like it output power and maybe rf mode would at least show up but I've never seen any telemetry values until the link was made and only while it was still working. I know Crossfire and ELRS both do this. I think it has something to do with the fact that all the values that are sourced from the tx are actually sent to the Rx and then return with the other telemetry data. But I have no clue why it would be like this.

rmaia3d commented 1 year ago

So is the documentation at https://www.expresslrs.org/software/dynamic-transmit-power/ wrong, or are we looking at TPWR as reported by the TX module to the TX (i.e. the receiver is not really involved)? If the latter, how does checking tpwr_id give any info about whether we're getting telemetry from the receiver?

You won't see any of the telemetry data until a connection to the receiver is made. I've never understood why exactly. It seems like things like it output power and maybe rf mode would at least show up but I've never seen any telemetry values until the link was made and only while it was still working. I know Crossfire and ELRS both do this. I think it has something to do with the fact that all the values that are sourced from the tx are actually sent to the Rx and then return with the other telemetry data. But I have no clue why it would be like this.

After getting input from some ELRS devs (at the ELRS Discord), this is exactly the case. The TX module will only send telemetry packets to EdgeTX/OpenTX when it detects a link has been established with the Rx. No link, no telemetry packets. The Rx isn't involved in this "transaction" (i.e., the data doesn't get sent to the Rx and then comes back via the OTA link), it is directly from the Tx module to the handset OS via the module's CRSF connection. And since the Rx is not involved, it doesn't matter what OTA mode the link is operating, be it wide, hybrid switch or full resolution modes. The TX module will always report TPWR in the packets sent to the handset. Again, this is the packet sent from the Tx module to EdgeTX/OpenTX, not the packet coming OTA from the Rx to the Tx module. Maybe here is the source of confusion, it's a different, "internal" packet.

@stronnag With this confirmation, I believe the PR can be merged successfully. :+1

Junnicutt commented 1 year ago

That makes sense. I was thinking of the TPWR bring sent for osd. Sorry for the confusion. Thanks for explaining that all so well.