-
Notifications
You must be signed in to change notification settings - Fork 131
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
update to imu docs #42
base: master
Are you sure you want to change the base?
Conversation
Expanded configuration to include manual workaround to IMU misaddressing & calibration bug. Fixed typos and adjusted structure to improve overall information flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these changes; there are a lot of nice improvements. See my comment regarding changing the IMU address; I've made that much easier.
@@ -72,6 +69,28 @@ Valid settings are from 0 to 6: | |||
- `5` 10Hz | |||
- `6` 5Hz | |||
|
|||
<br> | |||
|
|||
2. In `/home/pi/projects/donkeycar/donkeycar/parts/` find `imu.py` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've actually landed a pull requests that exposes a configuration so there is not need to edit this manually; just change the configuration value; See autorope/donkeycar#1064. The configuraton changes here https://github.com/autorope/donkeycar/pull/1064/files#diff-dd7806493c6284e60142951760a6712d8b3f90a8e007e710d4033691f74753edR447. So it would be excellent if you could incorporate instructions for changing that configuration here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to understand what's happening at the provided recent merge.
==> configurable imu addressing (?); derives from imu part file w/ new (?) IMU_ADDRESS config (?)
Thus, and please do correct me, @Ezward
- the imu docs should instead reflect setting
addr=0x68
toaddr=IMU_ADDRESS
on line29
... in addition to commenting out thecalibrateMPU6500()
script
... & settingaddress_mpu_slave = 0x0c
... not sure whereIMU_ADDRESS
would be initially defined here
Or
- are you suggested the imu docs should additionally be extended to contain info on how to modify:
IMU_ADDRESS = 0x68 # if AD0 pin is pulled high them address is 0x69, otherwise it is 0x68
...which can be found at:donkeycar/templates/cfg_path_follow.py
(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think option 2 seems more likely.
So, I should include instructions how to configure IMU_ADDRESS
in:
donkeycar/templates/cfg_path_follow.py::414
&
donkeycar/templates/cfg_complete.py::447
Appreciate the feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am suggesting 2. We no longer need to edit any source code to change the address; so that should be removed. The user just needs to set the value of "IMU_ADDRESS" in their myconfig.py file; so that should added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I studied the IMU_ADDRESS commit further to try and understand why source code changes to imu.py are no longer necessary and I did not reach the same conclusion; I left the source code edits in case configurations differ among setups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/autorope/donkeycar/blob/f8dc720596d04c3cb14ff0a34d771ef39baf1897/donkeycar/templates/complete.py#L263 https://github.com/autorope/donkeycar/blob/f8dc720596d04c3cb14ff0a34d771ef39baf1897/donkeycar/templates/complete.py#L805 The imu part has be updated to take in the necessary parameters and complete.py has been updated to use them.
I also see that the instruction include setting the address_mpu_slave=0x0c
; can you explain? My understanding what that the slave setting is if you have two IMU's and one is slaved to the other.
I also see that the instructions include commenting out the sensor calibration; can you explain? The calibrateMPU6500 is still appropriate even if using an MPU9250; the MPU9250 is basically just an MPU6500 plus an magnetometer; we we still want to calibrate the giro/accelerometers so we know which way is down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment; I think we still need to hash out the manual changes.
@@ -72,6 +69,28 @@ Valid settings are from 0 to 6: | |||
- `5` 10Hz | |||
- `6` 5Hz | |||
|
|||
<br> | |||
|
|||
2. In `/home/pi/projects/donkeycar/donkeycar/parts/` find `imu.py` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/autorope/donkeycar/blob/f8dc720596d04c3cb14ff0a34d771ef39baf1897/donkeycar/templates/complete.py#L263 https://github.com/autorope/donkeycar/blob/f8dc720596d04c3cb14ff0a34d771ef39baf1897/donkeycar/templates/complete.py#L805 The imu part has be updated to take in the necessary parameters and complete.py has been updated to use them.
I also see that the instruction include setting the address_mpu_slave=0x0c
; can you explain? My understanding what that the slave setting is if you have two IMU's and one is slaved to the other.
I also see that the instructions include commenting out the sensor calibration; can you explain? The calibrateMPU6500 is still appropriate even if using an MPU9250; the MPU9250 is basically just an MPU6500 plus an magnetometer; we we still want to calibrate the giro/accelerometers so we know which way is down.
Expanded configuration to include manual workaround to IMU misaddressing & calibration bug.
Adjusted structure to improve overall information flow.