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

update to imu docs #42

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

update to imu docs #42

wants to merge 3 commits into from

Conversation

mbz4
Copy link

@mbz4 mbz4 commented Dec 2, 2022

Expanded configuration to include manual workaround to IMU misaddressing & calibration bug.
Adjusted structure to improve overall information flow.

Expanded configuration to include manual workaround to IMU misaddressing & calibration bug.
Fixed typos and adjusted structure to improve overall information flow.
Copy link
Contributor

@Ezward Ezward left a 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`
Copy link
Contributor

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.

Copy link
Author

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

  1. the imu docs should instead reflect setting addr=0x68 to addr=IMU_ADDRESS on line 29
    ... in addition to commenting out the calibrateMPU6500() script
    ... & setting address_mpu_slave = 0x0c
    ... not sure where IMU_ADDRESS would be initially defined here

Or

  1. 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 (?)

Copy link
Author

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:

  1. donkeycar/templates/cfg_path_follow.py::414

&

  1. donkeycar/templates/cfg_complete.py::447

Appreciate the feedback

Copy link
Contributor

@Ezward Ezward Dec 6, 2022

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.

Copy link
Author

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

Copy link
Contributor

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.

@mbz4 mbz4 marked this pull request as draft December 12, 2022 14:11
@mbz4 mbz4 marked this pull request as ready for review December 12, 2022 14:33
@mbz4 mbz4 requested a review from Ezward December 12, 2022 14:33
Copy link
Contributor

@Ezward Ezward left a 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`
Copy link
Contributor

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.

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.

2 participants