Xilinx / DPU-PYNQ

DPU on PYNQ
Apache License 2.0
199 stars 68 forks source link

fix readme for Ultra96 and ZCU104 to get pynq-dpu notebooks #88

Closed DavideConficconi closed 1 year ago

DavideConficconi commented 2 years ago

Referencing to issue #87 to install pynq-dpu notebooks

skalade commented 2 years ago

Hi there,

Thank you for the PR. Referring back to your issue, what were your exact reproduction steps? Were you connecting to the board via a serial or ssh session? Most people won't need to source those scripts to install the notebooks if running from the jupyter lab terminal (which I admit should be more clear in the readme).

If you go through installation steps from the jupyter lab terminal you shouldn't see these issues. Only readme change I would make is "simply run:" -> "simply run in jupyter lab terminal:" and have a warning at the end in case you are in an ssh session instead with the steps that solved your issue.

Thanks Shawn

DavideConficconi commented 2 years ago

Hi Shawn,

As you correctly guessed I was connecting to the board not through jupyter (both serial and ssh). I did not know that the process should happen through the jupyter lab terminal.

Currently, I even experienced some issues in the ZCU104 setup. I will try your procedure through jupyter notebooks terminal and get back to you.

In case everything works it might be a good solution your proposal of simply saying to run in jupyter lab terminal. Anyhow, I believe that whatever installation procedure I should be able to login on the board via ssh/serial and test a command-line python script. Do you think this is valuable? In case maybe it is helpful to add a section in the README for the new PYNQ venv procedure.

Thanks for the feedback

Davide

DavideConficconi commented 2 years ago

Hello @skalade, I just had the testing time.

On a brand-new pynq image on ZCU104 it works from jupyter terminal. What do you believe is better? I think adding explicit reference to jupyter notebook terminals and a section to install from ssh is a valuable alternative. In case I can just adjust the readme of this PR.

Please let me know your thought :)

Best Davide

skalade commented 2 years ago

Hi @DavideConficconi,

Just specifying the jupyter lab terminal in the beginning would be great if you could make that adjustment. If you could make the ssh instructions not appear mandatory as in the current PR, but an alternative (perhaps optional) subsection for anyone doing it over ssh/serial, I agree, would indeed be a valuable addition.

Thanks Shawn

DavideConficconi commented 2 years ago

Hi @skalade, I should have committed the updates to the readme as you suggested. Let me know if there is anything you believe should be updated

Thanks Davide

skalade commented 1 year ago

Thanks for the contribution!