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

Add MAC command tests and improve MAC layer implementation #183

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

Conversation

non-det-alle
Copy link
Collaborator

@non-det-alle non-det-alle commented Nov 18, 2024

These changes started as an initiative to improve test coverage because the CI was failing in PR #179, with the initial objective of covering mac-command.cc.

This has proven to be way more challenging than expected due to the numerous inconsistencies of the code with the LoRaWAN specifications, bug prone snippets and overall messy state of the MAC layer code inherited by previous contributors. Expectedly, all these aspects have arisen in full effect since I begun writing tight tests for the functionality of MAC layer commands as received by end devices. Very often, correct test implementation required a priori changes to add functions, to fix internal behaviors or even to re-design multiple classes.

Obviously, even more work will be required to reach a satisfying state of the device-side MAC layer implementation (I am talking about dubious device classes inheritance, reception window implementation full of boilerplate and duplication, unrealistic duty-cycle management #163, etc.) but at least the present changes bring code related to existing MAC commands (more commands still need to be implemented in the future) up to speed. Notable major rework also affected the devices' frequency channel management framework (LogicalLoraChannelHelper, LogicalChannelHelper, SubBand) to correctly handle configurations apported by MAC commands.

I know this is a huge PR, but - aside some minor global changes (better time printing, annotating variable/functions with unit of measure of physical quantities) that can be always separated from this PR - most changes have a strong interdependence and are not trivial to break up without introducing bugged commits. I tried my best to at least have a clean organization of changes in multiple commits, to also help review. Here is the list of changes as found inside this PR's commits from oldest to most recent with some additional annotations in italics:

  • Comment out logging in tests: this is the recommended state of tests logs in other modules, as they appear to affect other test suites while running multiple, possibly corrupting the output
  • Refactor DB to Db for consistency with CamelCase style
  • Refactor m_controlDataRate/DataRateAdaptation to ADR bit as in LoRaWAN specifications
  • LinkCheckAns: Add Margin field unit of measure + fix type and add getters for both Margin and GwCnt
  • Align LinkAdrReq MAC command to specifications naming scheme and remove regional dependencies
  • LinkAdrReq: Refactor MaxNumbTx to NbTrans as in LoRaWAN specifications + correct default value
  • Tx power initialization should be in dBm ERP, not EIRP, as these values are directly used by the PHY layer for path loss computation
  • Avoid Object initialization for MAC commands (CreateObject -> Create, in the future they should be downgraded from Object to SimpleRefCount)
  • Rewrite OnLinkAdrReq function from reputable source
  • Tx power dBm values to double + more robust AdrComponent::GetTxPowerIndex impl
  • LoRaWAN Headers: store MAC commands to vector instead of list, clean-up and modernize c++, fix printing output
  • Add getters to LinkAdrAns MAC command
  • Implement LinkCheckAns and LinkAdrReq tests
  • DutyCycleReq: align terminology with specs, move logic from header to end-device, implement tests
  • Tests: use NS_TEST_ASSERT_MSG_EQ in place of NS_TEST_EXPECT_MSG_EQ where needed
  • Fix MAC command frequency encoding endianness
  • RxParamSetupReq: add RxParamSetupAns getters, clean-up, fix frequency unit of measure, add tests
  • DevStatusReq: implementation and tests
  • (hopefully) annotate all frequency variables with MHz/Hz
  • Isolate unit test scope, add LinkAdrAns/RxParamSetupAns creation printing, clarify margin code
  • Refactor Create to Install in LoraPhyHelper and LorawanMacHelper to avoid name collision with ns3::Create
  • Evaluate null Ptr in return statements as indicated by the Ptr class documentation
  • Rework the SubBand class
    • add missing EU868 radio sub-bands and fix tests
    • downgrade SubBand from Object to SimpleRefCount
    • remove unused default constructor/destructor
    • refactor BelongsToSubBand to Contains for clarity
    • re-add last frequency getter
  • Rework the LogicalLoraChannel class
    • downgrade LogicalLoraChannel from Object to SimpleRefCount
    • remove unused default constructor/destructor and incomplete initializer constructor
    • remove setters (constructor initialization only)
    • refactor SetEnabledForUplink to EnableForUplink (align with DisableForUplink)
  • LinkCheckAns: Fill margin field with meaningful values
  • Rework LogicalLoraChannelHelper class
    • redo the channel storage model to be aligned with LoRaWAN specifications: a fixed size array of channels accessed by index
    • GetRawChannelArray replaces GetChannelList and GetEnabledChannelList as the only way to get channels: the MAC layer now performs its channel-related tasks (get wait time, get TX channel, change channel mask) outside of this class with just one sweep through of the array
    • AddChannel becomes just SetChannel at index, in line with the channel storage structure found in the LoRaWAN specifications
    • Similarly, discard RemoveChannel function: channels can be overwritten in the internal array with SetChannel as in specifications
    • downgrade LogicalLoraChannelHelper from Object to SimpleRefCount
    • add size initialization to constructor, remove default constructor
    • remove aggregated duty-cycle management (already managed by external MAC layer)
    • FIX duty-cycle next transmission time calculation in AddEvent
    • add channel validity check
    • add GetWaitingTime overload by frequency (that's what it is needed to get the wait time, no need to create an ad-hoc channel instance just to retrieve the wait time)
    • add AddEvent overload by frequency (same reason as above)
    • add GetTxPowerForChannel overload by frequency (same reason as above)
    • remove channel addition by frequency, leading to incomplete tx channel creation (i.e. default values) + param duplication. This now mandates explicit external creation of channel instances to then be passed to the helper by pointer
    • remove AddSubBand with init params, now only by pointer (same reason as above)
  • Rework MacCommand classes
    • remove unused destructors
    • remove setters (mandate initializer constructor use)
    • add missing getters for implemented commands
    • add implementation TODOs
    • improve printing and align teminology with specs
    • add overflow asserts for field with size < 8 bits
    • remove empty lines and boilerplate
  • Refactor waitingTime to waitTime (English proofing)
  • Rework EndDeviceLorawanMac class
    • re-implement GetNextTransmissionDelay and GetChannelForTx
    • remove now obsolete Shuffle method
    • add setter and getter for transmission power
    • correct ChMaskCntl spelling as per specifications
    • remove AddSubBand and SetLogicalChannel
  • More LinkAdrReq test cases + use actual device matrix to test RX1Offset validity
  • Implement OnNewChannelReq and add MAC command test cases
  • Time-related code improvements
    • better time printing using Time::As()
    • prefer more general Time(0) instead Seconds(0) or others when indicatign zero time
    • use time methods Time::IsZero(), Time::IsPositive(), etc. when working with time
    • prefer shorter Now() in place of Simulator::Now()
  • Fix new doxygen warnings

Let's check if CodeCov is happy with the coverage. Of course I am open to try and break up these changes in multiple PRs to help review. Although, with smaller PRs code coverage may be partial.

- add missing EU868 radio sub-bands and fix tests
- downgrade SubBand from Object to SimpleRefCount
- remove unused default constructor/destructor
- refactor BelongsToSubBand to Contains for clarity
- re-add last frequency getter
- downgrade LogicalLoraChannel from Object to SimpleRefCount
- remove unused default constructor/destructor and incomplete initializer constructor
- remove setters (constructor initialization only)
- refactor SetEnabledForUplink to EnableForUplink (align with DisableForUplink)
- redo the channel storage model to be aligned with LoRaWAN specifications: a fixed size array of channels accessed by index
- GetRawChannelArray replaces GetChannelList and GetEnabledChannelList as the only way to get channels: the MAC layer now performs its channel-related tasks (get wait time, get TX channel, change channel mask) outside of this class with just one sweep through of the array
- AddChannel becomes just SetChannel at index, in line with the channel storage structure found in the LoRaWAN specifications
- Similarly, discard RemoveChannel function: channels can be overwritten in the internal array with SetChannel as in specifications

- downgrade LogicalLoraChannelHelper from Object to SimpleRefCount
- add size initialization to constructor, remove default constructor
- remove aggregated duty-cycle management (already managed by external MAC layer)
- FIX duty-cycle next transmission time calculation in AddEvent
- add channel validity check

- add GetWaitingTime overload by frequency (that's what it is needed to get the wait time, no need to create an ad-hoc channel instance just to retrieve the wait time)
- add AddEvent overload by frequency (same reason as above)
- add GetTxPowerForChannel overload by frequency (same reason as above)

- remove channel addition by frequency, leading to incomplete tx channel creation (i.e. default values) + param duplication. This now mandates explicit external creation of channel instances to then be passed to the helper by pointer
- remove AddSubBand with init params, now only by pointer (same reason as above)
- remove unused destructors
- remove setters (mandate initializer constructor use)
- add missing getters for implemented commands
- add implementation TODOs
- improve printing and align teminology with specs
- add overflow asserts for field with size < 8 bits
- remove empty lines and boilerplate
- re-implement GetNextTransmissionDelay and GetChannelForTx
- remove now obsolete Shuffle method
- add setter and getter for transmission power
- correct ChMaskCntl spelling as per specifications
- remove AddSubBand and SetLogicalChannel
- better time printing using Time::As()
- prefer more general Time(0) instead Seconds(0) or others when indicatign zero time
- use time methods Time::IsZero(), Time::IsPositive(), etc. when working with time
- prefer shorter Now() in place of Simulator::Now()
@non-det-alle non-det-alle changed the title Add MAC command tests and improve MAC layer implementation while at it Add MAC command tests and improve MAC layer implementation Nov 18, 2024
@non-det-alle non-det-alle self-assigned this Nov 18, 2024
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 85.61020% with 158 lines in your changes missing coverage. Please review.

Project coverage is 84.56%. Comparing base (c1bd68b) to head (ee04065).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
model/mac-command.cc 59.74% 95 Missing ⚠️
model/end-device-lorawan-mac.cc 89.52% 22 Missing ⚠️
model/lora-frame-header.cc 65.00% 21 Missing ⚠️
model/class-a-end-device-lorawan-mac.cc 78.57% 9 Missing ⚠️
model/sub-band.cc 69.23% 4 Missing ⚠️
helper/lora-helper.cc 88.88% 2 Missing ⚠️
model/end-device-status.cc 71.42% 2 Missing ⚠️
model/adr-component.cc 91.66% 1 Missing ⚠️
model/lora-channel.cc 0.00% 1 Missing ⚠️
model/lora-net-device.cc 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #183      +/-   ##
===========================================
+ Coverage    77.66%   84.56%   +6.90%     
===========================================
  Files           66       67       +1     
  Lines         6831     7017     +186     
===========================================
+ Hits          5305     5934     +629     
+ Misses        1526     1083     -443     

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

@non-det-alle
Copy link
Collaborator Author

@pagmatt What do you think it's best for review? Ideally we should try to split this into multiple PRs, to also have more descriptive commits in the history of the develop branch.

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.

1 participant