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.

Sign up for GitHub

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

Jump to bottom

STANAG 4607 packets + mission segment + dwell segment #1716

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

STANAG 4607 packets + mission segment + dwell segment #1716

wants to merge 10 commits into from

Conversation

hdefazio
Copy link
Member

@hdefazio hdefazio commented Dec 20, 2022 •
edited
Loading

Initial version of a STANAG 4607 packet reader.

@kwcvrobot
Copy link
Collaborator

Kwiver CentOS build succeeded.

@hdefazio hdefazio force-pushed the dev/stanag_4607_mission branch 3 times, most recently from 3253cd3 to 3586050 Compare December 20, 2022 16:34
@hdefazio hdefazio force-pushed the dev/stanag_4607_mission branch from c4dea5b to 4d933ae Compare December 20, 2022 16:41
@kwcvrobot
Copy link
Collaborator

Kwiver Linux build succeeded.

@kwcvrobot
Copy link
Collaborator

Kwiver CentOS build succeeded.

@kwcvrobot
Copy link
Collaborator

Kwiver Mac build succeeded.

@hdefazio
Copy link
Member Author

@judajake / @dstoup PTAL

@kwcvrobot
Copy link
Collaborator

Kwiver Windows build succeeded.

@hdefazio hdefazio force-pushed the dev/stanag_4607_mission branch from b4e72d0 to 632b536 Compare January 23, 2023 18:16
@kwcvrobot
Copy link
Collaborator

Kwiver Linux build succeeded.

@kwcvrobot
Copy link
Collaborator

Kwiver CentOS build succeeded.

@hdefazio hdefazio changed the title STANAG 4607 packets + mission segment STANAG 4607 packets + mission segment + dwell segment Jan 23, 2023
@kwcvrobot
Copy link
Collaborator

Kwiver Mac build succeeded.

@hdefazio
Copy link
Member Author

@judajake PTAL: This reads the files I have so far

judajake
judajake reviewed Jan 23, 2023
View reviewed changes
Copy link
Collaborator

@judajake judajake left a comment

Choose a reason for hiding this comment

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

A couple of suggestions. I think there would also be some value in adding a bit of documentation to point people to the standard.

arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_segment_lookup.cxx Outdated Show resolved Hide resolved
@hdefazio hdefazio force-pushed the dev/stanag_4607_mission branch from 86ef8d2 to f969ba0 Compare January 23, 2023 21:43
@kwcvrobot
Copy link
Collaborator

Kwiver CentOS build succeeded.

@kwcvrobot
Copy link
Collaborator

Kwiver Linux build succeeded.

@kwcvrobot
Copy link
Collaborator

Kwiver Mac build succeeded.

@dstoup
Copy link
Collaborator

dstoup commented Jan 26, 2023

I will look in more detail later but quick question, is this edition B? If so, I wonder if it makes sense to call that out in the files names/code. The other thought is that the first edition isn't used anymore so it doesn't matter. Thoughts?

@hdefazio
Copy link
Member Author

@dstoup The document I was working off of says

"ANNEX A TO
STANAG 4607
(Edition 3)"

I'd vote for just documenting it somewhere for now. Should we add a README?

@dstoup
Copy link
Collaborator

dstoup commented Jan 26, 2023

Sorry for the noise, I was misreading 4607 as 4676 :/

bradh
bradh reviewed Feb 8, 2023
View reviewed changes
Copy link

@bradh bradh left a comment

Choose a reason for hiding this comment

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

Calling it "gmti" instead of "stanag" in the paths / names would be more descriptive.

CMakeLists.txt
@@ -112,6 +112,7 @@ OPTION(KWIVER_ENABLE_SPROKIT "Enable building sprokit" OFF )
if(KWIVER_ENABLE_ARROWS)
OPTION(KWIVER_ENABLE_MVG "Enable Multi-View Geometry Arrow" ON )
OPTION(KWIVER_ENABLE_KLV "Enable Key-Length-Value Metadata Arrow" ON)
OPTION(KWIVER_ENABLE_STANAG "Enable NATO Standardization Agreement Arrow" ON)
Copy link

Choose a reason for hiding this comment

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

There are a bunch of STANAGs, of which 4607 is just one (and 4676 is probably also interesting, along with 4609). Perhaps

Suggested change
OPTION(KWIVER_ENABLE_STANAG "Enable NATO Standardization Agreement Arrow" ON)
OPTION(KWIVER_ENABLE_4607 "Enable NATO Standardization Agreement 4607 (GMTI) Arrow" ON)

and corresponding changes below.

Copy link
Member Author

Choose a reason for hiding this comment

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

@judajake Do you have a preference between making each STANAG its own arrow or having one arrow for all of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bias is towards having one arrow. Longterm, hopefully, more will be supported.

Copy link

Choose a reason for hiding this comment

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

I think you already have two (since that is what the KLV arrow is - STANAG 4609). So it would not be consistent to say that this is the NATO Standardization Agreement Arrow.

arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/segments/stanag_4607_dwell_segment.cxx Outdated Show resolved Hide resolved
arrows/stanag/stanag_4607_packet.cxx Show resolved Hide resolved
arrows/stanag/stanag_4607_packet.h Show resolved Hide resolved
@hdefazio hdefazio force-pushed the dev/stanag_4607_mission branch from 3315ce4 to 6c2f1f1 Compare February 15, 2023 17:57
bradh
bradh reviewed Feb 15, 2023
View reviewed changes
arrows/stanag/stanag_util.cxx
float
float_to_binary_angle( float value, int n )
{
return value * 1.40625 * ( 1 / pow( 2, n - 8 ) );
Copy link

Choose a reason for hiding this comment

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

One thing you might like to consider here (and for the SA case) is that n will only ever be one of two values (16 or 32). So you can turn this into a straight scaling factor, either with if (n == ...) construct or by defining two functions for each case (e.g. float_to_binary_angle_16()). Then you don't have the pay the cost of the pow() call each time.

judajake
judajake approved these changes Feb 17, 2023
View reviewed changes
CMakeLists.txt
@@ -112,6 +112,7 @@ OPTION(KWIVER_ENABLE_SPROKIT "Enable building sprokit" OFF )
if(KWIVER_ENABLE_ARROWS)
OPTION(KWIVER_ENABLE_MVG "Enable Multi-View Geometry Arrow" ON )
OPTION(KWIVER_ENABLE_KLV "Enable Key-Length-Value Metadata Arrow" ON)
OPTION(KWIVER_ENABLE_STANAG "Enable NATO Standardization Agreement Arrow" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

My bias is towards having one arrow. Longterm, hopefully, more will be supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradh bradh bradh left review comments

@judajake judajake judajake approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hdefazio @kwcvrobot @dstoup @bradh @judajake