DS402: Fix logic errors in the power state machine and generalize it. #264
Conversation
The NOT READY TO SWITCH ON and FAULT REACTION ACTIVE states can only be reached if the drive encounters an error or is still busy with its self-testing. Raise an exception in case these states are requested via the property. In extension, only the drive can trigger a transition to the FAULT state, so that is never valid as an end goal requested by the user.
Go through all possible target states and display where the next_state_for_enabling() function would lead to based on the original state. Mark transitions which can happen directly because they are in the TRANSITIONTABLE.
Mock up a BaseNode402 object and compare the _next_state() behavior to the simple lookups from the underlying transition tables.
Don't special case the OPERATION ENABLED state, as the mechanism formulated for it is actually usable for almost all states. Instead, check whether there is a direct transition and return that immediately before consulting next_state_for_enabling().
The goal is to provide automatic transition paths for any state, not only OPERATION ENABLED. Reflect that in the naming, without changing the actual logic yet.
As there is a direct transition from QUICK STOP ACTIVE to OPERATION ENABLED, remove that from the transition paths. Instead go through the SWITCH ON DISABLED state, closing the cycle to make it work for anything between SWITCH ON DISABLED and OPERATION ENABLED. Also remove the self-reference OPERATION ENABLED to itself, which is useless. The whole state changing code will only be called if the target state and the current state do not match.
Transitions 7 and 10 are duplicated and certainly wrong in the quickstop context. The only transition toward QUICK STOP ACTIVE is from OPERATION ENABLED.
af-silva
left a comment
af-silva
left a comment
There was a problem hiding this comment.
Maybe it would be better to move the test script into the test folder at the root or create a new folder for tests that target the DS402 profile (e.g., canopen/test/ds402).
|
@af-silva did you have a chance to test yet or is the approval only related to reviewing the code? It's running fine in my application so from my side, I think it's ready to merge. |
|
Sorry your reply was showing up delayed... Okay I will try to move it to a subdirectory. Not sure how to import the package then, as relative imports can only be used from within the package. |
|
Never mind about the import question. I see the existing test files are somewhat different, they are unit tests. The new script added here is rather meant to be used as a manual diagnostics tool. Not sure how that should be integrated into the unit test suite, or if it should just live within the same directory structure but not really be hooked up to the test suite... |
It's just from reviewing the code. Here I have to trust you for validation on actual hardware, since I don't have access to it right now. Everything seems ok, you spot the wrong transitions that I have introduce and fix them, nice work! |
hum ok, maybe then crate a test or tools inside the ds402 profile, to better separation and even allowing the creation/extension of other tolls? I know the file is called "test_" but I would put it inside a folder for tests or tools... |
2 participants
Extend the DS402 State Machine implementation to reach more states automatically by going through several transitions. So far that was only possible for the OPERATION ENABLED state. Add a testing script to verify how the logic works and fix some bugs found using the test script.
See individual commit messages for detailed explanations.