ARMmbed / mbed-semtech-lora-rf-drivers

Semtech's LoRa RF drivers for mbed OS
Other
31 stars 25 forks source link

FRF Calculation Inaccuracy #50

Closed mattbrown015 closed 4 years ago

mattbrown015 commented 4 years ago

I'm not sure if this is an observation, a bug or pointless rambling!

I've just been looking at SX1276_LoRaRadio::set_channel, which is the same as SX1272_LoRaRadio::set_channel.

2 years ago @hasnainvirk changed the code so that the value of the FRF register is calculated using single-precision rather than double-precision, see 1a45254. (This change is hidden in this PR Removing software RX timeouts in RX chain #22, which I think is a bit misleading but that's another issue!)

Changing double precision to float We shouldn't use double precision as most of te embedded processors may not support it.

It might well be true that many of the embedded processors supported by Mbed OS will have to use emulated double-precision floating-point but it is also important to consider the accuracy of the result.

As far as I can tell, this small text change means the value of FRF is wrong by something like 2000 Hz.

For example, if the requested frequency is 867100000 Hz, the single-precision calculation yields an FRF of 0xd8c667 whereas using double-precision and FREQ_STEP 61.035 yields 0xd8c68a. (0xd8c68a - 0xd8c667) * 61.035 2136.225 Hz.

This code was originally copied from SX1276SetChannel in sx1276.c and it still uses double. I'm also intrigued about the selection of 61.03515625; clearly the original author realised that 61.035 isn't representable in single- or double-precision but double-precision can do better than 61.03515625. I wonder if the code started life as float and got changed to double but the significance of FREQ_STEP was missed.

I'm using this driver on EU868 LoRaWAN devices. They just work, I don't see many packets go missing in the lab so is this 2000 Hz error significant?? I guess not if no one else is complaining.

But, it seems a shame to me to start with 2000 Hz error because of maths, there must be a better way! At some point , in adverse conditions, this 2000 Hz is going to mean a packet is lost that may otherwise have made it.

mattbrown015 commented 4 years ago

I've now realised that I'd gone down the wrong path!

It is not single- or double-precision that is important in the calculation it's FREQ_STEP.

On my system, for 867100000,...

(uint32_t)((float) freq / (float) FREQ_STEP) yields 0xd8c667 (uint32_t)((double) freq / (double) FREQ_STEP) yields 0xd8c666 (uint32_t)((float) freq / (float) 61.035) yields 0xd8c68b (uint32_t)((double) freq / (double) 61.035) yields 0xd8c68a

Where 0xd8c667 61.035 = 867097816.845 and 0xd8c68b 61.035 = 867100014.105.

I think this is because the nearest single-precision value to 61.035 is 61.034999847412109375.

61.035-61.034999847412109375 = 0.000000152587890625 whereas 61.03515625-61.035 = 0.00015625.

AFAICT, gcc is rounding 61.035 down to 61.034999847412109375.

The behaviour of floating-point data conversions is probably platform dependant but I bet most platforms and compilers behave the same!

So the question is...

Should FREQ_STEP be 61.035?

Or, phrasing as a bug "incorrect FREQ_STEP value"!

mattbrown015 commented 4 years ago

I'm beginning to think I'm wrong about FREQ_STEP being 61.035!

On page 109 of rev 6 of the SX1276 datasheet in the RegFrfLsb description it says "Resolution is 61.035 Hz". This is what made me thing FREQ_STEP was 61.035.

On page 36 in the Frequency Settings section it says "Fstep = Fosc/2^19". Therefore Fstep = 32000000/2^19 = 61.03515625.

I'm guessing that 61.03515625 is the correct value and the description on page 109 is just a rounding down! The calculation on page 36 seems to make sense.

See SX1276 FRF Register Frequency Step. When Semtech comment on my forum topic I'll close this issue.

mattbrown015 commented 4 years ago

I've had it confirmed that #define FREQ_STEP 61.03515625 is correct so I'm closing this issue.

61.03515625 is representable in single-precision so that's good!

I think what I wrote above about the FRF register value in different situation means that the change from double- to single-precision has caused a different in rounding and FRF value may have changed by 1 bit. Hopefully this is insignificant!