Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65] #1108

fpezzinosn · 2022-04-06T22:08:45Z

Description

@swift-nav/devinfra

Adds the SSR_ORBIT_CLOCK_BOUNDS message to the SSR package as described here.

It also cherrypicks a fix for rust generation #1109 related to missing short_desc on the spec.

API compatibility

Does this change introduce a API compatibility risk?

It doesn't. I'm only adding one new message following the procedure.

API compatibility plan

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

JIRA Reference

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

python/tests/sbp/test_table.py

spec/yaml/swiftnav/sbp/integrity.yaml

fpezzinosn · 2022-04-06T22:30:30Z

spec/yaml/swiftnav/sbp/integrity.yaml

+          desc: Clock Bound Standard Deviation
+
+ - MSG_SSR_ORBIT_CLOCK_BOUNDS:
+    id: 1234


This id is a placeholder, It must be updated to the final value before merging the PR.

I think the ID here depends on if we go with this as a new "Integrity" label or put them under the existing SSR section.
@swift-nav/devinfra is there a best practice for selecting the message id? I've looked at the existing messages and they seem to be grouped with somewhat similar IDs in their respective sections.

I went with ID 1502 since the bounds provided in this message are the ones for MSG_SSR_ORBIT_CLOCK (1501)

I think the message IDs don't need to be as tightly grouped these days. Considering it is tricky figuring out the next free ID.

Does make mapping show all the currently used IDs or only the public ones?
Because I was basing it on that info.

IsakTjernberg

Great start! I have some comments/thoughts on the message definition. I also wonder if we shouldn't put this in the existing SSR section?
They are very much tied to the rest of the SSR format and in the Excel sheet are named such as:
MSG_SSR_ORBIT_CLOCK_BOUNDS.

spec/yaml/swiftnav/sbp/integrity.yaml

IsakTjernberg · 2022-04-07T11:42:30Z

spec/yaml/swiftnav/sbp/integrity.yaml

+          desc: Clock Bound Standard Deviation
+
+ - MSG_SSR_ORBIT_CLOCK_BOUNDS:
+    id: 1234


I think the ID here depends on if we go with this as a new "Integrity" label or put them under the existing SSR section.
@swift-nav/devinfra is there a best practice for selecting the message id? I've looked at the existing messages and they seem to be grouped with somewhat similar IDs in their respective sections.

spec/yaml/swiftnav/sbp/integrity.yaml

fpezzinosn · 2022-04-07T16:32:57Z

I have moved the message into the existing SSR package.

notoriaga · 2022-04-07T18:06:38Z

@fpezzinosn this should fix the rust generation problem - #1109 (could also add a short_desc field to your message. Or maybe you could make what you have as desc the short_desc and write something longer for the desc). Either way feel free to merge those changes into this pr

fpezzinosn · 2022-04-07T18:52:51Z

@fpezzinosn this should fix the rust generation problem - #1109 (could also add a short_desc field to your message. Or maybe you could make what you have as desc the short_desc and write something longer for the desc). Either way feel free to merge those changes into this pr

Yes! That was the issue
Thank you!

IsakTjernberg

This looks great, just some small renaming suggested here from me. I'll let Devinfra have the final say but I'm good with merging this after my comment is adressed.
Let's take the PR out of draft status?

spec/yaml/swiftnav/sbp/ssr.yaml

silverjam · 2022-04-13T21:23:56Z

spec/yaml/swiftnav/sbp/ssr.yaml

+           desc: Constellation ID to which the SVs belong.
+       - nb_sat:
+           type: u8
+           desc: Number of satellites. Encoded following RTCM DF387 specification.


Encoded following RTCM DF387 specification.

Is this true for this particular field?

Maybe you are right. In the Excel file we are saying that this field is similar to the existing RTCM Data Field. Perhaps Encoded isn't the best choice of word. (And also DF387 is only 6 bits long)

Maybe "Number of satellites, follows the RTCM DF387 specification." -- if it's only expected to be 6-bit, we should also document the valid ranges (0-63).

Yes probably we should drop the reference to RTCM here. After all it's just a number?
And I think we won't limit the range to only 6-bit.

We don't get any space savings from using int6 here since SBP isn't bit packed. It's just an extra step for the user to go through. I'd say get rid of the RTCM reference and just have a plain old u8

I think if we design these messages to more closely match their RTCM equivalents, we'll have an easier time when we go through the conversion effort. Data fidelity and correctness is more important than user experience. So... the question is, do we want do this now... or later?

Risks:

On doing it now: delaying progress on getting the end-to-end solution working with SBP

On doing it later: it'll get forgotten or we'll be in a situation where we'll lose fidelity or have to drop data when converting to RTCM

In general this also relates to this issue here: #1108 (comment)

These messages don't have RTCM equivalents. Any conversion to RTCM will be just wrapping them in the Swift proprietary RTCM message. The fields which are being referenced from RTCM though exist in other RTCM SSR message types which we will need to compare the values against, so there it makes sense to use a similar definition.

Alright, @woodfell's suggestion makes sense in that case

Removed all the references to RTCM where the field is just a number. Left the ones regarding satellites IDs and update interval since they have some special meaning. The comment now reads Similar to RTCM DF for context.

silverjam · 2022-04-13T22:17:15Z

spec/yaml/swiftnav/sbp/ssr.yaml

+           desc: Header of a bounds message.
+       - ssr_iod:
+           type: u8
+           desc: IOD of the SSR bound. Encoded following RTCM DF413 specification.


Are all of these values expected to use all 8 bits? Would it make sense to pack these values into a larger type as a collection of bitfields? This would allow use to more closely model the RTCM fields.

Something similar to this: https://github.com/swift-nav/libsbp/blob/master/spec/yaml/swiftnav/sbp/system.yaml#L227 but instead of flags something like metadata

Or probably more similar to this: https://github.com/swift-nav/libsbp/blob/master/spec/yaml/swiftnav/sbp/system.yaml#L200 -- since we won't be able to specify the values field in the yaml spec

@silverjam do you know how this works for other already existing SBP messages which mimic RTCM fields?
For example SSR_CODE_BIASES here:

libsbp/spec/yaml/swiftnav/sbp/ssr.yaml

Lines 293 to 297 in 8a6c621

- update_interval:

type: u8

desc: >

Update interval between consecutive corrections. Encoded

following RTCM DF391 specification.

The update_interval is specified as a u8, but it also says it's encoded as DF391 which is only 4 bits.
I would think we still fill out all 8 bits, but the interpretation of it's value is then according to DF391?

Not sure, but I guess if we already have this "problem" there's no need to solve it here. I would think though that there's few scenarios: (1) we can't encode our current messages into RTCM equivalents; (2) we'd need to scale values in order to encode them or (3) we'd need to split our messages over multiple rtcm messages to encoded them.

spec/yaml/swiftnav/sbp/ssr.yaml

IsakTjernberg · 2022-04-25T15:16:12Z

@swift-nav/devinfra any objections to merging this?

silverjam

@swift-nav/devinfra any objections to merging this?

Merging to master canonicalizes and "releases" the message. This should mean that the message is finalized and unlikely to change. If the message needs to change then we need to deal with retiring the old message for any changes that we need to make.

I think we need a strategy for handing these messages that will work with our existing testing and quality workflow while allowing us to iterate on these messages without incurring the overhead of having to rev and retire messages.

fpezzinosn · 2022-05-03T22:27:01Z

I've created a new staging branch as discussed and the PR now points to that branch.

* Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65] (#1108) * Add support for MSG_SSR_CODE_PHASE_BIASES_BOUNDS message [OTA-119] (#1127) * Add Atmospheric Corrections and Bounds [OTA-120] (#1131) Co-authored-by: Jason Mobarak <jason@swift-nav.com> * Add support for MSG_SSR_ORBIT_CLOCK_BOUNDS_DEGRADATION message [OTA-133] (#1137) * Add Integrity Flags messages [OTA-121] (#1132) * Add transformation parameter message [OTA-149] (#1144) * Add leap second message [OTA-148] (#1145) * Clean up the documentation for the new messages [OTA-150] (#1149) Co-authored-by: swiftnav-svc-jenkins <42622338+swiftnav-svc-jenkins@users.noreply.github.com> Co-authored-by: Jason Mobarak <jason@swift-nav.com> * Updates messages to ICD v1.2 [OTA-161] (#1155) * make all Co-authored-by: Jason Mobarak <jason@swift-nav.com> Co-authored-by: swiftnav-svc-jenkins <42622338+swiftnav-svc-jenkins@users.noreply.github.com>

fpezzinosn self-assigned this Apr 6, 2022

fpezzinosn commented Apr 6, 2022

View reviewed changes

python/tests/sbp/test_table.py Outdated Show resolved Hide resolved

spec/yaml/swiftnav/sbp/integrity.yaml Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

fpezzinosn requested a review from IsakTjernberg April 6, 2022 22:25

fpezzinosn commented Apr 6, 2022

View reviewed changes

IsakTjernberg reviewed Apr 7, 2022

View reviewed changes

fpezzinosn force-pushed the fpezzinosn/OTA-65 branch from 1f81241 to 78297d0 Compare April 7, 2022 16:03

notoriaga mentioned this pull request Apr 7, 2022

fix rust gen #1109

Closed

fpezzinosn force-pushed the fpezzinosn/OTA-65 branch from 78297d0 to f59dfe4 Compare April 7, 2022 18:50

This comment was marked as resolved.

Sign in to view

fpezzinosn force-pushed the fpezzinosn/OTA-65 branch from cc00d97 to 37fc5d4 Compare April 12, 2022 13:20

IsakTjernberg reviewed Apr 13, 2022

View reviewed changes

spec/yaml/swiftnav/sbp/ssr.yaml Outdated Show resolved Hide resolved

fpezzinosn force-pushed the fpezzinosn/OTA-65 branch from 2a41b59 to 984548d Compare April 13, 2022 18:17

fpezzinosn marked this pull request as ready for review April 13, 2022 18:25

fpezzinosn requested review from a team as code owners April 13, 2022 18:25

silverjam reviewed Apr 13, 2022

View reviewed changes

woodfell reviewed Apr 13, 2022

View reviewed changes

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

spec/yaml/swiftnav/sbp/ssr.yaml Outdated Show resolved Hide resolved

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

This comment was marked as resolved.

Sign in to view

IsakTjernberg approved these changes Apr 25, 2022

View reviewed changes

fpezzinosn requested review from jungleraptor, ljbade and silverjam April 27, 2022 12:38

fpezzinosn requested review from notoriaga and woodfell April 27, 2022 12:38

notoriaga approved these changes Apr 27, 2022

View reviewed changes

silverjam suggested changes Apr 28, 2022

View reviewed changes

silverjam force-pushed the master branch from da0ae95 to b423ed3 Compare May 3, 2022 06:47

woodfell approved these changes May 3, 2022

View reviewed changes

fpezzinosn changed the base branch from master to staging May 3, 2022 21:25

fpezzinosn added 2 commits May 3, 2022 17:39

Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65]

ea77529

make all

b03a7e1

fpezzinosn force-pushed the fpezzinosn/OTA-65 branch from c451972 to b03a7e1 Compare May 3, 2022 22:24

fpezzinosn requested a review from silverjam May 3, 2022 22:26

silverjam approved these changes May 3, 2022

View reviewed changes

fpezzinosn merged commit 147153e into staging May 3, 2022

fpezzinosn deleted the fpezzinosn/OTA-65 branch May 3, 2022 22:55

fpezzinosn mentioned this pull request May 4, 2022

Add support for MSG_SSR_CODE_PHASE_BIASES_BOUNDS message [OTA-119] #1127

Merged

fpezzinosn added a commit that referenced this pull request Jun 17, 2022

Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65] (#1108)

86c07da

	- update_interval:
	type: u8
	desc: >
	Update interval between consecutive corrections. Encoded
	following RTCM DF391 specification.

Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65] #1108

Add support for SSR_ORBIT_CLOCK_BOUNDS message [OTA-65] #1108

Uh oh!

Conversation

fpezzinosn commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

API compatibility

API compatibility plan

JIRA Reference

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IsakTjernberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fpezzinosn commented Apr 7, 2022

Uh oh!

notoriaga commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fpezzinosn commented Apr 7, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

IsakTjernberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

fpezzinosn commented Apr 6, 2022 •

edited

Loading

notoriaga commented Apr 7, 2022 •

edited

Loading