robotology / event-driven

neuromorphic sensor integration with YARP and iCub
BSD 3-Clause "New" or "Revised" License
78 stars 31 forks source link

issues with event-driven tutorials #29

Closed arrenglover closed 5 years ago

arrenglover commented 5 years ago

Please leave feedback on tutorials here:

  1. mistakes
  2. missing steps
  3. missing tutorials
  4. changing a method to an improved version
lsrosa commented 5 years ago
  1. ~Add a source ~/.bashrc after exporting INSTALL_DIR, YARP_DATA_DIRS, and PATH.~

  2. ~Change make install -j4 to make install -j$(nproc) to get the machine number of cores automatically.~

  3. ~Installing yarp is set to use the build YCM instead of its installation. On the same fashion, installing event-driven uses the build YCM and build yarp. Once the user deletes the source and built folders (imagining an user instead of a developer), yarp and event-driven will probably stop working. The usual thing to do is to point to the installation folder. Change the cmake variables as: -DYCM_DIR=\~/projects/YCM/build → -DYCM_DIR=$INSTALL_DIR -DYARP_DIR=\~/projects/yarp/build → -DYARP_DIR=$INSTALL_DIR event-driven tests with dataset and hardware+camera work fine with the above change.~

aikolina commented 5 years ago

~Personally, I find a bit difficult reading the README of the documentation because of the many new lines. I understand the intention of highlighting the links to the tutorials, but the line breaking looks un-natural and the links are already sufficiently visible thanks to the different text colouring.~

I would prefer something like this, instead:

Hardware

aikolina commented 5 years ago

There is a styling issue in line 27 of 1viewer.md.

Instead of using blockquotes:

yarp namespace /

It would be better to use code blocks:

yarp namespace /<yourname>

In this way, code is nicely highlighted (not YARP related keywords, tough) and special symbols should not disappear due to markdown styling.

lsrosa commented 5 years ago

~On the Testing your install with the viewer tutorial 1viewer.md.~

~The first commands seem to be for both testing with the dataset and camera:~

download the sample dataset from here and unpack to a location of your choosing. set the yarp namespace yarp namespace / run a yarpserver yarpserver --write

~These commands could be moved to the section Using actual hardware/camera? Skip ahead. Using a dataset? Follow these instructions:~

aikolina commented 5 years ago

For easy changes (e.g. adding link, typos, etc.), I would suggest to make a new branch with the suggested changes and see the difference with respect to cmake_update branch by making a PR. In this way, as long as someone else checks the PR, half of the work is already done!

I tried to do so for 1viewer.md, see PR #30.

What do you think about it @arrenglover?

lsrosa commented 5 years ago

On Writing an example module for event-driven tutorial example_module.md

While personalizing the project, commands to change the name of the example-module.ini and app_example-module.xml are missing: mv example-module.ini <my-module-name>.ini mv app_example-module.xml app_<my-module-name>.xml

arrenglover commented 5 years ago

For easy changes (e.g. adding link, typos, etc.), I would suggest to make a new branch with the suggested changes and see the difference with respect to cmake_update branch by making a PR. In this way, as long as someone else checks the PR, half of the work is already done!

I tried to do so for 1viewer.md, see PR #30.

What do you think about it @arrenglover?

It's a good idea. feel free to put all changes into a PR. I would like to perform the update to devel tomorrow.

aikolina commented 5 years ago

While personalizing the project, commands to change the name of the example-module.ini and app_example-module.xml are missing: mv example-module.ini <my-module-name>.ini mv app_example-module.xml app_<my-module-name>.xml

@lsrosa I think I applied the changes you suggested in PR #30, could you please double check?

lsrosa commented 5 years ago

While personalizing the project, commands to change the name of the example-module.ini and app_example-module.xml are missing: mv example-module.ini <my-module-name>.ini mv app_example-module.xml app_<my-module-name>.xml

@lsrosa I think I applied the changes you suggested in PR #30, could you please double check?

@aikolina great. However, maybe we can change the three lines

mv <my-module-name>/example-module.cpp <my-module-name>/<my-module-name>.cpp mv example-module.ini <my-module-name>.ini mv app_example-module.xml app_<my-module-name>.xml

to rename '~s/example-module/<my-module-name>/g' *, if rename is not installed: sudo apt-get install rename

We can suggest to sed the lines on the CMakelists and .cpp files as well, but sed is dangerous (regex are not reversible) and I think that is good for the user/developer to spot where these definitions are in the code.

lsrosa commented 5 years ago

On the Writing an example module for event-driven tutorial

There are some definitions in the example-module that were not updated to , resulting in both names mixed in the yarpmanager. It follows the extra necessary modifications:

  1. Add line 24 to the list of lines to be changed on .cpp file

    On line 7, 19, and 124 change the class, constructor and declaration to . ctrl+o, ctrl+x

  2. Add to change the first line of file .ini name /example-modulename /<my-modulo-name>

  3. Add to change all entries of file app_.xml, here the sed command will be useful sed -n 's/example-module/<my-modulo-name>/g' <my-modulo-name>.xml

After this, the entries in the yarpmanager display the correct name (<my-module-name>)

aikolina commented 5 years ago

@lsrosa Can you make all the changes you suggested to the Writing an example module for event-driven tutorial on the PR I'm working on too? Thanks!

arrenglover commented 5 years ago

Thanks @aikolina and @lsrosa. I've pulled in your changes up to here. I made a change to the install process. ~Can someone check the new version works?~ Done