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

Removes units and details of probable future use from reserved message fields [OTA-197] #1202

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

jtec
Copy link
Contributor

@jtec jtec commented Aug 1, 2022

Description

This PR removes details of their likely future use from the description of reserved message fields.

It also adds a comment on what the message leap second fields represent instad of referencing the GPS ICD and specifies that the week number is a GPS week.

Rationale

It is very likely that the reserved fields will be used for drift parameters if needed, but this possible future choice should not be reflected in today's field documentation.

We originally considered also merging all reserved fields into one single reserved bitfield. However, if we compare the two scenarios

  1. Merge all reserved bytes into one bitfield
    -> once we decide to use the reserved bytes, we re-divide this bitfield into separate fields, breaking wire format and thus backwards compatibility
  2. Keep the data types of reserved fields and just name them reserved_i
  • subscenario A) once we decide to use them, keeping data types unchanged, SBP clients only need to rename fields in their internal data structures that those reserved fields are encoded into -> as we don't guarantee field names this scenario would not break backwards compatibility
  • subscenario B) If we should decide to change the data type of one or more fields, we would break wire format and thus backwards compatibility

So pre-defining the data field types according to their likely future use lowers the likelihood of breaking changes in the future.

API compatibility

This PR does not change the binary representation or message field types but only removes the description and units of reserved fields which are not in use.

JIRA Reference

https://swift-nav.atlassian.net/browse/OTA-197

@jtec jtec requested a review from a team as a code owner August 1, 2022 14:58
@jtec jtec added the draft label Aug 1, 2022
@jtec jtec marked this pull request as draft August 1, 2022 17:09
@jtec
Copy link
Contributor Author

jtec commented Aug 1, 2022

@silverjam @woodfell

Ideally I would like re-arrange the fields to have all reserved bytes in one bitfield - how do we manage those kinds of API-breaking changes?

@silverjam
Copy link
Contributor

@silverjam @woodfell

Ideally I would like re-arrange the fields to have all reserved bytes in one bitfield - how do we manage those kinds of API-breaking changes?

You would need to special case the validator script so that it would recognize this change and allow it -- probably the easiest way to do this is to just rename the fields to <reserved0>...<reservedN> and keep the types the same.

See https://github.com/swift-nav/libsbp/blob/master/scripts/spec_validator.py

@silverjam
Copy link
Contributor

@silverjam @woodfell
Ideally I would like re-arrange the fields to have all reserved bytes in one bitfield - how do we manage those kinds of API-breaking changes?

You would need to special case the validator script so that it would recognize this change and allow it -- probably the easiest way to do this is to just rename the fields to <reserved0>...<reservedN> and keep the types the same.

See https://github.com/swift-nav/libsbp/blob/master/scripts/spec_validator.py

cc @isaactorz

@jtec jtec marked this pull request as ready for review August 9, 2022 09:14
@jtec jtec requested review from a team, silverjam and notoriaga as code owners August 9, 2022 09:14
@jtec
Copy link
Contributor Author

jtec commented Aug 9, 2022

On second thought, if we compare the two scenarios

  1. We merge all reserved bytes into one bitfield -> once we decide to use the reserved bytes, we re-divide this bitfield into separate fields, breaking wire format and thus backwards compatibility
  2. We keep the data types of reserved fields and just name them reserved_i
    • subscenario A) once we decide to use them, keeping data types unchanged, SBP clients only need to rename fields in their internal data structures that those reserved fields are encoded into -> as we don't guarantee field names I guess that would not even break backwards compatibility?
    • subscenario B) If we should decide to change the data type of one or more fields, we would break wire format and thus backwards compatibility

So as I see it if we keep the data types the likelihood of changing wire format in the future is actually lower (!)

@jtec
Copy link
Contributor Author

jtec commented Aug 9, 2022

@silverjam I rebased in master, re-generated all files with "make clean && make all" and get a version inconsistency here: https://github.com/swift-nav/libsbp/runs/7745497456?check_suite_focus=true

An idea what I am missing here?

@silverjam
Copy link
Contributor

@silverjam I rebased in master, re-generated all files with "make clean && make all" and get a version inconsistency here: https://github.com/swift-nav/libsbp/runs/7745497456?check_suite_focus=true

An idea what I am missing here?

If you're running locally you'll need to pull tags to get the version to generate correctly: git pull --tags

Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

@jtec Please update the PR description with all the final rational before merging so we can properly trace this decision if needed in the future

@jtec jtec requested a review from fpezzinosn August 10, 2022 09:51
@jtec
Copy link
Contributor Author

jtec commented Aug 10, 2022

@fpezzinosn This PR makes some adjustments to the message documentation that I missed in the design doc - would love for you to have a quick look!

@jtec jtec merged commit ae47f69 into master Aug 10, 2022
@jtec jtec deleted the janbolting/ota-197 branch August 10, 2022 13:47
@silverjam silverjam removed the draft label Aug 10, 2022
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.

4 participants