Skip to content

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.


<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.


<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