fix(device)!: socket exception and process rebuild

[wuwentao]

changes list:

1. process all the socket send/recv in try/except to prevent socket error
2. always close socket after exception matched and force reconnect to refresh
3. increase query timeout from 1 to 2, and only apply it to query cmd/msg
4. recovery socket timeout after query and recv socket msg done in each for loop.
5. rebuild process to remove the duplicate message parse.
6. rename some func and args
7. with current changes, I don't see any timeout exception in my local device testing, if there is any new error exist, we can continue improve it in future PRs.

Fixes #290

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Enhanced error handling for device discovery, now reporting NoSupportedProtocol for unsupported devices.
  • Streamlined socket connection process, improving reliability and clarity in communication.
  • Introduced MessageResult enumeration for improved clarity in message parsing.

• Bug Fixes

  • Updated tests to reflect changes in exception handling, ensuring accurate responses to device compatibility issues.

[coderabbitai]

Walkthrough

The changes involve significant modifications to error handling and communication logic within the midealocal package. The discover method in cli.py has updated exception handling, replacing RefreshFailed with NoSupportedProtocol. In device.py, the connection process has been simplified, and new constants for timeouts have been introduced. The message parsing mechanism has been enhanced with a new enumeration, MessageResult, replacing ParseMessageResult. Tests have been updated accordingly to reflect these changes in exception handling and message processing.

Changes

Files	Change Summary
midealocal/cli.py	Updated exception handling in the discover method, replacing RefreshFailed with NoSupportedProtocol.
midealocal/device.py	Streamlined the connect method, introduced SOCKET_TIMEOUT and QUERY_TIMEOUT, updated message sending methods to include a query parameter, and modified message parsing to use MessageResult.
tests/cli_test.py	Adjusted test_discover to expect NoSupportedProtocol instead of RefreshFailed, updating assertions accordingly.
tests/device_test.py	Replaced ParseMessageResult with MessageResult and updated tests to expect NoSupportedProtocol, reflecting changes in error handling and message parsing logic.

Assessment against linked issues

