#444: Add V5 IMU Wrapper Class by invalidflaw · Pull Request #452 · OkapiLib/OkapiLib

invalidflaw · 2020-08-03T04:01:51Z

Description of the Change

Adds a new wrapper class derived from ContinuousRotarySensor for the V5 IMU. The constructor accepts a V5 port number and an enum axis definition to measure. The z(yaw) axis is configured as default. All axes return an angle in the range of -180 to 180, much like the existing ADIgyro class does. To measure more axes, separate IMU instances should be made by the user.

As suggested in the issue, a value, "offset" is used to reset the sensor. The PROS API does not allow for an easy zero process, so the IMU::reset() command will change the offset parameter to match the current measured angle. In the event of an IMU::get() command occurs outside the range of -180 to 180, the offset is inverted by adding -360*(offset/sign(offset)) to the original offset.

Because the reset command does not calibrate the IMU, I added an IMU::calibrate() method. The constructor does not call this as the v5 port is automatically configured and is calibrated on brain startup. Any further calibration should be controlled by the user.

Because the IMU is an accelerometer as well as a gyro, I added an IMU::getAccl() command to retreive the acceleration value along the instanced axis.

Motivation

Adding a wrapper class for the IMU will allow it to be integrated into odometry and chassis models at a later date. Implementing the sensor within okapi allows users to program their robot using the same namespace.

Possible Drawbacks

The V5 IMU chooses one of 6 orientations based on the calibration process, then assigns the XYZ axes accordingly. I am unsure how/if this should be documented.

Verification Process

I placed the IMU on a robot chassis read the gyro data using the PROSC API and the new IMU class. Both classes returned the same euler angles.

To test the reset function, I set the IMU to measure robot yaw. I started the program, and rotated the chassis 90 degrees, after which I pressed a button on my controller which was set to run the reset function. After doing so, the robot continued to read -180 to 180 with the 0 now located at the original 90 degrees.

Applicable Issues

Closes #444

Octogonapus

This is approved pending CI passes and a kernel bug fix.

theol0403

Just some tiny nits. Otherwise looks good.

All axes return an angle in the range of -180 to 180

Imo I don't think this is nice to use. You have to deal with the input to your control algorithms jumping if you don't constrain the angle after you do any setpoint-reading calculations. The IMU can return an unconstrained range through a different API method, which might be nicer to use. Up to you two, it's not a huge deal either way.

theol0403 · 2020-08-09T06:57:05Z

+   */
+  IMU(std::uint8_t iport, IMUAxes iaxis = IMUAxes::z);
+
+  virtual ~IMU();


Explicitly defining a virtual destructor isn't needed as it inherits the virtual destructor from base classes.

ADIGyro probably shouldn't have one, either.

theol0403 · 2020-08-09T06:58:20Z

+   *
+   * @return The current sensor value or `PROS_ERR`.
+   */
+  double getAcceleration();


Can this function be const?

theol0403 · 2020-08-09T07:03:15Z

+    return PROS_ERR;
+  }
+
+  if (angle > 180 || angle < -180) {


This functionality matches the recently added okapilib helper functions to constrain an angle to 180 degrees. Perhaps use those instead?

invalidflaw · 2020-08-09T13:14:42Z

When I was developing this class, I settled on the [-180, 180] range for a couple of reasons.

First, the PROS API for the IMU is pretty limited. I don't believe there is a command which gives me an unbounded rotation for all 3 axes. In my testing, pros::c::get_yaw(), pros::c::get_pitch() and pros::c::get_roll() all simply return the euler angles, which I am using in this class.

The second reason I chose this range was to match the existing gyro better. I would need to test the ADIgyro again, but I believe its range is [-1800,1800]. In order to retain some compatibility, I wanted these to use the same general tracking behavior. It is possible to implement unbounded counting in software, but I believe we should then update ADIgyro as well to reflect this behavior change. Consistent behavior will be needed for my next project of adding a gyro to odometry.

theol0403 · 2020-08-10T00:47:21Z

Sounds good. Did you want to use

OkapiLib/include/okapi/api/odometry/odomMath.hpp

Line 63 in a9b3ead

static QAngle constrainAngle180(const QAngle &angle);

for the angle wrapping? I'm sure you could also move the function to mathUtil.hpp which puts the function in a centrally available place. Doing this has the added benefit of being able to remap values which require more than 360 degrees of correction.

Octogonapus · 2020-08-10T01:48:23Z

I agree with Theo, I think moving that function to mathUtil.hpp and then using it here would be a good idea.

invalidflaw · 2020-08-13T14:53:04Z

I'm a little confused by this. Is the proposal to make the IMU and ADIgyro class return unbounded rotation values, and then convert that using constrainAngle180?

Octogonapus · 2020-08-14T22:11:02Z

I'm a little confused by this. Is the proposal to make the IMU and ADIgyro class return unbounded rotation values, and then convert that using constrainAngle180?

I believe the proposal is to use constrainAngle180 here instead of copying that functionality here.

invalidflaw · 2020-08-16T22:59:05Z

In that case, the next step is to remove getRemapped from both IMU and ADIGyro classes. Correct?

Octogonapus · 2020-08-16T23:56:27Z

In that case, the next step is to remove getRemapped from both IMU and ADIGyro classes. Correct?

No. We can't break backward compatibility.

invalidflaw · 2020-08-17T00:37:28Z

I must first apologize. I think I am more confused than before. Looking at differences between ADIGyro and IMU, IMU get returns a bounded -180 to 180 degree angle value, while the ADIGyro returns unbounded. I see value in making these two get functions return the same values. As to the getRemapped issue, is the goal to replace the mathUtil remapRange function with constrainAngle180? My only concern for that is you lose the ability to specify the return range. Is that considered acceptable for most uses?

theol0403 · 2020-08-17T21:10:08Z

Sorry for any confusion, all I was saying was to replace

  if (angle > 180 || angle < -180) {
    angle += std::copysign(360, offset);
  }

with

angle = constrainAngle180(angle);

to remove code duplication.

invalidflaw · 2020-08-20T14:40:54Z

Ok, that makes sense. As to the lack of angle wrapping, should I spend a bit of time and make the IMU return unbounded by default? The existing ADIgyro returns unbounded. I'm a little worried that if these two don't match, our upcoming project to implement gyro based odometry may become difficult.

Octogonapus · 2020-08-22T21:33:10Z

Ok, that makes sense. As to the lack of angle wrapping, should I spend a bit of time and make the IMU return unbounded by default? The existing ADIgyro returns unbounded. I'm a little worried that if these two don't match, our upcoming project to implement gyro based odometry may become difficult.

I still am against supporting (by default) odometry incorporating the ADI gyro. I don't think it's worth you putting in the extra work here to match that functionality. We'd probably want to convert back to a bounded angle anyway.

invalidflaw · 2020-08-25T16:55:13Z

Implemented the suggestion from @theol0403 . I would like to give it a test, but if it works and there are no further concerns. I think this is ready for review.

theol0403 · 2020-08-25T22:08:49Z

Looks good. Ideally the constrainAngle180 method should be moved to mathUtil.hpp, so it can be in a common place (IMU should not depend on OdomMath). However, if @Octogonapus is happy, I'm happy.

Octogonapus · 2020-08-26T00:01:20Z

I am fine with leaving it in odomMath.

I pushed an update to fix the return value from calibrate in the event that it times out. We will need to test this function once a new kernel release is out that contains purduesigbots/pros#236. So I am essentially approving this; we just need a kernel release now.

invalidflaw · 2020-08-30T22:12:12Z

Just gave the latest code a test, works as expected.

# Conflicts: # include/okapi/impl/device/rotarysensor/adiGyro.hpp # src/impl/device/rotarysensor/adiGyro.cpp

Octogonapus · 2020-10-02T22:47:34Z

@invalidflaw or @kunwarsahni01 can one of you please test the IMU, especially calibration?

invalidflaw · 2020-10-02T22:51:45Z

I'll give it a try tomorrow morning.

invalidflaw · 2020-10-03T16:37:43Z

Just tested this morning. The IMU works as expected. Calibration and IMU disconnect are handled properly.

Octogonapus self-requested a review August 4, 2020 23:00

Octogonapus assigned invalidflaw Aug 4, 2020

Octogonapus added the feature A brand new feature label Aug 4, 2020

Octogonapus suggested changes Aug 4, 2020

View reviewed changes

Octogonapus previously approved these changes Aug 8, 2020

View reviewed changes

theol0403 previously approved these changes Aug 9, 2020

View reviewed changes

Octogonapus requested review from Octogonapus and theol0403 August 9, 2020 19:48

invalidflaw dismissed stale reviews from theol0403 and Octogonapus via c4d4111 August 25, 2020 16:53

Octogonapus mentioned this pull request Sep 13, 2020

Update kernel #454

Merged

invalidflaw and others added 4 commits October 2, 2020 18:31

Add V5 IMU Class and Axis Enum

36aa4a4

Update IMU Documentation

f445c3d

Create Private IMU read angle method

d621185

clang-format

7d2466e

Octogonapus and others added 6 commits October 2, 2020 18:31

Ensure calibration finishes or times out. Clean up docs.

043091c

Make the handling of PROS_ERR consistent with the other device classes

73bdfb9

Remove redudant destructor. Make acceleration function const.

96838cf

# Conflicts: # include/okapi/impl/device/rotarysensor/adiGyro.hpp # src/impl/device/rotarysensor/adiGyro.cpp

Replace Redundant Angle Wrapping Function

c623397

Fix return value from calibrate

dccfb97

Update IMU after rebase

c62fe1c

Octogonapus force-pushed the feature/ARR/#444_V5IMU branch from a8be2f8 to c62fe1c Compare October 2, 2020 22:43

Use consistent language

4d98d5d

Octogonapus merged commit 4f4df6d into OkapiLib:develop Oct 3, 2020

Conversation

invalidflaw commented Aug 3, 2020 • edited by Octogonapus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Motivation

Possible Drawbacks

Verification Process

Applicable Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Octogonapus left a comment

Choose a reason for hiding this comment

Uh oh!

theol0403 left a comment

Choose a reason for hiding this comment

Uh oh!

theol0403 Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

Octogonapus Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

theol0403 Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

theol0403 Aug 9, 2020

Choose a reason for hiding this comment

Uh oh!

invalidflaw commented Aug 9, 2020

Uh oh!

theol0403 commented Aug 10, 2020

Uh oh!

Octogonapus commented Aug 10, 2020

Uh oh!

invalidflaw commented Aug 13, 2020

Uh oh!

Octogonapus commented Aug 14, 2020

Uh oh!

invalidflaw commented Aug 16, 2020

Uh oh!

Octogonapus commented Aug 16, 2020

Uh oh!

invalidflaw commented Aug 17, 2020

Uh oh!

theol0403 commented Aug 17, 2020

Uh oh!

invalidflaw commented Aug 20, 2020

Uh oh!

Octogonapus commented Aug 22, 2020

Uh oh!

invalidflaw commented Aug 25, 2020

Uh oh!

theol0403 commented Aug 25, 2020

Uh oh!

Octogonapus commented Aug 26, 2020

Uh oh!

invalidflaw commented Aug 30, 2020

Uh oh!

Octogonapus commented Oct 2, 2020

Uh oh!

invalidflaw commented Oct 2, 2020

Uh oh!

invalidflaw commented Oct 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

invalidflaw commented Aug 3, 2020 •

edited by Octogonapus

Loading