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 new update status message [ASF-128] #1095

Merged
merged 7 commits into from
Jan 28, 2022

Conversation

reimerix
Copy link
Contributor

@reimerix reimerix commented Jan 27, 2022

This pull requests adds the message definition and generated bindings for a new diagnostic message.

Copy link
Contributor

@richarddeurloo richarddeurloo left a comment

Choose a reason for hiding this comment

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

I only did a quick pass on the generated files. I assume these will be fine. I added some questions about the message spec.

desc: Reserved for future use
- n_available_meas:
type: u8
desc: Number of available measurement updates in this epoch
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is a bit confusing to me now. It suggests that it reports an accumulation of updates. Why not call it measurements instead of measurement updates? Same for the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change this - will do that with the next commits that will add the new test cases.

be generated asynchronously to the solution messages and will be emitted
anytime a sensor update is being processed.
fields:
- time:
Copy link
Contributor

Choose a reason for hiding this comment

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

You changed this from tow in the design doc to time. Which time is it? If it's variable, should we explicitly include a time source flag?

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 wanted to keep the door for that open and found the tow field name confusing e.g. if outputting a CPU local time. We have 32 reserved bits in the flags field. I anticipate us using a portion of them in a common way for all sensors, while other bits might have specialised meanings for every sensor. We could use some of the common bits in the future to signal which time we're reporting (CPU local, GNSS, or something else)

Comment on lines +453 to +459
- 0: GNSS position
- 1: GNSS average velocity
- 2: GNSS instantaneous velocity
- 3: Wheel ticks
- 4: Wheel speed
- 5: IMU
- 6: Time differences of carrier phase
Copy link
Contributor

@richarddeurloo richarddeurloo Jan 27, 2022

Choose a reason for hiding this comment

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

Is there a way to define this one time and use it both here and in SOLN_META? Although the description don't match exactly. Was this intentional?

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 I've found a way to do this, but it would break compatibility for existing libsbp consumers. We would probably need to deprecate the existing SOLN_META message and define a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

How would it break compatibility for existing libsbp consumers? I guess we keep the meaning of the types the same, right?

Copy link
Contributor Author

@reimerix reimerix Jan 27, 2022

Choose a reason for hiding this comment

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

Because the type of the field would be a struct with a single u8 rather than a u8.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more along the line of defining the list in the spec yamls and then reuse that definition in other locations without changing the underlying type. I saw @CptStubbs doing it in the drive yaml. See also: https://stackoverflow.com/questions/8466223/reuse-a-block-of-code-in-yaml. But I'm not sure that's an option here, since the messages are defined in separate files. I didn't really think it through.

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 did that in the drive.yamls originally but it doesn't work here due to the way the SBP generator works. I spent more than a justifiable amount of time on this today and explored a number of options:

  • Use references as you pointed out: Those do not work because SOLN_META and this new message are defined in different sections. I tried placing the definition in a file that is included in both YAML specs but include in the generator is not the same as include in C and it doesn't work
  • Tried defining an embedded_type message in a new sensors.yaml file and got that working but then realised it would break compatibility with existing users of libsbp
  • Tried moving the new message to the solution meta section and then using references. This did not work for some reason either. The generator would not generate the field definitions for the new message.

If anyone else wants to try this out feel free to do so, but I'm giving up on this for now.

Copy link
Contributor

@GuillaumeDec GuillaumeDec left a comment

Choose a reason for hiding this comment

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

LGTM! 🙏

Copy link
Contributor

@richarddeurloo richarddeurloo left a comment

Choose a reason for hiding this comment

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

LGTM

@reimerix reimerix merged commit d118a68 into master Jan 28, 2022
@reimerix reimerix deleted the reimerix/asf-128/add-new-update-status-message branch January 28, 2022 11:39
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