Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Ti comm dev #17

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from
Draft

Ti comm dev #17

wants to merge 60 commits into from

Conversation

ihagedo
Copy link

@ihagedo ihagedo commented Feb 27, 2021

What is a quick description of the change?

Merging finished serial comm modules into the master.

Is this fixing an issue?

#21

Were any issues created as a result of this change?

None.

Are there more details that are relevant?

Fixed library dependency issues and created a better modularized code for debugging.

Future fixes include packet depreciation over time for echo call-back functionality.

Check lists (check x in [ ] of list items)

  • Test written/updating
  • Tests passing
  • Coding style (indentation, etc)

Any additional comments?

Copy link
Member

@zghera zghera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After trying the install, I ran across a couple of things that we should probably chat offline about before I do any more reviews. While the submodules init seemed to work fine, the autogen.sh did not. I also ran catkinmake to see what would happen and ended up getting some import errors for libserialport. I looked around and also saw that the CMakeLists.txt config does not look updated.

Updated the instructions after our conversation. While the install of libserialport seemed to work fine, there are still two issues that we know of

  1. When changing the references of libserialport in CMakiLists.txt to the new folder, there are now errors that result when catkinmake-ing due to warnings in some of the included files.
  2. The ti_comm node does not show up in the node graph.

Isaac and Zach are currently looking into issues 1 and 2, respectively.

@@ -0,0 +1 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this empty file?

README.md Outdated
Comment on lines 21 to 42
At the top of each branch your want to build the library for you should run the following command and then refer to the readme in src/ti_comm/src
```
git submodule update --init --recursive
```
Copy link
Member

@zghera zghera Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
At the top of each branch your want to build the library for you should run the following command and then refer to the readme in src/ti_comm/src
```
git submodule update --init --recursive
```
The serial communication node relies on a serial communication library that needs to be built each time you check out a new branch (including master when initially cloning the repo). To properly build the library, run the following commands from the top of the repo directory:
1. `git submodule update --init --recursive`
2. `cd src/ti_comm/src/libserialport`
3. `./autogen.sh`
4. `sudo apt-get install autoconf`
5. `./configure`
6. `make`
7. `sudo make install`
8. Add it to your LD_LIBRARY_PATH with `export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/local/lib"`
I would encourage you to make a bash alias/function so you don't have to run all of these commands on the creation of every new branch.

@@ -0,0 +1,10 @@
How to Build Libserialport
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file as a whole if you agree with the suggestion to move its contents t the top-level README

@zghera
Copy link
Member

zghera commented Feb 28, 2021

After trying the install, I ran across a couple of things that we should probably chat offline about before I do any more reviews. While the submodules init seemed to work fine, the autogen.sh did not. I also ran catkinmake to see what would happen and ended up getting some import errors for libserialport. I looked around and also saw that the CMakeLists.txt config does not look updated.

Updated the instructions after our conversation. While the install of libserialport seemed to work fine, there are still two issues that we know of

  1. When changing the references of libserialport in CMakiLists.txt to the new folder, there are now errors that result when catkinmake-ing due to warnings in some of the included files.
  2. The ti_comm node does not show up in the node graph.

Isaac and Zach are currently looking into issues 1 and 2, respectively.

Update on issue 2 here: When reverting to the old CMakeLists.txt file (build the liberialport.0.1.0 lib not the new one), the ti_comm ROS node did not show up because of a memory error when running the node (see below). This is possible due to the imports in amp_serial_jetson.cpp not using the correct libserialport imports or something similar. So it is evident that issue 1 listed above (warnings when changing CMakeLists.txt to correct libserialport files) must be solved before anything else.

...
process[ti_comm_node-5]: started with pid [59575]
Error: Failed: OS error code: 2, message: 'No such file or directory'
*** Error in `/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node': free(): invalid pointer: 0x00000000006304c0 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777f5)[0x7f1760cda7f5]
/lib/x86_64-linux-gnu/libc.so.6(process[map_server-6]: started with pid [59582]
+0x8038a)[0x7f1760ce338a]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f1760ce758c]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(sp_free_config+0x6b)[0x422b8b]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(_Z11end_program14amp_err_code_t+0x15)[0x41758d]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(_Z5check9sp_return14amp_err_code_t+0x7f)[0x417631]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(_Z28amp_serial_jetson_initializeP7sp_port+0x4c)[0x416dfc]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(main+0x91)[0x416901]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f1760c83840]
/home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node(_start+0x29)[0x416639]
======= Memory map: ========
00400000-00430000 r-xp 00000000 08:01 1179153                            /home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node
0062f000-00630000 r--p 0002f000 08:01 1179153                            /home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node
00630000-00631000 rw-p 00030000 08:01 1179153                            /home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node
00bf7000-00c29000 rw-p 00000000 00:00 0                                  [heap]
7f1754000000-7f1754021000 rw-p 00000000 00:00 0 
... lots more stuff ...
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
process[rviz-8]: started with pid [59602]
[ INFO] [1614548600.821921476]: Loading map from image "/home/zghera/Documents/AMP-v1/src/kart_2dnav/maps/autocross.png"
[ INFO] [1614548601.878863044]: Requesting the map...
[ WARN] [1614548601.891817468]: Request for map failed; trying again...
[ti_comm_node-5] process has died [pid 59575, exit code -6, cmd /home/zghera/Documents/AMP-v1/devel/lib/ti_comm/ti_comm_node __name:=ti_comm_node __log:=/home/zghera/.ros/log/f798fab4-7a0d-11eb-a03a-000c29203ca9/ti_comm_node-5.log].
log file: /home/zghera/.ros/log/f798fab4-7a0d-11eb-a03a-000c29203ca9/ti_comm_node-5*.log
...

@zghera zghera marked this pull request as draft January 31, 2022 17:02
ihagedo and others added 5 commits February 20, 2022 13:55
This fixes the issue with memcpy call on control packet by changin control packet v_speed and v_angle from int to char in amp_serial_jetson.h. Previously this caused an issue where serial packet looked in msg[0] and msg[1] bytes to get the values of speed and angle, but the control packet was an 8 byte struct of 2 integers which caused msg[0] and msg[1] to be the values of the first int in the struct.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate MCU Comm Node
3 participants