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

Support LSM6DSV IMU #308

Closed
wants to merge 65 commits into from
Closed

Support LSM6DSV IMU #308

wants to merge 65 commits into from

Conversation

wigwagwent
Copy link
Contributor

This adds support for the lsm6dsv family of IMUs. They are better than bmi160 and seems comparable to the bno but need more users testing them.

This merge should probably be squashed into 1 commit.

@@ -35,6 +35,7 @@
#define IMU_BMI160 8
#define IMU_ICM20948 9
#define IMU_ICM42688 10
#define IMU_LSM6DSV 12
Copy link
Member

Choose a reason for hiding this comment

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

This should be noted when merging, depending on the order of merging PRs this might be awkward to skip a number.

@@ -34,8 +34,15 @@ The following IMUs and their corresponding `IMU` values are supported by the fir
* ICM-20948 (IMU_ICM20948)
* Using fusion in internal DMP for 6Dof or 9DoF, 9DoF mode requires good magnetic environment.
* Comment out `USE_6DOF` in `debug.h` for 9DoF mode.
* Experimental support!\
Copy link
Member

Choose a reason for hiding this comment

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

Something broke here, what's that \?

@@ -0,0 +1,4080 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Please include a link to the original repository.

Comment on lines +113 to +117
{
uint8_t intPin = extraParam;
sensor = new LSM6DSVSensor(sensorID, imuType, address, rotation, sclPin, sdaPin, intPin);
}
break;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
uint8_t intPin = extraParam;
sensor = new LSM6DSVSensor(sensorID, imuType, address, rotation, sclPin, sdaPin, intPin);
}
break;
sensor = new LSM6DSVSensor(sensorID, imuType, address, rotation, sclPin, sdaPin, extraParam);
break;

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest against this change, as the original change follow the pattern of how other cases in this switch statement has done this

image

@@ -72,7 +72,7 @@ class Quat {
Vector3 get_euler_yxz() const;

void set_euler(const Vector3& p_euler) { set_euler_yxz(p_euler); };
Vector3 get_euler() const { return get_euler_yxz(); };
Vector3 get_euler() const { return get_euler_xyz(); };
Copy link
Member

Choose a reason for hiding this comment

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

Instead of modifying get_euler() you should rather use the get_euler_xyz() in your code.

@@ -34,8 +34,15 @@ The following IMUs and their corresponding `IMU` values are supported by the fir
* ICM-20948 (IMU_ICM20948)
Copy link
Member

Choose a reason for hiding this comment

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

GitHub doesn't allow me to request changes without a comment...

Copy link
Member

@Eirenliel Eirenliel left a comment

Choose a reason for hiding this comment

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

Please address @TheDevMinerTV's comments

@unlogisch04
Copy link
Contributor

Is this still needed if we go for #322 ?

@gorbit99
Copy link

gorbit99 commented May 4, 2024

Is this still needed if we go for #322 ?

This probably shouldn't be preferred over sfusion, at least not without a cleanup.

@unlogisch04
Copy link
Contributor

I convert that to draft, as sfusion seems to be the better solution.
@wigwagwent feel free to close it when you see it the same way.

@unlogisch04 unlogisch04 marked this pull request as draft June 19, 2024 22:00
@wigwagwent wigwagwent closed this Jun 21, 2024
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.

None yet

9 participants