Skip to content

Conversation

fpezzinosn
Copy link
Contributor

@fpezzinosn fpezzinosn commented May 4, 2022

Description

@swift-nav/devinfra

Reserves the IDs associated with the Satellite Bounds messages that are defined in the staging branch.

API compatibility

Does this change introduce a API compatibility risk?

No

API compatibility plan

If the above is "Yes", please detail the compatibility (or migration) plan:

JIRA Reference

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

@fpezzinosn fpezzinosn requested review from a team and notoriaga as code owners May 4, 2022 16:09
@fpezzinosn
Copy link
Contributor Author

@silverjam Is this what you were referring as stubbing in your staging proposal?

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.

Yes, this is exactly what I meant

@fpezzinosn fpezzinosn self-assigned this May 5, 2022
@fpezzinosn fpezzinosn changed the title Add stubbed version of Satellite Bounds [OTA-65][OTA-119] Add stubbed version of new Integrity Bounds/Flags [OTA-134] May 6, 2022
@fpezzinosn fpezzinosn marked this pull request as draft May 6, 2022 19:59
@fpezzinosn
Copy link
Contributor Author

Will convert this into a draft until all the other messages are merged into staging.

@fpezzinosn fpezzinosn force-pushed the fpezzinosn/OTA-65-119-stub branch 2 times, most recently from d01824f to 54d3e14 Compare May 9, 2022 20:08
@fpezzinosn fpezzinosn marked this pull request as ready for review May 9, 2022 20:10
@fpezzinosn fpezzinosn requested a review from silverjam May 9, 2022 21:24
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.

We need a couple things before we can merge:

  • an indication in the version when we're on a staging branch (something a -staging should be appended e.g. v4.2.0-g<git_short_hash>-staging).
  • a compiler macro in sbp.h that indicates if we're on a staging branch e.g. #define SBP_STAGING 1

We can implement here and merge master into staging or implement in master, then cherry pick to staging.

These facilities will be used when we cut a release of Starling and disable parsing of the new messages.

@@ -789,7 +828,7 @@ definitions:
short_desc: Deprecated
public: False
replaced_by:
- MSG_SSR_TILE_DEFINITION
- MSG_SSR_TILE_DEFINITION_DEP
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these deprecated symbols be _DEP_A? Seems to be a common pattern in other deprecated messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though we used first _DEP and when that one is already used, we switched to _DEP_A.

short_desc: High level integrity flags
public: False
fields:
- stub:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan for all these stubs? Are they going to be included in a release and then redefined later on? Or are they going to be defined properly on this PR before merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of a new proposal.
https://swift-nav.atlassian.net/wiki/spaces/~jason/pages/2198831192/Proposal+for+SBP+message+staging
In the staging branch the full definition of the message is going to be defined. While in master this stubbed version reserve the ids. Once the messages are mature enough and don't need to have any fields updated, they will be promoted from staging to master.

@fpezzinosn
Copy link
Contributor Author

fpezzinosn commented May 12, 2022

We need a couple things before we can merge:

  • an indication in the version when we're on a staging branch (something a -staging should be appended e.g. v4.2.0-g<git_short_hash>-staging).
  • a compiler macro in sbp.h that indicates if we're on a staging branch e.g. #define SBP_STAGING 1

We can implement here and merge master into staging or implement in master, then cherry pick to staging.

These facilities will be used when we cut a release of Starling and disable parsing of the new messages.

@silverjam Is this the correct place to append -staging so that it propagates everywhere?

def parse_release_version(release: str) -> ReleaseVersion:
major, minor, patch = release.split('.')[:3]
major = major.lstrip('v')
if '-' in patch:
patch = patch.split('-')[0]
patch_pre = '{}-alpha'.format(int(patch.split('-')[0]) + 1)
full_version = "{}.{}.{}".format(major, minor, patch_pre)
else:
patch_pre = patch
full_version = "{}.{}.{}".format(major, minor, patch)
return ReleaseVersion(major=major,
minor=minor,
patch=patch,
patch_pre=patch_pre,
full_version=full_version)

@silverjam
Copy link
Contributor

We need a couple things before we can merge:

  • an indication in the version when we're on a staging branch (something a -staging should be appended e.g. v4.2.0-g<git_short_hash>-staging).
  • a compiler macro in sbp.h that indicates if we're on a staging branch e.g. #define SBP_STAGING 1

We can implement here and merge master into staging or implement in master, then cherry pick to staging.
These facilities will be used when we cut a release of Starling and disable parsing of the new messages.

@silverjam Is this the correct place to append -staging so that it propagates everywhere?

def parse_release_version(release: str) -> ReleaseVersion:
major, minor, patch = release.split('.')[:3]
major = major.lstrip('v')
if '-' in patch:
patch = patch.split('-')[0]
patch_pre = '{}-alpha'.format(int(patch.split('-')[0]) + 1)
full_version = "{}.{}.{}".format(major, minor, patch_pre)
else:
patch_pre = patch
full_version = "{}.{}.{}".format(major, minor, patch)
return ReleaseVersion(major=major,
minor=minor,
patch=patch,
patch_pre=patch_pre,
full_version=full_version)

Yes, this looks correct

Comment on lines +183 to +189
gen-c_args = -i $(SBP_SPEC_DIR) \
-o $(SWIFTNAV_ROOT)/c \
-r $(SBP_VERSION)
ifeq ($(SBP_STAGING), 1)
gen-c_args += -s
endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the sure if it is the best approach to append a flag conditionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine, this makes the behavior more explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

As an alternative I could see moving this behavior inside the generator an implicitly deciding if we're in a "staging" mode based on some environmental condition, but again, passing the switch makes the behavior more explicit

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.

When merging please rebase and squash into two commits (make sure to select "rebase and merge" when merging the PR, we want two physical commits to go to the master branch):

  • message stubs
  • support for "SBP_STAGING"

We'll then do a merge commit of master into staging and revert the "message stubs" commit.

@silverjam silverjam requested a review from woodfell May 16, 2022 22:09
@silverjam
Copy link
Contributor

@fpezzinosn should be able to rebase and merge this PR now

@fpezzinosn fpezzinosn force-pushed the fpezzinosn/OTA-65-119-stub branch from bf69f6d to 24859a2 Compare May 17, 2022 13:44
@fpezzinosn fpezzinosn merged commit d39b889 into master May 17, 2022
@fpezzinosn fpezzinosn deleted the fpezzinosn/OTA-65-119-stub branch May 17, 2022 14:38
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