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 #184

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

Conversation

non-det-alle
Copy link
Collaborator

@non-det-alle non-det-alle commented Jan 24, 2025

This PR is an effort to break #183 into more manageable changes for review purposes.

These changes have the goal to add test coverage for the LoRaWAN MAC commands currently implemented and the expected behavior when they are received by a device (i.e., setting the correct parameters etc.) This is still a big PR because (as you can imagine) these functionalities touch a large portion of the MAC layer itself. While I was at it I made an effort to "rework" some classes, to clean up the code, to actually expose what we need to access and to fill-in missing functionality. This was necessary to be able to write correct tests.

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 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
  • 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
  • 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
  • Isolate unit test scope, add LinkAdrAns/RxParamSetupAns creation printing, clarify margin code
  • 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
  • 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
    • move documentation to fix doxygen warning
  • More LinkAdrReq test cases + use actual device matrix to test RX1Offset validity
  • Implement OnNewChannelReq and add MAC command test cases
  • Tx power initialization should be in dBm ERP, not EIRP, as these values are directly used by the PHY layer for path loss computation + more robust AdrComponent::GetTxPowerIndex implementation

@non-det-alle non-det-alle requested a review from pagmatt January 24, 2025 17:48
- 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
- move documentation to fix doxygen warning
+ more robust GetTxPowerIndex implementation
@non-det-alle
Copy link
Collaborator Author

Had to fix a dependency issue with our CI pipeline that was producing and error during documentation checks. Rebased all commits onto develop.

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 84.86141% with 142 lines in your changes missing coverage. Please review.

Project coverage is 84.55%. Comparing base (18bbfa5) to head (63e0e6f).

Files with missing lines Patch % Lines
model/mac-command.cc 58.33% 95 Missing ⚠️
model/end-device-lorawan-mac.cc 90.09% 20 Missing ⚠️
model/lora-frame-header.cc 67.24% 19 Missing ⚠️
model/class-a-end-device-lorawan-mac.cc 87.87% 4 Missing ⚠️
model/sub-band.cc 50.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #184      +/-   ##
===========================================
+ Coverage    77.64%   84.55%   +6.90%     
===========================================
  Files           66       67       +1     
  Lines         6831     7017     +186     
===========================================
+ Hits          5304     5933     +629     
+ Misses        1527     1084     -443     

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

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