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

Fix LoRaWAN 1.1 support #2307

Merged
merged 4 commits into from
Apr 6, 2020

Conversation

rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Apr 6, 2020

Summary

References #2171

Changes

  • Fix 1.1 uplink MIC calculation naming (aligned it with the spec directly) - block called b0 currently is actually b1 according to the spec, that also caused the confusion in making me think that the implementation is incorrect, let's fix this to avoid this happening in the future.
  • Fix DeviceModeInd being marked as "initiated by NS"
  • Add myself to pkg/crypto CODEOWNERS

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, database and configuration, according to the stability commitments in README.md.
  • Testing: The changes are covered with unit tests. The changes are tested manually as well.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@rvolosatovs rvolosatovs added the bug Something isn't working label Apr 6, 2020
@rvolosatovs rvolosatovs added this to the April 2020 milestone Apr 6, 2020
@rvolosatovs rvolosatovs self-assigned this Apr 6, 2020
@rvolosatovs
Copy link
Contributor Author

Tested this with FF1705 running mbed-os with patch from ARMmbed/mbed-os#12762 applied on top of feature-lorawan-1-1

@coveralls
Copy link

coveralls commented Apr 6, 2020

Coverage Status

Coverage decreased (-0.0006%) to 73.528% when pulling cb57888 on rvolosatovs:fix/lorawan-1.1 into 0ec93de on TheThingsNetwork:master.

Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but the lack of unit tests is a bit "smelly" IMO.

@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Apr 6, 2020

The InitiatedByDevice fields in pkg/encoding/lorawan/mac.go is not something we can or should unit test, since that's just static data. This will be tested in Network Server indirectly though in #2221

Regarding pkg/crypto I agree, but I'd expect the original implementation to be already covered by existing unit tests? I'll see if I can add something quick, it would be great to have some single "source of truth" to test against, unfortunately, the spec does not contain that, so the best we can do is to just run the function once and then test that it gives the same output in the test given same input, so we test against the result of the function we're testing, which is not ideal. Since there are no actual functional changes in pkg/crypto, no reason to add tests here

@rvolosatovs rvolosatovs merged commit 2168871 into TheThingsNetwork:master Apr 6, 2020
@rvolosatovs rvolosatovs deleted the fix/lorawan-1.1 branch April 6, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants