Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/sx127x: reduce use of floats #19697

Merged
merged 1 commit into from Jun 13, 2023
Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented May 31, 2023

Contribution description

Convert the floating point arithmetic to integer arithmetic.

Beware: I neither tested this nor did I try to understand the calculations.

Testing procedure

  • air time computation should yield roughly the same result
  • frequency / channel computation should yield roughly the same result

Issues/PRs references

#19614

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support labels May 31, 2023
@jia200x
Copy link
Member

jia200x commented May 31, 2023

The original Semtech pkg hardcoded this information in the driver. In practice it doesn't make sense, as it is specific to the MAC config and has nothing to do with the transceiver.
GNRC LoRaWAN uses a lookup table based implementation to calculate that. Shouldn't we simply deprecate getting time on air from the driver and just call that function directly from the pkg contrib?

@maribu
Copy link
Member Author

maribu commented May 31, 2023

Agreed. But I don't think we need to deprecate this before removing as I see this as an internal API.

Would you mind to PR this? I can rebase the channel computation on top of that.

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels May 31, 2023
@maribu maribu changed the title drivers/sx127x: avoid using floats drivers/sx127x: reduce use of floats Jun 9, 2023
@maribu
Copy link
Member Author

maribu commented Jun 9, 2023

I updated this now to only do the straight forward conversions in the hope that the airtime computation is about to get dropped anyway.

@jia200x
Copy link
Member

jia200x commented Jun 12, 2023

I updated this now to only do the straight forward conversions in the hope that the airtime computation is about to get dropped anyway.

Let's just merge this one then. I realized the lora_time_on_air function does not consider using other BW values. This may be a problem for certain LoRaWAN regions. Although it is easy to fix, it requires to extend the function.

@maribu
Copy link
Member Author

maribu commented Jun 12, 2023

It still needs ACK, though 😉

@jia200x
Copy link
Member

jia200x commented Jun 12, 2023

sure, I would just like to test it before

Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. The new function yields the same values as the float version.

I noticed that this function seems to overestimates the ToA for low pkt_len values. But this was already the case with the float version and it shouldn't be a problem for LoRaWAN

/** @brief Frequency resolution in Hz */
#ifndef LORA_FREQUENCY_RESOLUTION_DEFAULT
#define LORA_FREQUENCY_RESOLUTION_DEFAULT (61.03515625)
/** @brief Frequency resolution in nano Hz */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to add to comment this is 32MHz/2^19

@maribu
Copy link
Member Author

maribu commented Jun 12, 2023

I added the comment @kfessel suggested and the white space changes uncrustify preferred. (So only white space changes from a C compiler's point of view since the last testing.)

Should be good to go now.

@maribu
Copy link
Member Author

maribu commented Jun 13, 2023

bors merge

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 13, 2023
@riot-ci
Copy link

riot-ci commented Jun 13, 2023

Murdock results

✔️ PASSED

826095d drivers/sx127x: reduce use of floats

Success Failures Total Runtime
6934 0 6934 11m:42s

Artifacts

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 13, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 5e7c6c2 into RIOT-OS:master Jun 13, 2023
26 checks passed
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
@maribu maribu deleted the drivers/sx127x/floats branch December 5, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants