Skip to content

Conversation

lloydmaza
Copy link
Contributor

@lloydmaza lloydmaza commented Jan 7, 2022

Context: the MSG_VEL_COG spec was defined at the end of 2021 to support a customer need for non-fusion heading output. The message was defined quickly and without rigorous review, and a handful of issues were identified only after it was released. Upon further review, it was determined that we should update the message to fix these issues rather than define a new message altogether.
Its use has been limited to a single release branch (Starling v1.2.20), meaning the message has only been used by one external stakeholder. Deprecation is not imperative in this case to maintain usability, so the risk of this update appears relatively small and is favored as a quicker and less cumbersome solution.

Changes made to the message definition:

  • flags field increased from 8 bits to 16
  • "Velocity mode" and "INS mode" flags increased from 1 bit wide to 2 (as is standard with other velocity messages)
  • "Type of reported TOW" flag added (as is standard with other velocity messages)
  • "COG frozen" flag added to indicate when COG is frozen
  • Wording changed on vel_d field to v_up

All other files changed as a result of updated bindings or updated test definitions.

Note: I had some trouble updating the test definitions, but eventually worked something out. My ad hoc procedure is written out here.

@lloydmaza lloydmaza force-pushed the lloyd/cog_message_improvements branch from 4d9d047 to f906fd6 Compare January 7, 2022 07:08
@reimerix reimerix changed the title [TES-129] Improvements to MSG_VEL_COG Improvements to MSG_VEL_COG [TES-129] Jan 7, 2022
Copy link
Contributor

@reimerix reimerix left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks for writing up your process for updating the test definitions!

@silverjam
Copy link
Contributor

Can we write up the rational for mutation of the existing message rather than depreciation of the existing message and the creation of a new one? We've technically released the existing message, so we need to be careful here.

@lloydmaza
Copy link
Contributor Author

@silverjam great point, I've updated the description to include the rationalization for this approach. Let me know if you have any disagreements with the reasoning laid out - we've already encountered issues due to rushed implementation, so I'd like to avoid that going forward if we're introducing more risk than I've anticipated.

Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

Thanks for writing up the reasoning on why we are skipping deprecation

@lloydmaza lloydmaza merged commit 07c4dfe into master Jan 7, 2022
@lloydmaza lloydmaza deleted the lloyd/cog_message_improvements branch January 7, 2022 17:57
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.

3 participants