SX128x RSSI Calc Update

[jlpoltrack]

Updates the RSSI calculation to account for when the SNR is negative.

Borrowed logic from Datasheet / ELRS: https://github.com/ExpressLRS/ExpressLRS/blob/master/src/lib/SX1280Driver/SX1280.cpp#L580-L597

[olliw42]

at the time I did that I was thinking about this too, but somehow decided against it
not sure anymore why. I guess I rather would prefer to get the raw values from this function and rather do the correction otherwise.
maybe one should add a return int8_t* RssiSyncCorrected to the function?

you need to be carefull with adding the offset since the result must not exceed the int8_t range. So you either use int16 internally and constrain it then to INT8_MIN, or you check this doesn't happen otherwise.

pl do not use camelcase variables inside the function, but lower case with '_', So, e.g. rssi_tmp, snr_tmp, neg_ofs; (temp sound smuch like temperature). One could thus also just use rssi, snr, ofs.

[jlpoltrack]

maybe one should add a return int8_t* RssiSyncCorrected to the function?

Assume that you mean this would return 3 pieces of data: Rssi, RssiCorrected and Snr. Could do this but see pieces like this would likely need to be updated as well: maybe one should add a return int8_t* RssiSyncCorrected to the function? So may just be simpler to have two pieces returned.

you need to be carefull with adding the offset since the result must not exceed the int8_t range. So you either use int16 internally and constrain it then to INT8_MIN, or you check this doesn't happen otherwise.

I added this - feedback here appreciated.

pl do not use camelcase variables inside the function, but lower case with '_', So, e.g. rssi_tmp, snr_tmp, neg_ofs; (temp sound smuch like temperature). One could thus also just use rssi, snr, ofs.

No problem, will update.

[olliw42]

why did this internal int16 calculus go? is there an argument which I'm missing that it can't underflow?

[jlpoltrack]

Was debugging why that version wouldn't compile and didn't realize I had forgot a semicolon on the if statement (whoops) so made it simpler while I was trying to debug. I do agree that it makes sense to do the math here with int16 to protect for overflow.

In reality, I think the RSSI values won't be less than -100 and SNR values won't be less than -20 which means int8 would probably be fine. Probably wishful thinking that the chip wouldn't provide values that could overflow but better to be safe.

[olliw42]

the lowets rssi depends on the lora mode ... this is supposed to be a universal driver, so we shouldn't make any assumption on what we only use it for :)

[olliw42]

@jlpoltrack
sorry for having lost track a bit
I've looked over this, also for the sx1276, and my suggestion is to streamline all this

• pl change GetPacketStatus(int8_t* RssiSync, int8_t* Snr) to GetPacketStatus(int16_t* RssiSync, int8_t* Snr) (also in the header)

• the code could then be simplified to e.g.

   *RssiSync = -(int16_t)(status[0] / 2);
   *Snr = (int8_t)status[1] / 4;
   if (*Snr < 0) {
      *RssiSync += *Snr;
  }

We then have to ensure in the higher-level drivers in the main mLRS repo that we constrain to -128...127.

Does this make sense?

[jlpoltrack]

I've made these changes but should the constrain be -127 to 0? From the datasheet, I don't see how rssiSync could be > 0. Let me know:

[olliw42]

many thx
I hpe the changes looked ok for you, I just pulled it out of my head
I think the constrain we want to add to the higher sx drivers, like it is done for the sx1276 in the MLRS repo
yeah, you are correct, we should constrain there to -127...0 (since it's just 7 bits in the oer-the-air frames)

[jlpoltrack]

Submitted this as well: olliw42/mLRS#66. 1RSS looks okay on the bench at short range.

[olliw42]

many THX!