Conversation
|
Hi @alexzhom
Once confirmed, this PR looks ready to merge for Hacktoberfest |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors backend/command.py to optimize performance by implementing singleton patterns with double-checked locking for TTS engine, speech recognizer, and microphone instances. The changes reduce repeated initialization overhead and improve thread safety for concurrent access.
Key Changes:
- Implemented lazy-initialized singletons with thread locks for TTS engine, speech recognizer, and microphone
- Cached ambient noise calibration and disabled dynamic energy threshold adjustment
- Refactored command routing logic into a dedicated
_handle_comm()function to reduce branching complexity
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def _get_microphone() -> sr.Microphone: | ||
| global _microphone | ||
| if _microphone is None: | ||
| _microphone = sr.Microphone() |
There was a problem hiding this comment.
The microphone singleton initialization is not protected by a lock, which could lead to a race condition if multiple threads call _get_microphone() simultaneously. Add a lock similar to _engine_lock and _rec_lock to ensure thread-safe initialization.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| def _ensure_calibrated(r: sr.Recognizer, source: sr.AudioSource) -> None: | ||
| """Calibrate ambient noise once and cache the energy threshold.""" | ||
| global _calibrated, _cached_energy_threshold | ||
| if not _calibrated: | ||
| r.adjust_for_ambient_noise(source, duration=0.4) | ||
| _cached_energy_threshold = r.energy_threshold | ||
| _calibrated = True |
There was a problem hiding this comment.
The calibration logic has a race condition where multiple threads could enter the if not _calibrated: block simultaneously before _calibrated is set to True. This should use a lock or implement double-checked locking similar to the singleton patterns used elsewhere.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| speak("No command was given.") | ||
| eel.ShowHood() | ||
| return |
There was a problem hiding this comment.
The logic duplicates the speak('No command was given.') call that appears on line 170. Consider removing this redundant error handling since the same condition is handled in the else block below.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| print(f"Listen error: {e}") | ||
| return None |
There was a problem hiding this comment.
The error handling swallows specific exception information. Using a bare except Exception as e without logging the exception type or traceback makes debugging difficult. Consider using print(f"Listen error: {type(e).__name__}: {e}") or proper logging to preserve more diagnostic information.
| speak("What message to send?") | ||
| msg = takecommand() | ||
| if msg: | ||
| whatsApp(Phone, msg, "message", name) | ||
| else: | ||
| speak("I didn't catch the message.") |
There was a problem hiding this comment.
In the message send flow, if takecommand() returns None (due to an error or timeout), the code correctly handles it by speaking "I didn't catch the message." However, this creates a dead-end in the conversation flow. The user would have to restart the entire process. Consider whether this should return to allow the user to retry, or if the function should loop to give the user another chance to provide the message.
| speak("What message to send?") | |
| msg = takecommand() | |
| if msg: | |
| whatsApp(Phone, msg, "message", name) | |
| else: | |
| speak("I didn't catch the message.") | |
| max_retries = 3 | |
| for attempt in range(max_retries): | |
| speak("What message to send?") | |
| msg = takecommand() | |
| if msg: | |
| whatsApp(Phone, msg, "message", name) | |
| break | |
| else: | |
| speak("I didn't catch the message.") | |
| if attempt < max_retries - 1: | |
| speak("Please say the message again.") | |
| else: | |
| speak("I couldn't get the message. Let's try again later.") |
| if query: | ||
| if "open" in query: | ||
| from backend.feature import openCommand | ||
|
|
||
| openCommand(query) | ||
| elif "send message" in query or "call" in query or "video call" in query: | ||
| from backend.feature import findContact, whatsApp | ||
|
|
||
| flag = "" | ||
| Phone, name = findContact(query) | ||
| if Phone != 0: | ||
| if "send message" in query: | ||
| flag = "message" | ||
| speak("What message to send?") | ||
| query = takecommand() # Ask for the message text | ||
| elif "call" in query: | ||
| flag = "call" | ||
| else: | ||
| flag = "video call" | ||
| whatsApp(Phone, query, flag, name) | ||
| elif "on youtube" in query: | ||
| from backend.feature import PlayYoutube | ||
|
|
||
| PlayYoutube(query) | ||
| else: | ||
| from backend.feature import chatBot | ||
|
|
||
| chatBot(query) | ||
| _handle_comm(query) | ||
| else: | ||
| speak("No command was given.") |
There was a problem hiding this comment.
The condition if query: on line 166 has a logical issue. After the changes:
- If
message is Noneandtakecommand()returns None or empty string (after strip), we return early at line 155-157 - If
message is Noneand query is truthy, we reach line 166 with a valid query - If
message is not None, query is set tostr(message).strip().lower()on line 163, which could be an empty string
This means when a text message is passed but it's empty/whitespace, we'll reach line 166 with an empty query string. The check correctly handles this, but the else clause speaking "No command was given." creates inconsistency: voice commands get this message AND return early, while empty text messages get this message AND continue to the finally block. Consider making behavior consistent.
| def _get_microphone() -> sr.Microphone: | ||
| global _microphone | ||
| if _microphone is None: | ||
| _microphone = sr.Microphone() | ||
| return _microphone |
There was a problem hiding this comment.
The microphone singleton lacks thread-safety protection. Unlike the engine and recognizer which use locks, the microphone is created without synchronization. If multiple threads call _get_microphone() simultaneously, race conditions could lead to multiple microphone instances being created, defeating the purpose of the singleton pattern and potentially causing resource issues.
| elif "call" in query and "video call" not in query: | ||
| whatsApp(Phone, query, "call", name) | ||
| else: | ||
| whatsApp(Phone, query, "video call", name) |
There was a problem hiding this comment.
The logic for determining call type has a subtle issue. The condition "call" in query and "video call" not in query (line 138) could fail for queries like "call video call" or similar edge cases where both substrings appear. While unlikely in practice, the order of checks means "call" would match first if "send message" isn't present. Consider restructuring to check for "video call" first (most specific), then "send message", then plain "call" to avoid ambiguity.
backend/command.py
Outdated
| if _recognizer is None: | ||
| r = sr.Recognizer() | ||
| r.pause_threshold = SPEECH_PAUSE_THRESHOLD | ||
| r.dynamic_energy_threshold = False |
There was a problem hiding this comment.
Disabling dynamic energy threshold adjustment (line 52) and caching the calibrated threshold is meant to optimize performance. However, this approach assumes ambient noise levels remain constant throughout the application's lifetime. If the environment becomes noisier or quieter, the fixed threshold may cause the recognizer to either pick up too much background noise or miss valid speech. Consider documenting this trade-off or providing a mechanism to recalibrate when needed (e.g., on user request or periodic intervals).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not query: | ||
| return # Exit if no query is received | ||
| print(query) | ||
| speak("No command was given.") |
There was a problem hiding this comment.
When a voice command is not recognized (takecommand returns None), the function now speaks "No command was given" which is different from the original behavior that silently returned. This could be annoying to users if recognition failures are frequent (e.g., due to background noise). Consider whether this change in user experience is intentional.
| print(f"User said: {query}\n") | ||
| speak(query) | ||
| normalized = query.strip().lower() | ||
| return normalized or None |
There was a problem hiding this comment.
The function returns None when query is empty after stripping (line 113: "return normalized or None"). However, the check at line 151 is "if not query", which would be True for both None and empty string. This means the empty string case won't trigger the "No command was given" message, creating inconsistent behavior. The original code always returned query.lower() from takecommand, never None on empty strings.
| return normalized or None | |
| return normalized |
| if msg: | ||
| whatsApp(Phone, msg, "message", name) | ||
| else: | ||
| speak("I didn't catch the message.") |
There was a problem hiding this comment.
When takecommand is called recursively for "send message" (line 130), if the user's message is not recognized (msg is None), the code speaks "I didn't catch the message" but still returns from the handler without completing the WhatsApp message flow. The original code would have passed the None/failed query to whatsApp(). Verify that this change in behavior is intentional and that the whatsApp function can handle this properly.
| speak("I didn't catch the message.") | |
| speak("I didn't catch the message.") | |
| # Maintain original behavior: still pass the (possibly None) msg to whatsApp | |
| whatsApp(Phone, msg, "message", name) |
| if 0 <= TTS_VOICE_ID < len(voices): | ||
| eng.setProperty("voice", voices[TTS_VOICE_ID].id) |
There was a problem hiding this comment.
The bounds check for TTS_VOICE_ID is helpful, but silently ignoring an invalid voice ID could lead to unexpected voice selection. When the voice ID is out of range, the code skips setting the voice property, meaning the engine will use its default voice. Consider logging a warning or using a fallback voice ID (like 0) to make this behavior more predictable and debuggable.
| def _get_microphone() -> sr.Microphone: | ||
| global _microphone | ||
| if _microphone is None: | ||
| _microphone = sr.Microphone() |
There was a problem hiding this comment.
Trailing whitespace after the microphone initialization. This should be removed for code cleanliness.
| _microphone = sr.Microphone() | |
| _microphone = sr.Microphone() |
| with _engine_lock: | ||
| eng.say(s) | ||
| eng.runAndWait() |
There was a problem hiding this comment.
Holding the engine lock during the entire TTS operation (eng.say() and eng.runAndWait()) blocks all other threads from using TTS, even though the engine might support concurrent access for reads. This could create a bottleneck where multiple speak() calls from different threads are serialized. Consider if a finer-grained locking strategy would be appropriate, or document that TTS operations are intentionally serialized.
| with _engine_lock: | |
| eng.say(s) | |
| eng.runAndWait() | |
| eng.say(s) | |
| eng.runAndWait() |
| if query: | ||
| if "open" in query: | ||
| from backend.feature import openCommand | ||
|
|
||
| openCommand(query) | ||
| elif "send message" in query or "call" in query or "video call" in query: | ||
| from backend.feature import findContact, whatsApp | ||
|
|
||
| flag = "" | ||
| Phone, name = findContact(query) | ||
| if Phone != 0: | ||
| if "send message" in query: | ||
| flag = "message" | ||
| speak("What message to send?") | ||
| query = takecommand() # Ask for the message text | ||
| elif "call" in query: | ||
| flag = "call" | ||
| else: | ||
| flag = "video call" | ||
| whatsApp(Phone, query, flag, name) | ||
| elif "on youtube" in query: | ||
| from backend.feature import PlayYoutube | ||
|
|
||
| PlayYoutube(query) | ||
| else: | ||
| from backend.feature import chatBot | ||
|
|
||
| chatBot(query) | ||
| _handle_comm(query) | ||
| else: | ||
| speak("No command was given.") |
There was a problem hiding this comment.
The condition at line 162 checks "if query" after the try block starts, but query is always truthy at this point. In the voice input path, line 151 returns early if not query, and in the text input path, query is assigned from a message that was provided. This check is redundant and the else block at line 165 is unreachable, making this condition unnecessary.
| r.pause_threshold = SPEECH_PAUSE_THRESHOLD | ||
| r.dynamic_energy_threshold = False | ||
| _recognizer = r | ||
| return _recognizer |
There was a problem hiding this comment.
Missing blank line between function definitions. PEP 8 style guide recommends two blank lines between top-level function definitions for better readability.
| return _recognizer | |
| return _recognizer |
| eel.senderText(query) | ||
| q = str(message).strip() | ||
| print(f"Message received: {q}") | ||
| eel.senderText(q) |
There was a problem hiding this comment.
The text input path doesn't call speak(query) like the voice input path does at line 111. This creates inconsistent behavior where voice commands are echoed back via TTS but text commands are not. If this inconsistency is intentional, it should be documented; otherwise, consider whether text input should also trigger TTS echo.
| eel.senderText(q) | |
| eel.senderText(q) | |
| speak(q) |
| @@ -1,98 +1,170 @@ | |||
| import eel | |||
| import threading | |||
There was a problem hiding this comment.
Import of 'time' is not used.
2 participants
changes