DS402: Simplify setup procedure. #259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reading the PDO configuration is possible in OPERATIONAL or PRE-OPERATIONAL states, so switching that is unnecessary. The application should be responsible to handle such transitions, and the library function should be usable without disturbing the application logic. Changing the DS402 state machine to SWITCH ON DISABLED is also not necessary. The drive may be in whatever state from a previous usage, and then the change to SWITCH ON DISABLED may even trigger an exception because there is no way to reach it directly. So this transition should also be the application's responsibility.
SDOs are allowed in all but the STOPPED state. That would lead to a timeout and an SdoCommunicationError exception. Checking the NMT state here raises an exception without a timeout involved.
If the application already configured the PDOs and called .save() on the pdo.Maps object, there is no sense in reading everything back again in the setup_pdos() method. Provide an optional argument to disable that behavior. A call to subscribe to the PDOs from the network is added because that side-effect of pdo.read() is necessary for the TPDO callback to work.
Add an optional argument which is simply passed down to setup_pdos() to choose whether reading the PDO configuration is necessary.
Besides the changes regarding setup_402_state_machine(), there were numerous errors where the documentation talks about nonexistent or differently named attributes. Also fix the description regaring what the method actually does. It won't configure the TPDO1 to contain the Statusword, but only check the PDO configuration for Statusword and Controlword presence.
acolomb
force-pushed
the
simplify-setup-ds402
branch
from
3b51936 to
edc4b1c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #256 to avoid conflicts, thus should be merged after it. If needed I can rebase after merging #256.EDIT: Rebased on top of master with #256 merged.
The
BaseNode402.setup_402_state_machine()method does some unnecessary things that should rather be handled in the application code. Switching the NMT state and DS402 power state was already questioned in a code comment, and I agree that it should not happen here unconditionally.In the same spirit, reading all of the node's PDO configuration is detrimental to the application in case it has previously called
pdo.save(), causing lots of needless SDO traffic. So that part is made optional through a function argument.Overall this should make the library's convenience methods more useful in context of a more complex application. The downside of course is that a simple script to get some DS402 controller working would need a little more state switching outside of the library code.