-
Notifications
You must be signed in to change notification settings - Fork 298
Add support for proprietary PASHR sentence #189
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
base: ros2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -199,21 +199,30 @@ def convert_deg_to_rads(degs): | |
| "VTG": [ | ||
| ("true_course", convert_deg_to_rads, 1), | ||
| ("speed", convert_knots_to_mps, 5) | ||
| ], | ||
| "PASHR": [ | ||
| ("utc_time", convert_time, 1), | ||
| ("heading", safe_float, 2), | ||
| ("roll", safe_float, 4), | ||
| ("pitch", safe_float, 5), | ||
|
Comment on lines
+204
to
+207
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to this source, there are several other fields in the PASHR sentence. Why not include those here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I quickly updated the driver while working on a robot of opportunity. I didn't have the time to add the other fields and make sure they were being paresed properly. Unfortunately, I didn't record raw NMEA data so I could implement and test those extra fields now. I decided to create this PR since partial support for a new sentence is better than none in my opinion.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it stands, I don't think I can accept this partial implementation. I understand if you don't have the time or resources to complete this work. Hopefully someone else will be able to. |
||
| ] | ||
| } | ||
|
|
||
|
|
||
| def parse_nmea_sentence(nmea_sentence): | ||
| # Check for a valid nmea sentence | ||
|
|
||
| if not re.match(r'(^\$GP|^\$GN|^\$GL|^\$IN).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | ||
| if not re.match(r'(^\$GP|^\$GN|^\$GL|^\$IN|^\$P).*\*[0-9A-Fa-f]{2}$', nmea_sentence): | ||
| logger.debug("Regex didn't match, sentence not valid NMEA? Sentence was: %s" | ||
| % repr(nmea_sentence)) | ||
| return False | ||
| fields = [field.strip(',') for field in nmea_sentence.split(',')] | ||
|
|
||
| # Ignore the $ and talker ID portions (e.g. GP) | ||
| sentence_type = fields[0][3:] | ||
| # Support proprietary sentences that start with P | ||
| if fields[0][0:2] == "$P": | ||
| sentence_type = fields[0][1:] | ||
|
|
||
| if sentence_type not in parse_maps: | ||
| logger.debug("Sentence type %s not in parse map, ignoring." | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you note above, this orientation is in a different frame than the heading. Having conflicting orientation conventions published with the same frame will be confusing to clients and could be problematic if both are fed into the same localization filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it could cause confusion. Practically, if a device reports orientation using a $PASHR sentence, you shouldn't need to use a separate heading sentence. Maybe is should be documented that a user should use the heading or orientation output, but not both?
In my experience, devices that output a $PASHR sentence are high end integrated system that provide a navigation solution that does not need extre filtering so we don't use a localization filter.