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

Added geolocation to spatial extension within annotations #282

Merged
merged 9 commits into from Jun 29, 2023

Conversation

777arc
Copy link
Member

@777arc 777arc commented Apr 19, 2023

To capture the estimated or known location of a transmitter/emission being annotated

Copy link
Member

@jacobagilbert jacobagilbert left a comment

Choose a reason for hiding this comment

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

This seems OK but the next thing that will be desired is a way to capture uncertainty... is that something you have a good idea for?

@@ -203,15 +203,20 @@ object:
|----|--------|----|-----|-----------|
|`signal_azimuth`|false|double|degrees|Azimuth in degrees east of true north associated with the specific annotation.|
|`signal_bearing`|false|[bearing](spatial.sigmf-ext.md#01-the-bearing-object)|N/A|Bearing associated with the specific annotation.|
| `geolocation` | false | GeoJSON `point` Object | N/A | The location (either known or estimated) of the transmitter corresponding to this annotation |
Copy link
Member

Choose a reason for hiding this comment

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

lets call this signal_geolocation or emitter_geolocation to avoid confusion with the Annotations Scope latitude and longitude fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we were getting rid of those 2 rows anyway
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly those 2 rows are very confusing because it says use "core:geolocation" but doesnt explain anything else, like if it means rx side or tx side geolocation

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is indeed confusing. Originally these fields were designed to hold the system location and were annotations-scoped to allow changing position. This predates me a bit, but I think everyone agrees it was not the ideal choice.

These have been deprecated since 0.9 and will be formally removed in 2.0

@jacobagilbert jacobagilbert force-pushed the geolocation-in-spatial-annotations branch from 00fdc26 to 95618da Compare April 26, 2023 04:45
@777arc
Copy link
Member Author

777arc commented Jun 20, 2023

@gmabey or @Nepomuceno can you give this a sanity check before we merge it?

@777arc
Copy link
Member Author

777arc commented Jun 20, 2023

also @jacobagilbert do we bump v1.0.0 to like 1.1.0?

@jacobagilbert
Copy link
Member

yea, we should.

@Nepomuceno
Copy link
Contributor

Looks good. That would definitely be used by us.

@777arc
Copy link
Member Author

777arc commented Jun 20, 2023

ok @jacobagilbert i believe it's ready to squash+merge

@jacobagilbert jacobagilbert merged commit 6353dcc into sigmf-v1.x Jun 29, 2023
@argilo
Copy link
Contributor

argilo commented Oct 5, 2023

The extension version number in the example should probably be updated too.

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.

None yet

4 participants