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

Added shutdown and reboot commands #30

Merged
merged 2 commits into from May 24, 2017
Merged

Added shutdown and reboot commands #30

merged 2 commits into from May 24, 2017

Conversation

@ktinkerer
Copy link
Contributor

@ktinkerer ktinkerer commented May 11, 2017

Adds commands to shutdown and reboot the pi.

@googlebot
Copy link

@googlebot googlebot commented May 11, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 11, 2017

I signed it

Loading

@googlebot
Copy link

@googlebot googlebot commented May 11, 2017

CLAs look good, thanks!

Loading

src/action.py Outdated
self.command = command

def run(self, voice_command):
if (self.command == "shutdown"):
Copy link
Collaborator

@ensonic ensonic May 11, 2017

Choose a reason for hiding this comment

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

No () needed in python, also below.

Loading

Copy link
Contributor Author

@ktinkerer ktinkerer May 12, 2017

Choose a reason for hiding this comment

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

Oops, that's my rusty python!

Loading

src/action.py Outdated
# =========================================

actor.add_keyword(_('power off'), PowerCommand(say, 'shutdown'))
actor.add_keyword(_('reboot'), PowerCommand(say, 'reboot'))
Copy link
Collaborator

@ensonic ensonic May 11, 2017

Choose a reason for hiding this comment

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

I am concerned that this might trigger in a search "How to I reboot my laptop" :/

Loading

Copy link
Contributor Author

@ktinkerer ktinkerer May 12, 2017

Choose a reason for hiding this comment

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

Yes that's a good point, perhaps a more specific phrase?

Loading

Copy link
Collaborator

@ensonic ensonic May 12, 2017

Choose a reason for hiding this comment

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

Yes, that is one solution. Maybe we keep these commented out by default or we consider a config file to enable/disable a set of commands. But to be honest I don't have a perfect solution in mind right now.

Loading

Copy link
Contributor Author

@ktinkerer ktinkerer May 13, 2017

Choose a reason for hiding this comment

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

Yes maybe commented out so that users know they have enabled the commands. The problem is at the moment that once you've built the AIY Projects box you end up with a headless pi with ssh turned off. You can't shut it down safely without attaching a keyboard and a monitor.

Loading

Choose a reason for hiding this comment

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

I made the keyword "pi reboot"

It does kickoff if you say "how do I pi reboot blah blah blah" but that wont be used anytime soon

Loading

Copy link
Member

@drigz drigz May 15, 2017

Choose a reason for hiding this comment

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

@nigelstewart Does "pi reboot" work well? I'd be worried it would be recognized as "pie reboot", but maybe Google's speech recognition is cleverer than that!

If so, it seems like a good solution to me.

Loading

Copy link

@nigelstewart nigelstewart May 15, 2017

Choose a reason for hiding this comment

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

ha! its funny you should ask. It works very well. I originally put "Pie" but the self conceded little board interpreted it as "Pi" (as displayed when run manually via the main.py script) so I played along and took off the "e"

I did use a different command to accomplish this, here is my entry:

actor.add_keyword(('Pi power off'), SpeakShellCommandOutput(say, "sudo shutdown now",('failed to shutdown')))

I also added reboot:

actor.add_keyword(_('Pi reboot'), SpeakShellCommandOutput(say, "sudo shutdown -r now", _('failed to reboot')))

Both commands are working as expected

Loading

@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 15, 2017

Hi, please make sure travis is happy with your PR. Follow the Details link above to see a report. Also please don't add extra change to this PR if they are belonging to a different feature (radio support).

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 15, 2017

Sorry that was my Github mess up, not realising it would interfere with this PR, I've removed all those changes, they were the ones causing the error.

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 17, 2017

Do I need to git reset to remove those extra commits? Wanted to check before I make more of a mess of it?

Loading

@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 17, 2017

There are many ways. Just to make sure you don't loose anything I'd recommend to do

git format-patch origin

This will save a patch file for each commit. Now you can do

git rebase -i origin

and delete the lines of the commits that we don't have. Once the PR is merged you can

git am xxx.patch

to apply them again.

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 17, 2017

Thank you for your help, I think I've got there in the end, I will learn git eventually! But travis still shows an error?

Loading

@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 17, 2017

Yep, still there:
https://travis-ci.org/google/aiyprojects-raspbian/builds/233372818#L172

You can run the check locally:

pep8 --max-line-length=100 .

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 18, 2017

Thanks for that tip, missing blank line, passes now.

Loading

@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 24, 2017

@ktinkerer can you please rebase the PR and make the commands 'pi power off' and 'pi reboot'. Then we can merge it.

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented May 24, 2017

Codecov Report

Merging #30 into master will increase coverage by 0.09%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #30      +/-   ##
=========================================
+ Coverage    9.67%   9.76%   +0.09%     
=========================================
  Files           3       3              
  Lines        1705    1720      +15     
  Branches      293     295       +2     
=========================================
+ Hits          165     168       +3     
- Misses       1530    1542      +12     
  Partials       10      10
Impacted Files Coverage Δ
src/action.py 60% <20%> (-5.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9571936...77ebd68. Read the comment docs.

Loading

@ktinkerer
Copy link
Contributor Author

@ktinkerer ktinkerer commented May 24, 2017

Is that right?

Loading

@drigz drigz merged commit 250b8cb into google:master May 24, 2017
4 checks passed
Loading
@drigz
Copy link
Member

@drigz drigz commented May 24, 2017

Looks great, thanks for working on it!

Loading

Rexypoo added a commit to Rexypoo/aiyprojects-raspbian that referenced this issue Jun 1, 2017
* Added shutdown and reboot commands

* added pi to the keyword
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants