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

Fix converse and snooze #110

Open
wants to merge 11 commits into
base: 20.08
Choose a base branch
from
170 changes: 140 additions & 30 deletions __init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import pytz
from datetime import datetime, timedelta
from os.path import join, abspath, dirname, isfile
import re
import time

from alsaaudio import Mixer

from adapt.intent import IntentBuilder
from mycroft import MycroftSkill, intent_handler
from mycroft.configuration.config import LocalConf, USER_CONFIG
from mycroft.configuration.config import LocalConf, DEFAULT_CONFIG
krisgesling marked this conversation as resolved.
Show resolved Hide resolved
from mycroft.messagebus.message import Message
from mycroft.util import play_mp3
from mycroft.util.format import nice_date_time, nice_time, nice_date, join_list
Expand All @@ -46,7 +45,6 @@
describe_repeat_rule,
)


# WORKING PHRASES/SEQUENCES:
# Set an alarm
# for 9
Expand All @@ -70,7 +68,6 @@
# > 7pm tomorrow
# Cancel it


class AlarmSkill(MycroftSkill):
"""The official Alarm Skill for Mycroft AI."""

Expand Down Expand Up @@ -163,6 +160,10 @@ def initialize(self):
# Support query for active alarms from other skills
self.add_event("private.mycroftai.has_alarm", self.on_has_alarm)

# establish local timezone from config
self.local_tz = self.config_core.get("location").get("timezone").get("code")
krisgesling marked this conversation as resolved.
Show resolved Hide resolved
self.log.info("Local timezone configured for %s" % (self.local_tz,))

def on_has_alarm(self, message):
"""Reply to requests for alarm on/off status."""
total = len(self.settings["alarm"])
Expand Down Expand Up @@ -662,6 +663,117 @@ def _get_alarm_matches(
# No matches found
return (status[2], None)

def _fallback_get_alarm_matches(self, utt):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention that this will fully replace _get_alarm_matches() or is it intended to be distinct in some way?

Copy link
Author

Choose a reason for hiding this comment

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

It is used when the normal flow can not find any

# TODO this needs to become a class variable both here and above
status = ["All", "Matched", "No Match Found", "User Cancelled", "Next"]
return_status = status[2]
alarm_list = self.settings["alarm"]
krisgesling marked this conversation as resolved.
Show resolved Hide resolved

self.log.debug("utt:%s, alarms:%s" % (utt, self.settings["alarm"]))

# Lingua-franca workaround-001 - day of week and date confuses LF terribly.
# if a day of the week (monday, friday, etc) is included with a
# valid date like "monday april 5th" LF will return bad data so we make
# sure we never include both. "monday april 5th" becomes "april 5th".
Copy link
Contributor

Choose a reason for hiding this comment

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

I've logged this as an issue on that repo. Seems safe enough to do if a valid date exists, though plenty hacky and we should remove it asap.

Choose a reason for hiding this comment

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

That parser's being rewritten, because there are all sorts of problems like this. I'd suggest adding a TODO for MycroftAI/lingua-franca#96 which should make this unnecessary, and provide more robust coverage for the same conditions.

Obviously wouldn't be prudent to offer a timetable on that, as there are multiple design choices up in the air in chat, and several higher priorities for everyone on LF. Still, Jarbas has written a lot of the code. He might be due to push, as well.


# I'm sure to get blowback during pr for this ...
Copy link
Contributor

Choose a reason for hiding this comment

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

yes... :)

months = ['january', 'february', 'march', 'april', 'may', 'june', 'july', 'august', 'september', 'october', 'november', 'december']
days = ['monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday', 'sunday']

for month in months:
if utt.find(month) > -1 and re.search(r'\d+', utt):
# if we have a month and a number/day
# whack any days of week terms
for day in days:
utt = utt.replace(day,'')

self.log.debug("LF scrubbed utt = %s" % (utt,))

when, utt_no_datetime = extract_datetime(utt) or (None, utt)
self.log.debug("when: %s (this is in local timezone), what: %s" % (when, utt_no_datetime))

when_utc = when.astimezone(pytz.utc)
Copy link
Contributor

Choose a reason for hiding this comment

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

be better to use mycroft.util.time.to_utc()

self.log.debug("when_utc: %s (this is in utc)" % (when_utc,))

have_time = False
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be more descriptive, eg utterance_has_time?

Copy link
Author

Choose a reason for hiding this comment

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

This means we believe we have a time. Its not strictly utterance dependent. It is derived. You may rename it if you like.

if when_utc is None:
self.log.warning("Request contains no discernable date or time")
krisgesling marked this conversation as resolved.
Show resolved Hide resolved
else:
# assumption 1 - if time is midnight we probably don't have a time
# in other words the user probably said something like 'monday the 25th'
# or tomorrow.
# we can look for the term 'midnight' in the utterance and
# if present we can assume we DO have a time (midnight = 00:00:00),
# otherwise we assume we don't have a time
user_time = when.strftime("%H:%M:%S")
if user_time == "00:00:00" and utt.find("midnight") == -1:
krisgesling marked this conversation as resolved.
Show resolved Hide resolved
self.log.debug("we have no user time ===> 00:00:00")
else:
have_time = True

self.log.debug("Do we have a user time:%s" % (have_time,))

alarm_matches = []

user_dow = None # if user said a day of the week, this is it

result = re.findall('(mon|tues|wed|thurs|fri|sat|sun)day', utt)
if result:
utt_dow = result[0] + 'day'
user_dow = days.index(utt_dow) if utt_dow in days else None

self.log.debug("user_dow:%s" % (user_dow,))

