Add support for proprietary PASHR sentence #189
Conversation
evenator
left a comment
evenator
left a comment
There was a problem hiding this comment.
Thanks for this contribution. I'm sorry that I'm only getting to review it now. I have a couple of comments that I'd like you to consider.
| ("utc_time", convert_time, 1), | ||
| ("heading", safe_float, 2), | ||
| ("roll", safe_float, 4), | ||
| ("pitch", safe_float, 5), |
There was a problem hiding this comment.
According to this source, there are several other fields in the PASHR sentence. Why not include those here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if data['roll'] and data['pitch'] and data['heading']: | ||
| current_orientation = QuaternionStamped() | ||
| current_orientation.header.stamp = current_time | ||
| current_orientation.header.frame_id = frame_id |
There was a problem hiding this comment.
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.
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.
2 participants
The $PASHR sentense is used by many intergrated motion systems, especially in the marine domain, to report heading, pitch and roll data.