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

[J-Turtle] Fix uninitialized values in NavSatFix and add missing NavSatStatus UNKNOWN #220

Merged
merged 4 commits into from
Apr 2, 2024
Merged

[J-Turtle] Fix uninitialized values in NavSatFix and add missing NavSatStatus UNKNOWN #220

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf35230
Fix unitialized values in NavSatFix and add missing UNKNOWN
Ryanf55 Apr 4, 2023
e8b50ab
Fix default initialization instead of constants
Ryanf55 Apr 4, 2023
af94745
Define SERVICE_UNKNOWN
Ryanf55 Apr 4, 2023
48b0080
Update sensor_msgs/msg/NavSatStatus.msg
tfoote Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sensor_msgs/msg/NavSatFix.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,4 @@ uint8 COVARIANCE_TYPE_APPROXIMATED = 1
uint8 COVARIANCE_TYPE_DIAGONAL_KNOWN = 2
uint8 COVARIANCE_TYPE_KNOWN = 3

uint8 position_covariance_type
uint8 position_covariance_type = COVARIANCE_TYPE_UNKNOWN
Ryanf55 marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion sensor_msgs/msg/NavSatStatus.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
# type and the last time differential corrections were received. A
# fix is valid when status >= STATUS_FIX.

int8 STATUS_UNKNOWN = -2 # status is not yet set
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me to have an uninitialized state as the default.

Copy link
Contributor Author

@Ryanf55 Ryanf55 Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I view it as a safety problem that the default state is "healthy". Because ROSIDL supports a default constructor in C++, users of NavSatFix are not required to fill this out. Thus, if they take shortcuts, only fit out the lat-long fields, and lose GPS signal,they now report the status as having a fix even though the data is invalid and might not realize it.

Copy link

Choose a reason for hiding this comment

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

Doesn't defaulting to STATUS_NO_FIX fix the problem?

Copy link

Choose a reason for hiding this comment

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

You could also default the latitude and longitude to NAN to avoid accidental visits to Null Island. https://en.wikipedia.org/wiki/Null_Island

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't defaulting to STATUS_NO_FIX fix the problem?

Yes, but adding a new value makes it exactly clear between these two situations:

  1. The ROS driver has started but it has not received a fix status message from the GPS =>STATUS_UNKNOWN
  2. The ROS driver has communicated with the GPS, and the GPS said it has no fix => STATUS_NO_FIX

I'd vote for keeping these as distinct as one means you might have a wiring or baud issue, and the other means that you just need to wait for it to warm up or get a better view of the sky.

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 am a fan of setting the latitude and longitude to NAN as default (assuming the ROS IDL lets us do that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could also default the latitude and longitude to NAN to avoid accidental visits to Null Island. https://en.wikipedia.org/wiki/Null_Island

This is not currently possible. See my docs PR which explains what's currently possible with NaN.
ros2/ros2_documentation#4210

I am working on adding support for NaN defaults and NaN constants, but it's not clear whether it will make the Jazzy release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding in the NaN logic is reasonable, but I think that should be a sanity check of the data, more than the canonical way to know what the status is. Checking all field values for NaN can be very expensive versus being able to quickly check an enumeration and stop processing data early.

Copy link
Contributor Author

@Ryanf55 Ryanf55 Mar 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would be more of a NaN fan if tooling supported it better, but there's currently no way to to set a NaN value in DDS IDL, nor is it supported in JSON. See here for implementation notes. I'd prefer not hold up this PR which was intended as a bugfix with other things like NaN support.
ros2/rosidl#789

int8 STATUS_NO_FIX = -1 # unable to fix position
int8 STATUS_FIX = 0 # unaugmented fix
int8 STATUS_SBAS_FIX = 1 # with satellite-based augmentation
int8 STATUS_GBAS_FIX = 2 # with ground-based augmentation

int8 status
int8 status = STATUS_UNKNOWN
Ryanf55 marked this conversation as resolved.
Show resolved Hide resolved

# Bits defining which Global Navigation Satellite System signals were
# used by the receiver.
Expand Down