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

Lower stored MAC state data #5599

Merged
merged 8 commits into from
Jul 18, 2022
Merged

Conversation

adriansmares
Copy link
Contributor

@adriansmares adriansmares commented Jul 14, 2022

Summary

Closes #5127

Changes

  • As part of the MACState message, add minimized versions of UplinkMessage, DownlinkMessage, Message, TxSettings and RxMetadata
    • The scope here is to ensure that fields which are not going to be available from the 'real' UplinkMessage/DownlinkMessage are not accidentally used (i.e. using zero values as if they were intentional)
  • Use the minimized versions as part of the Network Server MAC state handling
  • Fix LoRaWAN 1.1 AFCntDown sanity check
    • The old check did not take into account the fact that the frame header frame counter is 16 bit, and instead the full (32bit) frame counter should be used for checks

Testing

Unit testing.

Regressions

The code that actually uses these fields is unchanged in this PR - if a field was skipped in the minimal representation, it would result in a compile error. The only error prone operation is the conversion between the normal message and the minimal message, which I've double checked and also covered via existing unit tests.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • 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.

@adriansmares adriansmares added this to the v3.21.0 milestone Jul 14, 2022
@adriansmares adriansmares self-assigned this Jul 14, 2022
@github-actions github-actions bot added c/network server This is related to the Network Server compat/api This could affect API compatibility labels Jul 14, 2022
@adriansmares adriansmares force-pushed the feature/5127-lower-data-storage branch 4 times, most recently from ceb5541 to b422407 Compare July 14, 2022 16:58
@adriansmares adriansmares force-pushed the feature/5127-lower-data-storage branch from 786ad65 to 5fd271a Compare July 14, 2022 17:48
@adriansmares adriansmares marked this pull request as ready for review July 14, 2022 18:08
@adriansmares adriansmares force-pushed the feature/5127-lower-data-storage branch from 1d8ed7a to c842e66 Compare July 14, 2022 18:47
@adriansmares adriansmares changed the base branch from v3.20 to v3.21 July 15, 2022 10:24
@adriansmares adriansmares force-pushed the feature/5127-lower-data-storage branch from 15a059e to b8d1578 Compare July 15, 2022 14:12
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

I would also be fine with keep using the original messages but define a field mask for storage.

Code quality wise I don't think it's necessary to declare all fields reserved with the original message's field name as they diverge. If it's just to make sure that our future selves don't add fields with the same numbers and old binary data gets unmarshaled and lead to corrupted data, we can just declare the reservations in short form.

@adriansmares
Copy link
Contributor Author

I've minimised the reserved by removing the comments and joining the statements.

Regarding the field mask - The field mask tooling does not support applying field masks to repeated fields - see TheThingsIndustries/protoc-gen-fieldmask#37 , and a lot of the dropped fields are in rx_metadata.

Offtopic: We probably should fix that before we add field masks to webhooks, since most of the filtering that I expect will be in the rx_metadata of ApplicationUp.

@adriansmares adriansmares force-pushed the feature/5127-lower-data-storage branch from 4f3c9de to 02b89ea Compare July 18, 2022 12:21
@adriansmares adriansmares merged commit 5047fe7 into v3.21 Jul 18, 2022
@adriansmares adriansmares deleted the feature/5127-lower-data-storage branch July 18, 2022 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/network server This is related to the Network Server compat/api This could affect API compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lower data stored for recent traffic
2 participants