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

[WIP] Pr compasscal offdiag #9136

Closed
wants to merge 9 commits into from
Closed

[WIP] Pr compasscal offdiag #9136

wants to merge 9 commits into from

Conversation

dagar
Copy link
Member

@dagar dagar commented Mar 22, 2018

Opening this so we don't lose track of it.

/* flip axes and negate value for y */
new_report.y = ((yraw_f * _range_scale) - _scale.y_offset) * _scale.y_scale;
new_report.y += ((xraw_f * _range_scale) - _scale.x_offset) * _scale.x_offdiag;
new_report.z += ((zraw_f * _range_scale) - _scale.z_offset) * _scale.z_offdiag;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be new_report.y += ((zraw_f * _range_scale) - _scale.z_offset) * _scale.z_offdiag;

@dagar
Copy link
Member Author

dagar commented Mar 30, 2018

Thanks @AlexisTM. I left this open as a reminder to review the entire magnetometer calibration.

@mhkabir
Copy link
Member

mhkabir commented Apr 15, 2018

Is this PR in a workable state? I have an application which needs compensation for soft iron distortions, and was wondering if it is safe to carefully start using this.

@dagar
Copy link
Member Author

dagar commented Apr 15, 2018

Is this PR in a workable state?

I doubt it, but we could use this as a starting point.

@mhkabir
Copy link
Member

mhkabir commented Apr 16, 2018

OK, I updated this locally and fixed a few things. Should be in a working state now, but I haven't tested yet.

I also want to switch the application of the calibration into vector/matrix math, something like :

matrix::Vector3f mag(xraw_f * _range_scale,
		yraw_f * _range_scale,
		zraw_f * _range_scale);
// apply offsets
matrix Vector3f offsets(_scale.x_offset,
			_scale.y_offset,
			_scale.z_offset);

mag = mag - offsets;

// apply scale calibration
matrix::Matrix3f scales(_scale.x_scale,	_scale.x_offdiag, _scale.y_offdiag,
        		_scale.x_offdiag, _scale.y_scale, _scale.z_offdiag,
        		_scale.y_offdiag, _scale.z_offdiag, _scale.z_scale);

mag = scales * mag;

versus currently :

mag_report.x = ((xraw_f * _mag_range_scale) - _mag_scale.x_offset) * _mag_scale.x_scale;
mag_report.x += ((yraw_f * _mag_range_scale) - _mag_scale.y_offset) * _mag_scale.x_offdiag;
mag_report.x += ((zraw_f * _mag_range_scale) - _mag_scale.z_offset) * _mag_scale.y_offdiag;
       
mag_report.y = ((yraw_f * _mag_range_scale) - _mag_scale.y_offset) * _mag_scale.y_scale;
mag_report.y += ((xraw_f * _mag_range_scale) - _mag_scale.x_offset) * _mag_scale.x_offdiag;
mag_report.y += ((zraw_f * _mag_range_scale) - _mag_scale.z_offset) * _mag_scale.z_offdiag;
       
mag_report.z = ((zraw_f * _mag_range_scale) - _mag_scale.z_offset) * _mag_scale.z_scale;
mag_report.z += ((xraw_f * _mag_range_scale) - _mag_scale.x_offset) * _mag_scale.y_offdiag;
mag_report.z += ((yraw_f * _mag_range_scale) - _mag_scale.y_offset) * _mag_scale.z_offdiag;

What do you think, @dagar @AlexisTM.

@mhkabir
Copy link
Member

mhkabir commented Apr 16, 2018

PR updated and rebased.

General TODOs for sensor calibration pipeline :

  • Some new drivers may need changes for the off-diagonal calibration application
  • Horrible boilerplate for applying calibrations in every driver (accel/gyro/mag) should be moved into drv_<sensor>.h files.
  • Potentially move application of calibration to matrix/vector math
  • We should remove IOCTLs from sensor drivers and replace with uORB "service calls" or similar.

@dagar dagar removed this from the Release v1.8.0 milestone May 4, 2018
@mhkabir
Copy link
Member

mhkabir commented May 7, 2018

@dagar I flew this a few times. Seems fine. Also gets rid of MAG SENSORS INCONSISTENT on a particular vehicle for me. Without the soft iron compensation, my GPS compass and the onboard compasses had some offsets between then.

@@ -1656,8 +1659,17 @@ LSM303D::mag_measure()
rotate_3f(_rotation, xraw_f, yraw_f, zraw_f);

mag_report.x = ((xraw_f * _mag_range_scale) - _mag_scale.x_offset) * _mag_scale.x_scale;
mag_report.x += ((yraw_f * _mag_range_scale) - _mag_scale.y_offset) * _mag_scale.x_offdiag;
Copy link
Member Author

Choose a reason for hiding this comment

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

This ends up being a lot of verbose duplicated code. Why don't we just start using Matrix?

@dagar
Copy link
Member Author

dagar commented May 7, 2018

Does anyone know the history here? I'm wondering why it wasn't already done.

@dagar dagar requested a review from priseborough May 7, 2018 00:16
@dagar
Copy link
Member Author

dagar commented May 7, 2018

@priseborough could you review this as well? Any specific suggestions for testing/validation?

@@ -56,10 +56,13 @@
struct mag_calibration_s {
Copy link
Member Author

@dagar dagar May 7, 2018

Choose a reason for hiding this comment

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

I believe we could add member initializers here instead of doing it manually in every driver's constructor (if it's no longer used in any C code).

@dagar dagar added this to the Release v2.0.0 milestone Jul 7, 2018
@stale
Copy link

stale bot commented Jan 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Feb 4, 2019

Closing as stale.

@dagar
Copy link
Member Author

dagar commented Aug 21, 2020

Completed in #15235.

@dagar dagar deleted the pr-compasscal-offdiag branch August 21, 2020 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants