Skip to content
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.

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

add MsgAcknowledge #1263

Merged
merged 16 commits into from
Dec 13, 2022
Merged

add MsgAcknowledge #1263

merged 16 commits into from
Dec 13, 2022

Conversation

notoriaga
Copy link
Contributor

Description

@swift-nav/devinfra

Implements message defined here - https://swift-nav.atlassian.net/wiki/spaces/ENG/pages/2385609303/Acknowledge+Message+-+Requirements+Doc#2.-Interface-Requirements

None of the existing packages seemed right so I created a skylark package, happy to change that around if anyone has a better idea

JIRA Reference

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

@notoriaga notoriaga requested review from a team and silverjam as code owners December 8, 2022 20:22
fields:
- request_counter:
type: u8
desc: Echo of the request counter field from the corresponding CRA message, or 255 if no request counter was provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

To me a "counter" implies a sequence that starts at zero and I'm not sure that that's important for this message, I've proposed to System Design that this be reword/recast as a "request ID" so that we don't have the implied behavior of a counter-- do you have thoughts on this?

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 agree, especially considering we just added stream_counter and on_demand_counter which behavior exactly as you describe, I'll see about changing the documentation

spec/yaml/swiftnav/sbp/skylark.yaml Outdated Show resolved Hide resolved
Comment on lines 31 to 39
fields:
- 0-7:
desc: Response code
values:
- 0: Ok
- 1: Out of coverage
- 2: Forbidden
- 3: Invalid request
- 4: Invalid area id
Copy link
Contributor

Choose a reason for hiding this comment

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

this generates "masking" code:

#define SBP_ACKNOWLEDGE_RESPONSE_CODE_MASK (0xffu)
#define SBP_ACKNOWLEDGE_RESPONSE_CODE_SHIFT (0u)
#define SBP_ACKNOWLEDGE_RESPONSE_CODE_GET(flags)               \
  ((u8)((u8)((flags) >> SBP_ACKNOWLEDGE_RESPONSE_CODE_SHIFT) & \
        SBP_ACKNOWLEDGE_RESPONSE_CODE_MASK))
#define SBP_ACKNOWLEDGE_RESPONSE_CODE_SET(flags, val)                      \
  do {                                                                     \
    (flags) = (u8)((flags & (~(SBP_ACKNOWLEDGE_RESPONSE_CODE_MASK          \
                               << SBP_ACKNOWLEDGE_RESPONSE_CODE_SHIFT))) | \
                   (((val) & (SBP_ACKNOWLEDGE_RESPONSE_CODE_MASK))         \
                    << (SBP_ACKNOWLEDGE_RESPONSE_CODE_SHIFT)));            \
  } while (0)

Which is strange as this variable doesn't represent a bit mask field. Is there any type that represents enumerator types rather bit fields?

With the current setup, we are going to be limited to only being able to add 3 more error codes in the future rather than 200 more possible values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I thought what I have should work, should be the same as

desc: Signal constellation, band and code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that generates similar code. I guess if you have

fields:
   - 0-7: ... # or 0 to whatever size int it is

we could probably just skip generating the mask/shifting bit and just generate the constants like -

#define SBP_GNSSSIGNAL_GPS_L1CA (0)

Comment on lines 41 to 44
type: u16
desc: >
Contains the message group(s) that will be sent in response from the corresponding CRA correction mask.
An echo of the correction mask field from the corresponding CRA message.
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be a bit mask field.

Name Bit Description
Key 0 Public Signing Key
Low-Rate Messages 1 ITRF, UTC and APC Messages
Ephemeris 2 Ephemeris data
Satellite Clock 3 Clocks
Satellite Orbit 4 Orbit
Satellite code bias 5 Code Biases
Satellite phase bias 6 Phase Biases
Atmospherics 7 Tile Definition, STEC and Grid data
Integrity 8 Bounds, Degradation, and Flags
Reserved 9-15  

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh right thanks, added it

c/include/libsbp/integrity_macros.h Show resolved Hide resolved
Comment on lines +1 to +2
{
"copyright": [

Choose a reason for hiding this comment

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

Is this msg part of this PR as well?

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 think someone forgot to run all the code gen at some point, and missed some for this message -

- MSG_POSE_RELATIVE:

proto/navigation.proto Show resolved Hide resolved
@RReichert RReichert self-requested a review December 13, 2022 01:41
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