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

VectorNav: Reorganize message packets #27585

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

lashVN
Copy link
Contributor

@lashVN lashVN commented Jul 17, 2024

A few key outputs for logging are missing, and the current structure prevents 800Hz IMU data using TYPE::INS. I would split the messages into:

IMU packet, EKF packet, GNSS packet

There should also be additional logging messages for the new packets coming through

This both aids in debugging, and allows use of VectorNav uncertainty and status information for internal publishing and use

@lashVN lashVN force-pushed the reorganize-message-packets branch from 8c7d8e8 to 39461cb Compare July 17, 2024 23:02
@lashVN
Copy link
Contributor Author

lashVN commented Jul 17, 2024

@peterbarker @IamPete1

I've tested that I can arm on a VN-210 and VN-100, and that all new logging messages output correct data. Please let me know if there's something else necessary. Unfortunately flight testing at our facility is quite a bit more difficult.

Edit:

Looks like I still need to update the SIM code to use the new packets. I'll resolve that tomorrow. Feel free to begin reviewing the 2 primary files, as I think those are ready.

@lashVN lashVN force-pushed the reorganize-message-packets branch 3 times, most recently from ee9bfac to 35a71f6 Compare July 17, 2024 23:50
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

You will also need to update the simulators to match.

Is the total data rate more or less? What rate is it trying to write all the logs?

libraries/AP_ExternalAHRS/AP_ExternalAHRS_VectorNav.cpp Outdated Show resolved Hide resolved
@@ -625,11 +518,7 @@ void AP_ExternalAHRS_VectorNav::process_ahrs_packet(const uint8_t *b)
#if AP_COMPASS_EXTERNALAHRS_ENABLED
{
AP_ExternalAHRS::mag_data_message_t mag;
if (use_uncomp) {
Copy link
Member

Choose a reason for hiding this comment

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

This existing functionality has been removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the naming of uncompaccel/uncompgyro/uncompmag implies they're handled the same, a distinct difference is the mag (compensated) isn't estimated by the internal EKF; our on-board Hard Iron estimator is a pretty rarely used feature, so it seemed like unnecessary bloat and bytes to me (especially given we're using it in our high-rate message). If we think that's still valuable I can readd it.

Copy link
Member

Choose a reason for hiding this comment

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

It is not only removed for mag? Its removed for accel/gyro too?

Copy link
Contributor Author

@lashVN lashVN Jul 18, 2024

Choose a reason for hiding this comment

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

It's available for accel/gyro, here. The diff is a bit misleading

Edit, not clear why permalink didnt' work. New file line 498, diff line 602

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK. So is the mag compensated or not now? My preference would be uncompensated I think. Maybe it is possible to turn off the compensation on the VN side so the user can still pick if they really care?

My argument for uncompensated is that AP should be able to do a better job of calibration as it can use battery current if it needs to where as the VN does not have that information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's technically using compensated, but unless the user has explicitly configured the VN to run the HSI estimator, the compensated and uncompensated outputs are both uncompensated.

libraries/AP_ExternalAHRS/AP_ExternalAHRS_VectorNav.h Outdated Show resolved Hide resolved
libraries/AP_ExternalAHRS/AP_ExternalAHRS_VectorNav.h Outdated Show resolved Hide resolved
@lashVN
Copy link
Contributor Author

lashVN commented Jul 18, 2024

You will also need to update the simulators to match.

Is the total data rate more or less? What rate is it trying to write all the logs?

I will update the simulators in my next push.

The total data rate is the same by default (50Hz + 50Hz + 5Hz), but allows for higher IMU rates. I believe because the messages were combined, the INS (used on VN-300) could only achieve 400Hz + 5Hz. It can now output at 800Hz + 50Hz + 5Hz.

It may be reasonable to introduce a config parameter to allow customizing the update rate of the EKF packet (which most imporantly includes attitude) but I decided to defer that to later work.

@lashVN lashVN force-pushed the reorganize-message-packets branch from d79355a to 23c72e6 Compare July 19, 2024 02:51
@lashVN lashVN changed the title Reorganize message packets VectorNav: Reorganize message packets Jul 19, 2024
@lashVN lashVN force-pushed the reorganize-message-packets branch 2 times, most recently from 9122b33 to 8aeba58 Compare July 21, 2024 16:02
In AHRS Mode, split the single message to an IMU packet and an AHRS packet; in EKF Mode, split the two messages into an IMU message, an EKF packet, and a GNSS packet.
Simplify message header definition to consolidate and eliminate the need for static asserts
Update healthy logic and use to represent new packet structure
Replace EAH3 message with messages per-packet
Add Ypr as configured output in the EKF message
Redefine messages to 3 INS packets, and send as appropriate
Resolve SITL failures due to too-long logger message headers
Remove unnecessary header
Switch parameters to default initialization
Change pointer casting to prevent a const_cast, and remove doubled sync byte when echoing ASCII messages
Fix Valgrind report by preventing use of nmea_printf on buffers which may not be null-terminated
Resolve SITL failures due to multiply defined logger message by pulling the call to a single method
@lashVN lashVN force-pushed the reorganize-message-packets branch from 8aeba58 to d27859b Compare July 21, 2024 16:40
@tridge tridge merged commit d7ecf5f into ArduPilot:master Jul 29, 2024
93 checks passed
@lashVN lashVN deleted the reorganize-message-packets branch July 29, 2024 19:02
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.

3 participants