Imu improvements #35
Merged
Imu improvements #35
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Actually increment num_vals so we're using more than just the last measurement Add short delay before calibration loop to ensure IMU has started measurements Delay measurements by update_time rather than a magic number Optimize calibration loop by doing division after the loop finishes
Change ODR to 208Hz for both accelerometer and gyroscope Change default update_time to 5ms (close as possible to 208Hz) Change FS range of both sensors to maximum (not clipping is more important than fine resolution here)
Check WHO_AM_I register (should do something intelligent if it fails) Set RESET and BOOT bits to reset sensor during initialization
Function calls were overriding previously set values
Should really set the start time *after* waiting for the sensor to get going
|
Hold on, don't accept yet, just found a bug |
|
Fixed! |
ksiegall
requested changes
Jul 14, 2023
|
Another comment for a pretty big improvement to this code; now that we're providing a way to change the data rates, we probably don't just want to be assuming that the imu update time is 5ms (which is an approximation all on it's own). What you can do is change the update timer to use frequency instead of period (set after calibration) and then pass in the currently set ODR for the gyro, which would eliminate any drift that would have resulted from using the wrong frequency. That would probably be a good change to make before this gets merged in |
|
Good point, I can change the timer to use |
|
While talking about improvements to main.py. We could go to blink codes on
the LED for certain exceptions. That way if something happens when they are
not attached to the computer, like a wire coming off the rangefinder, there
is a chance they could fix it.
Frank
…On Sun, Jul 16, 2023 at 12:01 PM Dryw Wade ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In XRPLib/imu.py
<#35 (comment)>
:
> @@ -56,10 +59,24 @@ def __init__(self, scl_pin: int, sda_pin: int, addr):
self._power = True
self._power_a = 0x10
self._power_g = 0x10
- # ODR_XL=1 FS_XL=0
- self._setreg(LSM6DSO_CTRL1_XL, 0x10)
- # ODR_G=1 FS_125=1
- self._setreg(LSM6DSO_CTRL2_G, 0x12)
+
+ # Check WHO_AM_I register to verify IMU is connected
+ foo = self._getreg(LSM6DSO_WHO_AM_I)
+ if foo != 0x6C:
+ # Getting here indicates sensor isn't connected
+ # TODO - do somehting intelligent here
All good points! I guess one other consideration we should make is how
frequently we expect these problems to occur. For example, I expect the IMU
to fail very rarely (it's soldered directly to the board and tested in
production), whereas something like the ultrasonic sensor is much more
likely to go wrong (users may forget to plug the sensor in, or wire it up
incorrectly, or there's an intermittent connection, or...).
As for how to indicate problems, I don't know if there's a "best" solution
here, each seems to have a downside. But after thinking about it some more,
I'm personally leaning towards throwing exceptions in most cases.
Exceptions explain the problem to the user in plain text, so there's no
ambiguity about what the problem is. If it's something that's easily fixed
(eg. plug in the ultrasonic cable), then the user can quickly fix it and
keep going. If it's something that's not easily fixed (eg. the IMU fails to
communicate), the user should really be informed about it. Plus, it's
pretty easy for the user to catch any exceptions that come up, and I think
it'd be good for students to learn about exceptions at some point.
Truthfully, I don't really have any strong opinions on the best way to
indicate problems. Each has their benefits and drawbacks, so I don't think
any of them are bad choices. I don't think I have anything more to say on
the subject, so I'll leave it up to you!
—
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKMHRGUPLLZPLQTLW4JZKTXQQF3XANCNFSM6AAAAAA2JQW25I>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Add new imu_defs.py with the following: Both possible I2C addresses Register address definitions Struct layout definitions with bit fields to match registers Dictionaries with possible ODR and FS values Note - to reduce file size, only registers that are used have been included. It should be easy to expand these as needed Add the following member functions: is_connected() reset() _set_bdu() _set_if_inc() Refactor imu.py to use these new features
Timer now uses frequency instead of period Frequency is now calculated when gyro_rate() is called Remove update_time parameter from calibrate() Add _start_timer() and stop_timer()
|
I like the idea of blink codes in main.py! I'm cleaning up some of the IMU code right now. Do we really need Edit: I've removed it for now. If we want it back, I can revert the commit |
Remove unused member variables Add comments to __init__() Add constants for mg and mpds per least significant bit
It has some edge cases with the rate functions. We could handle them, but that's more effort than just removing power(). It's not really needed for the XRP, but users can still reduce power consumption by sending '0Hz' to the rate functions.
Bits and bytes have better naming parity between each other
Burst reading speeds up data transmission a lot, up to 3x faster than reading single bytes when reading lots of registers (eg. reading 12 bytes at 400kHz should be almost 500us faster) Also best to minimize the amount of time spent hogging the I2C bus in case something else needs it
Needs to be defined before calling gyro_rate()
Forgot to change from gyro to accel when copying, oops!
Need to index into bytearray rather than reassigning the variable
mg/LSB and dps/LSB are defined for the minimum range of each sensor, so need to scale when using different ranges
|
Ok, finally got around to testing this. Had to fix a couple mistakes, but should be all working now! |
ksiegall
requested changes
Jul 17, 2023
Also add _reset_member_variables() to reduce duplicate code in reset() and __init__()
|
We could also add to the curriculum an IMU chapter, so they learn about IMU
calibration. And understand it better. Seems like something we would want
to teach.
…On Mon, Jul 17, 2023 at 9:23 PM Dryw Wade ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In XRPLib/imu.py
<#35 (comment)>
:
> - avg_vals[1][2] = (avg_vals[1][2]*num_vals+cur_vals[1][2])/(num_vals+1)
- time.sleep(0.05)
+ avg_vals[1][0] += cur_vals[1][0]
+ avg_vals[1][1] += cur_vals[1][1]
+ avg_vals[1][2] += cur_vals[1][2]
+ # Increment counter and wait for next loop
+ num_vals += 1
+ time.sleep(update_time / 1000)
+
+ # Compute averages
+ avg_vals[0][0] /= num_vals
+ avg_vals[0][1] /= num_vals
+ avg_vals[0][2] /= num_vals
+ avg_vals[1][0] /= num_vals
+ avg_vals[1][1] /= num_vals
+ avg_vals[1][2] /= num_vals
avg_vals[0][vertical_axis] -= 1000 #in mg
My main concern is that many (most?) people won't even know there's a
calibration that occurs, and that the robot needs to be completely
motionless during it. I could totally foresee a lot of people moving the
robot around during that time for whatever reason (eg. carrying the robot
while turning it on, or positioning it on a line just after clicking the
run button in the IDE, or lightly bumping the robot, or...), then having
their robot not drive straight or not turn to the correct angles. That will
inevitably cause confusion, and will probably make some people think the
robot is just bad, or theirs is broken. I can't count how many times I've
seen hidden behavior like this trip up students.
I wouldn't require teachers relay that information (they've got enough to
do already), plus there's a lot more use cases for the XRP than just
classes. IMO it should be trivial for a user to discover that there is a
calibration routine, and that the robot needs to be motionless during it.
At a minimum, I think adding some prints statements before and after would
be good. I think it'd also help to have it documented in multiple places
(eg. the API documentation and the online curriculum). Or even better would
be storing the calibration values in a file (could add a button in the IDE
for this) so users don't have to worry about it all the time (assuming the
IMU doesn't need to be calibrated frequently, need to test that).
Whether the angles should be relative or absolute with respect to gravity,
this may actually depend on the AHRS algorithm. Most require the
accelerometer (list of algorithms here
<https://ahrs.readthedocs.io/en/latest/#new-in-version-0-3>), and I would
imagine those would bring the roll and pitch to zero if the accelerometer
measures (x,y,z)=(0,0,1). I suppose you could add offsets for the roll and
pitch if that's really desired. Idk if relative or absolute angles are
best, but I'd personally lean towards absolute.
Also with regards to the accelerometer calibration, one other
consideration is that the X and Y axis bounds should really be as close to
+/- 1g as possible when the board is tilted sideways. After playing with my
board, if the calibration is done on a non-level surface, the bounds can
end up being wrong (eg. +1.1 and -0.9). So probably best to mention that
the calibration should be done on a level surface, in addition to being
motionless.
—
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKMHRCJ7LXA23AMT5MZYY3XQXQSHANCNFSM6AAAAAA2JQW25I>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
ksiegall
approved these changes
Jul 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A handful of general improvements to the IMU class. Notable changes include:
num_valswas never incremented)