lora: remove LORA_FREQUENCY_RESOLUTION_DEFAULT from LoRa configurations by jia200x · Pull Request #13318 · RIOT-OS/RIOT

jia200x · 2020-02-07T13:07:47Z

Contribution description

The LORA_FREQUENCY_RESOLUTION_DEFAULT variable is the frequency step of the SX127x transceiver. This is not intended to be configured by the user and it's not specific to LoRa.

This PR moves thisconfiguration to a SX127X_FREQUENCY_STEP macro.

The immediate follow up of this PR is LoRa and LoRaWAN Kconfig configurations.

Testing procedure

It should be enough to check if sx127x applications (gnrc_lorawan, lorawan example) still compile.

Issues/PRs references

None so far

drivers/include/sx127x.h

drivers/sx127x/sx127x_getset.c

jia200x · 2020-02-07T16:32:38Z

@benpicco I added some static inline functions for that. Code size reduces a bit

benpicco · 2020-02-07T16:35:39Z

Great, now sx127x_get_time_on_air() is the only remaining double user.

drivers/sx127x/sx127x_internal.c

benpicco · 2020-02-10T10:16:32Z

The driver is also weird in that you configure a frequency whereas with all other radio drivers you configure a channel (and the driver might convert it to a frequency if the hardware requires it).

But that's already a good improvement! Please squash.

jia200x · 2020-02-10T10:29:59Z

The driver is also weird in that you configure a frequency whereas with all other radio drivers you configure a channel (and the driver might convert it to a frequency if the hardware requires it).

The problem is, there are different PHY layers that use the LoRa modulation. In case of IEEE802.15.4 is easy because it's almost certain there's a IEEE802.15.4 user on top of a 2.4 GHz BPSK (so the channel is the tuple "(channel number, channel page)"). The closest concept to channels here is the tuple "(DR, frequency)", but only for LoRaWAN.

The LORA_FREQUENCY_RESOLUTION_DEFAULT variable is the frequency step of the SX127x transceiver. This is not intended to be configured by the user, and it's not specific to LoRa. This commit moves this configuration to a SX127X_FREQUENCY_STEP macro.

jia200x · 2020-02-10T10:33:24Z

@benpicco done!

benpicco · 2020-02-10T10:37:52Z

so the channel is the tuple "(channel number, channel page)"

Not anymore since 802.15.4g-2012 / 802.15.4-2015 😢

jia200x · 2020-02-10T10:51:08Z

Not anymore since 802.15.4g-2012 / 802.15.4-2015 cry

Really? What are they using now?

jia200x · 2020-02-10T10:57:14Z

Really? What are they using now?

Just checked the 2015 standard, and yes, the new bands don't seem to have channel page (only for the bands present prior to the 2012 standard)

benpicco · 2020-02-10T10:58:41Z

There can be multiple modulation schemes on a channel page now

jia200x · 2020-02-10T10:59:43Z

There can be multiple modulation schemes on a channel page now

Thanks!

benpicco

Code looks good, I don't have hardware to test but I don't expect any changes in the outcome.

jia200x · 2020-02-11T10:17:15Z

thanks!

@aabadie are you ok with this change?

leandrolanzieri

This does not seem to be working. On test/driver_sx127x I get wrong channels:

> channel get
2020-02-13 17:32:40,613 #  channel get
2020-02-13 17:32:40,615 # Channel: 12661926
> channel set 868000000
2020-02-13 17:42:06,592 #  channel set 868000000
2020-02-13 17:42:06,593 # New channel set
> channel get
2020-02-13 17:42:12,695 #  channel get
2020-02-13 17:42:12,696 # Channel: 12361938

benpicco · 2020-02-13T16:56:26Z

drivers/sx127x/sx127x_getset.c

+ */
+static inline uint32_t _freq_to_hw_value(uint32_t freq)
+{
+    return (freq << 8) / 15625;


Suggested change

return (freq << 8) / 15625;

return (freq / 15625) << 8;

I had no idea freq would be this big.

Hmmmm this can produce an offset of 256 in the Hw value :(

Given a=int(freq / 15625) the integer division and b=(freq mod 15625)/15625 the decimal reminder (so freq/15625 == a+b, with 0<=b<1)

Then (1):
(freq/15625) * 2^8 == (a + b) * 2^8 == a*2^8 + b* 2^8

But (2):
int(freq/15625)*256 == a*256.

So (1) - (2) is b*256.

I tested it with real numbers:

>>> freq 16777215 >>> int(freq<<8)/(5**6) 274877.89056 >>> int(freq/(5**6))<<8 274688

Hmm, how about

static inline uint32_t _freq_to_hw_value(uint32_t freq) { freq <<= 2; unsigned pow = 6; do { freq /= 5; freq <<= 1; } while (--pow); return freq; }

This gets much better, but generates a difference of "2" in worst scenario (120 Hz in the carrier frequency).
I have the impression it shouldn't affect that much (it's 0.00001% of drift in the 868 MHz band).

benpicco · 2020-02-13T17:01:33Z

drivers/sx127x/sx127x_getset.c

+ */
+static inline uint32_t _hw_value_to_freq(uint32_t frf)
+{
+    return (frf * 15625) >> 8;


Suggested change

return (frf * 15625) >> 8;

return ((frf * 125) >> 8) * 125;

benpicco · 2020-02-13T17:13:43Z

I guess we might have to cast to uint64_t, still not as bad as pulling in the soft float lib on devices without a FPU.

jia200x · 2020-02-13T17:40:40Z

I guess we might have to cast to uint64_t, still not as bad as pulling in the soft float lib on devices without a FPU.

It shouldn't be necessary. The max value of frf is 0xFFFFFF. It doesn't overflow if you multiply it with something smaller than 256.

E.g:

>>> MAX=0xFFFFFF
>>> a=MAX*125
>>> hex(a)
'0x7cffff83'
>>> b=a>>8
>>> hex(b)
'0x7cffff'
>>> c=b*125
>>> hex(c)
'0x3d08ff83'
>>> UINT32_MAX=0xFFFFFFFF
>>> a < UINT32_MAX and b < UINT32_MAX and c < UINT32_MAX
True

The LORA_FREQUENCY_RESOLUTION_DEFAULT variable is the frequency step of the SX127x transceiver. This is not intended to be configured by the user, and it's not specific to LoRa. This commit moves this configuration to a SX127X_FREQUENCY_STEP macro.

jia200x · 2020-02-14T11:04:59Z

I tested the last push with LoRaWAN. Now it seems to work.

benpicco · 2020-02-14T11:11:19Z

drivers/sx127x/sx127x_getset.c

 {
-    return (frf * 15625) >> 8;
+    unsigned pow = 6;
+    frf >>= 2;


better do the shift at the end so precision is not lost here.

miri64 · 2020-06-25T14:04:59Z

@jia200x waiting for author action ;-)

stale · 2020-12-28T04:58:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jia200x · 2020-12-31T11:51:37Z

self-ping :)

jia200x added the Area: LoRa Area: LoRa radio support label Feb 7, 2020

jia200x requested review from aabadie and leandrolanzieri February 7, 2020 13:07

benpicco reviewed Feb 7, 2020

View reviewed changes

drivers/include/sx127x.h Outdated Show resolved Hide resolved

benpicco reviewed Feb 7, 2020

View reviewed changes

drivers/sx127x/sx127x_getset.c Outdated Show resolved Hide resolved

benpicco reviewed Feb 7, 2020

View reviewed changes

drivers/sx127x/sx127x_internal.c Outdated Show resolved Hide resolved

benpicco reviewed Feb 7, 2020

View reviewed changes

drivers/sx127x/sx127x_internal.c Outdated Show resolved Hide resolved

jia200x force-pushed the pr/lora/fix_configs branch from a3b1437 to 202d1ea Compare February 10, 2020 10:32

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 10, 2020

benpicco approved these changes Feb 10, 2020

View reviewed changes

leandrolanzieri requested changes Feb 13, 2020

View reviewed changes

benpicco reviewed Feb 13, 2020

View reviewed changes

benpicco reviewed Feb 14, 2020

View reviewed changes

akshaim mentioned this pull request Sep 11, 2020

net/lora : Expose configurations to Kconfig #15007

Merged

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Dec 28, 2020

stale bot closed this Feb 2, 2021

Conversation

jia200x commented Feb 7, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

jia200x commented Feb 7, 2020

Uh oh!

benpicco commented Feb 7, 2020

Uh oh!

Uh oh!

Uh oh!

benpicco commented Feb 10, 2020

Uh oh!

jia200x commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Feb 10, 2020

Uh oh!

benpicco commented Feb 10, 2020

Uh oh!

jia200x commented Feb 10, 2020

Uh oh!

jia200x commented Feb 10, 2020

Uh oh!

benpicco commented Feb 10, 2020

Uh oh!

jia200x commented Feb 10, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Feb 11, 2020

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

jia200x Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 13, 2020

Uh oh!

jia200x commented Feb 13, 2020

Uh oh!

jia200x commented Feb 14, 2020

Uh oh!

benpicco Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

stale bot commented Dec 28, 2020

Uh oh!

jia200x commented Dec 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jia200x commented Feb 10, 2020 •

edited

Loading

benpicco Feb 13, 2020 •

edited

Loading