olliw42 / mLRS

2.4 GHz & 915/868 MHz & 433 MHz/70 cm LoRa based telemetry and radio link for remote controlled vehicles
GNU General Public License v3.0
266 stars 55 forks source link

Improve LQ over rc channel function #180

Closed brad112358 closed 2 months ago

brad112358 commented 2 months ago

Rename the functions which calculate RSSI and LQ for use on an RC channel to be consistent with their purpose and use. Adjust range of LQ calculation to occupy the full (100%) RC range for more accurate LQ readings without adjusting the range in Ardupilot parameters.

Now, with the default Ardupilot range, the correct full LQ range will now be reported when using RC override. But the range is still not quite correct when using sbus since Ardupilot and PX4 do not agree with mLRS on the exact range of sbus channel values. But, at least we get closer with a maximum of 99% reported instead of 94%. Betaflight and INAV do agree mLRS with respect to sbus channel range.

olliw42 commented 2 months ago

great. thx. could you please add a _crsf_sbus to the function names? This is sort of the unit, rc only could be interpreted to mean mLRS' internal rc representation.

olliw42 commented 2 months ago

if the difference is 100% vs 99%, isn't it maybe possible to play a bit foul and make it a bit larger so that the integer arithmetics makes it 100% for both?

brad112358 commented 2 months ago

great. thx. could you please add a _crsf_sbus to the function names? This is sort of the unit, rc only could be interpreted to mean mLRS' internal rc representation.

That's the point. These do convert to internal rc representation and that's how they are used. The conversion to sbus is done later.

brad112358 commented 2 months ago

if the difference is 100% vs 99%, isn't it maybe possible to play a bit foul and make it a bit larger so that the integer arithmetics makes it 100% for both?

Yes, and I considered this, but I assumed you wouldn't like it much and I don't either. But, since you suggest it, I'll give it a try.

olliw42 commented 2 months ago

great. thx. could you please add a _crsf_sbus to the function names? This is sort of the unit, rc only could be interpreted to mean mLRS' internal rc representation.

That's the point. These do convert to internal rc representation and that's how they are used. The conversion to sbus is done later.

ah, you are right. Thx.

they should get then better comments too. the comment on the 2nd is then highly missleading something maybe like // convert to internal rc units, bla bla bla

olliw42 commented 2 months ago

if the difference is 100% vs 99%, isn't it maybe possible to play a bit foul and make it a bit larger so that the integer arithmetics makes it 100% for both?

Yes, and I considered this, but I assumed you wouldn't like it much and I don't either. But, since you suggest it, I'll give it a try.

we just would want to properly document this, but if it does the trick...

but I'm fine either way, as said my heart is really not on this ;)

brad112358 commented 2 months ago

It looks like the fudge strategy might not quite work. It appears that in some places autopilots round and other places they truncate in converting from the PWM value to a LQ integer percentage. This reduces the amount of fudge we can add for Ardupilot without risking going over to the next value in Betaflight. I think what I have here is the best we can do without also fixing Ardupilot or making our sbus range specific to Ardupilot's quirks. To be honest, I'm not sure how to prove that Ardupilot and Px4 are wrong and that Betaflight and mLRS and Otx are correct. The numbers for sbus all seem to be reverse-engineered and reviewing git logs shows the best guess has changed a lot over the years for both families of FC firmware.

olliw42 commented 2 months ago

so we go with what you have maybe you find time to somewhat improve the comments, if not fine, I will then try retrospectively

To be honest, I'm not sure how to prove that Ardupilot and Px4 are wrong and that Betaflight and mLRS and Otx are correct. The numbers for sbus all seem to be reverse-engineered and reviewing git logs shows the best guess has changed a lot over the years for both families of FC firmware.

my saying somewhere else :) sbus "spec" was different 10 years ago ...

brad112358 commented 2 months ago

Your existing minimal function comments were sufficient for me to understand what's going on in the code once I realized the function names were misleading. If you want to provide some guidance on what you are looking for, I'm happy to add more comments, but perhaps it's faster to commit this and add what you want on top.

olliw42 commented 2 months ago

ok, let's do this then :)

MANY THX!!

brad112358 commented 2 months ago

Perhaps I missed your edited comments on comments. Sorry

olliw42 commented 2 months ago

Perhaps I missed your edited comments on comments. Sorry

don't worry :) thx again :)

brad112358 commented 2 months ago

Thanks!