Conversation
A couple more/better intents
WalkthroughThis update implements new functions in Changes
Sequence Diagram(s)Device Verification & InitializationsequenceDiagram
participant E as EnclosureControlSkill
participant V as Verify Function
participant S as Settings Manager
E->>V: verify_mark_1()
alt Device is Mark 1
V-->>E: Success
E->>S: Check default settings
alt Defaults missing
E->>S: _create_settings() & _load_defaults()
end
else Device not Mark 1
V-->>E: Log error
E->>E: Raise NotImplementedError
end
Eye Color Change FlowsequenceDiagram
participant U as User
participant I as Intent Handler
participant E as EnclosureControlSkill
U->>I: Issue eye color command
I->>E: Call handle_default/custom eye color
E->>E: Execute set_eye_color()
E->>Settings: Update eye color settings
E-->>I: Return confirmation
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
locale/en-us/set.current.eye.color.dialog (1)
1-1: Consider adding punctuation for clarity.
A question typically ends with a question mark. Suggested change:-Would you like to change to this color now +Would you like to change to this color now?__init__.py (4)
3-14: Remove unused imports.
Lines 3, 7, 11, 14 import symbols that are not used in the file. Removing them helps keep the code clean.-import datetime from pathlib import Path from ast import literal_eval as parse_tuple from ovos_color_parser import color_from_description, sRGBAColor -from ovos_color_parser.matching import lookup_name from ovos_utils import create_daemon, classproperty from ovos_utils.log import LOG from ovos_utils.process_utils import RuntimeRequirements -from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler from ovos_workshop.intents import IntentBuilder from ovos_workshop.skills import OVOSSkill -from ovos_workshop.skills.api import SkillApi from ovos_bus_client.message import Message from ovos_i2c_detection import is_mark_1🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
7-7:
ovos_color_parser.matching.lookup_nameimported but unused(F401)
11-11:
ovos_workshop.decorators.resting_screen_handlerimported but unused(F401)
14-14:
ovos_workshop.skills.api.SkillApiimported but unused(F401)
37-53: Simplify logic and remove unused variable.
- The local variable
erroris never used.- Replace
if not device == "MARK1":withif device != "MARK1":to be more idiomatic.def verify_mark_1(): # Check for output from ovos-i2csound i2c = Path("/etc/OpenVoiceOS/i2c_platform") - error = "" if i2c.exists(): with open(i2c, 'r') as f: device = f.read().rstrip() - if not device == "MARK1": + if device != "MARK1": LOG.error = f"ovos-i2cdetect detected device {device}." else: return True # Check one more time just in case initialization was not complete if not is_mark_1(): LOG.error("This device is not a Mark 1.") else: return True return False🧰 Tools
🪛 Ruff (0.8.2)
40-40: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
44-44: Use
device != 'MARK1'instead ofnot device == 'MARK1'Replace with
!=operator(SIM201)
306-311: Clean up commented code or replace with debug logging.
The commented-out speech lines might no longer be needed. Consider removing them or converting them into an on-demand debug statement.def handle_narrow_eyes(self, message): - # self.speak("this is my evil face") self.enclosure.eyes_narrow() - # self.enclosure.eyes_color(255, 0, 0)
422-441: Combine nested if statements for brevity.
These two consecutiveifstatements at lines 438-439 can be merged into one for more concise logic.- if self.settings.get("current_eye_color") != self.settings["defaults"]["default_eye_color"]: - if self.ask_yesno('set.current.eye.color').lower() == "yes": - LOG.info("said change color") - self.set_eye_color(match) + if ( + self.settings.get("current_eye_color") != self.settings["defaults"]["default_eye_color"] + and self.ask_yesno('set.current.eye.color').lower() == "yes" + ): + LOG.info("said change color") + self.set_eye_color(match)🧰 Tools
🪛 Ruff (0.8.2)
438-439: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
__init__.py(9 hunks)locale/en-us/color.entity(0 hunks)locale/en-us/color.voc(1 hunks)locale/en-us/default.eye.color.current.intent(1 hunks)locale/en-us/default.eye.color.intent(1 hunks)locale/en-us/default.voc(1 hunks)locale/en-us/enclosure.voc(1 hunks)locale/en-us/set.current.eye.color.dialog(1 hunks)
💤 Files with no reviewable changes (1)
- locale/en-us/color.entity
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
11-11: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
14-14: ovos_workshop.skills.api.SkillApi imported but unused
(F401)
40-40: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
44-44: Use device != 'MARK1' instead of not device == 'MARK1'
Replace with != operator
(SIM201)
438-439: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (9)
locale/en-us/default.voc (1)
1-1: Simple and effective vocabulary entryThe vocabulary entry for "default" is correctly defined. This will be used by the skill to recognize when a user wants to interact with default settings.
locale/en-us/color.voc (1)
1-2: Good inclusion of both singular and plural formsIncluding both "color" and "colors" forms ensures the skill can properly recognize user utterances regardless of which form they use. This improves the natural language processing capabilities.
locale/en-us/enclosure.voc (1)
2-2: Improved terminology for more accurate user interactionChanging from "your body" to "your face" is a more accurate representation for a Mark 1 device, which primarily interacts through its face/display. This change makes the vocabulary more intuitive and aligned with how users perceive the device.
locale/en-us/default.eye.color.current.intent (1)
1-4: Well-structured intent patterns with comprehensive variationsThe intent patterns provide excellent flexibility for users to set the default eye color to the current color. The patterns systematically cover various phrasings with:
- Different action verbs (change|set|make)
- Optional determiners (the|your)
- Variations in word order and structure
- Optional words to accommodate natural speech patterns
This comprehensive approach ensures the skill can understand a wide range of user expressions for the same intent, enhancing the user experience.
locale/en-us/default.eye.color.intent (1)
1-3: Looks good—just confirm fallback flow for the third pattern.
The third pattern(change|set) default eye (color|colors)does not capture the{color}slot and should rely on a follow-up prompt. If that’s intentional, please verify via testing.__init__.py (4)
443-451: Default eye color current logic looks good.
The new handler properly copies the current color to defaults, then provides verbal feedback.
461-490: Color parsing changes appear consistent.
The_parse_to_rgb()method now returns ansRGBAColorobject orNone. This is an improvement in readability and versatility for color handling.
495-502: No concerns with default eye loading logic.
Properly checks for existing defaults, handles fallback, and sets the enclosure eye color accordingly.
351-368:Details
✅ Verification successful
Verify updated function signature usage.
The function now requirescolorand no longer acceptsrgbormake_default. Confirm that all callers across the codebase have been updated to avoid passing removed parameters.
🏁 Script executed:
#!/bin/bash # Search for calls to set_eye_color that may still be using old parameters rg -A3 "set_eye_color\("Length of output: 1285
Verified: Function signature usage is up-to-date.
All invocations ofset_eye_colornow correctly use only thecolorand optionalspeakparameters. No callers are passing any removed parameters (such asrgbormake_default).
- Found calls in
__init__.pyusing:
self.set_eye_color(self.default_eye_color)self.set_eye_color(custom_rgb)self.set_eye_color(color=match)self.set_eye_color(match)self.set_eye_color(color, speak=False)No outdated parameters were detected.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
__init__.py (1)
351-372: 🛠️ Refactor suggestionFix potential issues in set_eye_color method.
The method has issues with error handling and potential uninitialized variables.
-def set_eye_color(self, color, speak=True):#=None, rgb=None, speak=True):# name=None, speak=True): +def set_eye_color(self, color, speak=True): """ Change the eye color on the faceplate, update saved setting + + Args: + color: An sRGBAColor object or string description of a color + speak: Whether to speak a confirmation """ + r, g, b = None, None, None + name = None + if color is not None: color_rgb = self._parse_to_rgb(color) if color_rgb is not None: (r, g, b) = (color_rgb.r, color_rgb.g, color_rgb.b) name = color_rgb.name else: return # no color provided! + # Ensure RGB values are valid + if r is None or g is None or b is None: + self.log.error('Invalid color values') + if speak: + self.speak_dialog('error.set.color') + return + try: self.enclosure.eyes_color(r, g, b) if speak: self.speak_dialog('set.color.success') # Update saved color + if "current_eye_color" not in self.settings: + self.settings["current_eye_color"] = {} self.settings['current_eye_color']["rgb"] = [r, g, b] self.settings['current_eye_color']["name"] = name - except Exception: - self.log.debug('Bad color code: ' + str(color)) + except Exception as e: + self.log.error(f'Error setting eye color: {e}') if speak: self.speak_dialog('error.set.color')
🧹 Nitpick comments (5)
__init__.py (5)
61-69: Clean up commented code and add error handling for defaults.There are commented-out lines that should be removed or properly implemented, and the default initialization lacks some error handling.
- # self.datetime_api = None self.thread = None self.playing = False self.animations = [] - if not self.settings.get("defaults"): - self._create_defaults() - self._load_defaults() - # callback_time = now_local() + datetime.timedelta(seconds=60) - # self.schedule_repeating_event(self.update_dt, callback_time, 10) + try: + if not self.settings.get("defaults"): + self._create_defaults() + self._load_defaults() + except Exception as e: + LOG.error(f"Error initializing defaults: {e}") + self._create_defaults() + self._load_defaults() self.add_event('mycroft.ready', self.handle_default_eyes)
177-188: Add docstring and improve _create_defaults method.The method needs a proper docstring and could use better comments regarding the default color choice.
def _create_defaults(self): + """Create default settings for eye color and other parameters. + + This initializes default settings if they don't exist, setting + the default eye color to "Mycroft blue" (#22A7F0). + """ LOG.info("creating defaults") - # Using "mycroft blue" as a default color - # TODO: figure out naming conventions - - # color = color_from_description("mycroft blue") This does not return the correct color - # https://github.com/OpenVoiceOS/ovos-color-parser/issues/26 - # It works when getting the color from a hex code + # Using "Mycroft blue" as default - workaround for bug in color_from_description + # See: https://github.com/OpenVoiceOS/ovos-color-parser/issues/26 color = sRGBAColor.from_hex_str("#22A7F0", name="Mycroft blue", description="blue") - self.settings["defaults"] = {"default_eye_color": {"rgb": [color.r, color.g, color.b], - "name": color.name}} + self.settings["defaults"] = { + "default_eye_color": { + "rgb": [color.r, color.g, color.b], + "name": color.name + } + } + # Initialize current_eye_color if it doesn't exist + if not self.settings.get("current_eye_color"): + self.settings["current_eye_color"] = { + "rgb": [color.r, color.g, color.b], + "name": color.name + }
443-451: Improve handle_default_eye_color_current method.The method lacks proper error handling and should validate the current color before setting it as default.
@intent_handler('default.eye.color.current.intent') def handle_default_eye_color_current(self, message): """ Callback to set the default eye color from the current color Args: message (dict): messagebus message from intent parser """ - self.settings["defaults"]["default_eye_color"] = self.settings.get("current_eye_color") - self.speak("I set the default color") + try: + current_color = self.settings.get("current_eye_color") + if current_color and "rgb" in current_color: + if "defaults" not in self.settings: + self.settings["defaults"] = {} + self.settings["defaults"]["default_eye_color"] = current_color + self.speak_dialog("default.color.set") + else: + LOG.error("No current eye color defined") + self.speak_dialog('error.set.color') + except Exception as e: + LOG.error(f"Error setting default color: {e}") + self.speak_dialog('error.set.color')
453-492: Refactor _parse_to_rgb method for better clarity and error handling.The method has confusing control flow and could be simplified.
def _parse_to_rgb(self, color): """ Convert color descriptor to RGB Parse a color name ('dark blue'), hex ('#000088') or rgb tuple '(0,0,128)' to an RGB tuple. Args: color (str): RGB, Hex, or color from color_from_description Returns: - (r, g, b) (tuple): Tuple of rgb values (0-255) or None + sRGBAColor: Color object or None if parsing fails """ if not color: return None - # check if named color is valid + # If already an sRGBAColor, return it try: if isinstance(color, sRGBAColor): return color - else: - return None except Exception: - pass + LOG.debug("Error checking if color is sRGBAColor") # check if rgb tuple like '(0,0,128)' try: (r, g, b) = parse_tuple(color) if 0 <= r <= 255 and 0 <= g <= 255 and 0 <= b <= 255: return sRGBAColor(r, g, b) - else: - return None except Exception: - pass + LOG.debug("Error parsing RGB tuple") # Finally check if color is hex, like '#0000cc' or '0000cc' try: - return sRGBAColor(_hex_to_rgb(color)) + rgb = _hex_to_rgb(color) + if rgb: + return sRGBAColor(*rgb) except Exception: - return None + LOG.debug("Error parsing hex color") return None
123-190: Consider removing commented-out code blocks.Multiple large blocks of commented-out code make the file harder to read and maintain. Consider removing these blocks or moving them to separate experimental/development files.
These commented-out sections (particularly lines 123-203) are cluttering the codebase. Either:
- Remove them entirely if they're not needed
- Move them to a separate development/experimental file
- Add clear comments explaining why they're kept as commented code
This will improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__init__.py(9 hunks)locale/en-us/eye.color.intent(1 hunks)locale/en-us/narrow.voc(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
11-11: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
14-14: ovos_workshop.skills.api.SkillApi imported but unused
(F401)
40-40: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
44-44: Use device != 'MARK1' instead of not device == 'MARK1'
Replace with != operator
(SIM201)
438-439: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (6)
locale/en-us/narrow.voc (1)
1-2: LGTM! Addition of 'squint' expands user interaction options.The addition of "squint" to the vocabulary list provides users with an alternative word to trigger the narrow eyes functionality, enhancing the natural language capabilities of the skill.
locale/en-us/eye.color.intent (1)
7-7: LGTM! Good addition of alternative phrasing.Adding "make (the|your) (eye|eyes) {color}" provides users with a more natural way to express the intent to change eye colors, complementing the existing "change" and "set" commands.
__init__.py (4)
7-7:Details
✅ Verification successful
Remove unused lookup_name import.
The
lookup_namefunction is imported but never used in the code, as indicated by the static analysis.-from ovos_color_parser.matching import lookup_name from ovos_utils import create_daemon, classproperty
🏁 Script executed:
#!/bin/bash # Check if lookup_name is actually used in the file rg "lookup_name" __init__.pyLength of output: 79
Remove unused
lookup_nameimport
The grep results confirm that thelookup_nameimport in__init__.pyis not used anywhere in the file. Please remove the following line:
- Remove:
from ovos_color_parser.matching import lookup_name🧰 Tools
🪛 Ruff (0.8.2)
7-7:
ovos_color_parser.matching.lookup_nameimported but unused(F401)
3-4:Details
✅ Verification successful
Remove unused datetime import.
The
datetimemodule is imported but not used in the code, as indicated by the static analysis.-import datetime from pathlib import Path
🏁 Script executed:
#!/bin/bash # Check if datetime is actually used in the file rg "datetime\." __init__.pyLength of output: 100
Clarification: Remove Unused
datetimeImportThe grep result shows that the only occurrence of
datetimeis within a commented-out line. Since commented code does not count as active usage, the static analysis is correct—thedatetimeimport is indeed unused and should be removed.
- Action: Remove the
import datetimestatement as shown in the provided diff.-import datetime from pathlib import Path🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
308-311:Details
❓ Verification inconclusive
Revisit commented out code in handle_narrow_eyes method.
The commented lines suggest this function was intended to include speech and change the eye color, which might be worth considering.
It appears the original implementation included speech feedback and changed the eye color to red to complete the "evil face" effect. Consider whether this behavior should be restored.
🏁 Script executed:
#!/bin/bash # Check for other implementations that might indicate the preferred behavior rg "narrow.*eyes" --include="*.py" -A 5 -B 5Length of output: 488
Action: Verify Intended Behavior in
handle_narrow_eyesThe commented-out code in
handle_narrow_eyes(specifically lines 308–311 in__init__.py) originally included a speech feedback and an eye color change (to red) that could contribute to the “evil face” effect. Currently, only the eyes narrowing remains active. There’s no clear evidence from an automated scan indicating a standard across the codebase for these features. Please manually verify if restoring the speech viaself.speak("this is my evil face")and the eye color change is part of the intended design or if they should remain removed.
11-11:Details
✅ Verification successful
Remove unused resting_screen_handler import.
The
resting_screen_handleris imported but never used in the code, as indicated by the static analysis.-from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler from ovos_workshop.intents import IntentBuilder
🏁 Script executed:
#!/bin/bash # Check if resting_screen_handler is actually used in the file rg "resting_screen_handler" __init__.py # Check if any @resting_screen_handler decorators are used rg "@resting_screen_handler" __init__.pyLength of output: 156
ACTION: Remove the unused
resting_screen_handlerimport from the__init__.pyfile.
The static search confirmed that aside from being imported,
resting_screen_handlerisn’t used anywhere in the file (no decorator usage via@resting_screen_handlerwas found).Please update the import line as follows:
-from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler from ovos_workshop.intents import IntentBuilder🧰 Tools
🪛 Ruff (0.8.2)
11-11:
ovos_workshop.decorators.resting_screen_handlerimported but unused(F401)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
__init__.py (1)
351-369: 🛠️ Refactor suggestionImproved eye color setting implementation.
The simplified signature for
set_eye_coloris better, and the code now properly stores both RGB values and the color name in the settings. However, there's one potential issue:def set_eye_color(self, color, speak=True):#=None, rgb=None, speak=True):# name=None, speak=True): """ Change the eye color on the faceplate, update saved setting """ if color is not None: color_rgb = self._parse_to_rgb(color) if color_rgb is not None: (r, g, b) = (color_rgb.r, color_rgb.g, color_rgb.b) name = color_rgb.name else: return # no color provided! try: self.enclosure.eyes_color(r, g, b) if speak: self.speak_dialog('set.color.success') # Update saved color + if 'current_eye_color' not in self.settings: + self.settings['current_eye_color'] = {} self.settings['current_eye_color']["rgb"] = [r, g, b] self.settings['current_eye_color']["name"] = name except Exception: self.log.debug('Bad color code: ' + str(color)) if speak: self.speak_dialog('error.set.color')
♻️ Duplicate comments (4)
__init__.py (4)
168-175:⚠️ Potential issueAdd error handling to _load_defaults method.
The
_load_defaultsmethod doesn't have any error handling, which could cause issues if the settings are malformed or if any of the expected keys are missing.def _load_defaults(self): - defaults = self.settings.get("defaults").get("default_eye_color") - (r, g, b) = defaults.get("rgb") - self.default_eye_color = sRGBAColor(r, g, b) - self.set_eye_color(self.default_eye_color) - # TODO: Eye position? - # TODO: Mouth position? - self.enclosure.mouth_reset() + try: + defaults = self.settings.get("defaults", {}).get("default_eye_color", {}) + rgb = defaults.get("rgb") + if rgb and len(rgb) == 3: + (r, g, b) = rgb + self.default_eye_color = sRGBAColor(r, g, b) + self.set_eye_color(self.default_eye_color) + else: + LOG.warning("Invalid default eye color, recreating defaults") + self._create_defaults() + self._load_defaults() + # TODO: Eye position? + # TODO: Mouth position? + self.enclosure.mouth_reset() + except Exception as e: + LOG.error(f"Error loading defaults: {e}") + self._create_defaults() + self._load_defaults()
421-442:⚠️ Potential issueFix the handle_default_eye_color method.
This method has several issues, including incorrect setting of default values and a potential bug in the conditional logic.
@intent_handler('default.eye.color.intent') def handle_default_eye_color(self, message): """ Callback to set the default eye color from list Args: message (dict): messagebus message from intent parser """ LOG.info("setting the default eye color") color_str = (message.data.get('color', None) or self.get_response('color.need')) if color_str: match = color_from_description(color_str) if match is not None: color_rgb = self._parse_to_rgb(match) if color_rgb is not None: (r, g, b) = color_rgb.r, color_rgb.g, color_rgb.b - self.settings["defaults"]["default_eye_color"] = [r, g, b] - if self.settings.get("current_eye_color") != self.settings["defaults"]["default_eye_color"]: + # Update the defaults with the proper structure + if "defaults" not in self.settings: + self.settings["defaults"] = {} + if "default_eye_color" not in self.settings["defaults"]: + self.settings["defaults"]["default_eye_color"] = {} + + # Store both RGB values and name + self.settings["defaults"]["default_eye_color"] = { + "rgb": [r, g, b], + "name": color_rgb.name + } + + # Compare current color with new default + current_rgb = self.settings.get("current_eye_color", {}).get("rgb", []) + default_rgb = [r, g, b] + if current_rgb != default_rgb: if self.ask_yesno('set.current.eye.color').lower() == "yes": LOG.info("said change color") self.set_eye_color(match) + else: + self.speak_dialog("default.color.set") + else: + self.speak_dialog('color.not.exist') + else: + self.speak_dialog('color.not.exist')🧰 Tools
🪛 Ruff (0.8.2)
438-439: Use a single
ifstatement instead of nestedifstatements(SIM102)
494-514:⚠️ Potential issueFix logical issues in handle_default_eyes method.
The method has potential issues with error handling and unnecessary code execution.
def handle_default_eyes(self): + """Reset eye color to default settings.""" settings = self.settings.get("defaults") if settings: # Handle default eyes try: - (r, g, b) = settings.get("default_eye_color").get("rgb") - name = settings.get("default_eye_color").get("name", None) - color = sRGBAColor(r, g, b, name=name) - self.set_eye_color(color, speak=False) + default_eye_color = settings.get("default_eye_color", {}) + if default_eye_color and "rgb" in default_eye_color: + (r, g, b) = default_eye_color.get("rgb") + name = default_eye_color.get("name", None) + color = sRGBAColor(r, g, b, name=name) + self.set_eye_color(color, speak=False) + else: + LOG.warning("Missing default eye color settings") + self._create_defaults() + self._load_defaults() + return except ValueError: + LOG.error("Invalid default eye color values") self._create_defaults() self._load_defaults() + return + except Exception as e: + LOG.error(f"Error setting default eye color: {e}") + self._create_defaults() + self._load_defaults() + return + if settings.get("default_eye_position"): LOG.info("Not implemented yet") else: self.enclosure.eyes_reset() - else: + LOG.info("No defaults found, creating them") self._create_defaults() self._load_defaults()
37-53:⚠️ Potential issueFix issues in verify_mark_1() function.
The current implementation has several problems:
- The
errorvariable is defined but never used- Incorrect logging syntax at line 45 (
LOG.error =instead ofLOG.error())- No error handling for file operations
- Using
not device == "MARK1"instead of the clearerdevice != "MARK1"def verify_mark_1(): # Check for output from ovos-i2csound i2c = Path("/etc/OpenVoiceOS/i2c_platform") - error = "" if i2c.exists(): - with open(i2c, 'r') as f: - device = f.read().rstrip() - if not device == "MARK1": - LOG.error = f"ovos-i2cdetect detected device {device}." - else: - return True + try: + with open(i2c, 'r') as f: + device = f.read().rstrip() + if device != "MARK1": + LOG.error(f"ovos-i2cdetect detected device {device}.") + else: + return True + except Exception as e: + LOG.error(f"Error reading i2c platform file: {e}") # Check one more time just in case initialization was not complete if not is_mark_1(): LOG.error("This device is not a Mark 1.") else: return True return False🧰 Tools
🪛 Ruff (0.8.2)
40-40: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
44-44: Use
device != 'MARK1'instead ofnot device == 'MARK1'Replace with
!=operator(SIM201)
🧹 Nitpick comments (2)
__init__.py (2)
3-4: Clean up unused imports.Several imports are declared but never used in the code:
datetime(line 3)lookup_namefromovos_color_parser.matching(line 7)resting_screen_handlerfromovos_workshop.decorators(line 11)SkillApifromovos_workshop.skills.api(line 14)-import datetime +# import datetime (if needed in future) from pathlib import Path from ast import literal_eval as parse_tuple from ovos_color_parser import color_from_description, sRGBAColor -from ovos_color_parser.matching import lookup_name from ovos_utils import create_daemon, classproperty from ovos_utils.log import LOG from ovos_utils.process_utils import RuntimeRequirements -from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler from ovos_workshop.intents import IntentBuilder from ovos_workshop.skills import OVOSSkill -from ovos_workshop.skills.api import SkillApi from ovos_bus_client.message import MessageAlso applies to: 7-7, 11-11, 14-14
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
148-167: Remove commented out code.The file contains large blocks of commented code that aren't being used. This adds unnecessary clutter and makes the code harder to read. Consider removing these blocks if they're not needed.
Since these are large blocks of commented code related to datetime functionality that isn't being used, they should be removed entirely to improve code readability.
Also applies to: 190-203
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__init__.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
11-11: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
14-14: ovos_workshop.skills.api.SkillApi imported but unused
(F401)
40-40: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
44-44: Use device != 'MARK1' instead of not device == 'MARK1'
Replace with != operator
(SIM201)
438-439: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (4)
__init__.py (4)
177-188: Well implemented default color creation method.The
_create_defaultsmethod is well implemented, using a specific Mycroft blue color and providing a clear comment about the issue with the color parser. The structure of the defaults is also well defined.
401-403: Good simplification of custom eye color implementation.The code now properly uses the
sRGBAColorclass and simplified the function call toset_eye_color.
453-493: Improved _parse_to_rgb method.The method now returns an
sRGBAColorobject directly, which simplifies color handling throughout the code. The logic checks are properly implemented.
65-67: Good implementation of defaults initialization.The code now properly checks if defaults exist and creates them if needed, then loads them. This ensures the skill will always have proper default values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
__init__.py (1)
473-488:⚠️ Potential issueFix bug in
handle_default_eye_color_current()method.The method incorrectly assigns the current eye color to the entire defaults dictionary instead of just the default_eye_color key.
- self.settings["defaults"] = current_eye_color + self.settings["defaults"]["default_eye_color"] = current_eye_color
🧹 Nitpick comments (4)
__init__.py (4)
3-3: Remove unused importdatetime.The
datetimeimport is only used in commented-out code and is not needed in the active codebase.-import datetime🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
7-7: Remove unused importlookup_name.The
lookup_namefunction fromovos_color_parser.matchingis imported but never used in the code.-from ovos_color_parser.matching import lookup_name🧰 Tools
🪛 Ruff (0.8.2)
7-7:
ovos_color_parser.matching.lookup_nameimported but unused(F401)
11-11: Remove unused importresting_screen_handler.The
resting_screen_handlerdecorator is imported but never used in the code.-from ovos_workshop.decorators import intent_handler, resting_screen_handler +from ovos_workshop.decorators import intent_handler🧰 Tools
🪛 Ruff (0.8.2)
11-11:
ovos_workshop.decorators.resting_screen_handlerimported but unused(F401)
438-472: Fix a typo in the error message.There's a typo in the error message on line 470:
- LOG.error(f"Coluld not parse color {match}") + LOG.error(f"Could not parse color {match}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__init__.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
11-11: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
14-14: ovos_workshop.skills.api.SkillApi imported but unused
(F401)
40-40: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
🔇 Additional comments (7)
__init__.py (7)
14-14: Remove unused importSkillApi.The
SkillApiis imported but only used in commented-out code.🧰 Tools
🪛 Ruff (0.8.2)
14-14:
ovos_workshop.skills.api.SkillApiimported but unused(F401)
37-53: Fix issues in theverify_mark_1()function.The function has several issues:
- The
errorvariable on line 40 is defined but never used- There's no error handling for file operations
- The function can be simplified
🧰 Tools
🪛 Ruff (0.8.2)
40-40: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
169-186: Add more robust error handling to_load_defaults()method.The method has basic error handling, but it could be more robust with specific error messages and validation for the RGB values.
365-389: Improved eye color handling implementation.The simplified
set_eye_color()method with better structure and error handling is a good improvement. The method now properly updates the settings with both RGB values and color name.
418-420: Clean implementation of custom RGB color handling.Good refactoring to use the
sRGBAColorclass consistently.
490-529: Good improvement to the_parse_to_rgb()method.The method now properly returns an
sRGBAColorobject instead of a tuple, which is more consistent with the rest of the codebase. The error handling is also improved.
531-558: Improved implementation ofhandle_default_eyes()method.The method now properly handles loading and applying default eye colors with good error handling for various failure cases.
|
Partially solves OpenVoiceOS/ovos-PHAL-plugin-mk1#11 More in progress |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
__init__.py (1)
474-488:⚠️ Potential issueFix handle_default_eye_color_current method's logic.
There is a critical issue where the entire settings["defaults"] gets overwritten with the current_eye_color structure.
@intent_handler('default.eye.color.current.intent') def handle_default_eye_color_current(self, message): """ Callback to set the default eye color from the current color Args: message (dict): messagebus message from intent parser """ current_eye_color = self.settings.get("current_eye_color") if current_eye_color: if "defaults" not in self.settings: self.settings["defaults"] = {} - self.settings["defaults"] = current_eye_color + self.settings["defaults"]["default_eye_color"] = current_eye_color + self.speak_dialog("default.color.set") else: LOG.error("Could not get a current eye color") + self.speak_dialog("error.no.current.color") + return - self.speak("I set the default color")
🧹 Nitpick comments (3)
__init__.py (3)
3-4: Remove unused imports to improve code readability.Several imports are not being used in the active code and should be removed:
- Line 3:
import datetime- Only used in commented-out code- Line 7:
from ovos_color_parser.matching import lookup_name- Line 14:
from ovos_workshop.skills.api import SkillApi- Only used in commented-out code-import datetime +# import datetime (uncomment if datetime functionality is restored) from pathlib import Path from ast import literal_eval as parse_tuple from ovos_color_parser import color_from_description, sRGBAColor -from ovos_color_parser.matching import lookup_name from ovos_utils import create_daemon, classproperty from ovos_utils.log import LOG from ovos_utils.process_utils import RuntimeRequirements from ovos_workshop.decorators import intent_handler, resting_screen_handler from ovos_workshop.intents import IntentBuilder from ovos_workshop.skills import OVOSSkill -from ovos_workshop.skills.api import SkillApi from ovos_bus_client.message import MessageAlso applies to: 7-7, 14-14
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
365-387: Improve error handling in set_eye_color method.The method has been simplified to only accept a color object, which is good, but could benefit from better error handling.
-def set_eye_color(self, color, speak=True):#=None, rgb=None, speak=True):# name=None, speak=True): +def set_eye_color(self, color, speak=True): """ Change the eye color on the faceplate, update saved setting """ if color is not None: color_rgb = self._parse_to_rgb(color) if color_rgb is not None: (r, g, b) = (color_rgb.r, color_rgb.g, color_rgb.b) name = color_rgb.name try: self.enclosure.eyes_color(r, g, b) if speak: self.speak_dialog('set.color.success') # Update saved color if "current_eye_color" not in self.settings: self.settings["current_eye_color"] = {} self.settings["current_eye_color"] = { "rgb": [r, g, b], "name": name} - except Exception: + except Exception as e: - LOG.debug('Bad color code: ' + str(color)) + LOG.error(f'Failed to set eye color: {e}') if speak: self.speak_dialog('error.set.color') + else: + LOG.warning(f'Invalid color provided: {color}') + if speak: + self.speak_dialog('error.set.color') else: return
504-529: Improve error handling in _parse_to_rgb method.The method has been updated to return sRGBAColor objects, but has redundant return statements and could use better error handling.
# check if named color is valid try: if isinstance(color, sRGBAColor): return color else: return None except Exception: pass # check if rgb tuple like '(0,0,128)' try: (r, g, b) = parse_tuple(color) if 0 <= r <= 255 and 0 <= g <= 255 and 0 <= b <= 255: return sRGBAColor(r, g, b) else: return None except Exception: pass # Finally check if color is hex, like '#0000cc' or '0000cc' try: - return sRGBAColor(_hex_to_rgb(color)) + rgb = _hex_to_rgb(color) + if rgb: + return sRGBAColor(*rgb) + return None except Exception: return None -return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__init__.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
11-11: ovos_workshop.decorators.resting_screen_handler imported but unused
(F401)
14-14: ovos_workshop.skills.api.SkillApi imported but unused
(F401)
40-40: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
🔇 Additional comments (8)
__init__.py (8)
37-53: Fix issues in the verify_mark_1 function.The function has several issues:
- The
errorvariable is defined but never used- There's an incorrect assignment to
LOG.errorinstead of calling the method- Missing error handling for file operations
def verify_mark_1(): # Check for output from ovos-i2csound i2c = Path("/etc/OpenVoiceOS/i2c_platform") - error = "" if i2c.exists(): - with open(i2c, 'r') as f: - device = f.read().rstrip() - if device != "MARK1": - LOG.error = f"ovos-i2cdetect detected device {device}." - else: - return True + try: + with open(i2c, 'r') as f: + device = f.read().rstrip() + if device != "MARK1": + LOG.error(f"ovos-i2cdetect detected device {device}.") + else: + return True + except Exception as e: + LOG.error(f"Error reading i2c platform file: {e}") # Check one more time just in case initialization was not complete if not is_mark_1(): LOG.error("This device is not a Mark 1.") else: return True return False🧰 Tools
🪛 Ruff (0.8.2)
40-40: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
169-186: Add more robust error handling to _load_defaults method.The method should safeguard against missing or malformed settings more thoroughly.
def _load_defaults(self): try: defaults = self.settings.get("defaults", {}).get("default_eye_color", {}) rgb = defaults.get("rgb") if rgb and len(rgb) == 3: (r, g, b) = rgb - self.set_eye_color(sRGBAColor(r, g, b)) + name = defaults.get("name") + self.set_eye_color(sRGBAColor(r, g, b, name=name), speak=False) else: LOG.warning("Invalid default eye color, recreating defaults") self._create_settings() self._load_defaults() # TODO: Eye position? # TODO: Mouth position? self.enclosure.mouth_reset() except Exception as e: - LOG.error(f"error loading defaults {e}") + LOG.error(f"Error loading defaults: {e}") self._create_settings() self._load_defaults()
197-202: Good implementation of default color settings.The implementation properly initializes the default and current eye color settings with both RGB values and name.
437-473: Fix the handle_default_eye_color method's structure.This method has several issues with structure and error handling, and should provide better feedback to the user.
@intent_handler('default.eye.color.intent') def handle_default_eye_color(self, message): """ Callback to set the default eye color from list Args: message (dict): messagebus message from intent parser """ LOG.info("setting the default eye color") color_str = (message.data.get('color', None) or self.get_response('color.need')) if color_str: match = color_from_description(color_str) if match is not None: color_rgb = self._parse_to_rgb(match) if color_rgb is not None: (r, g, b) = color_rgb.r, color_rgb.g, color_rgb.b if "defaults" not in self.settings: self.settings["defaults"] = {} if "default_eye_color" not in self.settings["defaults"]: self.settings["defaults"]["default_eye_color"] = {} # Save the default settings self.settings["defaults"]["default_eye_color"] = { "rgb": [r, g, b], "name": color_rgb.name} current_rgb = self.settings.get("current_eye_color", {}).get("rgb", []) default_rgb = [r, g, b] if current_rgb != default_rgb: if self.ask_yesno('set.current.eye.color').lower() == "yes": self.set_eye_color(match) + self.speak_dialog("default.color.set") else: - LOG.error(f"Coluld not parse color {match}") + LOG.error(f"Could not parse color {match}") + self.speak_dialog('color.not.exist') else: self.speak_dialog("color.not.exist")
531-557: Improve handle_default_eyes method.The method has been updated with better error handling, but could benefit from better structure and clearer code.
def handle_default_eyes(self): + """Reset eye color to default settings.""" settings = self.settings.get("defaults") if settings: # Handle default eyes try: default_eye_color = settings.get("default_eye_color", {}) if default_eye_color and "rgb" in default_eye_color: (r, g, b) = default_eye_color.get("rgb") name = default_eye_color.get("name", None) color = sRGBAColor(r, g, b, name=name) self.set_eye_color(color, speak=False) + else: + LOG.warning("Invalid default eye color structure") + self._create_settings() + self._load_defaults() + return except ValueError: LOG.error("Invalid default eye color settings") self._create_settings() self._load_defaults() + return except Exception as e: LOG.error(f"Could not load default eye color. {e}") self._create_settings() self._load_defaults() + return if settings.get("default_eye_position"): LOG.info("Not implemented yet") + # TODO: Implement default eye position + else: + self.enclosure.eyes_reset() else: LOG.info("No default eye color found, creating them") self._create_settings() self._load_defaults()
361-361: Good improvement to handle_enclosure_crazy_eyes.Adding the call to
handle_default_eyes()ensures the eyes are properly reset to default after the crazy animation ends.
91-97: Good refactoring of color definitions.The change to using
sRGBAColorobjects directly is a good improvement for code clarity and consistency.
66-68: Good default settings initialization.The addition of default settings creation if they don't exist is a good improvement for robustness.
better default color handling
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
__init__.py (2)
36-52:⚠️ Potential issueFix multiple issues in verify_mark_1() function.
The function has several issues, including unused variables, potential file I/O errors, and incorrect logging syntax.
def verify_mark_1(): # Check for output from ovos-i2csound i2c = Path("/etc/OpenVoiceOS/i2c_platform") - error = "" if i2c.exists(): - with open(i2c, 'r') as f: - device = f.read().rstrip() - if device != "MARK1": - LOG.error = f"ovos-i2cdetect detected device {device}." - else: - return True + try: + with open(i2c, 'r') as f: + device = f.read().rstrip() + if device != "MARK1": + LOG.error(f"ovos-i2cdetect detected device {device}.") + else: + return True + except Exception as e: + LOG.error(f"Error reading i2c platform file: {e}") # Check one more time just in case initialization was not complete if not is_mark_1(): LOG.error("This device is not a Mark 1.") else: return True return False🧰 Tools
🪛 Ruff (0.8.2)
39-39: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
443-459:⚠️ Potential issueFix critical bug in handle_default_eye_color_current method.
The method incorrectly overwrites the entire "defaults" dictionary with just the eye color data.
@intent_handler('default.eye.color.current.intent') def handle_default_eye_color_current(self, message): """ Callback to set the default eye color from the current color Args: message (dict): messagebus message from intent parser """ current_eye_color = self.settings.get("current_eye_color") if current_eye_color: if "defaults" not in self.settings: self.settings["defaults"] = {} - self.settings["defaults"] = current_eye_color + self.settings["defaults"]["default_eye_color"] = current_eye_color + self.speak("I set the default color") else: LOG.error("Could not get a current eye color") + self.speak_dialog("error.no.current.color") - - self.speak("I set the default color")
🧹 Nitpick comments (3)
__init__.py (3)
3-4: Remove unused imports.There are several unused imports in the file that should be removed to keep the code clean.
-import datetime -from ovos_color_parser.matching import lookup_name -from ovos_utils import create_daemon, classproperty, create_killable_daemon +from ovos_utils import create_daemon, classpropertyAlso applies to: 7-8
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
408-442: Fix typo in error message and ensure consistent dialog usage.There's a typo in the log message and inconsistent use of dialog resources.
- self.speak("Default color is set") + self.speak_dialog("default.color.set") else: - LOG.error(f"Coluld not parse color {match}") + LOG.error(f"Could not parse color {match}")
461-504: Update function docstring and fix error with _hex_to_rgb conversion.The docstring still mentions returning a tuple but the function now returns an sRGBAColor object. Also, the code doesn't handle the case where _hex_to_rgb returns None.
def _parse_to_rgb(self, color): """ Convert color descriptor to RGB Parse a color name ('dark blue'), hex ('#000088') or rgb tuple '(0,0,128)' to an RGB tuple. Args: color (str): RGB, Hex, or color from color_from_description Returns: - (r, g, b) (tuple): Tuple of rgb values (0-255) or None + sRGBAColor: Color object or None """ if not color: return None # ... rest of the code ... # Finally check if color is hex, like '#0000cc' or '0000cc' try: - return sRGBAColor(_hex_to_rgb(color)) + rgb = _hex_to_rgb(color) + if rgb: + return sRGBAColor(*rgb) + return None except Exception: return None - - return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__init__.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
8-8: ovos_utils.create_killable_daemon imported but unused
(F401)
39-39: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
🔇 Additional comments (3)
__init__.py (3)
143-160: Good implementation of _load_defaults with robust error handling.The method properly handles different error scenarios and falls back to creating default settings when needed.
505-532: Good implementation of handle_default_eyes with proper error handling.The method correctly checks for default settings and handles errors gracefully.
163-176:Details
❓ Verification inconclusive
Review LOG.default method usage.
The
LOG.default()method on line 164 is unusual - standard Python logging doesn't have a "default" log level.
🏁 Script executed:
#!/bin/bash # Check if LOG.default exists in the codebase rg "LOG\.default" --type pyLength of output: 90
LOG.default Usage – Verify Custom Logger Intent
- It’s confirmed that
LOG.default("Creating default settings")exists in__init__.py(line 163) and isn’t a typo.- Since standard Python logging doesn't offer a "default" level, please verify that this custom log level is intentional.
- If using a custom logger, ensure that its behavior for the "default" method is documented and consistent with logging practices. Otherwise, consider switching to a conventional log level (e.g.,
LOG.info) for clarity.
| remaining_frames = len(self.animations) # Track how many frames need to execute | ||
| executed_frames = set() # Track which frames have been executed by index | ||
| current_eye_color = self.settings.get("current_eye_color", {"rgb": [34, 167, 240], "name": "Mycroft blue"}) | ||
|
|
||
| while self.playing and remaining_frames > 0: | ||
| for i, animation in enumerate(self.animations): | ||
| if animation["time"] <= time.time() and i not in executed_frames: | ||
| try: | ||
| if animation["args"] and animation["args"] == (None,): | ||
| animation["func"]() | ||
| else: | ||
| animation["func"](*animation["args"]) | ||
| executed_frames.add(i) # Mark this frame as done | ||
| remaining_frames -= 1 # Decrement remaining count | ||
| except Exception as e: | ||
| LOG.error(f"Animation failed: {e}") | ||
| executed_frames.add(i) # Still mark as done to avoid stalling | ||
| remaining_frames -= 1 | ||
| # Do not reschedule; let it run once | ||
| time.sleep(0.1) # Prevent tight CPU loop | ||
|
|
||
| self.playing = False # Explicitly stop the loop | ||
| self.thread = None # Thread ends naturally | ||
| self.enclosure.activate_mouth_events() | ||
| self.enclosure.mouth_reset() | ||
| self.enclosure.eyes_reset() | ||
|
|
||
| # self.handle_default_eyes() | ||
| # Restore initial current_eye_color if different from default color | ||
| if current_eye_color["rgb"] != self.settings.get("defaults", {}).get("default_eye_color")["rgb"]: | ||
| (r, g, b) = current_eye_color["rgb"] | ||
| try: | ||
| self.enclosure.eyes_color(r, g, b) | ||
| LOG.info(f"Restored eye color to {current_eye_color}") | ||
| except Exception as e: | ||
| LOG.error(f"Failed to restore eye color: {e}") | ||
| self.handle_default_eyes() | ||
| else: | ||
| self.handle_default_eyes() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential KeyError in eye color restoration logic.
The code on line 206 directly accesses nested dictionary keys without safely checking if they exist.
- if current_eye_color["rgb"] != self.settings.get("defaults", {}).get("default_eye_color")["rgb"]:
+ default_color = self.settings.get("defaults", {}).get("default_eye_color", {}).get("rgb")
+ if default_color and current_eye_color.get("rgb") != default_color:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remaining_frames = len(self.animations) # Track how many frames need to execute | |
| executed_frames = set() # Track which frames have been executed by index | |
| current_eye_color = self.settings.get("current_eye_color", {"rgb": [34, 167, 240], "name": "Mycroft blue"}) | |
| while self.playing and remaining_frames > 0: | |
| for i, animation in enumerate(self.animations): | |
| if animation["time"] <= time.time() and i not in executed_frames: | |
| try: | |
| if animation["args"] and animation["args"] == (None,): | |
| animation["func"]() | |
| else: | |
| animation["func"](*animation["args"]) | |
| executed_frames.add(i) # Mark this frame as done | |
| remaining_frames -= 1 # Decrement remaining count | |
| except Exception as e: | |
| LOG.error(f"Animation failed: {e}") | |
| executed_frames.add(i) # Still mark as done to avoid stalling | |
| remaining_frames -= 1 | |
| # Do not reschedule; let it run once | |
| time.sleep(0.1) # Prevent tight CPU loop | |
| self.playing = False # Explicitly stop the loop | |
| self.thread = None # Thread ends naturally | |
| self.enclosure.activate_mouth_events() | |
| self.enclosure.mouth_reset() | |
| self.enclosure.eyes_reset() | |
| # self.handle_default_eyes() | |
| # Restore initial current_eye_color if different from default color | |
| if current_eye_color["rgb"] != self.settings.get("defaults", {}).get("default_eye_color")["rgb"]: | |
| (r, g, b) = current_eye_color["rgb"] | |
| try: | |
| self.enclosure.eyes_color(r, g, b) | |
| LOG.info(f"Restored eye color to {current_eye_color}") | |
| except Exception as e: | |
| LOG.error(f"Failed to restore eye color: {e}") | |
| self.handle_default_eyes() | |
| else: | |
| self.handle_default_eyes() | |
| remaining_frames = len(self.animations) # Track how many frames need to execute | |
| executed_frames = set() # Track which frames have been executed by index | |
| current_eye_color = self.settings.get("current_eye_color", {"rgb": [34, 167, 240], "name": "Mycroft blue"}) | |
| while self.playing and remaining_frames > 0: | |
| for i, animation in enumerate(self.animations): | |
| if animation["time"] <= time.time() and i not in executed_frames: | |
| try: | |
| if animation["args"] and animation["args"] == (None,): | |
| animation["func"]() | |
| else: | |
| animation["func"](*animation["args"]) | |
| executed_frames.add(i) # Mark this frame as done | |
| remaining_frames -= 1 # Decrement remaining count | |
| except Exception as e: | |
| LOG.error(f"Animation failed: {e}") | |
| executed_frames.add(i) # Still mark as done to avoid stalling | |
| remaining_frames -= 1 | |
| # Do not reschedule; let it run once | |
| time.sleep(0.1) # Prevent tight CPU loop | |
| self.playing = False # Explicitly stop the loop | |
| self.thread = None # Thread ends naturally | |
| self.enclosure.activate_mouth_events() | |
| self.enclosure.mouth_reset() | |
| # self.handle_default_eyes() | |
| # Restore initial current_eye_color if different from default color | |
| - if current_eye_color["rgb"] != self.settings.get("defaults", {}).get("default_eye_color")["rgb"]: | |
| + default_color = self.settings.get("defaults", {}).get("default_eye_color", {}).get("rgb") | |
| + if default_color and current_eye_color.get("rgb") != default_color: | |
| (r, g, b) = current_eye_color["rgb"] | |
| try: | |
| self.enclosure.eyes_color(r, g, b) | |
| LOG.info(f"Restored eye color to {current_eye_color}") | |
| except Exception as e: | |
| LOG.error(f"Failed to restore eye color: {e}") | |
| self.handle_default_eyes() | |
| else: | |
| self.handle_default_eyes() |
This animation is unreliable and will be added back when another solution is found
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
__init__.py (4)
334-358:⚠️ Potential issueFix indentation and null check issues in set_eye_color method.
The method has indentation issues that could lead to undefined variables if color_rgb is None.
def set_eye_color(self, color, speak=True): """ Change the eye color on the faceplate, update saved setting """ if color is not None: color_rgb = self._parse_to_rgb(color) if color_rgb is not None: (r, g, b) = (color_rgb.r, color_rgb.g, color_rgb.b) name = color_rgb.name - try: - self.enclosure.eyes_color(r, g, b) - if speak: - self.speak_dialog('set.color.success') - # Update saved color - if "current_eye_color" not in self.settings: - self.settings["current_eye_color"] = {} - self.settings["current_eye_color"] = { - "rgb": [r, g, b], - "name": name} - - except Exception: - LOG.debug('Bad color code: ' + str(color)) - if speak: - self.speak_dialog('error.set.color') + try: + self.enclosure.eyes_color(r, g, b) + if speak: + self.speak_dialog('set.color.success') + # Update saved color + if "current_eye_color" not in self.settings: + self.settings["current_eye_color"] = {} + self.settings["current_eye_color"] = { + "rgb": [r, g, b], + "name": name} + + except Exception: + LOG.debug('Bad color code: ' + str(color)) + if speak: + self.speak_dialog('error.set.color') + else: + LOG.debug('Invalid color: ' + str(color)) + if speak: + self.speak_dialog('error.set.color') else: return
36-52:⚠️ Potential issueFix issues in the verify_mark_1() function.
Several issues need to be addressed:
- The
errorvariable is declared but never used- Incorrect logging syntax at line 44 (uses assignment instead of function call)
- Missing error handling for file operations
def verify_mark_1(): # Check for output from ovos-i2csound i2c = Path("/etc/OpenVoiceOS/i2c_platform") - error = "" if i2c.exists(): - with open(i2c, 'r') as f: - device = f.read().rstrip() - if device != "MARK1": - LOG.error = f"ovos-i2cdetect detected device {device}." - else: - return True + try: + with open(i2c, 'r') as f: + device = f.read().rstrip() + if device != "MARK1": + LOG.error(f"ovos-i2cdetect detected device {device}.") + else: + return True + except Exception as e: + LOG.error(f"Error reading i2c platform file: {e}") # Check one more time just in case initialization was not complete if not is_mark_1(): LOG.error("This device is not a Mark 1.") else: return True return False🧰 Tools
🪛 Ruff (0.8.2)
39-39: Local variable
erroris assigned to but never usedRemove assignment to unused variable
error(F841)
442-457:⚠️ Potential issueFix incorrect assignment to default settings.
The current implementation incorrectly assigns the current eye color to the entire defaults dictionary instead of just the default_eye_color field.
- self.settings["defaults"] = current_eye_color + self.settings["defaults"]["default_eye_color"] = current_eye_color
178-214:⚠️ Potential issueFix potential KeyError in line 205.
When restoring eye color, direct access to nested dictionary keys could lead to a KeyError if the key doesn't exist.
- if current_eye_color["rgb"] != self.settings.get("defaults", {}).get("default_eye_color")["rgb"]: + default_color = self.settings.get("defaults", {}).get("default_eye_color", {}).get("rgb") + if default_color and current_eye_color.get("rgb") != default_color:
🧹 Nitpick comments (3)
__init__.py (3)
3-3: Remove unused imports.The following imports are not used in the code and should be removed:
datetime(line 3)lookup_name(line 7)create_killable_daemon(line 8)-import datetime from pathlib import Path from ast import literal_eval as parse_tuple from ovos_color_parser import color_from_description, sRGBAColor -from ovos_color_parser.matching import lookup_name -from ovos_utils import create_daemon, classproperty, create_killable_daemon +from ovos_utils import create_daemon, classproperty from ovos_utils.log import LOGAlso applies to: 7-8
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
datetimeimported but unused(F401)
163-176: Good default settings creation, but consider adding a docstring.The method creates appropriate default settings with helpful comments explaining the reasoning behind color choices.
def _create_settings(self): + """Create default settings for eye color if they don't exist.""" LOG.default("Creating default settings") # Using "mycroft blue" as a default color # TODO: figure out naming conventions
407-441: Fix typo in error message.There's a typo in the error message on line 438.
- LOG.error(f"Coluld not parse color {match}") + LOG.error(f"Could not parse color {match}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
__init__.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
__init__.py
3-3: datetime imported but unused
(F401)
7-7: ovos_color_parser.matching.lookup_name imported but unused
(F401)
8-8: ovos_utils.create_killable_daemon imported but unused
(F401)
39-39: Local variable error is assigned to but never used
Remove assignment to unused variable error
(F841)
🔇 Additional comments (5)
__init__.py (5)
143-160: LGTM: Good error handling in _load_defaults().The implementation correctly loads default values and includes robust error handling. The method also handles the case of invalid default eye color by recreating the defaults.
387-389: LGTM: Good simplification of handle_custom_eye_color.The method now creates and passes an sRGBAColor object directly, which is cleaner and more concise.
403-403: LGTM: Good use of named parameters.Using named parameters makes the code more readable and less prone to errors.
459-502: LGTM: Good enhancements to _parse_to_rgb method.The method now consistently returns sRGBAColor objects rather than tuples, which improves type consistency throughout the codebase.
504-531: LGTM: Improved handle_default_eyes implementation.The method now has better error handling and properly handles the case where settings don't exist.
1 participant
A couple more/better intents
Summary by CodeRabbit
New Features
Localization Updates