add eknitter ip handling #727
Conversation
Com via ip
update for macos
update for macos
update for macos
change manual macOS for eKnitter
change manual macOS for eKnitter
update manual macOS for eKnitter
update manual macOS for eknitter
WalkthroughThe changes in this pull request introduce several enhancements across multiple modules in the AYAB project. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GuiMain
participant Engine
participant CommunicationIP
User->>GuiMain: Close Application
GuiMain->>Engine: close()
Engine->>CommunicationIP: close_socket()
GuiMain->>User: Application Closed
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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: 16
🧹 Outside diff range and nitpick comments (14)
src/main/python/main/ayab/thread.py (2)
37-40: Add logging and documentation for exception handlingWhile it's correct to catch exceptions in
__del__(as per PEP 361), silently swallowing them makes debugging harder. Consider adding logging and documentation.def __del__(self) -> None: + # Safely wait for thread completion during cleanup + # Exceptions are caught to prevent issues during interpreter shutdown try: self.wait() except Exception: + import logging + logging.debug("Exception while waiting for thread cleanup", exc_info=True) return
Line range hint
43-50: Enhance error handling with proper loggingThe current debug output using
def run(self) -> None: + import logging + logger = logging.getLogger(__name__) try: self.function(*self.args, **self.kwargs) except Exception: - for arg in self.args: - print(arg) - for key, value in self.kwargs.items(): - print(key, value) + logger.error( + "Thread execution failed - Function: %s, Args: %r, Kwargs: %r", + self.function.__name__, + self.args, + self.kwargs, + exc_info=True + ) raisesrc/main/python/main/ayab/engine/udp_monitor.py (3)
24-27: Improve code organization and configurability.Consider the following improvements:
- Use UPPERCASE for global constants:
LOCAL_PORT,BROADCAST.- Move
queueLockinto the class as it's only used there.- Make port numbers configurable through environment variables or configuration file.
-localPort = 12345 -broadcast = ("255.255.255.255", 12345) - -queueLock = threading.Lock() +LOCAL_PORT = int(os.getenv('AYAB_UDP_PORT', 12345)) +BROADCAST = ("255.255.255.255", LOCAL_PORT) + +class UDPMonitor(threading.Thread): + _queue_lock = threading.Lock()
82-88: Follow Python naming conventions and improve logging.
- Rename method to follow Python naming conventions (
get_ip_list)- Replace print with proper logging
-def getIPlist(self): - print("GetList") +def get_ip_list(self): + logging.debug("Retrieving IP list") result = list() if queueLock.acquire(True,0.1): result.extend(self.addresslist) queueLock.release() return result
29-88: Consider using async/await pattern.The current implementation uses threading for handling UDP monitoring. Consider refactoring to use Python's async/await pattern, which could provide better resource utilization and simpler synchronization:
- Replace threading with asyncio
- Use asyncio.DatagramProtocol for UDP handling
- Eliminate manual thread synchronization
Would you like me to provide an example implementation using async/await?
🧰 Tools
🪛 Ruff
58-58: Test for membership should be
not inConvert to
not in(E713)
58-58: Test for membership should be
not inConvert to
not in(E713)
62-62: Do not use bare
except(E722)
src/main/python/main/ayab/engine/engine.py (1)
79-81: Enhance close method robustness.The close method should include error handling and call the parent's close method to ensure proper cleanup.
def close(self) -> None: logging.info("StopUDPMonitor") - udpMonitor.stop() + try: + self.udpMonitor.stop() + except Exception as e: + self.__logger.error(f"Failed to stop UDP monitor: {e}") + super().close()src/main/python/main/ayab/engine/control.py (1)
176-182: Consider further improvements to error handling and code structure.
- The nested if-else structure could be simplified for better readability.
- The code still accesses
msg[5:21]without a length check after the version checks.Consider this alternative implementation:
- if len(msg) < 5: - if len(msg) < 4: - log += ", FW v?" - else: - log += ", FW v" + str(msg[2]) + "." + str(msg[3]) - else: - log += ", FW v" + str(msg[2]) + "." + str(msg[3]) + "." + str(msg[4]) - suffix = msg[5:21] - suffix_null_index = suffix.find(0) - suffix_str = suffix[: suffix_null_index + 1].decode() - if len(suffix_str) > 1: - log += "-" + suffix_str + # Parse firmware version + if len(msg) < 4: + log += ", FW v?" + elif len(msg) < 5: + log += f", FW v{msg[2]}.{msg[3]}" + else: + log += f", FW v{msg[2]}.{msg[3]}.{msg[4]}" + + # Parse suffix if available + if len(msg) > 5: + try: + suffix = msg[5:min(21, len(msg))] + suffix_null_index = suffix.find(0) + if suffix_null_index != -1: + suffix_str = suffix[:suffix_null_index].decode() + if suffix_str: + log += f"-{suffix_str}" + except UnicodeDecodeError: + self.logger.warning("Failed to decode firmware suffix")This refactoring:
- Simplifies the nested conditions
- Uses f-strings for better readability
- Adds bounds checking for suffix parsing
- Adds error handling for decode operations
- Removes redundant length check for suffix string
src/main/python/main/ayab/engine/communication_ip.py (7)
26-26: Remove unused importipaddress.The
ipaddressmodule is imported but not used anywhere in the code. Removing it will clean up the codebase.Apply this diff to remove the unused import:
-import ipaddress🧰 Tools
🪛 Ruff
26-26:
ipaddressimported but unusedRemove unused import:
ipaddress(F401)
49-52: Simplify theis_openmethod by returning the condition directly.You can simplify the method by returning the condition directly without the
if-elsestatement.Apply this diff:
def is_open(self): - if self.__sockTCP is not None: - return True - else: - return False + return self.__sockTCP is not None🧰 Tools
🪛 Ruff
49-52: Return the condition
self.__sockTCP is not NonedirectlyReplace with
return self.__sockTCP is not None(SIM103)
205-205: Rename unused loop variablejto_to indicate it's unused.The loop variable
jis not used within the loop body. Renaming it to_makes it clear that it's intentionally unused.Apply this diff:
def add_crc(crc, data): for i in range(len(data)): n = data[i] - for j in range(8): + for _ in range(8): f = (crc ^ n) & 1 crc >>= 1 if f:🧰 Tools
🪛 Ruff
205-205: Loop control variable
jnot used within loop bodyRename unused
jto_j(B007)
45-47: Avoid using__del__for resource cleanup.The
__del__method is not guaranteed to be called and can lead to unpredictable behavior. Use context managers or explicit cleanup methods instead.Consider removing the
__del__method and ensuring thatclose_socket()is called explicitly when needed.- def __del__(self): - return self.close_socket()
81-85: Simplify socket cleanup by removing redundantdelstatement.Deleting
self.__sockTCPand then setting it toNoneis redundant. Setting it toNoneis sufficient to release the reference.Apply this diff:
self.__sockTCP.close() - del self.__sockTCP self.logger.info("Closing TCP Socket successful.") except Exception as e: self.logger.error("Closing TCP Socket failed: %s", str(e)) self.__sockTCP = None🧰 Tools
🪛 Ruff
82-82: Do not use bare
except(E722)
38-38: Initialize logger outside of the class or per module.Calling
logging.basicConfiginside the__init__method can lead to multiple initializations, especially if multiple instances are created. It's better to configure logging once, preferably at the entry point of the application.Remove
logging.basicConfig(level=logging.DEBUG)from the__init__method.def __init__(self): - logging.basicConfig(level=logging.DEBUG) self.logger = logging.getLogger(type(self).__name__)
73-74: Fix typo in log message and improve error logging.There's a typo in the log message, and it's better to log the exception details.
Apply this diff:
- self.logger.info("Open TCP Socket faild") + self.logger.error("Open TCP Socket failed: %s", str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/main/python/main/ayab/ayab.py(3 hunks)src/main/python/main/ayab/engine/communication_ip.py(1 hunks)src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/udp_monitor.py(1 hunks)src/main/python/main/ayab/thread.py(2 hunks)
🧰 Additional context used
🪛 Ruff
src/main/python/main/ayab/engine/communication_ip.py
23-23: from .communication import * used; unable to detect undefined names
(F403)
26-26: ipaddress imported but unused
Remove unused import: ipaddress
(F401)
35-35: Communication may be undefined, or defined from star imports
(F405)
49-52: Return the condition self.__sockTCP is not None directly
Replace with return self.__sockTCP is not None
(SIM103)
72-72: Do not use bare except
(E722)
82-82: Do not use bare except
(E722)
101-101: Token may be undefined, or defined from star imports
(F405)
104-104: Token may be undefined, or defined from star imports
(F405)
110-110: Token may be undefined, or defined from star imports
(F405)
121-121: Machine may be undefined, or defined from star imports
(F405)
124-124: Token may be undefined, or defined from star imports
(F405)
145-145: Token may be undefined, or defined from star imports
(F405)
161-161: Token may be undefined, or defined from star imports
(F405)
163-163: Token may be undefined, or defined from star imports
(F405)
170-170: Token may be undefined, or defined from star imports
(F405)
205-205: Loop control variable j not used within loop body
Rename unused j to _j
(B007)
src/main/python/main/ayab/engine/udp_monitor.py
58-58: Test for membership should be not in
Convert to not in
(E713)
58-58: Test for membership should be not in
Convert to not in
(E713)
62-62: Do not use bare except
(E722)
🔇 Additional comments (13)
src/main/python/main/ayab/thread.py (2)
20-20: LGTM: Copyright notice properly updated
The copyright notice follows the existing format and includes proper attribution.
Line range hint 37-50: Verify thread cleanup in IP communication context
Given this thread utility is used by the new IP communication features, ensure proper thread cleanup in network scenarios.
✅ Verification successful
Let me try a different search strategy to verify the thread safety concerns.
Let me do one final check to understand the thread cleanup in IP communication context better.
Based on the gathered evidence, I can now provide a final response:
Thread cleanup is properly handled in IP communication context
The code review shows that:
- The GenericThread base class has proper exception handling in both wait() and run() methods
- IP communication module (communication_ip.py) implements proper cleanup with close_socket() method that handles TCP socket closure
- No nested wait() calls that could cause deadlocks were found
- The engine properly stops UDP monitors and control threads during cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for thread usage in IP communication code to verify cleanup
echo "Checking thread usage in IP communication..."
rg -l "GenericThread" | xargs rg "CommunicationIP"
# Look for potential deadlock scenarios in wait() calls
echo "Checking for nested wait() calls that might deadlock..."
ast-grep --pattern 'def $_(self, $$$):
$$$
self.wait()
$$$
self.wait()
$$$'
Length of output: 356
Script:
#!/bin/bash
# Search for all files containing GenericThread class usage
echo "Files containing GenericThread usage:"
rg -l "GenericThread"
# Search for IP communication related files
echo -e "\nFiles related to IP communication:"
rg -l "CommunicationIP|NetworkConnection|socket"
# Look for thread cleanup patterns
echo -e "\nThread cleanup patterns:"
rg "(?:join|stop|cleanup|close)\(\)" --type py -A 2
# Check exception handling in network communication
echo -e "\nException handling in network code:"
rg "try:.*?except.*?finally" -U --multiline --type py -A 3
Length of output: 3293
Script:
#!/bin/bash
# Check GenericThread usage in communication_ip.py
echo "GenericThread usage in IP communication:"
rg -A 10 "GenericThread" src/main/python/main/ayab/engine/communication_ip.py
# Check cleanup methods in communication_ip.py
echo -e "\nCleanup methods in IP communication:"
rg "def.*?(cleanup|close|stop|exit)" src/main/python/main/ayab/engine/communication_ip.py -A 5
# Check thread initialization and error handling
echo -e "\nThread initialization and error handling:"
rg "(?:start|run)\(\)" src/main/python/main/ayab/engine/communication_ip.py -A 5
Length of output: 1187
src/main/python/main/ayab/ayab.py (2)
20-20: LGTM: Copyright notice properly added
The copyright notice follows the existing format and is properly attributed.
62-65: Verify relationship between cleanup changes and IP handling
The changes focus on cleanup/shutdown logic, but the PR objective mentions "add eknitter ip handling". Could you clarify how these cleanup changes relate to the IP handling functionality? This might be important for ensuring proper cleanup of IP connections.
Let's check for IP-related code:
#!/bin/bash
# Search for IP-related code
rg -l "socket|tcp|udp|ip|connect|host|port"
# Search for network cleanup handlers
ast-grep --pattern 'def close($$$) {
$$$
socket.$$$
$$$
}'Also applies to: 142-142
src/main/python/main/ayab/engine/engine.py (1)
Line range hint 20-38: LGTM: Copyright and imports are properly updated.
The copyright notice and new imports for UDP monitoring functionality are correctly added.
src/main/python/main/ayab/engine/engine_fsm.py (3)
20-20: LGTM: Copyright notice properly added
The copyright notice follows the existing format and is correctly dated.
31-31: LGTM: Import statement properly placed
The import follows the existing pattern and is correctly grouped with other communication-related imports.
91-92: Verify integration with CommunicationIP class
Let's verify the integration points and error handling patterns.
✅ Verification successful
Let me generate additional verification scripts to check the implementation of critical methods in CommunicationIP class.
Let me generate one final verification script to check the complete implementation of CommunicationIP class.
Based on the complete verification of the CommunicationIP implementation, I can now provide the final response:
IP communication implementation is properly integrated
The CommunicationIP class correctly:
- Inherits from the base Communication class
- Implements all required methods including
open_serial,close_serial, and message handling - Provides proper TCP socket management with error handling
- Maintains compatibility with the existing communication protocol through API version 6
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check CommunicationIP implementation and usage
# Check CommunicationIP class implementation
echo "Checking CommunicationIP implementation:"
rg -A 10 "class CommunicationIP"
# Check error handling patterns in other communication classes
echo "Checking error handling in other communication classes:"
rg -A 5 "ERROR_|Exception|error" --type py
# Check if open_serial is overridden in CommunicationIP
echo "Checking open_serial implementation in CommunicationIP:"
ast-grep --pattern 'class CommunicationIP {
$$$
open_serial($$$) {
$$$
}
$$$
}'
Length of output: 23227
Script:
#!/bin/bash
# Check CommunicationIP class methods and inheritance
# Check Communication base class implementation
echo "Checking Communication base class:"
rg -A 10 "class Communication"
# Check if CommunicationIP implements required methods
echo "Checking CommunicationIP methods:"
ast-grep --pattern 'class CommunicationIP {
$$$
def open_serial($$$) {
$$$
}
$$$
}'
# Check TCP socket related methods in CommunicationIP
echo "Checking TCP socket methods:"
rg -A 10 "def.*sock" src/main/python/main/ayab/engine/communication_ip.py
# Check message handling in CommunicationIP
echo "Checking message handling:"
rg -A 10 "def.*msg" src/main/python/main/ayab/engine/communication_ip.py
Length of output: 4666
Script:
#!/bin/bash
# Check complete CommunicationIP implementation and its connection handling
# Show the complete CommunicationIP class implementation
echo "Complete CommunicationIP implementation:"
cat src/main/python/main/ayab/engine/communication_ip.py
# Check connection handling in engine_fsm
echo "Checking connection handling in engine_fsm:"
rg -A 10 "def open_serial|def check_serial" src/main/python/main/ayab/engine/communication.py
# Check if there are any tests for IP communication
echo "Checking IP communication tests:"
rg -l "CommunicationIP" src/main/python/main/ayab/tests/
Length of output: 7881
src/main/python/main/ayab/engine/control.py (2)
20-20: LGTM: Copyright notice updated appropriately.
The copyright notice has been properly updated to include the new contributor.
176-182: LGTM: Improved error handling for message parsing.
The added length checks prevent index out of bounds errors when parsing firmware version information.
src/main/python/main/ayab/engine/communication_ip.py (3)
170-170: Ensure safe decoding of messages in parse_API6 method.
Decoding messages without handling potential decoding errors can lead to exceptions.
Confirm that the message can always be decoded safely. If not, consider using a try-except block to handle UnicodeDecodeError.
self.logger.debug("unknown message: ")
- pp.pprint(msg[1: -1].decode())
+ try:
+ decoded_msg = msg[1:-1].decode()
+ except UnicodeDecodeError as e:
+ self.logger.error("Decoding message failed: %s", str(e))
+ decoded_msg = repr(msg[1:-1])
+ pp.pprint(decoded_msg)🧰 Tools
🪛 Ruff
170-170: Token may be undefined, or defined from star imports
(F405)
159-171: Handle unknown tokens gracefully in parse_API6 method.
When an unknown message is received, ensure that it's handled appropriately to avoid potential issues.
Verify that all possible tokens are accounted for and that unexpected messages are logged and managed correctly.
🧰 Tools
🪛 Ruff
161-161: Token may be undefined, or defined from star imports
(F405)
163-163: Token may be undefined, or defined from star imports
(F405)
170-170: Token may be undefined, or defined from star imports
(F405)
198-198:
Ensure command is properly encoded before sending.
When sending the command over the socket, ensure it's encoded to bytes.
Apply this diff:
- self.__sockTCP.send(bytes(cmd))
+ self.__sockTCP.send(cmd.encode('utf-8'))Likely invalid or redundant comment.
src/main/python/main/ayab/ayab.py
Outdated
| def closeEvent(self, event): | ||
| print("close") | ||
| self.engine.close() | ||
| event.accept() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance closeEvent implementation with proper logging and error handling
The closeEvent implementation could be improved in several ways:
- Replace print with proper logging
- Add error handling around engine.close()
- Add docstring explaining the purpose
Here's the suggested implementation:
def closeEvent(self, event):
- print("close")
- self.engine.close()
- event.accept()
+ """Handle the window close event by cleaning up resources.
+
+ Args:
+ event: The close event from Qt
+ """
+ logging.info("Application closing, cleaning up resources...")
+ try:
+ self.engine.close()
+ event.accept()
+ except Exception as e:
+ logging.error(f"Error during cleanup: {e}")
+ event.accept() # Still accept to allow closing📝 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.
| def closeEvent(self, event): | |
| print("close") | |
| self.engine.close() | |
| event.accept() | |
| def closeEvent(self, event): | |
| """Handle the window close event by cleaning up resources. | |
| Args: | |
| event: The close event from Qt | |
| """ | |
| logging.info("Application closing, cleaning up resources...") | |
| try: | |
| self.engine.close() | |
| event.accept() | |
| except Exception as e: | |
| logging.error(f"Error during cleanup: {e}") | |
| event.accept() # Still accept to allow closing |
| if self.__ser: | ||
| self.__ser.write(cmd) | ||
|
|
There was a problem hiding this comment.
Fix undefined attribute self.__ser in write_API6 method.
The attribute self.__ser is not defined in the CommunicationIP class, which will result in an AttributeError.
Apply this diff to use the correct socket attribute:
def write_API6(self, cmd: str) -> None:
# SLIP protocol, no CRC8
- if self.__ser:
- self.__ser.write(cmd)
+ if self.__sockTCP is not None:
+ self.__sockTCP.send(bytes(cmd, encoding='utf-8'))Ensure that the command cmd is encoded appropriately before sending.
📝 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.
| if self.__ser: | |
| self.__ser.write(cmd) | |
| if self.__sockTCP is not None: | |
| self.__sockTCP.send(bytes(cmd, encoding='utf-8')) | |
| This module handles IP communication, currently works in a synchronous way. | ||
| """ | ||
|
|
||
| from .communication import * |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid wildcard imports for clarity and maintainability.
Using from .communication import * can lead to namespace pollution and makes it harder to track dependencies. It's better to import only the necessary entities explicitly.
Apply this diff to replace the wildcard import with explicit imports:
-from .communication import *
+from .communication import Communication, Token, MachineThis change ensures that Communication, Token, and Machine are explicitly imported and helps static analysis tools function correctly.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
23-23: from .communication import * used; unable to detect undefined names
(F403)
Let's try ... @coderabbitai suggest changes to establish Python typing on the changed and added code portions, as typing is already established in the rest of our codebase as well. |
|
Tip For best results, initiate chat on the files or code changes. @dl1com: Certainly! Here are the suggested changes to add Python typing to the changed and added code portions.
from typing import Optional, List
class CommunicationIP(Communication):
def __init__(self) -> None:
# existing initialization code
def __del__(self) -> None:
# existing cleanup code
def is_open(self) -> bool:
# return True if the socket is open, otherwise False
def open_tcp(self, pPortname: Optional[str] = None) -> None:
# code to open TCP connection
def close_socket(self) -> None:
# code to close the socket
def send(self, data: bytes) -> None:
# code to send data
def req_info(self) -> None:
# code to request information
def req_test_API6(self) -> None:
# code for test API6 request
def req_start_API6(
self,
start_needle: int,
stop_needle: int,
continuous_reporting: int,
disable_hardware_beep: int
) -> None:
# code for start API6 request
def req_init_API6(self, machine: Machine) -> None:
# code for init API6 request
def cnf_line_API6(
self,
line_number: int,
color: int,
flags: int,
line_data: bytes
) -> None:
# code to confirm line API6
def update_API6(self) -> None:
# code to update API6
def parse_API6(self, msg: bytes) -> None:
# code to parse API6 message
def read_API6(self) -> bytes:
# code to read API6 data
def write_API6(self, cmd: str) -> None:
# code to write API6 command
def add_crc(crc: int, data: bytes) -> bytes:
# function to add CRC
import threading
from typing import List
class UDPMonitor(threading.Thread):
def __init__(self) -> None:
# initialization code
def run(self) -> None:
# code to run the thread
def stop(self) -> None:
# code to stop the thread
def getIPlist(self) -> List[str]:
# code to get list of IP addresses
def __del__(self) -> None:
# cleanup code
from typing import Optional, List
class Engine(QObject):
# other code
def close(self) -> None:
# code to close the engine
def __populate_ports(self, port_list: Optional[List[str]] = None) -> None:
# code to populate portsBy adding these type annotations, we align the new code with the typing standards already established in the rest of the codebase. ✏️ Learnings added
|
|
@yekomS also, please add some information bout what this PR is for, plus any information which should be added to the manual (i.e. in case there are any compatibility limitations with eKnitter versions or similar) |
|
Wrt IP handling, why a TCP rather than a websocket connection (assuming desktop becomes a webApp ultimately the latter looks like a more natural/generic approach) ? |
dl1com
force-pushed
the
eknitter-ayab-desktop-main
branch
from
39f1d90 to
0a4b204
Compare
|
@yekomS as AYAB v1.0 has been released, we could try to schedule the integration of your PR in one of the next releases. Can you take care of the findings mentioned above? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
src/main/python/main/ayab/engine/engine.py (1)
45-45: 🛠️ Refactor suggestionConsider instance-level UDP monitor initialization.
The UDP monitor is created as a global variable, which could cause issues if multiple Engine instances are created. Move the initialization into the Engine instance.
-udpMonitor = UDPMonitor() class Engine(SignalSender, QDockWidget): def __init__(self, parent: GuiMain): super().__init__(parent.signal_receiver) + self.udpMonitor = UDPMonitor()src/main/python/main/ayab/engine/udp_monitor.py (3)
42-47: 🛠️ Refactor suggestionImprove error handling in destructor.
The current implementation silently ignores all exceptions. Add proper error logging.
def __del__(self): try: self.stop() - except Exception: - return + except Exception as e: + import logging + logging.error(f"Error stopping UDP monitor: {e}")
75-81: 🛠️ Refactor suggestionImprove stop method implementation.
Replace print with proper logging and implement a more robust stopping mechanism.
def stop(self): - print("Stoping") + import logging + logging.info("Stopping UDP monitor") queueLock.acquire(1) self.exitFlag = True queueLock.release() - sleep(1) + # Wait for the thread to finish with a timeout + self.join(timeout=2.0)
48-74:⚠️ Potential issueFix multiple issues in the run method.
There are several issues in the run method that need to be addressed:
- Replace print statements with proper logging
- Fix bare except clause
- Use Python's "not in" syntax instead of "not(x in y)"
- Use context manager (with) for lock operations
-def run(self): - print("Starting ") - self.__sockUDP.bind((self.__ip_address ,localPort)) - - Run = True - while Run: - try: - dataadress = self.__sockUDP.recvfrom(250) - print("Recive") - adress = dataadress[1][0] - if not(adress in self.__ip_addresses[2]) and not(adress in self.addresslist): - queueLock.acquire(1) - self.addresslist.append(adress) - queueLock.release() - except: - sleep(1) - queueLock.acquire(1) - Run = not self.exitFlag - queueLock.release() - # print("RUN = ",Run) - - print("Exiting") - self.__sockUDP.close() - del self.__sockUDP - self.__sockUDP = None - return +def run(self) -> None: + import logging + logging.info("Starting UDP monitor") + self.__sockUDP.bind((self.__ip_address, localPort)) + + run_monitor = True + while run_monitor: + try: + data_address = self.__sockUDP.recvfrom(250) + logging.debug("Received UDP broadcast") + address = data_address[1][0] + if address not in self.__ip_addresses[2] and address not in self.addresslist: + with queueLock: + self.addresslist.append(address) + except socket.timeout: + # Expected timeout + pass + except Exception as e: + logging.error(f"Error in UDP monitor: {e}") + sleep(1) + + with queueLock: + run_monitor = not self.exitFlag + + logging.info("UDP monitor exiting") + self.__sockUDP.close() + del self.__sockUDP + self.__sockUDP = None🧰 Tools
🪛 Ruff (0.8.2)
58-58: Test for membership should be
not inConvert to
not in(E713)
58-58: Test for membership should be
not inConvert to
not in(E713)
62-62: Do not use bare
except(E722)
src/main/python/main/ayab/engine/communication_ip.py (6)
61-74:⚠️ Potential issueSpecify exception type in
exceptblock and correct typo in log message.Using a bare
except:is not recommended as it catches all exceptions, including system-exiting ones.- def open_tcp(self, pPortname=None): + def open_tcp(self, pPortname: Optional[str] = None) -> bool: try: self.__portname = pPortname self.__tarAddressPort = (self.__portname, remotePort) self.__sockTCP = socket.socket(family=socket.AF_INET, type=socket.SOCK_STREAM) self.__sockTCP.settimeout(10.0) self.__sockTCP.connect(self.__tarAddressPort) self.__sockTCP.settimeout(0.0) self.__sockTCP.setblocking(False) self.logger.info("Open TCP Socket successful") return True - except: - self.logger.info("Open TCP Socket faild") + except Exception as e: + self.logger.error(f"Open TCP Socket failed: {e}") return False🧰 Tools
🪛 Ruff (0.8.2)
72-72: Do not use bare
except(E722)
76-85: 🛠️ Refactor suggestionSpecify exception type in
exceptblock for clearer error handling.Catching all exceptions can mask errors. Specify the exception type to handle expected errors appropriately.
- def close_socket(self): + def close_socket(self) -> None: if self.__sockTCP is not None: try: self.__sockTCP.close() del self.__sockTCP self.logger.info("Closing TCP Socket successful.") - except: - self.logger.warning("Closing TCP Socket failed. (mem Leak?)") + except Exception as e: + self.logger.error(f"Closing TCP Socket failed: {e}") self.__sockTCP = None🧰 Tools
🪛 Ruff (0.8.2)
82-82: Do not use bare
except(E722)
86-98: 🛠️ Refactor suggestionImprove exception handling in send method.
The exception handling should use
str(e)instead of checking fore.messageattribute.- def send(self, data): + def send(self, data: bytearray) -> None: if self.__sockTCP is not None: try: self.__sockTCP.send(bytes(data)) # self.logger.info("SEND b'"+data+"'") sleep(0.5) except Exception as e: - if hasattr(e, 'message'): - self.logger.exception(e.message) - else: - self.logger.exception("Connection...Error") + self.logger.exception(f"Connection error: {str(e)}") self.close_socket()
172-193:⚠️ Potential issueUse correct exception handling when logging exceptions.
Update the exception handling to use
str(e)instead of checking fore.message.- def read_API6(self): + def read_API6(self) -> Optional[bytes]: """Read data from serial as SLIP packet.""" if self.__sockTCP is not None: try: data = self.__sockTCP.recv(1024) except BlockingIOError: data = bytes() except Exception as e: - if hasattr(e, 'message'): - self.logger.exception(e.message) - else: - self.logger.exception("Connection...Error") + self.logger.exception(f"Connection error: {str(e)}") self.close_socket() self.open_tcp(self.__portname) data = bytes()
195-199:⚠️ Potential issueFix undefined attribute
self.__serinwrite_API6method.The attribute
self.__seris not defined in theCommunicationIPclass, which will result in anAttributeError.def write_API6(self, cmd: str) -> None: # SLIP protocol, no CRC8 - if self.__ser: - self.__ser.write(cmd) + if self.__sockTCP is not None: + self.__sockTCP.send(bytes(cmd, encoding='utf-8'))
23-23: 🛠️ Refactor suggestionAvoid wildcard imports for clarity and maintainability.
Wildcard imports make it difficult to track dependencies and can lead to namespace pollution.
-from .communication import * +from .communication import Communication, Token, Machine🧰 Tools
🪛 Ruff (0.8.2)
23-23:
from .communication import *used; unable to detect undefined names(F403)
🧹 Nitpick comments (10)
src/main/python/main/ayab/engine/engine.py (2)
61-62: Add error handling for UDP monitor start.Starting the UDP monitor could fail for various reasons (e.g., port already in use). Add appropriate error handling.
logging.info("StartUDPMonitor") -udpMonitor.start() +try: + udpMonitor.start() +except Exception as e: + self.__logger.error(f"Failed to start UDP monitor: {e}")
191-192: Fix indentation with spaces instead of tabs.The Python style guide (PEP 8) recommends using spaces for indentation rather than tabs.
- if output.name != "CONNECTING_TO_MACHINE": - self.control.notification = output + if output.name != "CONNECTING_TO_MACHINE": + self.control.notification = outputsrc/main/python/main/ayab/engine/udp_monitor.py (2)
29-41: Improve class documentation and initialization.The class needs proper documentation to explain its purpose, and the initialization could be improved with better variable naming and type hints.
class UDPMonitor(threading.Thread): + """ + Monitor UDP broadcasts from AYAB-compatible devices. + + This class listens for UDP broadcasts on a specified port and maintains + a list of unique IP addresses from which it receives data. + """ def __init__(self) -> None: threading.Thread.__init__(self) self.exitFlag = False self.__sockUDP = socket.socket(family=socket.AF_INET, type=socket.SOCK_DGRAM) self.__sockUDP.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) self.__sockUDP.settimeout(0.1) self.__sockUDP.setblocking(False) - # self.__hostname = socket.gethostname() self.__ip_addresses = socket.gethostbyname_ex('localhost') self.__ip_address = "0.0.0.0" - self.addresslist = list() + self.addresslist: List[str] = []
82-88: Improve getIPlist method with type hints and logging.Use proper type hints for the return value and replace print with logging.
-def getIPlist(self): - print("GetList") - result = list() +def getIPlist(self) -> List[str]: + import logging + logging.debug("Getting IP address list") + result: List[str] = [] if queueLock.acquire(True,0.1): result.extend(self.addresslist) queueLock.release() return resultsrc/main/python/main/ayab/engine/communication_ip.py (6)
25-30: Optimize imports.The
ipaddressmodule is imported but never used in the code.from .communication import * import socket -import ipaddress import logging import pprint from time import sleep🧰 Tools
🪛 Ruff (0.8.2)
26-26:
ipaddressimported but unusedRemove unused import:
ipaddress(F401)
45-53: Simplify boolean return and add type hints.The
is_openmethod can be simplified and type hints should be added to methods.- def __del__(self): + def __del__(self) -> None: return self.close_socket() - def is_open(self): - if self.__sockTCP is not None: - return True - else: - return False + def is_open(self) -> bool: + return self.__sockTCP is not None🧰 Tools
🪛 Ruff (0.8.2)
49-52: Return the condition
self.__sockTCP is not NonedirectlyReplace with
return self.__sockTCP is not None(SIM103)
54-59: Replace print with logging and add type hints.Replace print statements with proper logging and add type hints to methods.
- def open_serial(self, portname=None): - print("open: " , portname) + def open_serial(self, portname: Optional[str] = None) -> bool: + self.logger.info(f"Opening serial port: {portname}") return self.open_tcp(portname) - def close_serial(self): + def close_serial(self) -> bool: return True
99-171: Add type hints to method signatures.Add proper type hints to methods to match the project's standards.
# NB this method must be the same for all API versions - def req_info(self): + def req_info(self) -> None: self.send([Token.reqInfo.value,self.version]) - def req_test_API6(self): + def req_test_API6(self) -> None: self.send([Token.reqTest.value]) def req_start_API6(self, start_needle, stop_needle, - continuous_reporting, disable_hardware_beep): + continuous_reporting: bool, disable_hardware_beep: bool) -> None: """Send a start message to the device.""" data = bytearray() data.append(Token.reqStart.value) data.append(start_needle) data.append(stop_needle) data.append( 1 * continuous_reporting + 2 * (not disable_hardware_beep)) hash = 0 hash = add_crc(hash, data) data.append(hash) data = self.send(data) - def req_init_API6(self, machine: Machine): + def req_init_API6(self, machine: Machine) -> None: """Send a start message to the device.""" data = bytearray() data.append(Token.reqInit.value) data.append(machine.value) hash = 0 hash = add_crc(hash, data) data.append(hash) data = self.send(data) - def cnf_line_API6(self, line_number, color, flags, line_data): + def cnf_line_API6(self, line_number: int, color: int, flags: int, line_data: bytes) -> None: """Send a line of data via the serial port. Send a line of data to the serial port. All arguments are mandatory. The data sent here is parsed by the Arduino controller which sets the knitting needles accordingly. Args: line_number (int): The line number to be sent. color (int): The yarn color to be sent. flags (int): The flags sent to the controller. line_data (bytes): The bytearray to be sent to needles. """ data = bytearray() data.append(Token.cnfLine.value) data.append(line_number) data.append(color) data.append(flags) data.extend(line_data) hash = 0 hash = add_crc(hash, data) data.append(hash) data = self.send(data) - def update_API6(self): + def update_API6(self) -> tuple: """Read data from serial and parse as SLIP packet.""" return self.parse_API6(self.read_API6()) - def parse_API6(self, msg): + def parse_API6(self, msg: Optional[bytes]) -> tuple: if msg is None: return None, Token.none, 0 # else for t in list(Token): if msg[0] == t.value: return msg, t, msg[1] # fallthrough self.logger.debug("unknown message: ") # drop crlf pp = pprint.PrettyPrinter(indent=4) pp.pprint(msg[1: -1].decode()) return msg, Token.unknown, 0🧰 Tools
🪛 Ruff (0.8.2)
101-101:
Tokenmay be undefined, or defined from star imports(F405)
104-104:
Tokenmay be undefined, or defined from star imports(F405)
110-110:
Tokenmay be undefined, or defined from star imports(F405)
121-121:
Machinemay be undefined, or defined from star imports(F405)
124-124:
Tokenmay be undefined, or defined from star imports(F405)
145-145:
Tokenmay be undefined, or defined from star imports(F405)
161-161:
Tokenmay be undefined, or defined from star imports(F405)
163-163:
Tokenmay be undefined, or defined from star imports(F405)
170-170:
Tokenmay be undefined, or defined from star imports(F405)
201-212: Add type hints toadd_crcfunction.Add proper type hints to the function to match the project's standards.
# CRC algorithm after Maxim/Dallas -def add_crc(crc, data): +def add_crc(crc: int, data: bytearray) -> int: for i in range(len(data)): n = data[i] for j in range(8): f = (crc ^ n) & 1 crc >>= 1 if f: crc ^= 0x8C n >>= 1 return crc & 0xFF🧰 Tools
🪛 Ruff (0.8.2)
205-205: Loop control variable
jnot used within loop bodyRename unused
jto_j(B007)
214-215: Add docstring to CommunicationException class.Provide documentation explaining when this exception is raised.
class CommunicationException(Exception): - pass + """Exception raised for errors in the communication with the device.""" + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/python/main/ayab/ayab.py(3 hunks)src/main/python/main/ayab/engine/communication_ip.py(1 hunks)src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/udp_monitor.py(1 hunks)src/main/python/main/ayab/thread.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/python/main/ayab/ayab.py
- src/main/python/main/ayab/engine/engine_fsm.py
- src/main/python/main/ayab/engine/control.py
- src/main/python/main/ayab/thread.py
🧰 Additional context used
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/udp_monitor.py
58-58: Test for membership should be not in
Convert to not in
(E713)
58-58: Test for membership should be not in
Convert to not in
(E713)
62-62: Do not use bare except
(E722)
src/main/python/main/ayab/engine/communication_ip.py
23-23: from .communication import * used; unable to detect undefined names
(F403)
26-26: ipaddress imported but unused
Remove unused import: ipaddress
(F401)
35-35: Communication may be undefined, or defined from star imports
(F405)
49-52: Return the condition self.__sockTCP is not None directly
Replace with return self.__sockTCP is not None
(SIM103)
72-72: Do not use bare except
(E722)
82-82: Do not use bare except
(E722)
101-101: Token may be undefined, or defined from star imports
(F405)
104-104: Token may be undefined, or defined from star imports
(F405)
110-110: Token may be undefined, or defined from star imports
(F405)
121-121: Machine may be undefined, or defined from star imports
(F405)
124-124: Token may be undefined, or defined from star imports
(F405)
145-145: Token may be undefined, or defined from star imports
(F405)
161-161: Token may be undefined, or defined from star imports
(F405)
163-163: Token may be undefined, or defined from star imports
(F405)
170-170: Token may be undefined, or defined from star imports
(F405)
205-205: Loop control variable j not used within loop body
Rename unused j to _j
(B007)
🔇 Additional comments (5)
src/main/python/main/ayab/engine/engine.py (4)
20-20: Great addition of copyright information.Thanks for adding your copyright information to the file header. This properly attributes your contributions to the eKnitter functionality.
37-37: Good module import.The import for UDPMonitor is properly placed with the other imports. This allows the file to access the UDP monitoring functionality.
78-80: Good cleanup implementation in close method.Adding the close method ensures proper cleanup of the UDP monitor when the engine is closed, which prevents resource leaks.
122-126: Replace print statement with logging and improve IP display.Print statements should be replaced with proper logging, and the handling of IP addresses could be improved with validation.
- ip_list = udpMonitor.getIPlist() - print(ip_list) - for item in ip_list: - # TODO: should display the info of the device. - combo_box.addItem(item) + ip_list = udpMonitor.getIPlist() + self.__logger.debug(f"Retrieved IP list: {ip_list}") + for ip in ip_list: + # TODO: should display the info of the device. + combo_box.addItem(ip)src/main/python/main/ayab/engine/communication_ip.py (1)
17-21: Good documentation for the module.The module header and docstring provide clear information about the purpose of this module.
…S/ayab-desktop into eknitter-ayab-desktop-main
…tter-ayab-desktop-main
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (12)
src/main/python/main/ayab/engine/engine.py (2)
45-45: 🛠️ Refactor suggestionConsider instance-level UDP monitor initialization.
The UDP monitor is currently initialized as a global variable, which could cause issues if multiple Engine instances are created. Consider moving the initialization into the instance.
-udpMonitor = UDPMonitor() class Engine(SignalSender, QDockWidget): def __init__(self, parent: GuiMain): super().__init__(parent.signal_receiver) + self.udpMonitor = UDPMonitor() logging.info("StartUDPMonitor") - udpMonitor.start() + try: + self.udpMonitor.start() + except Exception as e: + self.__logger.error(f"Failed to start UDP monitor: {e}")
123-127: 🛠️ Refactor suggestionEnhance IP address handling and remove debug print.
The IP address handling could be improved with validation, and the debug print statement should be replaced with proper logging.
- ip_list = udpMonitor.getIPlist() - print(ip_list) - for item in ip_list: - # TODO: should display the info of the device. - combo_box.addItem(item) + ip_list = self.udpMonitor.getIPlist() + self.__logger.debug(f"Retrieved IP list: {ip_list}") + for ip in ip_list: + combo_box.addItem(ip) # TODO: display device infosrc/main/python/main/ayab/engine/udp_monitor.py (5)
19-28: 🛠️ Refactor suggestionAdd proper type hints to match project standards.
According to the PR objectives, Python typing should be added to match existing standards in the project.
import threading import socket from time import sleep +from typing import List, Optional, Tuple, Any # Port for UDP -localPort = 12345 -broadcast = ("255.255.255.255", 12345) +localPort: int = 12345 +broadcast: Tuple[str, int] = ("255.255.255.255", 12345) -queueLock = threading.Lock() +queueLock: threading.Lock = threading.Lock()
42-47: 🛠️ Refactor suggestionImprove error handling in destructor.
The current implementation silently swallows all exceptions, which may hide important errors.
def __del__(self) -> None: try: self.stop() - except Exception: - return + except Exception as e: + import logging + logging.error(f"Error stopping UDP monitor during cleanup: {e}")
75-81: 🛠️ Refactor suggestionImprove stop method implementation.
Replace print with proper logging and consider a more robust stopping mechanism.
def stop(self) -> None: - print("Stoping") + logging.info("Stopping UDP monitor") - queueLock.acquire(True) - self.exitFlag = True - queueLock.release() - sleep(1) + with queueLock: + self.exitFlag = True + # Wait for the thread to finish with a timeout + self.join(timeout=2.0)
29-41: 🛠️ Refactor suggestionAdd documentation and improve class initialization.
The class lacks a proper docstring and several initialization issues need to be addressed.
class UDPMonitor(threading.Thread): + """ + Monitor UDP broadcasts from eKnitter devices on the network. + + This class creates a thread that listens for UDP broadcasts and maintains + a thread-safe list of unique IP addresses from which it receives data. + """ def __init__(self) -> None: - threading.Thread.__init__(self) + super().__init__(name="UDPMonitor") self.exitFlag = False self.__sockUDP: socket.socket = socket.socket(family=socket.AF_INET, type=socket.SOCK_DGRAM) self.__sockUDP.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1) self.__sockUDP.settimeout(0.1) self.__sockUDP.setblocking(False) - # self.__hostname = socket.gethostname() self.__ip_addresses = socket.gethostbyname_ex('localhost') self.__ip_address = "0.0.0.0" - self.addresslist: list[socket._RetAddress] = list() + self.addresslist: List[str] = []
48-74:⚠️ Potential issueCritical issues in run method need attention.
Several issues need to be addressed in the run method:
- Replace print statements with proper logging
- Fix bare except clause
- Improve thread synchronization
- Fix non-pythonic membership test
+import logging + def run(self) -> None: - print("Starting ") + logging.info("Starting UDP monitor") self.__sockUDP.bind((self.__ip_address ,localPort)) Run = True while Run: try: dataadress = self.__sockUDP.recvfrom(250) - print("Recive") + logging.debug("Received UDP broadcast") adress = dataadress[1][0] - if not(adress in self.__ip_addresses[2]) and not(adress in self.addresslist): - queueLock.acquire(True) - self.addresslist.append(adress) - queueLock.release() + if adress not in self.__ip_addresses[2] and adress not in self.addresslist: + with queueLock: + self.addresslist.append(adress) - except: + except socket.timeout: + # Expected timeout, continue polling + sleep(1) + except Exception as e: + logging.error(f"Error in UDP monitor: {e}") sleep(1) - queueLock.acquire(True) - Run = not self.exitFlag - queueLock.release() - # print("RUN = ",Run) + with queueLock: + Run = not self.exitFlag - print("Exiting") + logging.info("UDP monitor exiting") self.__sockUDP.close() del self.__sockUDP self.__sockUDP = socket.socket() - return🧰 Tools
🪛 Ruff (0.8.2)
58-58: Test for membership should be
not inConvert to
not in(E713)
58-58: Test for membership should be
not inConvert to
not in(E713)
62-62: Do not use bare
except(E722)
src/main/python/main/ayab/engine/communication_ip.py (5)
23-24: **Avoid wildcard imports **A previous review comment already flagged the use of wildcard imports in the project. This approach can lead to namespace pollution and difficulties in tracing dependencies. Please switch to explicit imports for both lines.
-from .communication import * -from ..machine import * +from .communication import Communication, Token, add_crc, CommunicationException +from ..machine import Machine🧰 Tools
🪛 Ruff (0.8.2)
23-23:
from .communication import *used; unable to detect undefined names(F403)
24-24:
from ..machine import *used; unable to detect undefined names(F403)
72-72: **Specify exception type instead of using bareexcept**This comment mirrors a previous review. Using
except:catches all exceptions including system-exiting ones. Restricting to an explicit exception type is more robust.- except: - self.logger.info("Open TCP Socket faild") + except Exception as e: + self.logger.error("Open TCP Socket failed: %s", str(e))🧰 Tools
🪛 Ruff (0.8.2)
72-72: Do not use bare
except(E722)
82-82: **Specify exception type instead of using bareexcept**This repeats a previous review comment. Catching all exceptions can mask issues. Please specify an explicit exception type.
- except: - self.logger.warning("Closing TCP Socket failed. (mem Leak?)") + except Exception as e: + self.logger.warning("Closing TCP Socket failed: %s", str(e))🧰 Tools
🪛 Ruff (0.8.2)
82-82: Do not use bare
except(E722)
93-96: **Usestr(e)instead ofe.message**A prior review noted that
e.messageis deprecated in Python 3. Usestr(e)to retrieve exception details for consistent logging.- if hasattr(e, 'message'): - self.logger.exception(e.message) - else: - self.logger.exception("Connection...Error") + self.logger.exception("Connection error: %s", str(e))
197-199:⚠️ Potential issue
self.__seris never defined in this classReferencing
self.__serhere can cause anAttributeErrorif the parent constructor isn't called. SinceCommunicationIPrelies on sockets instead of a serial device, consider removing or refactoring this block.- def write_API6(self, cmd: bytes | bytearray) -> None: - # SLIP protocol, no CRC8 - if self.__ser: - self.__ser.write(bytes(cmd)) + def write_API6(self, cmd: bytes | bytearray) -> None: + # This method references self.__ser from the parent class, + # but that may never be initialized. Refactor or remove as needed. + pass # or implement socket-based write if relevant
🧹 Nitpick comments (7)
src/main/python/main/ayab/menu.py (1)
48-48: Type ignore comment added for setupUi.The added type ignore comment is consistent with the PR's objective to incorporate Python typing into the codebase. This approach is commonly used when interfacing with UI code generated by tools like QtDesigner.
For long-term maintenance, consider creating proper type stubs for the UI classes rather than using type ignores. However, this is a pragmatic solution for the immediate needs of the project.
windows-build/build.bat (1)
1-7: Add error handling to build scriptThe script looks functional but lacks error checking and validation.
Consider enhancing with:
+@echo off set VERSION=%1 + +if "%VERSION%"=="" ( + echo Error: Version number not provided. + echo Usage: build.bat [version_number] + exit /b 1 +) python -m fbs freeze +if %ERRORLEVEL% neq 0 ( + echo Error: Failed to freeze the application. + exit /b %ERRORLEVEL% +) python -m fbs installer +if %ERRORLEVEL% neq 0 ( + echo Error: Failed to create the installer. + exit /b %ERRORLEVEL% +) +if not exist .\target\AYABSetup.exe ( + echo Error: Installer file not found. + exit /b 1 +) + xcopy /Y .\target\AYABSetup.exe .\target\AYAB-eKnitter-v%VERSION%-Setup.exe +if %ERRORLEVEL% neq 0 ( + echo Error: Failed to copy installer file. + exit /b %ERRORLEVEL% +) + +echo Build completed successfully. Installer created at .\target\AYAB-eKnitter-v%VERSION%-Setup.exesrc/main/python/main/ayab/scene.py (1)
95-95: Consider adding type annotation for rect_heightFor consistency with the typing standards mentioned in the PR objectives, consider adding a type annotation for the rect_height variable.
-rect_height = 7 +rect_height: int = 7src/main/python/main/ayab/engine/engine.py (1)
192-193: Fix indentation and maintain notification handling logic.The code uses tab indentation, which should be changed to spaces to adhere to Python style guidelines. The conditional logic is correct.
- if output.name != "CONNECTING_TO_MACHINE": - self.control.notification = output + if output.name != "CONNECTING_TO_MACHINE": + self.control.notification = outputsrc/main/python/main/ayab/engine/communication_ip.py (3)
49-52: Simplify theis_openmethod return statementInstead of an if-else block, you can directly return the condition for more concise code.
-def is_open(self) -> bool: - if self.__sockTCP is not None: - return True - else: - return False +def is_open(self) -> bool: + return self.__sockTCP is not None🧰 Tools
🪛 Ruff (0.8.2)
49-52: Return the condition
self.__sockTCP is not NonedirectlyReplace with
return self.__sockTCP is not None(SIM103)
73-74: Correct the typo in the log messageThere's a small misspelling: “faild” → “failed”.
- self.logger.info("Open TCP Socket faild") + self.logger.info("Open TCP Socket failed")
122-122: Update the docstring to reflect "init" rather than "start"The docstring says "Send a start message" whereas this method is clearly for initialization. Consider rewording for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
src/main/NSIS/ShellExecAsUser.dllis excluded by!**/*.dllsrc/main/icons/Icon.icois excluded by!**/*.icosrc/main/python/main/ayab/engine/eKnitter-reversed.pngis excluded by!**/*.pngsrc/main/python/main/ayab/engine/eKnitter.pngis excluded by!**/*.pngsrc/main/resources/base/ayab/firmware/esp32/win64/esptool.exeis excluded by!**/*.exe
📒 Files selected for processing (25)
CONTRIBUTING.md(1 hunks)README.md(1 hunks)requirements.txt(1 hunks)setup-environment.ps1(1 hunks)src/main/NSIS/Installer.nsi(1 hunks)src/main/python/main/ayab/about.py(1 hunks)src/main/python/main/ayab/about_gui.ui(2 hunks)src/main/python/main/ayab/ayab.py(5 hunks)src/main/python/main/ayab/engine/communication_ip.py(1 hunks)src/main/python/main/ayab/engine/eKnitter_rc.qrc(1 hunks)src/main/python/main/ayab/engine/eKnitter_reversed_rc.qrc(1 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/options.py(5 hunks)src/main/python/main/ayab/engine/options_gui.ui(1 hunks)src/main/python/main/ayab/engine/status.py(1 hunks)src/main/python/main/ayab/engine/udp_monitor.py(1 hunks)src/main/python/main/ayab/firmware_flash.py(3 hunks)src/main/python/main/ayab/firmware_flash_gui.ui(1 hunks)src/main/python/main/ayab/main_gui.ui(1 hunks)src/main/python/main/ayab/menu.py(1 hunks)src/main/python/main/ayab/preferences.py(1 hunks)src/main/python/main/ayab/scene.py(2 hunks)src/main/python/main/ayab/transforms.py(1 hunks)src/main/resources/base/ayab/firmware/firmware.json(1 hunks)windows-build/build.bat(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/main/python/main/ayab/firmware_flash_gui.ui
- src/main/python/main/ayab/main_gui.ui
- src/main/python/main/ayab/preferences.py
- src/main/python/main/ayab/about.py
- src/main/python/main/ayab/about_gui.ui
- src/main/python/main/ayab/engine/eKnitter_reversed_rc.qrc
- src/main/python/main/ayab/engine/eKnitter_rc.qrc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/python/main/ayab/ayab.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/python/main/ayab/engine/engine.py (2)
src/main/python/main/ayab/engine/udp_monitor.py (3)
UDPMonitor(29-88)stop(75-80)getIPlist(82-88)src/main/python/main/ayab/machine.py (1)
Machine(26-51)
src/main/python/main/ayab/engine/communication_ip.py (2)
src/main/python/main/ayab/engine/communication.py (3)
Communication(68-249)Token(38-65)add_crc(253-262)src/main/python/main/ayab/machine.py (1)
Machine(26-51)
src/main/python/main/ayab/engine/udp_monitor.py (2)
src/main/python/main/ayab/engine/control.py (1)
stop(120-124)src/main/python/main/ayab/engine/engine.py (1)
close(78-81)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
28-28: No empty links
null
(MD042, no-empty-links)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/options.py
68-68: Found useless expression. Either assign it to a variable or remove it.
(B018)
69-69: Found useless expression. Either assign it to a variable or remove it.
(B018)
src/main/python/main/ayab/engine/communication_ip.py
23-23: from .communication import * used; unable to detect undefined names
(F403)
24-24: from ..machine import * used; unable to detect undefined names
(F403)
35-35: Communication may be undefined, or defined from star imports
(F405)
49-52: Return the condition self.__sockTCP is not None directly
Replace with return self.__sockTCP is not None
(SIM103)
72-72: Do not use bare except
(E722)
82-82: Do not use bare except
(E722)
101-101: Token may be undefined, or defined from star imports
(F405)
104-104: Token may be undefined, or defined from star imports
(F405)
110-110: Token may be undefined, or defined from star imports
(F405)
117-117: add_crc may be undefined, or defined from star imports
(F405)
121-121: Machine may be undefined, or defined from star imports
(F405)
124-124: Token may be undefined, or defined from star imports
(F405)
127-127: add_crc may be undefined, or defined from star imports
(F405)
145-145: Token may be undefined, or defined from star imports
(F405)
151-151: add_crc may be undefined, or defined from star imports
(F405)
155-155: Token may be undefined, or defined from star imports
(F405)
159-159: Token may be undefined, or defined from star imports
(F405)
161-161: Token may be undefined, or defined from star imports
(F405)
163-163: Token may be undefined, or defined from star imports
(F405)
170-170: Token may be undefined, or defined from star imports
(F405)
src/main/python/main/ayab/engine/udp_monitor.py
58-58: Test for membership should be not in
Convert to not in
(E713)
58-58: Test for membership should be not in
Convert to not in
(E713)
62-62: Do not use bare except
(E722)
🔇 Additional comments (19)
requirements.txt (1)
9-9: Added esptool dependency for eKnitter support.The addition of
esptool==3.3.3is appropriate for supporting the eKnitter device integration. This package is used for communicating with ESP32/ESP8266 microcontrollers, which suggests the eKnitter is built on ESP hardware.src/main/python/main/ayab/engine/options_gui.ui (1)
250-250: Increased width for eKnitter icon.The width increase from 59 to 200 pixels for the auto_mirror_icon widget accommodates the new eKnitter graphics needed for the IP handling functionality.
Please verify that the new width of 200 pixels is appropriate for the eKnitter icon and doesn't cause any layout issues in the UI. If possible, consider testing this on different screen resolutions.
src/main/resources/base/ayab/firmware/firmware.json (1)
9-21: LGTM: eKnitter controller section added correctlyThe changes properly add a new section for the "eknitter" controller with two firmware entries, maintaining the existing JSON structure.
README.md (2)
89-89: LGTM: esptool installation added for macOSThe addition of esptool installation via Homebrew is appropriate for supporting eKnitter functionality.
96-100: LGTM: Added eKnitter-specific repository cloning instructionsThe instructions for cloning the repository with the eKnitter branch are clear and well-formatted.
setup-environment.ps1 (1)
38-39: LGTM: Added eKnitter resource compilation commandsThe new commands for generating PySide6 resource files follow the existing pattern and are correctly structured.
src/main/python/main/ayab/scene.py (5)
92-94: LGTM: Add consistent pen stylingA consistent pen style with appropriate width is now applied to all rectangle elements.
98-102: LGTM: Improved orange rectangle stylingThe rectangle position, dimensions, and color have been refined for better visual appearance.
104-106: LGTM: Improved green rectangle stylingThe rectangle styling has been consistently updated with specific hex color and proper pen application.
139-146: LGTM: Applied consistent styling to start needle indicatorThe start needle visualization now uses the consistent pen style defined earlier.
147-154: LGTM: Applied consistent styling to stop needle indicatorThe stop needle visualization now uses the consistent pen style defined earlier.
src/main/python/main/ayab/engine/options.py (3)
33-34: Good job renaming the resource identifiers to better reflect the eKnitter device.The rename from
lowercase_e_rctoeKnitter_rcmakes the code more descriptive and aligned with the device naming.
129-133: Resource path updates look good.The update of pixmap paths from "garamond-lowercase-e" to "eKnitter" aligns with the resource name changes and provides consistent naming.
183-188: Resource path updates consistent with earlier changes.The pixmap path updates in the refresh method match those in the
__auto_mirror_changedmethod, ensuring consistency.src/main/python/main/ayab/firmware_flash.py (2)
62-62: Type ignore comment added correctly.The added type ignore comment prevents type checking errors with the setupUi method, which is a common pattern when working with UI code generated by tools like Qt Designer.
158-165: Good conditional logic for handling eKnitter controllers.The code now correctly differentiates between eKnitter and other controller types, calling the appropriate command generation method.
src/main/python/main/ayab/engine/engine.py (3)
37-37: Good import for new UDPMonitor functionality.The import statement is well-placed and follows the existing import structure of the file.
61-64: Added logging and UDP monitor initialization.Good use of logging to indicate the start of the UDP monitor. The type ignore comment is correctly added to the setupUi call.
78-81: Clean implementation of resource cleanup.The
closemethod correctly stops the UDP monitor and logs the action. This is important for proper resource management.
| @@ -25,7 +25,7 @@ Pull requests are the best way to propose changes to the codebase (we use [Githu | |||
| In short, when you submit code changes, your submissions are understood to be under the same [GPL-3.0 License](https://choosealicense.com/licenses/gpl-3.0/) that covers the project. Feel free to contact the maintainers if that's a concern. | |||
|
|
|||
| ## Report bugs using Github's [issues](https://github.com/AllYarnsAreBeautiful/ayab-desktop/issues) | |||
| We use GitHub issues to track public bugs. Report a bug by [opening a new issue](https://github.com/AllYarnsAreBeautiful/ayab-desktop/issues/new); it's that easy! | |||
| We use GitHub issues to track public bugs. Report a bug by [opening a new issue](); it's that easy! | |||
There was a problem hiding this comment.
Fix the empty link in the issue reporting instructions.
The link for "opening a new issue" is currently empty, which breaks the functionality of the document and has been flagged by static analysis.
-We use GitHub issues to track public bugs. Report a bug by [opening a new issue](); it's that easy!
+We use GitHub issues to track public bugs. Report a bug by [opening a new issue](https://github.com/AllYarnsAreBeautiful/ayab-desktop/issues/new); it's that easy!📝 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.
| We use GitHub issues to track public bugs. Report a bug by [opening a new issue](); it's that easy! | |
| We use GitHub issues to track public bugs. Report a bug by [opening a new issue](https://github.com/AllYarnsAreBeautiful/ayab-desktop/issues/new); it's that easy! |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
28-28: No empty links
null
(MD042, no-empty-links)
| @@ -16,7 +16,7 @@ Function .onInit | |||
| ;Do not use InstallDir at all so we can detect empty $InstDir! | |||
| ${If} $InstDir == "" ; /D not used | |||
| ${If} $MultiUser.InstallMode == "AllUsers" | |||
| StrCpy $InstDir "C:\%{app_name}-v%{version}" | |||
| StrCpy $InstDir "$%HOMEDRIVE%\%{app_name}-v%{version}" | |||
There was a problem hiding this comment.
Check environment variable syntax in NSIS script
The current syntax $%HOMEDRIVE%\%{app_name}-v%{version} mixes two different ways of accessing environment variables in NSIS. NSIS typically uses either $HOMEDRIVE or %HOMEDRIVE% syntax, but not a combination of both.
Consider using one of these alternatives:
- StrCpy $InstDir "$%HOMEDRIVE%\%{app_name}-v%{version}"
+ StrCpy $InstDir "$HOMEDRIVE\%{app_name}-v%{version}"Or:
- StrCpy $InstDir "$%HOMEDRIVE%\%{app_name}-v%{version}"
+ StrCpy $InstDir "%HOMEDRIVE%\%{app_name}-v%{version}"📝 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.
| StrCpy $InstDir "$%HOMEDRIVE%\%{app_name}-v%{version}" | |
| StrCpy $InstDir "$HOMEDRIVE\%{app_name}-v%{version}" |
| @@ -158,7 +158,7 @@ class MirrorDialog(QDialog): | |||
| def __init__(self, parent: Mirrors): | |||
| super().__init__() # FIXME set the parent widget as GuiMain | |||
| self.__ui = Ui_Mirrors() | |||
| self.__ui.setupUi(self) | |||
| self.__ui.setupUi(self) # type: ignore | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type ignore comment added for setupUi method
Adding a type ignore comment is a temporary solution to bypass type checking. This appears to be part of a larger effort to address type checking issues across the codebase.
While this works as a temporary solution, it would be better to properly address the underlying type issue. Consider investigating why the type checker is raising an error here and fix it properly if possible.
🏁 Script executed:
#!/bin/bash
# Check how widespread the use of type ignore for setupUi is in the codebase
rg "setupUi.*# type: ignore" --type pyLength of output: 809
Investigate and Resolve the Underlying Type Issue for setupUi
The current use of # type: ignore on self.__ui.setupUi(self) (and similar calls in other files) is a temporary workaround that bypasses the type checker. Our verification shows this pattern is repeated across several files (e.g., transforms.py, preferences.py, engine/status.py, etc.). This suggests there may be a common issue with type definitions related to the setupUi method.
Suggestions:
- Investigate the Root Cause: Review why the type checker is raising an error for
setupUi. This might be due to missing or incompatible type hints in the UI library or generated UI code. - Apply a Proper Fix: If possible, add the necessary type annotations or adjust the UI classes so that the type checker no longer requires a temporary ignore. This can help prevent similar fixes scattered throughout the codebase.
- Consolidate the Fix: Once identified, consider a project-wide refactor to remove the repetitive
# type: ignoredirectives related tosetupUi.
| @@ -224,7 +224,7 @@ class StatusTab(Status, QWidget): | |||
| def __init__(self) -> None: | |||
| super().__init__() | |||
| self.ui = Ui_StatusTab() | |||
| self.ui.setupUi(self) | |||
| self.ui.setupUi(self) # type: ignore | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type ignore comment added for setupUi method
Similar to other UI files, a type ignore comment has been added to bypass type checking for the setupUi method call.
This is likely due to issues with PySide6 type stubs. It would be better to address this more systematically across the codebase rather than adding individual ignore comments.
🏁 Script executed:
#!/bin/bash
# Check if there are any available type stubs for PySide6 that could fix this issue
pip list | grep pyside6-stubs
# Search for an existing solution or pattern for handling PySide6 typing in the codebase
rg "from PySide6" --type py -A 5 | grep -i "type"Length of output: 1421
Action Required: Adopt a Systematic Type-Stubbing Approach for PySide6
In file src/main/python/main/ayab/engine/status.py (line 227), the call:
self.ui.setupUi(self) # type: ignorecurrently uses # type: ignore to bypass issues with type checking on the setupUi call. Our investigation shows that:
- No
pyside6-stubspackage is installed (as verified by thepip list | grep pyside6-stubscommand). - Across the codebase, similar UI files employ ad-hoc type suppression (using patterns like
if TYPE_CHECKING:) instead of a consolidated, systematic solution.
Recommendations:
- Explore integrating a centralized solution for PySide6 type stubs (for example, by installing or developing a
pyside6-stubspackage). - Consider refactoring the type-checking approach across UI-related modules to avoid scattering individual
# type: ignorecomments.
Please address this improvement holistically in the codebase to ensure consistency and maintainability.
| eKnitter_rc | ||
| eKnitter_reversed_rc |
There was a problem hiding this comment.
Fix useless expressions flagged by static analysis.
These class-level expressions don't do anything. They're redundant since you already imported the modules at the top level.
- eKnitter_rc
- eKnitter_reversed_rc📝 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.
| eKnitter_rc | |
| eKnitter_reversed_rc |
🧰 Tools
🪛 Ruff (0.8.2)
68-68: Found useless expression. Either assign it to a variable or remove it.
(B018)
69-69: Found useless expression. Either assign it to a variable or remove it.
(B018)
| def generate_command_eknitter( | ||
| self, base_dir: str, os_name: str, controller_name: str, firmware_name: str | ||
| ) -> Optional[str]: | ||
| if os_name == "Windows": | ||
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/win64/esptool.exe") | ||
| exe_route = "\"" + exe_route + "\"" | ||
| elif os_name == "Linux": | ||
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/linux-amd64/esptool") | ||
| elif os_name == "Darwin": # macOS | ||
| #exe_route = self.__parent_ui.app_context.get_resource("ayab/firmware/esp32/macos/esptool") | ||
| exe_route = "esptool.py" | ||
|
|
||
| binary_file = os.path.join( | ||
| self.__app_context.get_resource("ayab/firmware"), firmware_name | ||
| ) | ||
|
|
||
| if "fw" in firmware_name: | ||
| exec_command = ( | ||
| f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash" | ||
| + f" --flash_size 8MB 0x10000 {binary_file} --force" | ||
| ) | ||
| elif "fs" in firmware_name: | ||
| exec_command = ( | ||
| f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash" | ||
| + f" --flash_size 8MB 0x670000 {binary_file} --force" | ||
| ) | ||
| else: | ||
| self.__logger.debug("error firmware or filesystem") | ||
| return None | ||
|
|
||
| self.__logger.debug(exec_command) | ||
| return exec_command |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New function to generate ESP32 flash commands for eKnitter.
The implementation handles different operating systems appropriately. However, there are two issues to address:
- There's a commented-out line for macOS execution route which should be removed or implemented.
- Consider extracting the repeated command parts to reduce code duplication.
def generate_command_eknitter(
self, base_dir: str, os_name: str, controller_name: str, firmware_name: str
) -> Optional[str]:
if os_name == "Windows":
exe_route = self.__app_context.get_resource("ayab/firmware/esp32/win64/esptool.exe")
exe_route = "\"" + exe_route + "\""
elif os_name == "Linux":
exe_route = self.__app_context.get_resource("ayab/firmware/esp32/linux-amd64/esptool")
elif os_name == "Darwin": # macOS
- #exe_route = self.__parent_ui.app_context.get_resource("ayab/firmware/esp32/macos/esptool")
exe_route = "esptool.py"
binary_file = os.path.join(
self.__app_context.get_resource("ayab/firmware"), firmware_name
)
+ base_command = f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash --flash_size 8MB"
+
if "fw" in firmware_name:
- exec_command = (
- f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash"
- + f" --flash_size 8MB 0x10000 {binary_file} --force"
- )
+ exec_command = f"{base_command} 0x10000 {binary_file} --force"
elif "fs" in firmware_name:
- exec_command = (
- f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash"
- + f" --flash_size 8MB 0x670000 {binary_file} --force"
- )
+ exec_command = f"{base_command} 0x670000 {binary_file} --force"
else:
self.__logger.debug("error firmware or filesystem")
return None📝 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.
| def generate_command_eknitter( | |
| self, base_dir: str, os_name: str, controller_name: str, firmware_name: str | |
| ) -> Optional[str]: | |
| if os_name == "Windows": | |
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/win64/esptool.exe") | |
| exe_route = "\"" + exe_route + "\"" | |
| elif os_name == "Linux": | |
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/linux-amd64/esptool") | |
| elif os_name == "Darwin": # macOS | |
| #exe_route = self.__parent_ui.app_context.get_resource("ayab/firmware/esp32/macos/esptool") | |
| exe_route = "esptool.py" | |
| binary_file = os.path.join( | |
| self.__app_context.get_resource("ayab/firmware"), firmware_name | |
| ) | |
| if "fw" in firmware_name: | |
| exec_command = ( | |
| f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash" | |
| + f" --flash_size 8MB 0x10000 {binary_file} --force" | |
| ) | |
| elif "fs" in firmware_name: | |
| exec_command = ( | |
| f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash" | |
| + f" --flash_size 8MB 0x670000 {binary_file} --force" | |
| ) | |
| else: | |
| self.__logger.debug("error firmware or filesystem") | |
| return None | |
| self.__logger.debug(exec_command) | |
| return exec_command | |
| def generate_command_eknitter( | |
| self, base_dir: str, os_name: str, controller_name: str, firmware_name: str | |
| ) -> Optional[str]: | |
| if os_name == "Windows": | |
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/win64/esptool.exe") | |
| exe_route = "\"" + exe_route + "\"" | |
| elif os_name == "Linux": | |
| exe_route = self.__app_context.get_resource("ayab/firmware/esp32/linux-amd64/esptool") | |
| elif os_name == "Darwin": # macOS | |
| exe_route = "esptool.py" | |
| binary_file = os.path.join( | |
| self.__app_context.get_resource("ayab/firmware"), firmware_name | |
| ) | |
| base_command = f"{exe_route} -port {self.port} --baud 921600 --no-stub write_flash --flash_size 8MB" | |
| if "fw" in firmware_name: | |
| exec_command = f"{base_command} 0x10000 {binary_file} --force" | |
| elif "fs" in firmware_name: | |
| exec_command = f"{base_command} 0x670000 {binary_file} --force" | |
| else: | |
| self.__logger.debug("error firmware or filesystem") | |
| return None | |
| self.__logger.debug(exec_command) | |
| return exec_command |
| def getIPlist(self) -> list[str]: | ||
| print("GetList") | ||
| result: list[str] = list() | ||
| if queueLock.acquire(True,0.1): | ||
| result.extend(self.addresslist) | ||
| queueLock.release() | ||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve getIPlist method.
Use context manager for lock and replace print with logging.
def getIPlist(self) -> list[str]:
- print("GetList")
+ logging.debug("Getting IP address list")
result: list[str] = list()
- if queueLock.acquire(True,0.1):
+ try:
+ with queueLock:
+ result.extend(self.addresslist)
+ except Exception as e:
+ logging.error(f"Error retrieving IP list: {e}")
- result.extend(self.addresslist)
- queueLock.release()
return result📝 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.
| def getIPlist(self) -> list[str]: | |
| print("GetList") | |
| result: list[str] = list() | |
| if queueLock.acquire(True,0.1): | |
| result.extend(self.addresslist) | |
| queueLock.release() | |
| return result | |
| def getIPlist(self) -> list[str]: | |
| - print("GetList") | |
| + logging.debug("Getting IP address list") | |
| result: list[str] = list() | |
| - if queueLock.acquire(True,0.1): | |
| - result.extend(self.addresslist) | |
| - queueLock.release() | |
| + try: | |
| + with queueLock: | |
| + result.extend(self.addresslist) | |
| + except Exception as e: | |
| + logging.error(f"Error retrieving IP list: {e}") | |
| return result |
|
@yekomS the source branch fro this PR seems to have diverted from its initial intention. In case you are still interested in supplying the eKnitter network connectivity to AYAB, please do this on a suitable branch, so we only get the required portions of code for this. |
Summary by CodeRabbit
Release Notes
New Features
esptool.Bug Fixes
Documentation