Objective	Addressed	Explanation
Ensure connection handling is robust (#[290])	✅
Update error handling to reflect protocol changes (#[290])	✅

Possibly related PRs

• chore: ruff PLR0915 #145: Changes in the discover function and new exception handling relate to modifications in the main PR.
• fix(cli): authenticate on discover v3 device #263: Enhancements to the discover method's error handling align with changes made in the main PR.
• fix(cli): discover must return a list #266: Modifications to the discover method's return type to a list of devices relate to the core functionality discussed in the main PR.
• feat(cli): midealocal CLI tool #204: Changes in midealocal/cli.py related to the discover method's exception handling are directly connected to the modifications made in the main PR.
• chore(ruff): Docstrings #124: Introduces changes to exception handling in midealocal/device.py, which may relate to the protocol handling discussed in the main PR.

🐇 In the code where I hop and play,
New exceptions guide the way!
With messages parsed, clear and bright,
Device connections now feel right!
So let’s celebrate this change with cheer,
For smoother paths, we hold so dear! 🌟

---

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a602d9f and b2dd254.

Files selected for processing (1)

• midealocal/device.py (12 hunks)

Additional comments not posted (10)

midealocal/device.py (10)

271-276: LGTM!

The changes to the send_message function look good. The addition of the query parameter and passing it to the underlying send methods based on the protocol version is a clean approach.

---

278-297: LGTM!

The changes to the send_message_v2 function look good. Setting the socket timeout based on the query parameter and closing the socket on exceptions are appropriate error handling measures.

---

303-307: LGTM!

The changes to the send_message_v3 function look good. Encoding the data and passing the query parameter to send_message_v2 is a straightforward approach.

---

309-314: LGTM!

The changes to the build_send function look good. Passing the query parameter to send_message is consistent with the other changes.

---

322-357: LGTM!

The new _recv_message function looks good. It handles different scenarios such as socket availability, empty messages, successful receiving, timeouts, and exceptions appropriately. Closing the socket on exceptions and returning specific MessageResult values provides clear error handling and communication of the result.

---

359-443: LGTM!

The changes to the refresh_status function look good. The function now handles different message results and protocol support more comprehensively. Skipping unsupported protocols, using _recv_message for receiving responses, and raising a NoSupportedProtocol exception when all queries fail during the initial connection are appropriate improvements.

---

Line range hint 460-527: LGTM!

The changes to the parse_message function look good. Returning specific MessageResult values based on the parsing result improves the clarity and error handling of the function. The logic for decoding and processing messages based on the protocol version and payload type seems appropriate.

---

609-615: LGTM!

The changes to the close_socket function look good. Resetting the _unsupported_protocol list and _buffer, closing the socket, and handling OSError exceptions are appropriate steps for closing the socket connection. Setting _socket to None in the finally block ensures it is reset even if an exception occurs.

---

638-647: LGTM!

The new _recovery_timeout function looks good. Raising a SocketException if the socket is not available and setting the socket timeout to SOCKET_TIMEOUT are appropriate steps for recovering the socket timeout. Closing the socket if a TimeoutError occurs during setting the timeout handles the error scenario correctly.

---

648-668: LGTM!

The new _connect_loop function looks good. Attempting to connect to the device in a loop until a connection is established is a robust approach. Closing the socket, incrementing the retry count, and applying exponential backoff with a maximum sleep duration handles connection failures and retries appropriately. Resetting the retry count after a successful connection is a good practice.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 14.74359% with 133 lines in your changes missing coverage. Please review.

Project coverage is 39.93%. Comparing base (f2e5e24) to head (b2dd254).

Files with missing lines	Patch %	Lines
midealocal/device.py	13.63%	133 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   40.09%   39.93%   -0.16%     
==========================================
  Files          84       84              
  Lines        7489     7534      +45     
==========================================
+ Hits         3003     3009       +6     
- Misses       4486     4525      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coderabbitai]

Actionable comments posted: 5

Outside diff range and nitpick comments (1)

midealocal/device.py (1)

88-88: Improve exception docstring for clarity and consistency.

The docstring for NoSupportedProtocol could be more descriptive. Consider updating it to better reflect the purpose of the exception, such as: "Exception raised when no supported protocol is found for the device."

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f2e5e24 and d33ed73.

Files selected for processing (4)

• midealocal/cli.py (2 hunks)
• midealocal/device.py (13 hunks)
• tests/cli_test.py (3 hunks)
• tests/device_test.py (5 hunks)

Additional comments not posted (11)

tests/device_test.py (4)

10-12: Imports updated to include MessageResult and NoSupportedProtocol

The addition of MessageResult and NoSupportedProtocol to the imports aligns with the changes in exception handling and message result processing. This update ensures that the necessary classes are available for the tests.

---

67-67: Including NoSupportedProtocol in test cases

Adding NoSupportedProtocol to the parameterized exceptions in test_connect ensures that the test covers the scenario where this exception is raised. This enhances the test coverage for exception handling.

---

232-234: Updating parse_message side effects with MessageResult enums

The side effects for parse_message in the test_refresh_status method are updated to return MessageResult enums (SUCCESS, PADDING, ERROR). This aligns with the refactored code and ensures that the test accurately simulates different parsing outcomes.

---

249-253: Expecting NoSupportedProtocol exception in test cases

The test_refresh_status method now correctly expects NoSupportedProtocol to be raised when parse_message returns MessageResult.ERROR or when a TimeoutError occurs. This change reflects the updated exception handling logic in the code.

tests/cli_test.py (3)

17-17: Adding NoSupportedProtocol to imports for exception handling

The inclusion of NoSupportedProtocol in the imports ensures that the test can handle scenarios where a device uses an unsupported protocol version. This enhances the robustness of the exception handling in the tests.

---

137-137: Mocking refresh_status to raise NoSupportedProtocol

By adding NoSupportedProtocol to the side_effect of refresh_status_mock, the test now simulates the scenario where refresh_status encounters an unsupported protocol. This change effectively tests the code's ability to handle devices with unsupported protocols.

---

160-160: Testing discovery with a V2 device raising NoSupportedProtocol

The test case adds a scenario where a V2 device triggers a NoSupportedProtocol exception during discovery. This ensures that the CLI handles devices with unsupported protocols gracefully, aligning with the PR objectives to improve socket exception handling.

midealocal/cli.py (2)

23-28: Imports Updated Appropriately

The addition of NoSupportedProtocol to the imports ensures that exceptions related to unsupported protocols are properly handled.

---

129-129: Added Exception Handling for Unsupported Protocols

The except NoSupportedProtocol: block appropriately catches exceptions when a device does not support the required protocol. This enhances the robustness of the discovery process by gracefully handling devices with unsupported protocols.

midealocal/device.py (2)

335-338: Catch the correct exception class for timeouts in _recv_message.

In the _recv_message method, catching socket.timeout instead of TimeoutError ensures proper handling of socket timeout exceptions.

Modify the exception handling:

-except TimeoutError:
+except socket.timeout:

Likely invalid or redundant comment.

---

680-682: Catch the correct exception class for timeouts in run method.

In the run method, when handling socket timeouts, catch socket.timeout instead of TimeoutError to ensure accurate exception handling for socket operations.

Update the exception handling:

-except TimeoutError:
+except socket.timeout:

Likely invalid or redundant comment.

[rokam]

@wuwentao this is a breaking change. It looks good. Can you please add tests to cover the changed code?

[wuwentao]

@wuwentao this is a breaking change. It looks good. Can you please add tests to cover the changed code?

thanks for your review, but I don't thinks it's a good idea to add test now, most of the process is not fullly test and verified, we still need to improve and update it, once it stable, we can start to add test for it.

[wuwentao]

in addition, I don't think it can cover all, current process should not match the finally version, it just focus on fix the socket exception and some duplicate process rebuild, just to match the changes, especially for query socket timeout set and recovery.

I just found some new bug and push a new commit now, we may have new bug and issue with it, anyway, we can continue improve it and try to make it stable.

[rokam]

and push a new commit now, we may

in addition, I don't think it can cover all, current process should not match the finally version, it just focus on fix the socket exception and some duplicate process rebuild, just to match the changes, especially for query socket timeout set and recovery.

I just found some new bug and push a new commit now, we may have new bug and issue with it, anyway, we can continue improve it and try to make it stable.

I think tests will help us identify those bugs. Right now, with all those changes I still don't fully understand that and if this will fix it.

[wuwentao]

@rokam thanks, but we can only add unit test, it's not real device func test.... this is the key point.
all the unit tes only can test our code logic, and it can't test any device func, all of our unit test should design to test the code logic......
so I just think we can quickly release it and check/confirm it with real device func, then we can found the result.

anyway, we just need to publish a new release with the socket error bug, we have known this bug for too many weeks and pending the fix for a long time now. and please don't worry, I can confirm I can add some unit test later, this version should not a finally or stable version, it still have more step and process need to improve.

In a company DEV release process, they should have all the device/product to run real device func test, but we don't have all the devices.....
so I just can use all of my local device to test the logic and basic func, also refer to the old logic, then publish a release to double check with our user's device, got new bug/issue and fix it, improve it.

[wuwentao]

@rokam push a new commit again.
chagnes 1:
fix bug: when refresh_status, socket exist, and set query timeout to 2, send query to device and recv msg, in recv_msg got socket exception, it will close socket and trigger reconnect.
so recovery socket timeout should be skip/disable when timeout/exception exist, and continue to run the reconnect job....
in this step, I think we can only do connect and no need to probe protocol in future, but no plan to change it in current MR....
as it also required more time to test and verify it.

changes 2:
add detail process comments in def run() for future reference.

[wuwentao]

@rokam thanks, could you help to publish a new release for midea-local?