Synchronise end-of-line sounds between app and firmware #731
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces multiple changes across several classes in the AYAB software. Key modifications include the simplification of method signatures by removing unnecessary parameters, such as the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/python/main/ayab/engine/communication_mock.py (1)
46-48: Consider using a queue for thread-safe message handlingWhile changing from
dequetolistworks, consider usingQueuefrom thequeuemodule if this mock will be used in multi-threaded scenarios. TheQueueclass provides thread-safe operations which might be beneficial for simulating real hardware communication.- self.rx_msg_list = list() + from queue import Queue + self.rx_msg_list = Queue()src/main/python/main/ayab/ayab.py (1)
Line range hint
159-169: Consider adding documentation for sound behaviorWhile the method implementation is correct, it would be helpful to add documentation explaining the new sound behavior, especially since it's a key part of the PR objectives.
def finish_operation(self, operation: Operation) -> None: - """(Re-)enable UI elements after operation finishes.""" + """(Re-)enable UI elements after operation finishes. + + Note: Sound notifications are now handled by the firmware timing + to ensure proper synchronization with the actual knitting process. + """src/main/python/main/ayab/engine/control.py (1)
245-254: Add docstring to explain method's purpose.The implementation is correct, but adding a docstring would help explain the method's role in synchronizing end-of-line sounds.
Consider adding:
def cnf_final_line_API6(self, requested_line: int) -> None: + """Send a blank line as the final line to synchronize end-of-line sounds. + + This method is called during the FINISHING state to ensure the firmware + completes its final operations before the app signals completion. + + Args: + requested_line: The line number requested by the firmware + """ self.logger.debug("sending blank line as final line=%d", requested_line)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/engine/communication_mock.py(3 hunks)src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine.py(1 hunks)src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/output.py(1 hunks)src/main/python/main/ayab/signal_receiver.py(1 hunks)src/main/python/main/ayab/signal_sender.py(3 hunks)src/main/python/main/ayab/tests/test_communication_mock.py(1 hunks)
🔇 Additional comments (13)
src/main/python/main/ayab/signal_receiver.py (1)
61-61: LGTM! Signal signature simplified to support sound synchronization.
The removal of the boolean parameter from operation_finisher signal aligns with the PR's objective of improving sound synchronization between app and firmware.
Let's verify all usages of this signal to ensure the change is consistently applied:
✅ Verification successful
Let me verify the implementation of finish_operation to ensure it correctly handles the updated signal signature.
Let me try a different approach to find the finish_operation implementation using ripgrep.
Signal signature change is correctly implemented across the codebase
Based on the verification results:
- The
operation_finishersignal is defined withSignal(Operation)type - It's properly connected to
finish_operationmethod which accepts anOperationparameter - The signal is emitted correctly in
signal_sender.pyandengine.pywith an operation argument - The receiver method
finish_operationinayab.pycorrectly handles the operation parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for operation_finisher signal connections and emissions
rg "operation_finisher" -A 2 -B 2
Length of output: 1842
Script:
#!/bin/bash
# Search for finish_operation implementation
ast-grep --pattern 'def finish_operation($_) {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Search for finish_operation method definition
rg "def finish_operation" -A 5 -B 2
Length of output: 604
src/main/python/main/ayab/engine/output.py (1)
Line range hint 99-104: LGTM! Sound feedback enhanced for knitting completion.
The addition of the finish sound and updated notification message directly addresses the PR's objective of synchronizing end-of-line sounds between app and firmware.
src/main/python/main/ayab/tests/test_communication_mock.py (1)
110-115: Add documentation explaining the test's purpose.
While the test implementation is correct, it would benefit from documentation explaining how this alternating pattern relates to the sound synchronization feature.
Add a comment block explaining the test's purpose:
+ # Test the alternating behavior between line requests and idle states
+ # This pattern ensures proper timing for sound synchronization
+ # between the app and firmware
for i in range(0, 256):Let's check the test coverage:
✅ Verification successful
Based on the search results, I found evidence that the alternating pattern between line requests and no output is specifically designed to trigger end-of-line sound notifications in the UI. This is confirmed by the comment in communication_mock.py:
# Alternate between reqLine and no message
# (so that the UI makes the end-of-line sound for each row)Additionally, I found that the application uses sound notifications for important events, including end-of-line beeps, as evidenced by the translation strings and audio player implementation.
Add documentation explaining the test's purpose and sound synchronization.
+ # Test the alternating behavior between line requests and idle states
+ # This pattern ensures proper timing for end-of-line sound notifications
+ # by alternating between reqLine messages and no output
for i in range(0, 256):🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test cases
rg "test.*API6" --type python
Length of output: 62
Script:
#!/bin/bash
# Search for related test cases using ripgrep without file type filter
rg "test.*API6"
Length of output: 1682
Script:
#!/bin/bash
# Find Python test files and examine their content
fd "test.*\.py" | xargs cat
Length of output: 37552
Script:
#!/bin/bash
# Search for API6-related code
rg "API6"
Length of output: 9320
Script:
#!/bin/bash
# Search for sound synchronization related code
rg -B 2 -A 2 "sound|sync|timing"
Length of output: 36463
Script:
#!/bin/bash
# Look for related test documentation
rg -B 2 -A 2 "Test.*alternating|alternating.*test" --type py
Length of output: 62
src/main/python/main/ayab/engine/communication_mock.py (2)
32-32: LGTM: Constructor simplified
The removal of the unused step parameter simplifies the interface without affecting functionality.
98-114: Verify the alternating message pattern matches hardware behavior
The implementation alternates between sending request lines and no messages to simulate end-of-line sounds. This change aligns with the PR objective of synchronizing sounds, but we should verify it matches the actual hardware behavior.
✅ Verification successful
Based on the search results, I can now generate a final response:
Alternating message pattern correctly simulates hardware behavior
The implementation in communication_mock.py correctly simulates the hardware behavior:
- The mock alternates between
reqLineand no message to trigger UI sounds, which aligns with the actual hardware communication pattern as evidenced by:- Hardware communication (
communication.py) defines the samereqLinetoken (0x82) - Engine FSM (
engine_fsm.py) processes thesereqLinetokens to track knitting progress - Audio system (
audio.py,output.py) plays corresponding sounds for line completion - Test cases (
test_communication_mock.py) verify this alternating behavior: "Alternates between requesting a line and no output"
- Hardware communication (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the hardware communication follows similar pattern
# Look for similar alternating patterns in hardware communication code
# Search for reqLine token usage in actual hardware communication
rg -A 5 "reqLine" --type python
# Search for sound-related timing patterns
rg -A 5 "beep|sound|audio" --type python
Length of output: 136
Script:
#!/bin/bash
# Search for reqLine token usage in actual hardware communication without file type filter
rg -A 5 "reqLine"
# Search for sound-related timing patterns
rg -A 5 "beep|sound|audio"
# Find Python files that might contain communication logic
fd ".py$" | grep -i "comm\|serial"
Length of output: 22523
src/main/python/main/ayab/signal_sender.py (2)
52-53: LGTM: Method reformatting improves readability
The single-line format for emit_progress_bar_updater improves code consistency.
100-101: Verify all operation finish signal connections are updated
The removal of the beep parameter aligns with the PR's sound synchronization goals. However, we should verify all connections to this signal have been updated accordingly.
✅ Verification successful
All operation finish signal connections are properly updated
Based on the codebase analysis, all operation finish signal connections have been updated correctly. The search results show that:
- The
operation_finishersignal is defined without anybeepparameter - All connections and emissions of the signal are using the updated signature with just the
operationparameter - No legacy code using
beepparameter was found in any signal connections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to operation_finisher with beep parameter
# Search for operation_finisher calls
rg "operation_finisher.*beep" --type python
# Search for operation finish signal connections
rg "finished\.connect.*beep" --type python
Length of output: 152
Script:
#!/bin/bash
# Search for operation_finisher calls and connections without file type flag
rg "operation_finisher.*beep"
# Search for operation finish signal connections
rg "finished\.connect.*beep"
# Search for any operation_finisher related code
rg "operation_finisher"
# Search for signal connections related to operations
rg "finished\.connect"
Length of output: 865
src/main/python/main/ayab/ayab.py (1)
109-109: LGTM: Hardware test completion signal simplified
The lambda function simplification aligns with the new sound synchronization approach.
src/main/python/main/ayab/engine/engine_fsm.py (3)
54-54: LGTM: New state addition aligns with PR objectives.
The introduction of the FINISHING state provides a clear transition point for synchronizing end-of-line sounds between the app and firmware.
200-201: LGTM: Improved state transition logic.
The transition to FINISHING state instead of directly to FINISHED enables proper synchronization of end-of-line sounds by allowing the firmware to complete its final operations.
241-250: LGTM: Well-implemented finishing state handler.
The _API6_finishing method properly handles the final line request and ensures correct timing of the "work finished" sound by transitioning to FINISHED state only after the firmware confirms receipt of the final line.
src/main/python/main/ayab/engine/control.py (1)
213-214: LGTM: Improved end-of-pattern handling.
Setting flags = 0 ensures the firmware doesn't receive a premature end signal, allowing for proper synchronization of the final operations and associated sounds.
src/main/python/main/ayab/engine/engine.py (1)
197-197: LGTM: Simplified operation completion signaling.
The removal of the beep parameter from emit_operation_finisher aligns with the new approach of letting the firmware handle the end-of-line sounds naturally.
dl1com
force-pushed
the
fix-end-sounds
branch
from
bbca6b7 to
a9097c1
Compare
|
@Adrienne200 could you please try the test build of this branch to verify this achieves what it intends to do? |
I don't think so, I am using the API exactly as it is documented — as demonstrated by the fact that the firmware doesn't need to be modified for this code to work. |
|
I just triggered a new test release that now includes the newly merged fixes for #732 and #734 : https://github.com/jonathanperret/ayab-desktop/releases/tag/1.0.0-endsounds-3 |
|
Note that with this change, the FW doesn't move to init state anymore unless you effectively knit the blank row. |
Note that in general you do have to "knit" the "blank row", because while it will select every needle to In theory there can be a difference between the firmware being in By the way, the current firmware would not work if that reset did not occur (you can easily see this with This is all to say that yes, the current firmware and API are a bit broken, and will have to be improved, but I am trying to make a small change here for now. |
|
That reset is specific to the current AVR board, if you take a uno R4 minima or Wifi for example, then opening the serial port doesn't reset the device, i.e. behavior is not generic but specific to a few platforms It will be required as well moving forward with a webapp/wifi communication (no HW reset). From an user experience, the HW/shield beep will also be missing (unless you knit the blank row again). |
Again, I'm 100% onboard with improving the firmware and API in the future.
The beep is here. Have you tried knitting with |
|
No (remember that I've no machine/only coding for my daugther :-)) but I see that Knitter::stopKnitting() and therefore GlobalBeeper::endWork() won't be called until you finish the last line void Knitter::stopKnitting() const {
GlobalBeeper::endWork();
GlobalFsm::setState(OpState::init); |
Yes, but Believe me, I know that so many things are suboptimal in the whole thing. I'm just trying to start somewhere and improve things bit by bit here. I believe the change in this PR makes things incrementally better. Let's remember the focus here is to get to a state where we can make the first release of AYAB in over 5 years. |
|
I understand but I believe that adding the suggested indState is a step in the right direction while adding a fake new row is rather a workaround/a step in the wrong direction that you will have to undo moving forward. |
jonathanperret
force-pushed
the
fix-end-sounds
branch
2 times, most recently
from
3d338e3 to
f2b0f32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/python/main/ayab/engine/engine_fsm.py (1)
242-273: LGTM! Well-implemented state handlers with good error mitigation.The implementation properly handles:
- Sending the blank final line
- Preventing serial port closure issues
- Maintaining proper state transitions
Consider adding debug logging in
_API6_disconnectsimilar to other state transitions.@staticmethod def _API6_disconnect(control: Control, operation: Operation) -> Output: token, _ = control.check_serial_API6() if token == Token.cnfInfo: + control.logger.debug("State FINISHED") # We received a response to our final `reqInfo` request, # it is now safe to close the port. control.state = State.FINISHED return Output.KNITTING_FINISHEDsrc/main/python/main/ayab/engine/control.py (1)
248-258: LGTM! Clean implementation of final line handling.The method properly implements the blank final line sending with appropriate debug logging. Consider defining a constant for the flags to improve code maintainability.
class Control(SignalSender): + FINAL_LINE_FLAG = 1 + REGULAR_LINE_FLAG = 0 def cnf_final_line_API6(self, requested_line: int) -> None: self.logger.debug("sending blank line as final line=%d", requested_line) # prepare a blank line as the final line bits = bitarray(self.machine.width, endian="little") # send line to machine color = 0 # doesn't matter - flags = 1 # this is the last line + flags = self.FINAL_LINE_FLAG # this is the last line self.com.cnf_line_API6(requested_line, color, flags, bits.tobytes())src/main/python/main/ayab/engine/communication_mock.py (1)
109-109: Document return type change in docstring.The return type annotation now includes
None, but this isn't reflected in the docstring.- """Read and parse data packet.""" + """Read and parse data packet. + + Returns: + tuple[bytes | None, Token, int]: A tuple containing the parsed message, + token, and message length. Returns None as the first element when + no message is available. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/engine/communication_mock.py(3 hunks)src/main/python/main/ayab/engine/control.py(2 hunks)src/main/python/main/ayab/engine/engine.py(1 hunks)src/main/python/main/ayab/engine/engine_fsm.py(3 hunks)src/main/python/main/ayab/engine/output.py(3 hunks)src/main/python/main/ayab/signal_receiver.py(1 hunks)src/main/python/main/ayab/signal_sender.py(3 hunks)src/main/python/main/ayab/tests/test_communication_mock.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/python/main/ayab/tests/test_communication_mock.py
- src/main/python/main/ayab/signal_receiver.py
- src/main/python/main/ayab/engine/output.py
- src/main/python/main/ayab/signal_sender.py
- src/main/python/main/ayab/engine/engine.py
- src/main/python/main/ayab/ayab.py
🔇 Additional comments (6)
src/main/python/main/ayab/engine/engine_fsm.py (2)
54-55: LGTM! New states align with PR objectives.
The addition of DISCONNECT and FINISHING states provides a clean separation of concerns for handling the final knitting phase and proper port closure.
201-202: LGTM! Clean transition to new FINISHING state.
The change correctly implements the first part of the synchronization fix by transitioning to the FINISHING state instead of immediately finishing.
src/main/python/main/ayab/engine/control.py (1)
212-217: LGTM! Clear implementation of the new line handling approach.
The change correctly implements the core fix by never marking regular pattern lines as final, with good documentation explaining the rationale.
src/main/python/main/ayab/engine/communication_mock.py (3)
32-32: LGTM! Constructor signature simplified.
The removal of the unused step parameter streamlines the interface while maintaining the core functionality.
112-123: LGTM! Sound synchronization logic implemented correctly.
The alternating behavior between request lines and no messages effectively implements the sound synchronization requirements from the PR objectives. The implementation:
- Properly simulates the knitting process timing
- Ensures UI sounds are synchronized with the mock hardware
- Maintains a consistent state through
__started_row
46-48: Verify performance impact of using list.pop(0).
While changing from deque to list maintains FIFO behavior through pop(0), this operation has O(n) complexity compared to O(1) for deque.popleft(). Although this is a mock class used for testing, we should verify if this impacts performance with large message queues.
Consider keeping deque for better performance:
- self.rx_msg_list = list()
+ from collections import deque
+ self.rx_msg_list = deque()There were two sources for the delay: one deliberate delay of 1 second to "finish printing to knit progress window", which does not seem required anymore; and another caused by waiting for the serial port to be closed before emitting the finished sound: this was fixed by coupling the audio emission to printing the notification in the progress bar, rather than the actual end of the complete operation.
In order to let the UI play the end-of-line sounds, we need the communications mock to not emit reqLine messages back-to-back.
This did not really work, due to running on a non-UI thread.
jonathanperret
force-pushed
the
fix-end-sounds
branch
from
f2b0f32 to
5f9e751
Compare
|
|
Reported to be working by @Adrienne200 in Discord |
🤔 Problem
The end-of-line sounds are not synchronized between the firmware and the desktop app, fixes #617.
💡 Proposed solution
The main source of discrepancy is the fact that the desktop app closes the connection to the firmware as soon as it has sent the final pattern row. But this actually happens two full carriage passes (or one, with the garter carriage) before knitting is really complete.
The firmware itself continues to emit end-of-row sounds until the end of the last selecting row, i.e. one pass further than the desktop app.
This PR slightly changes the final row transmission logic: instead of marking the last patterning row as "final" and closing the connection, it sends that last row normally (not telling the firmware that it is the last), so that it gets the usual request for a next row from the firmware at the end of that pass, which the app now answers with a blank row.
This which lets the app emit a "work finished" sound at the same point that previous AYAB versions emitted an "end of work" beep from the hardware.
This also makes the message "Please knit until you hear the double beep sound" notification more correct, as the final pass (knitting the final selection of needles while moving them all back to B) will trigger one last sound from the hardware.
Two other minor issues around sounds are also fixed by this PR:
🤔 Remarks
This change may also "fix" #662, or at least reduce its impact — if the firmware now fails to "move on" from the final blank row, the result will be very close to having properly finished the process.
Also, the hardware sounds themselves need a bit of work, as for example the final "double beep sound" does not actually sound like a double beep, at least on the AYAB shield's standard buzzer. I have changes to the firmware that should improve that situation, which I will submit in an upcoming PR to
ayab-firmware.🔬 How to test
I have prepared a test release at https://github.com/jonathanperret/ayab-desktop/releases/tag/1.0.0-endsounds-4 that can be installed as usual. It does not include a new firmware (compared to 1.0.0 beta 3) so no reflashing should be necessary.
Note that this PR (and the test release above) now includes PR #740 which is a proper fix for #662, so neither it nor #617 should be reproducible on the test release.
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
FINISHINGstate in the state machine to enhance the knitting operation's control flow.Improvements
Bug Fixes