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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New files for Tensorflow Lite Micro Speech Example targetting the Eta… #24222

Merged
merged 8 commits into from Dec 27, 2018

Conversation

hshankar61
Copy link

… Compute ECM3531 evaluation board

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@hshankar61
Copy link
Author

hshankar61 commented Dec 7, 2018 via email

@hshankar61
Copy link
Author

CLA has been signed again

Copy link
Contributor

@petewarden petewarden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hari! I've added some initial comments, could you also run clang-format on the .c files to standardize them with the rest of the code base (selecting the Google style)? This is looking good, I hope we can get it in soon.

@@ -0,0 +1,97 @@
/*******************************************************************************
* Copyright (C) 2018 Eta Compute, Inc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this to be:

Suggested change
* Copyright (C) 2018 Eta Compute, Inc
* Copyright 2018 The TensorFlow Authors. All Rights Reserved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,88 @@
/*******************************************************************************
*
* Copyright (C) 2018 Eta Compute, Inc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this to be:

Suggested change
* Copyright (C) 2018 Eta Compute, Inc
* Copyright 2018 The TensorFlow Authors. All Rights Reserved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,35 @@
#!/usr/bin/python3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a copyright notice here?

Suggested change
#!/usr/bin/python3
#!/usr/bin/python3
# Copyright 2015 The TensorFlow Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# ==============================================================================

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,35 @@
#!/usr/bin/python3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a quick comment about usage, even if it's just pointing back to the main doc file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,592 @@
/*******************************************************************************
*
* Copyright (C) 2018 Eta Compute, Inc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this to be:

Suggested change
* Copyright (C) 2018 Eta Compute, Inc
* Copyright 2018 The TensorFlow Authors. All Rights Reserved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* limitations under the License.
*
******************************************************************************/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some brief comments about what this file is for.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#include "memio.h"


//#pragma GCC optimize ("align-functions=16")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this dead code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

//
//*****************************************************************************

#define NAKED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in one place that I see, can we just remove it entirely for clarity?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

petewarden
petewarden previously approved these changes Dec 10, 2018
@hshankar61
Copy link
Author

I signed it

@googlebot
Copy link

CLAs look good, thanks!

@petewarden
Copy link
Contributor

Is there something else we need to do to trigger the remaining checks that haven't completed? /cc @gunan

@gunan gunan added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 14, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 14, 2018
@petewarden
Copy link
Contributor

The failure in the MacOS Python2 and CC test appears to be unrelated to any code here:

==================== Test output for //tensorflow/core:platform_port_test:
Running main() from test_main.cc
[==========] Running 5 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from Port
[ RUN      ] Port.AlignedMalloc
[       OK ] Port.AlignedMalloc (0 ms)
[ RUN      ] Port.GetCurrentCPU
tensorflow/core/platform/port_test.cc:38: Failure
Expected: (cpu) >= (0), actual: -1 vs 0
[  FAILED  ] Port.GetCurrentCPU (0 ms)

@hshankar61
Copy link
Author

hshankar61 commented Dec 17, 2018 via email

@petewarden petewarden added the kokoro:force-run Tests on submitted change label Dec 17, 2018
@petewarden
Copy link
Contributor

I believe somebody internal should take care of the copybara check once the tests pass, and I've just seen that Gunhan was able to kick off tests by adding the kokoro:force-run flag, so I've tried doing the same to see if we can rerun the tests. Thanks for your patience, hopefully we'll have this through soon!

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 17, 2018
@hshankar61
Copy link
Author

Thanks Pete. Looks like the checks passed!

Causes a missing-copyright error on internal verification
@petewarden
Copy link
Contributor

The stub .sh file caused an error internally because it didn't have a copyright notice, so I've removed that and will rerun the tests.

@hshankar61
Copy link
Author

Should I check in the test_ecm3531_binary.sh file with the copyright added?

@petewarden
Copy link
Contributor

No, I removed it already so I think we're good.

@petewarden petewarden added the kokoro:force-run Tests on submitted change label Dec 18, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 18, 2018
@hshankar61
Copy link
Author

There are 3 checks now that are failing but earlier today it showed that all the tests passed. What gives?

@hshankar61
Copy link
Author

hshankar61 commented Dec 18, 2018 via email

@petewarden petewarden added the kokoro:force-run Tests on submitted change label Dec 18, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 18, 2018
@hshankar61
Copy link
Author

The previous errors were for Windows but now the MacOS and Ubuntu builds show errors. None of the errors are related to the files I checked in. So how does one make this process converge?

@gunan
Copy link
Contributor

gunan commented Dec 19, 2018

There were build breakages merged to master branch.
Now they are fixed, I will retrigger your tests.

@gunan gunan added the kokoro:force-run Tests on submitted change label Dec 19, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 19, 2018
@hshankar61
Copy link
Author

Thanks. This time the "MacOS Contrib" check has failed as earlier, but the "Ubuntu contrib" test which passed before now fails. But two of the earlier checks that failed, now pass ("MacOS Python2" and "Ubuntu Python3").

@petewarden petewarden added the kokoro:force-run Tests on submitted change label Dec 19, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 19, 2018
@tensorflow-copybara tensorflow-copybara merged commit 3eb35ec into tensorflow:master Dec 27, 2018
tensorflow-copybara pushed a commit that referenced this pull request Dec 27, 2018
PiperOrigin-RevId: 226966660
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants