Skip to content

Conversation

@antrikshsrivastava
Copy link
Contributor

Description

@swift-nav/devinfra

API compatibility

Does this change introduce a API compatibility risk?

API compatibility plan

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

JIRA Reference

https://swift-nav.atlassian.net/browse/BOARD-XXXX

@silverjam
Copy link
Contributor

@antrikshsrivastava Is this ready for review?

@antrikshsrivastava
Copy link
Contributor Author

@antrikshsrivastava Is this ready for review?

Not yet, it's not all working together yet, although this PR is unlikely to change much

@silverjam silverjam marked this pull request as draft January 19, 2023 21:19
@silverjam
Copy link
Contributor

Converted to draft so I'm not notified that this is pending review, please click "Ready for review" once this is finalized

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.

lgtm, please re-request review when this is ready for merge

Copy link

@jithk jithk left a comment

Choose a reason for hiding this comment

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

LGTM. Just curious - what caused the change in IMU_TYPE value from 2 to 4?

@antrikshsrivastava
Copy link
Contributor Author

LGTM. Just curious - what caused the change in IMU_TYPE value from 2 to 4?

Because some of the other numbers are taken up by the Android and ICM42670 here: https://github.com/swift-nav/starling/blob/master/starling_util/src/sbp/unpackers.cc#L22

@antrikshsrivastava antrikshsrivastava marked this pull request as ready for review February 12, 2023 23:35
@antrikshsrivastava
Copy link
Contributor Author

@silverjam how do we deal with the conflicts in this repo, I would think the PDF would just get overwritten? What about the RELEASE-VERSION change?

@silverjam
Copy link
Contributor

@silverjam how do we deal with the conflicts in this repo, I would think the PDF would just get overwritten? What about the RELEASE-VERSION change?

Merge in master and resolve all conflicts with "theirs" then regenerate everything.

@antrikshsrivastava
Copy link
Contributor Author

@silverjam this is ready to merge in whenever, I can't merge it even though I have 1 approval

Copy link
Contributor

@adrian-kong adrian-kong left a comment

Choose a reason for hiding this comment

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

voilà

EDIT nvm thought u need review from devinfra will need steve or jason

@antrikshsrivastava
Copy link
Contributor Author

Thanks @adrian-kong .. hmm it still says blocked for me:
Merging is blocked
Merging can be performed automatically with 1 approving review.

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.

Lgtm

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.

5 participants