Fix hardware test dialog #693
Conversation
The engine would get stuck in RUN_TEST state waiting for a message from the firmware that might never come. And there would be no way to exit that waiting loop from the UI. By returning to the main loop, we give the UI a chance to exit test mode.
The previous code waited for an `indState` message but the current firmware doesn't seem to be sending this. So the REQUEST_TEST state only has to emit `reqTest` and transition to CONFIRM_TEST. Maybe the state could be removed entirely.
The firmware definitely expects all commands to be wrapped in SLIP packets. The write_API6 method is only used by the hardware test commands, this is why this oversight went undetected.
There was a race condition caused by `HardwareTestDialog.reject` trying to end testing by forcing `__control.state` to `FINISHED` from the UI thread. If this occurred as the thread was about to enter `Control.operate`, the engine FSM would immediately jump from state `FINISHED` to state `CONNECT`, so `Engine.run` would never get a chance to notice the `FINISHED` state and exit its processing loop. This was causing random hangs when exiting the hardware test dialog. By making `FINISHED` a "dead-end" state, we make sure `Engine.run` eventually exits its loop, and the thread terminates.
There is no such property. The statement was always raising an error that was silently ignored.
WalkthroughThe recent changes enhance command management and error handling in the Ayab engine's communication and FSM components. The Changes
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 as PR comments)
Additionally, you can add 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/python/main/ayab/engine/communication.py (2 hunks)
- src/main/python/main/ayab/engine/engine_fsm.py (2 hunks)
- src/main/python/main/ayab/engine/hw_test_communication_mock.py (4 hunks)
- src/main/python/main/ayab/hw_test.py (4 hunks)
Additional comments not posted (17)
src/main/python/main/ayab/engine/hw_test_communication_mock.py (7)
53-53: LGTM! Enhanced debugging information.The added output message improves the debugging information by indicating the called command.
74-84: LGTM! Improved command help output.The updated help command output includes new commands and standardizes the format, enhancing clarity and consistency.
92-95: LGTM! Improved method clarity and maintainability.The method renaming and simplification enhance clarity and maintainability.
100-101: LGTM! Improved method clarity and maintainability.The method renaming and simplification enhance clarity and maintainability.
103-104: LGTM! Improved method clarity and maintainability.The method renaming and simplification enhance clarity and maintainability.
106-108: LGTM! Clear mechanism to stop auto-read and auto-test functionalities.The new method provides a clear mechanism to disable auto-read and auto-test functionalities, enhancing clarity and maintainability.
Line range hint
114-136: LGTM! Improved method clarity and robustness.The method renaming and additional validation enhance clarity and robustness.
src/main/python/main/ayab/hw_test.py (5)
46-58: LGTM! Updated commands list.The updated
commandslist reflects the new command structure and improves clarity by removing unused commands.
80-89: LGTM! Simplified button creation logic.The updated button creation logic simplifies the process and ensures proper configuration.
145-146: LGTM! Proper state reset for autoRead and autoTest buttons.The updated reject method ensures proper state reset for the
autoReadandautoTestbuttons.
156-163: LGTM! Enhanced control flow for button interactions.The updated __button_pushed method enhances control flow by allowing a more dynamic response to button states.
169-171: LGTM! Aligned with new command structure.The updated __set_solenoid method aligns with the new command structure and ensures proper command handling.
src/main/python/main/ayab/engine/communication.py (2)
50-60: LGTM! Updated Token enumeration.The updated
Tokenenumeration reflects a logical restructuring of command identifiers, suggesting a potential expansion of functionality or reorganization of command handling.
245-249: LGTM! Enhanced robustness and flexibility in write_API6 method.The modified write_API6 method enhances robustness by allowing greater flexibility in the types of data that can be passed and ensuring proper operation under different conditions.
src/main/python/main/ayab/engine/engine_fsm.py (3)
208-210: Verify robustness of the simplified_API6_request_testmethod.The method has been simplified to directly call
control.com.req_test_API6(), set the state toState.CONFIRM_TEST, and log the state change. Ensure thatcontrol.com.req_test_API6()handles all necessary checks and error reporting internally to maintain robustness.
235-237: Verify processing of test results in_API6_run_test.The method has been simplified to call
control.check_serial_API6()without any conditional checks or loops. Ensure thatcontrol.check_serial_API6()processes all incoming messages and handles necessary actions to maintain the correct processing of test results.
237-239: Verify impact of removed exception handling and state transition in_API6_finished.The method has removed exception handling around stopping the timer and the state transition to
State.CONNECT. Ensure that the timer's state is no longer critical at this point in the flow and that the cleanup process is handled correctly without these elements.
|
Looks good and works well, thanks for getting this sorted! |
It is, I’ll look into it over there.
If you’re talking about the “autoXxx” buttons, they’re supposed to stay highlighted while that test is running, and you can stop the test (i.e. emit
Good catch, I’ll fix this. |
|
It is not clear in the UI that they are toggles
…On Sun, Aug 11, 2024, 1:56 PM Jonathan Perret ***@***.***> wrote:
Looks good and works well, thanks for getting this sorted! Some comments-
Beep doesn't seem to work (I assume that's a problem for ayab-firmware
though)
It is, I’ll look into it over there.
We need to add a stop button to the dialog- while quit does issue stop,
it's kinda annoying to have to reopen the window if you want to switch test
commands.
If you’re talking about the “autoXxx” buttons, they’re supposed to stay
highlighted while that test is running, and you can stop the test (i.e.
emit stopCmd) by re-clicking the same button to toggle it. Isn’t that
happening for you?
Quit should probably be Close, as it's a modal not its own app.
Good catch, I’ll fix this.
—
Reply to this email directly, view it on GitHub
<#693 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSQJ54ET46OAZGU2EG2TXTZQ6QTJAVCNFSM6AAAAABLWUO6HOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBSHAZTSMRQHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/python/main/ayab/hw_test.py (4 hunks)
Additional comments not posted (4)
src/main/python/main/ayab/hw_test.py (4)
46-55: LGTM: Expanded commands list.The addition of new commands (
readEOLsensors,readEncoders,autoRead,autoTest) enhances the functionality of the hardware test dialog as intended.
77-87: LGTM: Improved button creation process.The direct instantiation of buttons and the use of
setAutoDefault(False)enhance the clarity and functionality of the button creation process.
153-160: LGTM: Enhanced button action handling.The updates to handle checkable buttons and the new commands improve the user interaction and feedback within the dialog.
166-168: LGTM: Improved solenoid control operation.The change to
setSingleand the use ofToken.setSingleCmd.valueenhance the specificity and clarity of solenoid operations.
On second thought… I didn't have the dialog in front of me when I replied, sorry. Likewise with the The thing is, I didn't design any of this UI. I just tried to make it work while changing as little of it as possible, because I'm afraid tweaking the UI is not going to help us get a release out soon. How about using this as it is now for the next release, it can't be worse than the current broken test mode that simply hangs? |
|
Fine
…On Sun, Aug 11, 2024, 3:58 PM Jonathan Perret ***@***.***> wrote:
Quit should probably be Close, as it's a modal not its own app.
Good catch, I’ll fix this.
On second thought… I didn't have the dialog in front of me when I replied,
sorry. quit here is one of the test command buttons, the command is
called quit in the API so it would be inconsistent to call it close IMHO.
It's not even in the proper place for a dialog "Close" button. I'll concede
that the fact it also closes the window is somewhat confusing. Though the
window can always be closed using the regular close button in the title bar.
Likewise with the autoTest/autoRead buttons, I agree it's not very
discoverable that they are toggles.
The thing is, I didn't design any of this UI. I just tried to make it work
while changing as little of it as possible, because I'm afraid tweaking the
UI is not going to help us get a release out soon.
How about using this as it is now for the next release, it can't be worse
than the current broken test mode that simply hangs?
—
Reply to this email directly, view it on GitHub
<#693 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSQJ5ZLX6H76NTRQTL4DMTZQ6663AVCNFSM6AAAAABLWUO6HOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBSHA3TCOJUGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@X-sam the fix for this is here, enjoy: AllYarnsAreBeautiful/ayab-firmware#199 |
This PR fixes the hardware test dialog, in simulation as well as with an actual Arduino connected. Note that this requires a recent firmware build as stack overflow issues prevented the firmware from reliably entering test mode without crashing.
Summary by CodeRabbit
New Features
setSingleCmd,setAllCmd,readEOLsensorsCmd,readEncodersCmd,autoReadCmd, andautoTestCmd.Bug Fixes
write_API6method to prevent potential issues when the serial connection is invalid.Improvements