Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to SDK Version 3.7 #23

Merged
merged 16 commits into from Sep 25, 2020
Merged

Upgrade to SDK Version 3.7 #23

merged 16 commits into from Sep 25, 2020

Conversation

SuperN1ck
Copy link
Contributor

@SuperN1ck SuperN1ck commented Sep 7, 2020

Hey,

I took some time and updated most of the examples to the latest SDK Version (3.7) for the GAP8. Not everything is perfected yet but since the SDK is also evolving quite quick it's hard to keep everything up-to-date.

  • test_functionalities/test_camera
    • Removed some unnecessary stuff
    • Added gray scale mode
    • Added QVGA-mode/quadratic mode
    • Added another docker volume which should be used for placing the common files
    • Unexpected Behavior When Using Camera聽#21 still remains is better but not optimal
    • Updated docs to inform about tiling problem
  • test_functionalities/uart_send_char
    • Untouched --> not tested
  • test_functionalities/wifi_jpeg_streamer
  • image_processing_examples/FaceDetection
    • Seems to work without streaming but face detection is not working correctly. accurate sometimes
    • But since similar to wifi_jpeg_streamer the streaming is also not working, it's hard to debug
    • Loading images from disk is put in the loop, but when opening the same image again it still breaks as before --> I guess some problem inside the SDK with closing files correctly?
    • Loading images from disk is really slow. I'm not sure why.
    • Streaming only works for SDK version 3.5
  • image_processing_examples/image_manipulations
    • No issues -- worked right away 馃憤
  • ai_examples/gapflow_model_extern
    • Copied from official SDK example
    • Followed the renaming from mnist --> model
    • Added cleaning of trained models (previously not done before)
    • Since for whatever reason in gap sdk they removed tensorflow from the requirements.txt-list for the nntool I manually added in the docker installation
    • Seems to work properly now including retraining
  • ai_examples/mnist_simple
    • Seems completely broken
    • Is also not updated in the official SDK-example
    • Completely removed

All examples were tested in the docker.

Happy to take some feedback. If I find time in the next few days I will probably investigate some of the issues that still remain.

@knmcguire
Copy link
Member

Hi! Thanks for all of this! I'm currently myself on a holiday so it might take some time to review this pull request but I can check if I have some time these days to try it out.

Since the SDK is evolving so quickly, it might be good to maybe just stick with one SDK for a little while maybe? That is kind of the reason why I made the docker files in the first place, since it takes a lot of time to update all of this and we are busy with many other projects as well. But I'm happy that you are contributing as much as you do, it is definitely helping out a lot!

The blocking problem I noticed in SDK 3.6 as well. So this only affects the streaming I guess? I heard from @Yaooooo that there is now an option to do the streaming compression from the cluster instead, so maybe that is something worth looking into?

I see that you are still pushing many changes to your PR so let me know if you are finished.

@SuperN1ck
Copy link
Contributor Author

Hey @knmcguire,
enjoy your holidays and take your time with the review! :)

Regarding sticking to one version I fully agree with you. What do you think, should we stick with 3.7 from now on since the current state of the repository (3.6) is also not fully functional? To add to that I just checked all examples within my 3.6-docker and get the same results as for the 3.7-docker. So nothing additionally breaks using the newer version but all of the previously mentioned issues also remain unfortunately.

Regarding the streaming, that sounds great. But in other words it's either to use an older version (like 3.4) or dig deep into the sdk as there is no example (yet?)?

@knmcguire
Copy link
Member

knmcguire commented Sep 9, 2020

It sounds good to stick with 3.7 for a while. For the streaming we should probably advise people to use the 3.5 SDK (since that is the one that still works). I've already added instructions in the readme how to make an image for previous versions as well, since that is the whole reason why I started to use docker so that I can easily switch between gapsdk:3.4, 3.5 etc :)

By the way, the neural network examples are indeed very old. If mnist_simple is completely broken than we should just remove it actually. It is not really platform depended anyway. But surprised that the gap_flow_extern seems to work in 3.7. I can't remember if it actually uses a working neural network but was more of an example on how to compile an empty network on the chip.

