-
-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Implement adding, deleting and updating the timer #78
Conversation
New commands are: * mirobo timer add --cron <cron string> --command <command> --params <params> * mirobo timer delete <id> * mirobo timer update [--on|--off] <id> Although the vacuum accepts command and params, the added cron entry will be just 'start_clean'
mirobo/vacuum.py
Outdated
|
||
def update_timer(self, timer_id: int, mode): | ||
if mode != "on" and mode != "off": | ||
raise DeviceException("update_timer needs to be either 'on' or 'off") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (81 > 79 characters)
mirobo/vacuum.py
Outdated
def add_timer(self, cron, command, parameters): | ||
import time | ||
ts = int(round(time.time() * 1000)) | ||
return self.send("set_timer", [[str(ts), [cron, [command, parameters]]]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (81 > 79 characters)
mirobo/vacuum_cli.py
Outdated
# Note ts == ID for changes | ||
click.echo(click.style("Timer #%s, id %s (ts: %s)" % ( | ||
idx, timer.id, timer.ts), bold=True, fg=color)) | ||
print(" %s" % timer.cron) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use print here instead of click.echo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
mirobo/vacuum_cli.py
Outdated
idx, timer.id, timer.ts), bold=True, fg=color)) | ||
print(" %s" % timer.cron) | ||
min, hr, x, y, days = timer.cron.split(' ') | ||
# hr is in gmt+8 (chinese time), TODO convert to local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful here. I assume the issue will be fixed in future. Users of the mi home app are affected, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the vacuum supports timezones nowadays, those should be times in active timezone. Removed the conversion and added a print-out for the timezone.
mirobo/vacuum_cli.py
Outdated
@click.option('--off', is_flag=True) | ||
@pass_dev | ||
def update(vac: mirobo.Vacuum, timer_id, on, off): | ||
"""Update (on or off) an existing scheduled.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable/Disable a stored timer. I would prefer "--enable" and "--disable" as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to --enable
and --disable
, """Enable/disable a timer.""" as help message to be consistent with other commands.
mirobo/vacuum.py
Outdated
return self.send("del_timer", [str(timer_id)]) | ||
|
||
def update_timer(self, timer_id: int, mode): | ||
if mode != "on" and mode != "off": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would suggest a "enum class" here. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would indeed, was thinking about whether to have it for a boolean here. I changed it by adding Vacuum.TimerState {On, Off} ;-)
README.md
Outdated
Activating/deactivating an existing timer, | ||
use `mirobo timer` to get the required id. | ||
``` | ||
$ mirobo timer update <id> [--off|--on] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed to update this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are fixed now, thanks!
mirobo/vacuum_cli.py
Outdated
hr = (int(hr) - 8) % 24 | ||
cron = "%s %s %s %s %s" % (min, hr, x, y, days) | ||
click.echo(" %s" % pretty_cron.prettify_cron(cron)) | ||
click.echo("Only 'on' and 'off' are valid for timer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one.
New commands are:
Although the vacuum accepts command and params, the added cron entry will be just 'start_clean'