Fix README.md so that it matches the blog at Tensorflow.org

tcal-x commented 4 years ago

People might just find this and start following the README instructions, and have a bad experience.

kgugala commented 4 years ago

@tcal-x can you point which part of the readme is obsolete? Which note do you mean? The TF blog one https://blog.tensorflow.org/2020/06/running-and-testing-tf-lite-on-microcontrollers.html? It has the same build and run instructions as README here in the repo.

tcal-x commented 4 years ago

Hi @kgugala ! The main issue is the switch to using virtualenv. The package is not listed in the prerequesites. And even after installing virtualenv, our intern found that he had add an explicit step regarding setting up the virtual environment.

His experience is listed here: https://docs.google.com/document/d/1-5bNNtFluOVwDGaotU9do_IA-ugx785VUGNtnCt0jCU/edit# -- it's private but I've added Karol and Peter. I'll copy the relevant part here:

(Optional) Error: no such file or directory When you got an error on the make command above with this error message, you might not encounter it if they fixed it, but sometimes if the script exited with error before, the environment will not be set up properly, but we can just create it at the same spot.

<some-path>/zephyr/venv-zephyr/bin/activate not found: no such file or directory

Just create a virtual environment for at the same directory that it point to

# `which python3` will expand the path to the executable that python3 
# invokes, it can be replaced by any other command like python or
# python3.6, etc, or you can specify the full path to the interpretervirtualenv 
<some-path>/zephyr/venv-zephyr --python=`which python3`

His approach might not be the correct one, but it's what he had to do to get it to work.

tcal-x commented 4 years ago

Also, README.md is missing the flterm instructions for connecting to the board and loading zephyr.bin. This was reported in issue #13.

pgielda commented 4 years ago

I would prefer that we fix README rather than remove it.

tcal-x commented 4 years ago

Hi Mateusz,

I ran through the updated demo instructions (https://github.com/antmicro/litex-vexriscv-tensorflow-lite-demo/tree/readme_fixes) a couple of times. These are my comments:

The added cmake update is good
The zephyr setup.run script needs sudo
We should instruct the users to follow zephyr's instructions to set these variables after installation:
- export ZEPHYR_TOOLCHAIN_VARIANT=zephyr
- export ZEPHYR_SDK_INSTALL_DIR=/opt/zephyr-sdk
The current virtualenv instructions are not quite right:
- you haven't cd'd into the tensorflow directory
- the requirements.txt file doesn't exist (yet)
- I think you need to fail in the tensorflow make first, then do those steps.

BUT, it's puzzling: now, if I DON'T do any virtualenv stuff, it works just fine. Maybe it fails if you don't set the ZEPHYR variables? That's the only thing I can think of, but I haven't tested this theory.

Before, I did see the "....zephyr/venv-zephyr/bin/activate not found: no such file or directory" error myself, and so did the intern, but now it seems to have disappeared for me. So it seems we don't need the "Create and configure a virtual environment for building demos:" block of instructions.

Did/do you see that error?

I've been running on new Ubuntu 18.04 virtual machines.

Oh, a couple more points about the demo improved README.md:

It still needs the lxterm/flterm command for loading the firmware onto Arty.
Maybe mention how to exit Renode nicely, by typing "quit" at the (machine-0) prompt

mateusz-holenko commented 4 years ago

Hi Mateusz,

I ran through the updated demo instructions (https://github.com/antmicro/litex-vexriscv-tensorflow-lite-demo/tree/readme_fixes) a couple of times. These are my comments:

The added cmake update is good

The zephyr setup.run script needs sudo

That's right - it's fixed on the branch now.

We should instruct the users to follow zephyr's instructions to set these variables after installation:

export ZEPHYR_TOOLCHAIN_VARIANT=zephyr

export ZEPHYR_SDK_INSTALL_DIR=/opt/zephyr-sdk

This can be automatically added as a part of SDK installation, but adding an explicit reminder is a good idea.

The current virtualenv instructions are not quite right:

you haven't cd'd into the tensorflow directory

the requirements.txt file doesn't exist (yet)

I think you need to fail in the tensorflow make first, then do those steps.

BUT, it's puzzling: now, if I DON'T do any virtualenv stuff, it works just fine. Maybe it fails if you don't set the ZEPHYR variables? That's the only thing I can think of, but I haven't tested this theory.

Before, I did see the "....zephyr/venv-zephyr/bin/activate not found: no such file or directory" error myself, and so did the intern, but now it seems to have disappeared for me. So it seems we don't need the "Create and configure a virtual environment for building demos:" block of instructions.

Did/do you see that error?

I've been running on new Ubuntu 18.04 virtual machines.

It looks like the virtualenv-related problems in TensorFlow were caused by the lack of virtualenv installed in the system when running the make command for the first time. With the current version of instructions I don't see the error anymore and I don't have to configure the virtual environment manually.

Oh, a couple more points about the demo improved README.md:

It still needs the lxterm/flterm command for loading the firmware onto Arty.

I included the flterm instruction from the TensorFlow blog note in the Readme file.

Maybe mention how to exit Renode nicely, by typing "quit" at the (machine-0) prompt

I added the final sentence describing how to exit Renode.

@tcal-x Does the current version of https://github.com/antmicro/litex-vexriscv-tensorflow-lite-demo/tree/readme_fixes look good to you? If so, I can merge it to master.

tcal-x commented 4 years ago

Hi @mateusz-holenko , here are my next round of comments.

For the flterm command, the path to zephyr.bin needs to be specified, or there needs to be an extra command to copy it from tensorflow/lite/micro/tools/make/gen/zephyr_vexriscv_x86_64/magic_wand/build/zephyr/zephyr.bin to ./.
For Renode, the script renode/litex-vexriscv-tflite.resc actually uses the precompiled ELF file, ../binaries/magic_wand/zephyr.elf. You could give the user the option, to use this as-is, and then they don't have to build anything else. Or, if they want to do the compilation and then use that generated executable, they could then either alter the script themselves, or you could provide an alternate script. But right now it's not quite right, since the user is told they need to build the executable, but then the pre-compiled version is used.

PiotrZierhoffer commented 4 years ago

This can be automatically added as a part of SDK installation, but adding an explicit reminder is a good idea.

@mateusz-holenko just a side note - /opt is not a default location for the zephyr sdk nowadays.

mateusz-holenko commented 4 years ago

@tcal-x I fixed the flterm instruction and extended the Renode section by adding commands explaining how to boot using the locally-built binary as well as the prebuilt one.

I also merged the changes to master so they are visible to anyone by default.

tcal-x commented 4 years ago

Thanks @mateusz-holenko! The README looks good; I'll give the demo a final run-through tomorrow and then close the issue.

antmicro / litex-vexriscv-tensorflow-lite-demo

Fix README.md so that it matches the blog at Tensorflow.org #18