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

PHAL #6

Merged
merged 1 commit into from
Feb 10, 2022
Merged

PHAL #6

merged 1 commit into from
Feb 10, 2022

Conversation

JarbasAl
Copy link
Member

@JarbasAl JarbasAl commented Nov 6, 2021

this goes together with OpenVoiceOS/ovos-utils#11

what used to be the enclosure template in ovos utils is now the PHAL plugin

@JarbasAl JarbasAl added the enhancement New feature or request label Nov 6, 2021
JarbasAl added a commit that referenced this pull request Dec 1, 2021
JarbasAl added a commit that referenced this pull request Dec 1, 2021
JarbasAl added a commit that referenced this pull request Dec 2, 2021
@JarbasAl JarbasAl marked this pull request as ready for review February 3, 2022 00:25
JarbasAl added a commit that referenced this pull request Feb 3, 2022
Copy link
Member

@NeonDaniel NeonDaniel left a comment

Choose a reason for hiding this comment

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

Noted missing/incorrect documentation

from ovos_utils.messagebus import get_mycroft_bus


class PHALPlugin:
Copy link
Member

Choose a reason for hiding this comment

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

Consider just PHAL to match TTS, STT, etc.

self.bus.on("enclosure.reset", self.on_reset)

# enclosure commands for Mycroft's Hardware.
self.bus.on("enclosure.system.reset", self.on_system_reset)
Copy link
Member

Choose a reason for hiding this comment

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

Might be cleaner to define the events/handlers as tuples in a param or property and iterate over that list since these are duplicated in start/stop methods. This also lets an extending class add handlers without having to add in the self.bus.on/off calls


# Audio Events
def on_record_begin(self, message=None):
''' listening started '''
Copy link
Member

Choose a reason for hiding this comment

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

Consider updating ''' to """ in docstrings for consistency

def on_no_internet(self, message=None):
"""

Args:
Copy link
Member

Choose a reason for hiding this comment

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

This should either be added to all handlers or removed from this one

def on_system_blink(self, message=None):
"""The 'eyes' should blink the given number of times.
Args:
times (int): number of times to blink
Copy link
Member

Choose a reason for hiding this comment

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

args don't match signature

Copy link
Member Author

@JarbasAl JarbasAl Feb 4, 2022

Choose a reason for hiding this comment

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

in all these args they are documenting what comes inside the message.data , they are not final and were copied over from original methods that emitted the message

as you can probably tell i copied over almost every defined event handler for enclosure in mycroft-core

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; figured it was copy/paste.. Maybe we should define some standard way to document expected data/context in these docstrings?

def _on_mouth_text(self, message=None):
"""Display text (scrolling as needed)
Args:
text (str): text string to display
Copy link
Member

Choose a reason for hiding this comment

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

args don't match signature

self.on_text(message)

def _on_mouth_display(self, message=None):
if self.mouth_events_active:
Copy link
Member

Choose a reason for hiding this comment

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

missing docstring

def on_viseme(self, message=None):
"""Display a viseme mouth shape for synched speech
Args:
code (int): 0 = shape for sounds like 'y' or 'aa'
Copy link
Member

Choose a reason for hiding this comment

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

args don't match signature

or half the face. You can use the 'x' parameter to cover the other
half of the faceplate.
Args:
img_code (str): text string that encodes a black and white image
Copy link
Member

Choose a reason for hiding this comment

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

args don't match signature

"""Show a the temperature and a weather icon

Args:
img_code (char): one of the following icon codes
Copy link
Member

Choose a reason for hiding this comment

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

args don't match signature

@JarbasAl
Copy link
Member Author

JarbasAl commented Feb 4, 2022

this PR is pending on the actual release of PHAL and may still change significantly, marking as draft while base class is ironed out

but keep the feedback coming!

@JarbasAl JarbasAl marked this pull request as draft February 4, 2022 01:46
@JarbasAl
Copy link
Member Author

JarbasAl commented Feb 4, 2022

this goes together with OpenVoiceOS/ovos-utils#11

what used to be the enclosure template in ovos utils is now the PHAL plugin

@JarbasAl
Copy link
Member Author

early merging to allow PHAL release 0.0.1

will refactor later

@JarbasAl JarbasAl marked this pull request as ready for review February 10, 2022 01:31
@JarbasAl JarbasAl merged commit 4a1b452 into master Feb 10, 2022
@JarbasAl JarbasAl deleted the feat/phal branch February 24, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants