Detectcone cleanup #3

arbulgazar · 2018-10-15T18:32:31Z

Removes unnacisaritly files
Improves structure
Adds automatic downloads of cluon and eigen lib
Updates for the use of cluon-v114

* Removes unused cluons * Removes static dependancy on Eigen, download fresh from git * Moves header files to include dir

Updates to latest cluon-v114

chrberger

The file https://github.com/chalmersfsd/cfsd18-perception-detectcone/blob/detectcone_cleanup/docker-compose.yml contains:

a specific user local path
the use of libcluon's POSIX shared memory implementation (which is faster than libcluon's SystemV IPC's shared memory implementation); the consequence, though, is that all microservices that belong to a shared memory processing chain (i.e., producers, consumers) must be using the same Docker base image (i.e., all must use Alpine Linux or all must use Ubuntu); mixing different Docker base images result in a not working processing chain
the file model is empty
the README.md file must be updated to contain information how the service is supposed to be used
does Travis still complete successfully?
there is no valid test case: https://github.com/chalmersfsd/cfsd18-perception-detectcone/blob/detectcone_cleanup/test/tests-cfsd18-perception-detectcone.cpp

chrberger · 2018-10-15T18:37:15Z

CMakeLists.txt

@@ -55,19 +55,21 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)
 find_package(Threads REQUIRED)
 ################################################################################
 # Extract cluon-msc from cluon-complete.hpp.
+
+execute_process(COMMAND wget -P ${CLUON_PATH} https://raw.githubusercontent.com/chrberger/libcluon/gh-pages/headeronly/${CLUON_COMPLETE})


@arbulgazar Is this line executed once or on any build?

@chrberger yes, executed on every build, but if the file is already there it is not downloaded again. Should I make this check explicitly?

The issue might be that if you want to build when you only have the repository at hand and no Internet connection, this step might fail?

Regardless - will add an explicit check, will be cleaner.

If you run this outside of the docker, e.g. natively, it will clutter the repository.

chrberger · 2018-10-15T18:38:24Z

include/detectcone.hpp

-
-
-
+  const double PI = 3.14159265;


@arbulgazar We should use the constant provided in #include (M_PI)

What about DEG2RAD and RAD2DEG?

Should be adjusted in the same way (including constexpr to let the compile provide these constants even during compile time for further optimizations)

bjornborg

I think it would be nice with added

read me with pictures or gif showing the function
usecase showcasing the functionality with data
update the travis script

things to be fixed

Dockerfile

And before we merge the PR, we should also make a version release to highlight that this is latest from cfsd18 @ZiweiHuang94 .

CMakeLists.txt

include/detectcone.hpp

bjornborg · 2018-10-16T08:41:55Z

CMakeLists.txt

@@ -20,8 +20,8 @@ project(cfsd18-perception-detectcone)
 ################################################################################


You might want to change the header author to Cfsd instead of Christian :D @chrberger .

Yes, please :-)

CMakeLists.txt

bjornborg · 2018-10-16T08:47:28Z

CMakeLists.txt

@@ -55,19 +55,21 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)
 find_package(Threads REQUIRED)
 ################################################################################
 # Extract cluon-msc from cluon-complete.hpp.
+
+execute_process(COMMAND wget -P ${CLUON_PATH} https://raw.githubusercontent.com/chrberger/libcluon/gh-pages/headeronly/${CLUON_COMPLETE})


If you run this outside of the docker, e.g. natively, it will clutter the repository.

…fsd18-perception-detectcone into detectcone_cleanup

Changed the alpine version to an older version for Gcc6. Gcc8 is not compatible with tinydnn Restructured the repo The config file is not really needed, the settings can be parsed through cmake command

Tweaked the eigen include

…9.1MB)

Detectcone cleanup.patch

bjornborg · 2018-10-30T14:11:31Z

Is it possible to use alpine as deployment base image on arch @chrberger ?

bjornborg · 2018-10-30T14:12:53Z

Left to do:

Update travis script
Update version control
Introduce unit tests for the module

chrberger · 2018-10-30T14:19:09Z

@bjornborg "Is it possible to use Alpine?"

Yes, when the following constraints are met:
a) the producing microservice that is giving us the image via a shared memory is also Alpine
b) when the producing microservice is not Alpine but the environment variable CLUON_SHAREDMEMORY_POSIX is either not set or set to 0 (cf. here: https://github.com/chrberger/libcluon/blob/master/libcluon/src/SharedMemory.cpp#L59-L61).

Reason: When the producer is non-Alpine and the receiver (ie., this microservice) is Alpine, the C-libraries are incompatible and hence, are not allowing using the fast POSIX shared memory implementation but only the (slightly) slower SysV IPC implementation.

arbulgazar added 3 commits September 22, 2018 20:48

Repository refactoring and cleanup

688308f

* Removes unused cluons * Removes static dependancy on Eigen, download fresh from git * Moves header files to include dir

Adds automatic cluon get from server

e5ad740

Updates to latest cluon-v114

Removes static cluon header

a0ff580

arbulgazar requested review from chrberger and bjornborg October 15, 2018 18:32

chrberger requested changes Oct 15, 2018

View reviewed changes

bjornborg requested changes Oct 16, 2018

View reviewed changes

ruguangyou and others added 13 commits October 16, 2018 11:47

Add some comments in main function

a5091b5

Update README.md

5c8ffcd

Merge branch 'detectcone_cleanup' of https://github.com/chalmersfsd/c…

3e8b9c8

…fsd18-perception-detectcone into detectcone_cleanup

Update README.md, add od4 message input and output

1305f5f

Update README.md

0153e52

Update README.md (cone color labels)

0666785

Fix missing thirdparty dir

c1ae065

Cleaned up some temporary files

d191825

Changed the alpine version to an older version for Gcc6. Gcc8 is not compatible with tinydnn Restructured the repo The config file is not really needed, the settings can be parsed through cmake command

Removed the config file

01b5f3a

Cleaned up cmake module mess, turns out that we didn't need them

580d74d

Tweaked the eigen include

Disabled upx compression for binary

50c575e

slim down by only copying dependent libraries (image size: 393MB -> 7…

ca24691

…9.1MB)

Merge pull request #4 from chalmersfsd/detectcone_cleanup.patch

efbea17

Detectcone cleanup.patch

bjornborg assigned ruguangyou, arbulgazar, namv1994 and ZiweiHuang94 Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detectcone cleanup #3

Detectcone cleanup #3

arbulgazar commented Oct 15, 2018 •

edited

Loading

chrberger left a comment

chrberger Oct 15, 2018

arbulgazar Oct 15, 2018

chrberger Oct 15, 2018

arbulgazar Oct 15, 2018

bjornborg Oct 16, 2018

chrberger Oct 15, 2018

arbulgazar Oct 15, 2018

chrberger Oct 15, 2018

bjornborg left a comment •

edited

Loading

bjornborg Oct 16, 2018

chrberger Oct 17, 2018

bjornborg Oct 16, 2018

bjornborg commented Oct 30, 2018

bjornborg commented Oct 30, 2018 •

edited

Loading

chrberger commented Oct 30, 2018

		@@ -20,8 +20,8 @@ project(cfsd18-perception-detectcone)
		################################################################################

Detectcone cleanup #3

Are you sure you want to change the base?

Detectcone cleanup #3

Conversation

arbulgazar commented Oct 15, 2018 • edited Loading

chrberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornborg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornborg commented Oct 30, 2018

bjornborg commented Oct 30, 2018 • edited Loading

chrberger commented Oct 30, 2018

arbulgazar commented Oct 15, 2018 •

edited

Loading

bjornborg left a comment •

edited

Loading

bjornborg commented Oct 30, 2018 •

edited

Loading