Add MSG_TEL_SV: per-SV telemetry message [POS-913] #1290

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.

Sign up for GitHub

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

Jump to bottom

Merged

ismolyakov merged 14 commits into master from ivan/po-905/msg-tel-sv

Jan 25, 2023

Contributor

ismolyakov commented Jan 20, 2023 •

edited

Loading

Description

@swift-nav/devinfra

this PR adds the first Starling telemetry message definition (MSG_TEL_SV).
the massage definition is validated through Starling prototype; CI and unit testing at Starling side passes.

Related PRs

Starling telemetry output infrastructure (output is enabled via feature flag): https://github.com/swift-nav/starling/pull/7425
Further telemetry output prototyping: https://github.com/swift-nav/starling/pull/7451

API compatibility

Upd: no risk to API compatibility as it is a new message, no existing fields are modified.

API compatibility plan

n/a

JIRA Reference

https://swift-nav.atlassian.net/browse/POS-913

ismolyakov requested review from a team, notoriaga and silverjam as code owners

January 20, 2023 19:40

ismolyakov requested review from ebethon and reimerix

January 20, 2023 19:41

Contributor

silverjam commented Jan 21, 2023 •

edited

Loading

API compatibility

Does this change introduce a API compatibility risk? -- not truly sure what I should be looking after to evaluate this risk :)

API compatibility plan

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

TBD?

This main thing we're looking for is changes in the wire format of a message (e.g. the number of bytes in a message and/or the format of the bytes in the message). Some discussion on the issues is part of this doc: https://swift-nav.atlassian.net/l/cp/XCuWGumf.

Adding a new message does not affect API compatibility from an SBP message perspective.

silverjam reviewed

View reviewed changes

Contributor

silverjam left a comment

@ismolyakov I don't see an update to sbp.pdf -- we might need to add telemetry.yaml to the pdf doc generation commands

richarddeurloo reviewed

View reviewed changes

spec/yaml/swiftnav/sbp/telemetry.yaml Outdated

Comment on lines 118 to 125

+                          desc: Flags to identify Starling component the telemetry is reported from.
+                          fields:
+                            - 0-7:
+                                desc: Starling component
+                                values:

Contributor

richarddeurloo Jan 23, 2023

In the design doc this is named Filter type instead of Starling component. Why was this changed? It seems very product name specific. I'd suggest keeping the more generic form.

Contributor Author

ismolyakov Jan 23, 2023

My thinking for using standalone and differential was to keep the names generic and not to include Starling-specific filter names (SPP, RTK and other) since the names are customer-facing.

No strong opinion here though, would you prefer SPP and RTK better @richarddeurloo ?

Contributor

richarddeurloo Jan 23, 2023

My comment was not about the values, but about the descriptions. I would not include the word Starling in there. My suggestions would be:

Suggested change

      
                        desc: Flags to identify Starling component the telemetry is reported from.
          
                        fields:
          
                          - 0-7:
          
                              desc: Starling component
          
                              values:
          
                        desc: Flags to identify the filter type from which the telemetry is reported.
          
                        fields:
          
                          - 0-7:
          
                              desc: Filter type:
          
                              values:

Contributor Author

ismolyakov Jan 23, 2023

makes total sense, thank you for clarification!

spec/yaml/swiftnav/sbp/telemetry.yaml Show resolved Hide resolved

reimerix approved these changes

View reviewed changes

Contributor

reimerix left a comment

LGTM! Just some questions and non-blocking suggestions.

spec/yaml/swiftnav/sbp/telemetry.yaml Outdated

+                    fields:
+                      - az:
+                          type: u8
+                          units: deg * 2g

Contributor

reimerix Jan 23, 2023

Typo? Should this be deg * 2?

Contributor Author

ismolyakov Jan 23, 2023

yes, it is indeed deg * 2: already corrected!

spec/yaml/swiftnav/sbp/telemetry.yaml

+                          desc: Observation availability at filter update
+                          fields:
+                            - 3-7:
+                                desc: Reserved

Contributor

reimerix Jan 23, 2023

Should we add a bit for measured Doppler as well?

spec/yaml/swiftnav/sbp/telemetry.yaml

Comment on lines +43 to +49

+                      - pseudorange_residual:
+                          type: s16
+                          units: 1 dm
+                          desc: Pseudorange observation residual

Contributor

reimerix Jan 23, 2023

Should we have a field for doppler residuals too?

Contributor Author

ismolyakov Jan 23, 2023

@reimerix , as of now, I am proposing to use phase_residual as one field for both computed-Doppler and carrier-phase since they are (currently) never output from the same filter.

An alternative is to have two additional s16 fields for measured and computed Doppler. This is more robust to future Starling architecture changes, but will increase the message size. How concerning is to add two s16 fields from bandwidth point of view?

spec/yaml/swiftnav/sbp/telemetry.yaml Outdated

Comment on lines 81 to 88

+                          fields:
+                            - 3-7:
+                                desc: Reserved
+                            - 0-2:
+                                desc: Reason for ephemeris invalidity
+                                values:
+                                  - 0: Valid
+                                  - 1: Invalid (general status, to be extended)

Contributor

reimerix Jan 23, 2023

How about this instead?

Suggested change

      
                        fields:
          
                          - 3-7:
          
                              desc: Reserved
          
                          - 0-2:
          
                              desc: Reason for ephemeris invalidity
          
                              values:
          
                                - 0: Valid
          
                                - 1: Invalid (general status, to be extended)
          
                        fields:
          
                          - 1-7:
          
                              desc: Reserved
          
                          - 0:
          
                              desc: Ephemeris available
          
                              values:
          
                                - 0: Valid ephemeris available
          
                                - 1: No valid ephemeris available (general status, to be extended in reserved fields)

Ivan Smolyakov added 6 commits

January 23, 2023 10:47


          auto: add msg_tel_sv

4e0482c


          typo

fa1bd1d


          manual: PR feedback

1d17672


          minor fix

05245ca


          shorten field description

2dacf5a


          auto: pr feedback

8121add

ismolyakov force-pushed the ivan/po-905/msg-tel-sv branch from 1b1bf3e to 8121add Compare

January 23, 2023 17:58

Contributor Author

ismolyakov commented Jan 23, 2023

@ismolyakov I don't see an update to sbp.pdf -- we might need to add telemetry.yaml to the pdf doc generation commands

@silverjam my bad, it is now generated after running make all

silverjam suggested changes

View reviewed changes

Contributor

silverjam left a comment

To satisfy test coverage requirements, please add a new test vector to spec/tests/yaml/swiftnav/sbp/telemetry/test_MsgTelSv.yaml (will need to create a new directory).

See the section on "New messages" in HOWTO.md:


          build: fix bazel build

89faeaa

Contributor

silverjam commented Jan 23, 2023

New PDF output looks good:

Ivan Smolyakov added 2 commits

January 24, 2023 11:04


          manual: add test MsgTelSv

0a66605


          Merge branch 'ivan/po-905/msg-tel-sv' of github.com:swift-nav/libsbp …

ad501de

…into ivan/po-905/msg-tel-sv

swiftnav-travis requested a review from swiftnav-adam

January 24, 2023 16:58

Ivan Smolyakov added 3 commits

January 24, 2023 17:11


          Merge remote-tracking branch 'origin/master' into ivan/po-905/msg-tel-sv

d2d9a25


          add test MsgTelSv


          remove comment

a7ff694

Contributor Author

ismolyakov commented Jan 24, 2023 •

edited

Loading

I am experiencing a failure in test_auto_check_sbp_telemetry_MsgTelSv unit test
/mnt/workspace/c/test/auto_check_sbp_telemetry_MsgTelSv.c:139:F:Automated_Suite_auto_check_sbp_telemetry_MsgTelSv:test_auto_check_sbp_telemetry_MsgTelSv:0: not enough data was written to dummy_buff (expected: 28, actual: 16)
, with no success in attempts to fix it.

@silverjam do you see anything immediately wrong with my generated test_MsgTelSv.yaml? Could it be related to what @Davinco experienced in his PR?

Contributor Author

ismolyakov commented Jan 24, 2023

I am experiencing a failure in test_auto_check_sbp_telemetry_MsgTelSv unit test /mnt/workspace/c/test/auto_check_sbp_telemetry_MsgTelSv.c:139:F:Automated_Suite_auto_check_sbp_telemetry_MsgTelSv:test_auto_check_sbp_telemetry_MsgTelSv:0: not enough data was written to dummy_buff (expected: 28, actual: 16) , with no success in attempts to fix it.

@silverjam do you see anything immediately wrong with my generated test_MsgTelSv.yaml? Could it be related to what @Davinco experienced in his PR?

trying to add c_decoded_fields: n_sv_tel: 1..

Contributor

silverjam commented Jan 24, 2023

@silverjam do you see anything immediately wrong with my generated test_MsgTelSv.yaml? Could it be related to what @Davinco experienced in his PR?

trying to add c_decoded_fields: n_sv_tel: 1..

Adding c_decoded_fields should fix this issue.


          add c_decoded_fields value to test definition

cb71044

Contributor Author

ismolyakov commented Jan 25, 2023

@silverjam do you see anything immediately wrong with my generated test_MsgTelSv.yaml? Could it be related to what @Davinco experienced in his PR?

trying to add c_decoded_fields: n_sv_tel: 1..

Adding c_decoded_fields should fix this issue.

it did! just waiting for hopefully one last make all to finish locally to ensure everything is generated.


          documentation

97e5148

silverjam approved these changes

View reviewed changes

Contributor Author

ismolyakov commented Jan 25, 2023

Thank you everyone for the comments and help! Merging as Benchmark stage fails in a similar way as the latest commits in master.

ismolyakov merged commit 75ace04 into master

ismolyakov deleted the ivan/po-905/msg-tel-sv branch

January 25, 2023 03:06

swiftnav-svc-jenkins mentioned this pull request

Auto submodule update: libsbp - Make the BaseTime inner public (#1332) swift-nav/libsettings#558

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richarddeurloo richarddeurloo left review comments

reimerix reimerix approved these changes

notoriaga Awaiting requested review from notoriaga notoriaga is a code owner

ebethon Awaiting requested review from ebethon

swiftnav-adam Awaiting requested review from swiftnav-adam

+1 more reviewer

silverjam silverjam approved these changes

Labels

None yet