I'll look into it again once I get back :)

@SuperN1ck
Copy link
Contributor Author

Alright! I just checked the streaming examples with 3.5 and can confirm that it also works for me with that version!

Yes removing the mnist_simple might make sense since there is also no activity in the sdk related issue: GreenWaves-Technologies/gap_sdk#125

Afaik gap_flow_extern trains a neural network (defined in model/train.py) from scratch and then runs the whole pipeline to put in on the gap8. There was actually not much work to make it run in the latest sdk version since this example was actually updated in the gap repository.

@SuperN1ck
Copy link
Contributor Author

Hey @knmcguire,
I updated my original post with the latest information. Let me know when you had time to review the code and have feedback available.

@knmcguire
Copy link
Member

Hi!

I'll be back next week so I've already started looking into it, so I'll have some comments for you soon. It would be good to start merging this into the code indeed to not keep it lingering around in PR limbo :)

It has become a pretty big pull request btw, but I won't be very picky since this is a bit of an experimental repo anyway for now!

@knmcguire
Copy link
Member

knmcguire commented Sep 18, 2020

I tried out with the new docker file + instructions and it worked right away on the image manipulations example :)
I like using the define export of the SDK version, however this needs to be done at every new terminal though with the instructions. Is it maybe better to say to add this to the bashrc instead?

Just a couple of things of things that I have tested

  • The facedetector example seems to work for me on 3.7 as well, maybe not with the streamer define on though, but it does work with the camera define.
  • The test camera has this error when I use the 3.7 docker file, but I guess that is because the PR didn't had the file with the demosaicking in yet:
test.c:23:10: fatal error: ../../common/img_proc.h: No such file or directory
 #include "../../common/img_proc.h"

I haven't gone through the gapflow example yet or worked on the streaming examples.

@SuperN1ck
Copy link
Contributor Author

SuperN1ck commented Sep 21, 2020

The test camera has this error when I use the 3.7 docker file, but I guess that is because the PR didn't had the file with the demosaicking in yet:

The problem is/was that the demosaicking function is placed in the common folder at the root of the GAP8-examples which is not mounted in the docker. But since the file is not used anywhere else I moved it in the test camera example.

Is it maybe better to say to add this to the bashrc instead?

Good point, I added a small paragraph about it

@knmcguire
Copy link
Member

So I tested out the send uart, that still works!

The gapflow_model_extern I get the following error when I use docker.

make: *** No rule to make target '../common/model_rules.mk'. Stop.

But when I run docker with cd /module/data/gapflow_model_extern in the ai_examples folder with this, it does work, but is maybe not super clear that this should be done like this.

I got from @Yaooooo some instructions as well to make it completely standalone, but maybe best for a next pull request ;)

@SuperN1ck
Copy link
Contributor Author

So I tested out the send uart, that still works!

Good to hear that the UART example still works.

But when I run docker with cd /module/data/gapflow_model_extern in the ai_examples folder with this, it does work, but is maybe not super clear that this should be done like this.

I put this at the end of the readme inside the gapflow_model_extern example, but I agree with you that it might be confusing using different docker commands. The only reason why one has to do it this way, is to also include the common-folder inside the docker. We could get rid of the common folder and move the two files inside the root of gapflow_model_extern (similar to the demosaicking)?

@knmcguire
Copy link
Member

Yeah maybe for now it is enough to include the .mk files from common and add it to the the gapflow_model_extern

@SuperN1ck
Copy link
Contributor Author

Done that now :)

@knmcguire
Copy link
Member

ah perfect! I was actually also right in the middle of doing this but for some reason I removed my SDK 3.7 image from docker so I had to build it again :P

Probably I'm going to start moving the readmes of docker and the gapflow readme to the doc folder. I think it is stable enough to also let other people know that they can use it.

@knmcguire knmcguire merged commit 399ffbc into bitcraze:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants