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

Event handling for active alarms #121

Merged
merged 6 commits into from
Sep 16, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
*.pyc
settings.json
mycroft
mycroft

# Pycharm project files
.idea/
10 changes: 10 additions & 0 deletions __init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,19 @@ def initialize(self):

# Support query for active alarms from other skills
self.add_event("private.mycroftai.has_alarm", self.on_has_alarm)
self.add_event("skill.alarm.query-active", self.handle_active_alarm_query)

def on_has_alarm(self, message):
"""Reply to requests for alarm on/off status."""
total = len(self.settings["alarm"])
self.bus.emit(message.response(data={"active_alarms": total}))

def handle_active_alarm_query(self, _):
"""Event handler for the skill.alarm.query-active command."""
event_data = {"active_alarms": bool(self.settings["alarm"])}
event = Message("skill.alarm.active-queried", data=event_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a message.response rather than a new Message?

Choose a reason for hiding this comment

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

What is the definition of "active alarm"? Any alarm at all (like for next week, or next year)? Or just in the next 24 hrs? Or just until midnight tonight? I don't care what the answer is... just that it's well-defined and documented.

Copy link
Member Author

@chrisveilleux chrisveilleux Aug 25, 2021

Choose a reason for hiding this comment

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

@krisgesling I am purposefully trying to avoid the request/response mechanism. in my reading on event driven architecture, the proper way to respond to a command or request is to emit an event indicating the task was completed.

Are we an event driven architecture or a request/response architecture? I don't think we should be both.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MetaGamer The current definition is whether or not any alarms exists that have not expired. I will add more information in the docstring.

Choose a reason for hiding this comment

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

FWIW, the HiveMind spec, which is a kind of superset of the current Mycroft stack, splits the difference by emitting a response containing ACK INSTRUCTION

(doesn't translate directly, because INSTRUCTION is an index, but it accomplishes cleanliness while preserving the two-way bus)

Copy link

@JarbasAl JarbasAl Aug 25, 2021

Choose a reason for hiding this comment

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

just gonna leave this here https://github.com/JarbasHiveMind/HiveMind-core/wiki/Mycroft-Messages

the reply/forward methods are essential for hivemind support

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand and agree that wait_for_response() is not useful in this scenario (as far as I understand the use case) the using .response() may still be of use so that messages follows a standardized format and indicates it's relation in a clear way. if the event emitted is only sent as a response (and not unprovoked due to other actions)

I do think wait_for_response() has a niche to make the logic flow easier to follow when execution is blocked until an event is sent is a result of a triggering event.

self.bus.emit(event)
chrisveilleux marked this conversation as resolved.
Show resolved Hide resolved

def set_alarm(self, when, name=None, repeat=None):
"""Set an alarm at the specified datetime."""
requested_time = when.replace(second=0, microsecond=0)
Expand Down Expand Up @@ -206,6 +213,9 @@ def _schedule(self):
self.schedule_event(
self._alarm_expired, to_system(alarm_dt), name="NextAlarm"
)
event_data = {"active_alarms": bool(self.settings["alarm"])}
event = Message("skill.alarm.scheduled", data=event_data)
self.bus.emit(event)

def _get_recurrence(self, utterance: str):
"""Get recurrence pattern from user utterance."""
Expand Down