Send dummy request before closing serial port #740
Conversation
WalkthroughThe changes introduce a new state called Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StateMachine
participant Firmware
participant FeedbackHandler
User->>StateMachine: Start knitting
StateMachine->>Firmware: Send knitting request
Firmware-->>StateMachine: Knitting in progress
StateMachine->>Firmware: Request final response
Firmware-->>StateMachine: Final response received
StateMachine->>StateMachine: Transition to DISCONNECT
StateMachine->>FeedbackHandler: Notify disconnection in progress
FeedbackHandler-->>User: Disconnection notification
StateMachine->>StateMachine: Call _API6_disconnect
StateMachine->>Firmware: Check for confirmation token
Firmware-->>StateMachine: Confirmation token received
StateMachine->>StateMachine: Transition to FINISHED
StateMachine-->>User: Knitting finished
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? 🪧 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 (
|
jonathanperret
force-pushed
the
safe-serial-close
branch
from
e222ffd to
1c2e6fe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/main/python/main/ayab/engine/engine_fsm.py (1)
54-54: Document the addition of theDISCONNECTstateThe new
DISCONNECTstate enhances the state machine's capability to manage the disconnection process. Please update any relevant documentation or comments to include this new state and its role in the state machine.src/main/python/main/ayab/engine/output.py (1)
36-36: Maintain logical grouping within theOutputenumerationConsider placing the new
DISCONNECTING_FROM_MACHINEvalue adjacent to related states likeCONNECTING_TO_MACHINEfor better readability and logical grouping within theOutputenum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/output.py(2 hunks)
🔇 Additional comments (1)
src/main/python/main/ayab/engine/output.py (1)
69-71: Verify dynamic method dispatch for new output state
Ensure that the handle method correctly dispatches to the _disconnecting_from_machine method when Output.DISCONNECTING_FROM_MACHINE is encountered. Since the dispatch relies on naming conventions, verify that the method name matches the enum member name in lowercase with a leading underscore.
To confirm, the method name _disconnecting_from_machine should match the enum member DISCONNECTING_FROM_MACHINE.
This avoids losing the last sent data with some USB/serial drivers.
jonathanperret
force-pushed
the
safe-serial-close
branch
from
1c2e6fe to
787e068
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/python/main/ayab/engine/engine_fsm.py (1)
200-211: Well-documented solution to prevent message lossThe implementation effectively addresses the issue of potentially dropped
cnfLinemessages by usingreqInfoas a sentinel. The detailed comments explain the problem and solution clearly.As mentioned in the PR objectives, consider evolving the API to ensure all exchanges are initiated by the desktop application in the future. This would provide a more robust communication pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/output.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/python/main/ayab/engine/output.py
🔇 Additional comments (2)
src/main/python/main/ayab/engine/engine_fsm.py (2)
54-54: LGTM: State enum addition is appropriate
The new DISCONNECT state provides a clear transitional state for handling the disconnection process.
251-261: LGTM: Clean implementation of disconnect handling
The method effectively handles the disconnection process by waiting for the cnfInfo response before transitioning to the FINISHED state. The implementation is simple and focused.
|
Review OK. For the future, the API should probably adapted for proper synchronuous communication rather this workaround (see lengthy discussions on Discord). |
Yes, this is to be kept in mind for the next iteration of the API which could be done just after 1.0 is out. |
|
Reported to be working by @Adrienne200 in Discord |
🐛 Problem
Hopefully fixes #662 .
💡 Proposed solution
One theory that is consistent with the observations in #662, is that the last
cnfLinemessage sent from the desktop app to the firmware gets lost when the serial port is closed immediately afterwards. Here is an example of this happening in another context: serialport/serialport-rs#117To avoid closing the serial port before the last data has been received by the firmware, in this PR we add an additional exchange initiated by the desktop app. A
reqInfois sent to the firmware, which immediately replies with acnfInfomessage. Because messages are sent and received in sequence, receiving this response confirms that previous messages have been transmitted and the serial port can be closed safely.🤔 Alternatives considered
To make sure the last bytes are transmitted, I considered adding a delay before closing the port, but this wouldn't really guarantee data was sent.
A cleaner solution in the longer term would be to evolve the API so that all exchanges would be initiated by the desktop app, instead of having an exception for
reqLine/cnfLine.🔬How to test
This test release includes the change proposed in this PR: https://github.com/jonathanperret/ayab-desktop/releases/tag/1.0.0-safe-disconnect-1
It does not require a new firmware, so no need to reflash before testing.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
DISCONNECTstate to improve the disconnection process after knitting operations.Improvements