# if the user says a day of the week (mon-sun) we
# keep them in a separate list of day of week matches
dow_matches = []

for alm in alarm_list:
self.log.debug(" Loop: %s" % (alm,))

# get utc time from alarm timestamp
dt_obj = datetime.fromtimestamp( alm['timestamp'] )
dt_obj = dt_obj.astimezone(pytz.utc)

# we also need a local version of our utc time
cfg_tz = pytz.timezone(self.local_tz)
dt_local = dt_obj.astimezone(cfg_tz)

if user_dow == dt_local.weekday():
dow_matches.append(alm)

if have_time:
# unambiguous
if when_utc == dt_obj:
alarm_matches.append(alm)
else:
self.log.debug(" Loop:have date but no time - disambiguate here!")
self.log.debug(" Loop: when:%s, dt_obj:%s" % (when_utc.strftime("%y-%m-%d"), dt_obj.strftime("%y-%m-%d")))
# do dates match ?
if when_utc.strftime("%y-%m-%d") == dt_obj.strftime("%y-%m-%d"):
self.log.debug("Note, date match with no time")
alarm_matches.append(alm)
krisgesling marked this conversation as resolved.
Show resolved Hide resolved

if len(alarm_matches) == 0:
# we can try some heuristics here
self.log.info("Entering heuristics phase, dow_matches:%s" % (dow_matches,))
alarm_matches = dow_matches


if len(alarm_matches) == 1:
return_status = status[1]
else:
if len(alarm_matches) > 1:
self.log.debug("Can't disambiguate, match count %s is > 1" % (len(alarm_matches,)))
self.speak_dialog("alarm.confused")

# meet previous ret code protocol
if len(alarm_matches) == 0:
alarm_matches = None

return return_status, alarm_matches


@intent_handler(
IntentBuilder("")
.require("Delete")
Expand Down Expand Up @@ -690,6 +802,12 @@ def handle_delete(self, message):
is_response=False,
)

if alarms is None:
# the poor _get_alarm_matches() method is a bit of a dim bulb.
# we'll inject some heuristics into it here
status, alarms = self._fallback_get_alarm_matches(utt)
self.log.info("No alarms was converted to - status:%s, alarms:%s" % (status, alarms))

if alarms:
total = len(alarms)
else:
Expand Down Expand Up @@ -739,11 +857,14 @@ def snooze_alarm(self, message):
if not has_expired_alarm(self.settings["alarm"]):
return

self.cancel_scheduled_event("Beep")
self.cancel_scheduled_event("NextAlarm")

self.__end_beep()
self.__end_flash()

utt = message.data.get("utterance") or ""
snooze_for = extract_number(utt)
snooze_for = extract_number(utt[0])
if not snooze_for or snooze_for < 1:
snooze_for = 9 # default to 9 minutes

Expand Down Expand Up @@ -784,8 +905,18 @@ def converse(self, utterances, lang="en-us"):
"""While an alarm is expired, check all utterances for Stop vocab."""
if has_expired_alarm(self.settings["alarm"]):
if utterances and self.voc_match(utterances[0], "StopBeeping"):
self._stop_expired_alarm()
return True # and consume this phrase
self._stop_expired_alarm()
elif self.voc_match(utterances[0], "Snooze"):
message = Message(
"internal.snooze",
data=dict(utterance=utterances)
)
self.snooze_alarm(message)
else:
# TODO deal with this
self.log.info("AlarmSkill:Converse confused by %s" % (utterances[0],))
Copy link
Contributor

Choose a reason for hiding this comment

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

this would trigger for any utterance does not contain Stop or Snooze. I don't think we want to do anything here at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we want to prompt the user - "do you want to turn off the alarm too?"


return True # and consume this phrase
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should only consume the phrase if we match on StopBeeping or Snooze - otherwise we are consuming every utterance if an alarm is expired.

This means Mycroft won't respond to anything until you either turn off the alarm or snooze.

Copy link
Author

Choose a reason for hiding this comment

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

This is an interesting point. We are technically in a converse state within an alarm activation. Do we want to open ourselves up to allowing other things to happen before we resolve the current issue with the active alarm? I don't think I know the answer to this so I simply get the code to pass the VK tests which are basically the spec. Now if you would like I suspect you could create a new VK test which initially fails and demonstrates the behavior you would like to alter. I would be open to this.


def stop(self, _=None):
"""Respond to system stop commands."""
Expand Down Expand Up @@ -868,28 +999,7 @@ def __end_beep(self):
pass
self.beep_process = None
self._restore_volume()
self._restore_listen_beep()

def _restore_listen_beep(self):
if "user_beep_setting" in self.settings:
# Wipe from local config
new_conf_values = {"confirm_listening": False}
user_config = LocalConf(USER_CONFIG)

if (
self.settings["user_beep_setting"] is None
and "confirm_listening" in user_config
):
del user_config["confirm_listening"]
else:
user_config.merge(
{"confirm_listening": self.settings["user_beep_setting"]}
)
user_config.store()

# Notify all processes to update their loaded configs
self.bus.emit(Message("configuration.updated"))
del self.settings["user_beep_setting"]
#self._restore_listen_beep()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're taking this out let's just delete it rather than commenting it out.

Copy link
Author

Choose a reason for hiding this comment

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

done


def _stop_expired_alarm(self):
if has_expired_alarm(self.settings["alarm"]):
Expand Down
2 changes: 2 additions & 0 deletions vocab/en-us/Snooze.voc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
snooze

2 changes: 1 addition & 1 deletion vocab/en-us/StopBeeping.voc
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ off
disable
ok
okay
alright
alright