Add support for QZSS constellation [OI-1792] #1414
Conversation
| env: | ||
| ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true |
There was a problem hiding this comment.
This was added in order to solve an error on the workflow. Discussion can be found here
cstolowicz
force-pushed
the
cstolowicz/OI-1792
branch
from
9196c41
to
66e4d03
Compare
| @@ -18,13 +18,13 @@ tests: | |||
| use_gps_sat: 1 | |||
| use_gal_sat: 2 | |||
| use_bds_sat: 3 | |||
| use_qzss_sat: 0 | |||
There was a problem hiding this comment.
can you try setting a value different that zero so that we can test if this new field is being encoded/decoded correctly?
this will probably affect the raw_packet, payload and crc representation, so those will need to also be updated
There was a problem hiding this comment.
I'll be able to tackle this change on Wednesday. I've been trying to obtain the CRC with the change and I've been having some issues with it, that's why I haven't uploaded the change yet
| uint32 use_qzss_sat = 10; | ||
| uint32 use_tropo_grid_points = 12; |
There was a problem hiding this comment.
I think partially using the reserved field is going to cause problems here. If the original intent for the expansion was to use those fields one by one, then they should have been marked all independently.
Maybe the correct procedure here is to mark the message as deprecated and create a V2? I am not completely sure, so it would be great if someone with more experience with this repo could chime in.
There was a problem hiding this comment.
We've discussed about changing the reserved field array for five different reserved fields but each field should have a different name. We've also discussed about deprecating the previous high level message. I believe these changes entail a more profound thought about how to design this new message, which may be outside of the scope of my task.
Wdyt about leaving it as I defined it and creating a ticket to revisit the way the reserved fields are defined, on this and other messages as well?
There was a problem hiding this comment.
I guess since the byte offsets are preserved, then this should be fine
cstolowicz
force-pushed
the
cstolowicz/OI-1792
branch
from
66e4d03
to
718fb5a
Compare
There was a problem hiding this comment.
it's hard for me to approve this without having a little more context. I can see where you are coming from with disabling the message validation CI check. We should be sure to re-enable it after you merge this PR
I believe the Benchmark CI check is just flat out broken because no runners are picking up the job
cstolowicz
force-pushed
the
cstolowicz/OI-1792
branch
from
5b2a055
to
073233b
Compare
@pcrumley To give you more context: We're working on adding integrity support for QZSS and that involves changing the high level flag messages to also report the status of the QZSS satellites. The new QZSS field will replace one of the reserved fields as stated on the ICD and coded here. Feel free to ask any questions if needed. |
There was a problem hiding this comment.
There is no runner that is going to pick-up this job https://github.com/swift-nav/libsbp/actions/runs/9876111694/job/27274391320?pr=1414
macos-11 is no longer a valid label
Yes @fpezzinosn , sorry, I forgot to merge against your branch |
cstolowicz
force-pushed
the
cstolowicz/OI-1792
branch
from
adb3a4c
to
4b48304
Compare
|
| uint32 use_qzss_sat = 10; | ||
| uint32 use_tropo_grid_points = 12; |
There was a problem hiding this comment.
I guess since the byte offsets are preserved, then this should be fine
Description
@swift-nav/devinfra
use_qzss_satfield to the High Level Flag Message which decreased the amount of reserved fields from 6 to 5.API compatibility
Does this change introduce a API compatibility risk?
API compatibility plan
JIRA Reference
https://swift-nav.atlassian.net/browse/OI-1792