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

Refactor all frequencies from MHz (double) to Hz (uint32_t) #179

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

non-det-alle
Copy link
Collaborator

In the LoRaWAN MAC protocol specifications, in the LoRa devices PHY hardware abstraction layer libraries, in the gateways' packet forwarder protocol code and in the network server configurations and internal representation, frequency values are encoded using 4 bytes (and in Hz). However, the module currently uses MHz values in floating point form to represent frequencies.

Aligning with real implementations slightly simplifies encoding and prevent floating point arithmetic issues. From personal experience, I once got a bug because multiplying frequency * 0.01 instead of frequency / 100 returned a .99999999-like value, and as a consequence all comparisons to retrieve the correct LogicalLoraChannel object yielded false.

This PR converts the type of frequency values used throughout the module from double to uint32_t, and it appends Hz to names of class members and function signature parameters representing frequencies (i.e. ending up in the API documentation).

This PR is an effort to break #135 into more digestible pieces.

@non-det-alle non-det-alle requested a review from pagmatt October 21, 2024 12:15
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 39 lines in your changes missing coverage. Please review.

Project coverage is 77.66%. Comparing base (82ea58a) to head (5567991).

Files with missing lines Patch % Lines
model/mac-command.cc 0.00% 11 Missing ⚠️
model/end-device-lorawan-mac.cc 27.27% 8 Missing ⚠️
model/gateway-lora-phy.cc 50.00% 4 Missing ⚠️
model/logical-lora-channel-helper.cc 60.00% 4 Missing ⚠️
model/lora-frame-header.cc 0.00% 4 Missing ⚠️
model/class-a-end-device-lorawan-mac.cc 57.14% 3 Missing ⚠️
model/end-device-status.cc 71.42% 2 Missing ⚠️
model/logical-lora-channel.cc 88.88% 1 Missing ⚠️
model/lora-channel.cc 66.66% 1 Missing ⚠️
model/sub-band.cc 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #179   +/-   ##
========================================
  Coverage    77.66%   77.66%           
========================================
  Files           66       66           
  Lines         6831     6833    +2     
========================================
+ Hits          5305     5307    +2     
  Misses        1526     1526           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@non-det-alle
Copy link
Collaborator Author

Codecov fails because the test coverage of changed lines is subpar to the overall coverage of the module. I'll try to add some tests to fix this while we are at it.

model/sub-band.h Outdated Show resolved Hide resolved
Copy link
Member

@pagmatt pagmatt left a comment

Choose a reason for hiding this comment

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

a few variables seem to be still double

@non-det-alle
Copy link
Collaborator Author

Thank you, I must have missed them. I'll check everything again.

@pagmatt pagmatt self-requested a review October 25, 2024 11:12
@non-det-alle
Copy link
Collaborator Author

non-det-alle commented Oct 25, 2024

@pagmatt if you agree, before merging this I'd like to both merge #180 and the tests required to cover the lines indicated by codecov (I have yet to create a PR for this). It feels the right order of actions given the dependency of changes bug<-tests<-this. Afterwards I'll take care of merging changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants