Ported the ROS2 fox drivers to ROS1 Noetic by VineetTambe · Pull Request #37 · f1tenth/vesc

VineetTambe · 2023-04-10T19:52:44Z

No description provided.

…sage to sensor messages IMU messages to beused by the madgwick fusion ros package

…ublished message

JWhitleyWork · 2023-04-11T15:52:09Z

It looks like the CHANGELOG.rst files and copyright/license headers were deleted. Please restore these.

hzheng40

Looks like a lot of files were MIT's old version. It'll be great if we could only included necessary changes.

vesc/package.xml

hzheng40 · 2023-04-11T15:57:11Z

vesc_ackermann/CMakeLists.txt

This looks like an older version too?

hzheng40 · 2023-04-11T16:15:32Z

Another comment is that in the ros2 branch, the imu msg is published by the vesc_driver instead of a separate wrapper. Is there any reason for this change?
Also, I would remove all the madgwick filter related code since that's not necessary for publishing the IMU msg.

VineetTambe · 2023-04-11T19:38:25Z

In the case of this noetic branch -
The vesc_driver still publishes the imu message as published by the original package (no change there).
The separate wrapper is to convert the vesc_imu_msg to the standard sensor_msgs::Imu structure in order for the madgwick filter to consume it.
I will remove those from the repo and keep them in a separate custom branch on my fork.

VineetTambe · 2023-04-11T19:45:48Z

Additionally, I wanted to ask can we update our names in the list of authors in the package.xml?

VineetTambe · 2023-04-11T21:07:17Z

I have reverted all the unnecessary changes to what was there originally. I am not sure what happened there. Please take a look and let me know if there are any more changes.

VineetTambe · 2023-04-20T19:57:34Z

Following up on this.
Could you let me know if the changes are acceptable??
Or if any other changes are necessary?

JWhitleyWork · 2023-07-31T19:31:50Z

vesc_ackermann/package.xml

+  <exec_depend>geometry_msgs</exec_depend>
+  <exec_depend>tf</exec_depend>
+  <exec_depend>ackermann_msgs</exec_depend>
+  <exec_depend>vesc_msgs</exec_depend>


Why did you split all of these out? <depend> is perfectly valid for Noetic.

Will fix and push the updates!

JWhitleyWork · 2023-07-31T19:35:14Z

vesc_driver/launch/vesc_driver_node.launch

-  <arg unless="$(arg debug)" name="launch_prefix" value="" />
+  <!-- <arg name="debug" default="false" /> -->
+  <!-- <arg if="$(arg debug)" name="launch_prefix" value="xterm -e gdb args" /> -->
+  <!-- <arg unless="$(arg debug)" name="launch_prefix" value="" /> -->


Remove unused code. git will keep the history and commenting out unused sections just leads to confusion.

JWhitleyWork · 2023-07-31T19:35:26Z

vesc_driver/package.xml

+  <exec_depend>roscpp</exec_depend>
+  <exec_depend>std_msgs</exec_depend>
+  <exec_depend>vesc_msgs</exec_depend>
+  <exec_depend>serial</exec_depend>


Again, why split these out?

JWhitleyWork · 2023-07-31T19:35:54Z

vesc_driver/src/vesc_driver.cpp

  // create vesc state (telemetry) publisher
  state_pub_ = nh.advertise<vesc_msgs::VescStateStamped>("sensors/core", 10);
+  imu_pub_ = nh.advertise<vesc_msgs::VescImuStamped>("sensors/imu", 10);
+  // imu_std_pub_ = nh.advertise<vesc_msgs::VescImuStamped>("sensors/imu/raw", 10);


Remove commented-out code.

JWhitleyWork · 2023-07-31T19:36:11Z

vesc_driver/src/vesc_driver.cpp

+    // std_imu_msg.orientation.w = imuData->q_w();
+    // std_imu_msg.orientation.x = imuData->q_x();
+    // std_imu_msg.orientation.y = imuData->q_y();
+    // std_imu_msg.orientation.z = imuData->q_z();


Remove commented-out code.

JWhitleyWork · 2023-07-31T19:37:14Z

vesc_driver/src/vesc_driver_node.cpp

-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
-// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
-// WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-


Please don't remove copyrights.

Will revert it back. I am not sure why that was removed.
Thanks for pointing that out!

JWhitleyWork · 2023-07-31T19:37:46Z

vesc_driver/src/vesc_packet.cpp

+//   int16_t v = static_cast<int16_t>((static_cast<uint16_t>(*(payload_.first + 3)) << 8) +
+//                                    static_cast<uint16_t>(*(payload_.first + 4)));
+//   return static_cast<double>(v) / 10.0;
+// }


Remove commented-out code.

JWhitleyWork · 2023-07-31T19:38:02Z

vesc_driver/src/vesc_packet.cpp

+//   if (mask_ & ((uint32_t)1 << 13)) {q1_ = getFloat32Auto(&ind);}
+//   if (mask_ & ((uint32_t)1 << 14)) {q2_ = getFloat32Auto(&ind);}
+//   if (mask_ & ((uint32_t)1 << 15)) {q3_ = getFloat32Auto(&ind);}
+// }


Remove commented-out code.

Will do a code cleanup and push the updates!

JWhitleyWork · 2023-07-31T19:38:29Z

vesc_msgs/package.xml


+  <exec_depend>std_msgs</exec_depend>
  <exec_depend>message_runtime</exec_depend>
+  <exec_depend>geometry_msgs</exec_depend>


No need to split out.

JWhitleyWork · 2023-07-31T19:40:09Z

vesc_ackermann/src/ackermann_to_vesc.cpp

-  if (!getRequiredParam(nh, "steering_angle_to_servo_gain", &steering_to_servo_gain_))
-    return;
-  if (!getRequiredParam(nh, "steering_angle_to_servo_offset", &steering_to_servo_offset_))
+  if (!getRequiredParam(nh, "steering_angle_to_servo_offset", steering_to_servo_offset_))


Several of these changes are breaking changes (removing existing parameters and renaming others). This means a new major version would have to be released. Not that I won't do it, just giving you a heads-up.

I will take a closer look and run some fixes for the noetic branch!
Thanks for the feedback! Very helpful to have another set of eyes on the code!

VineetTambe · 2023-08-23T17:15:25Z

Update:
I have resolved all the concerns highlighted and the Branch should be good to go.
However, instead of merging into "'main" I propose creating a new "noetic" branch.
This should prevent any breaking changes as highlighted here from causing any problems to others!

I am not sure how to go about creating a new branch in the base repo - could you help me out with that by creating a new branch named noetic from main and pushing it upsteam - so that I can edit the PR to merge into the new noetic branch?

VineetTambe and others added 8 commits April 9, 2023 18:47

[IMU]: added madgwick imu filter

f075df8

added car.launch

dee9a72

[Madgwick Fusion]: Added a new ros packages that convers VESC IMU mes…

fc021e2

…sage to sensor messages IMU messages to beused by the madgwick fusion ros package

[IMU Filter]: added remap params to launch file | added frame_id to p…

c2ac089

…ublished message

Update README.md

5d140e7

[IMU parser] updated frame ids for transforms

1817c25

Update README.md

c04814e

removed car.launch, which was used for testing purposes

e13d321

hzheng40 requested changes Apr 11, 2023

View reviewed changes

VineetTambe added 2 commits April 11, 2023 15:44

removed vesc_imu_msg_parser package and dependency on imu_tools

b2ae047

removed imu_tools from package.xml

ed42c65

VineetTambe added 6 commits April 11, 2023 16:35

restored package.xml to the version 1.1.0

5f37bb2

restored CHANGELOG.rst

42ae666

reverted ackerman msgs CmakeList

6f1ea6b

restored ros_lint build dependency for vesc_ackermann

de3c916

autoformatted code

c78c44f

restored liscences to all files

bf6e4d0

JWhitleyWork suggested changes Jul 31, 2023

View reviewed changes

VineetTambe added 4 commits August 23, 2023 13:06

revered exec_depend to depend

92bb299

reverted license change

0d0bf86

removed commented out / unused code

424ef1d

reverted madgwick filter for imu

6f22df8

Comments

Conversation

VineetTambe commented Apr 10, 2023

Uh oh!

JWhitleyWork commented Apr 11, 2023

Uh oh!

hzheng40 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzheng40 commented Apr 11, 2023

Uh oh!

VineetTambe commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VineetTambe commented Apr 11, 2023

Uh oh!

VineetTambe commented Apr 11, 2023

Uh oh!

VineetTambe commented Apr 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VineetTambe commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VineetTambe commented Apr 11, 2023 •

edited

Loading

VineetTambe commented Aug 23, 2023 •

edited

Loading