DS402: Homing fails depending on power state, unnecessary state changes

[acolomb]

The BaseNode402.homing() method tries to enter state SWITCHED ON upon entry. I think that's unnecessary, the application should handle these transitions. But more importantly, it actually fails in many cases, namely if the previous state is SWITCH ON DISABLED, the default power-up state of most devices. That's because there is an automatic way to reach OPERATION ENABLED over multiple intermediate steps, but the library does not know how to reach SWITCHED ON from any other state than OPERATION ENABLED or READY TO SWITCH ON. In addition, setting the op_mode property will already change to SWITCHED ON only if coming from OPERATION ENABLED (which is usually a good idea to avoid unexpected movement).

So I propose to just leave out that initial state switching, but wanted to discuss here first because it will probably break some people's application on upgrades. Note that switching the operation mode to HOMING is actually safe in any power state, at least on devices I encountered.

Another change I propose is not restoring the previous operation mode, at least as an option. Just needless bus traffic and delay in case the next operation mode must be decided by the application anyway. So I would add an optional argument with the default value restore_op_mode=False. Anyone opposed?

Same goes for restoring the power state after switching the operation mode. Not really necessary and in case of homing() it will even cause additional delays by switching back and forth. Leaving OPERATION ENABLED when switching modes should also be optional, because there are valid applications where e.g. Profile Velocity and Profile Position need to be switched during operation without delay.

I will PR any agreed upon changes, so this issue is just for tracking and finding consensus.

[acolomb]

Created PR #245 with some generic clean-up changes, which are hopefully less controversial than the things mentioned here.

[acolomb]

Thanks @af-silva for merging the changes so far, #245, #247 and #248.

I will push some PRs with the above mentioned additional changes piecewise, just so we have the code available for discussion. In general, we need to be aware of breaking changes for previous users. But as far as I can see, the code was in pretty rough shape and I doubt many people had been relying on its current quirks. In any case, if the proposed new behavior will be accepted, a big warning for in the change log would be in order I suppose.

Or what's your general stance on backwards compatibility regarding the DS402 implementation?

[acolomb]

Opened #249 to fix the main issue described here.

[acolomb]

Other changes to streamline what happens during operation mode switching are proposed in #251.

[af-silva]

Thanks @af-silva for merging the changes so far, #245, #247 and #248.

I will push some PRs with the above mentioned additional changes piecewise, just so we have the code available for discussion. In general, we need to be aware of breaking changes for previous users. But as far as I can see, the code was in pretty rough shape and I doubt many people had been relying on its current quirks. In any case, if the proposed new behavior will be accepted, a big warning for in the change log would be in order I suppose.

Or what's your general stance on backwards compatibility regarding the DS402 implementation?

Hi @acolomb,

Thanks for the time reviewing and improving the code ;)
I know the code was/is in a rough shape, but was what I manage to do in the time that i have to do so and with the knowledge I have at the time. This code was used and tested in a real application, and i know there are still work to do but deliberately didn't cover all the quirks of the DS402 implementation and just what I needed for my use case.

Every contribution and discussion helping improving this library and the DS402 profile is welcome, furthermore, getting the library and the DS402 profile to a true library standard (i.e., not depending on manufactures bells and whistles) is awesome.

Or what's your general stance on backwards compatibility regarding the DS402 implementation?

Give a fare warning and break things if they improve the usage, stability and maintainability of the code base.
I don't have access right now to hardware to validate changes, but we can discuss and I can help as much as I can.

Another change I propose is not restoring the previous operation mode, at least as an option. Just needless bus traffic and delay in case the next operation mode must be decided by the application anyway. So I would add an optional argument with the default value restore_op_mode=False. Anyone opposed?

Not at all, like a said this was due to my specific use case and was ported here.

[acolomb]

Thanks for the time reviewing and improving the code ;)

Thank you for reviewing :-)

I know the code was/is in a rough shape, but was what I manage to do in the time that i have to do so and with the knowledge I have at the time. This code was used and tested in a real application, and i know there are still work to do but deliberately didn't cover all the quirks of the DS402 implementation and just what I needed for my use case.

Quite similar to my case--I am currently working on a project in $day_job where we use eight drive controllers from Nanotec with this library. So I have something to test the changes with and my application code actually relies on the changes I proposed here (see https://github.com/acolomb/canopen/tree/proposed). Each change is tested on that setup and considering that Nanotec seems to follow the DS402 standard quite closely, I assume it should work for other manufacturers as well. Have some other DS402-supporting hardware, but not in active use currently.

So if anything seems unclear or questionable whether it works, I can offer specific testing. However most changes are more of a "don't do it all in the library, allow some interaction with the app in between" nature. That's where application compatibility will break, but not relying on different drive behavior.

Give a fare warning and break things if they improve the usage, stability and maintainability of the code base.
I don't have access right now to hardware to validate changes, but we can discuss and I can help as much as I can.

Then I look forward to some comments on the various PRs :-)

Another change I propose is not restoring the previous operation mode, at least as an option. Just needless bus traffic and delay in case the next operation mode must be decided by the application anyway. So I would add an optional argument with the default value restore_op_mode=False. Anyone opposed?

Not at all, like a said this was due to my specific use case and was ported here.

I have a commit ready for this, but it would conflict with #250 and is therefore based off that. Either I wait for that to get merged or we add the other change to it. Refer to my branch mentioned above to see the desired end result.

[acolomb]

@af-silva I hope you're not drowning in my flood of DS402-related pull requests :-) I tried to base them on master as far as possible, so you can pick whichever you want to review / merge first. But there will inevitably be merge conflicts at some point. Feel free to ping me to get those fixed, or refer to my "proposed" branch for the "correct" resolution.

Looking forward to getting this module in really good shape, and thanks again for your great ground work!

[acolomb]

The proposed restore_op_mode change is implemented in #262.

[af-silva]

@acolomb Sorry for not get back to you sooner. I'm will try to check and merge your changes during this week.

[af-silva]

@christiansandberg I purpose a new version to include the changes purposed by @acolomb. What do you think?
Also, @acolomb what are you thinking in terms of roadmap for the DS402 profile?
The also needs to be updated, at least the example sample I think.

[acolomb]

@af-silva Thanks for taking time to review and merge. Unfortunately there were some faulty merge commits which I have now corrected in #265. Sorry, I could not avoid all conflicts when trying to separate changes into multiple PRs. I honestly thought there would be more discussion about the individual changes... Next time just ping me when there's a conflict, I will then rebase or merge from master. Better to go slowly than having conflicts unnoticed 😉

Also, @acolomb what are you thinking in terms of roadmap for the DS402 profile?

I'm not sure what you mean, is there some Roadmap documentation? Regarding my proposed changes, the only missing piece now is #264, which I have also tested on hardware.

The also needs to be updated, at least the example sample I think.

You mean the documentation? I took care of the "Device profiles" page, but there was not much of an example for the p402 code. Knowing what the standard is supposed to do and now having the BaseNode402 class API documented should be enough to get started though.

I still don't know where we should put the incompatibility note / warning explaining how the application code needs to adapt for the next release. @christiansandberg any preferred place or existing file / wiki / ...?

[af-silva]

Hi @acolomb, ok that's set, I resolved the merge conflicts but didn't notice that parts of the code in other commits where being lost ... sorry.

Regarding the discussion, a looked at each of the commits and everything seemed right and a like your design decisions and organization, the code is cleaner and more maintainable, and like you said, it needs more work out of the library to get things working but also covers more use cases than before (it was a bit tailored to my needs and I try to be agnostic but the time was short). Also, I don't have access to hardware right now to validate, so I trust your judgement.

By roadmap I meant the overall implementation/changes you are planing, maybe it should be better to release a new version where the warning of breaking changes to ds402 was present.

Documentation on code should suffice, but if you have some working example on how to extend and use the ds402 you could also put it on the docs... it's just a thought, you are contributing already in much needed changes.

Last, the PR with changes to the state machine i will have a closer look and share my thought, I have to revisit the documentation for the ds402.

Thanks,
Good work 💪

[christiansandberg]

Sorry for not being involved in the discussions, but I have no clue about any profiles so you seem to have it under control. Great work both of you! 👏

Are the changes big enough to warrant a new major release (2.0)? If there is a risk that behavior will change for some people I think it is best to follow semantic versioning rules and try to explain the major differences in the release notes. @acolomb could you write a proposal for what we should write?

[acolomb]

As you may have guessed, I'm also scratching my own itch mainly, but trying to give back to the community by going the extra mile. 😉 So some more needed changes may possibly come up, but with the PRs so far I consider it usable and good enough for many purposes. No immediate plans for a further roadmap. Also my application is rather complex with eight drives involved and therefore not many script-style examples for easy consumption in the documentation.

It's not a revolutionary change where I'd have thought of bumping the major version, as it just concerns a small part of the code. But you're right that proper semantic versioning would require the bump, unless we considered the DS402 profile as unofficial and unsupported icing on the cake until now.

Either way we need a description of the breaking changes, so I'll try to come up with something in the days ahead.

[acolomb]

Sorry it took a while longer, but here's my suggestion for a NEWS entry (wherever we may put that):

Support for the CiA 402 device profile for "drives and motion control" was
overhauled in this release, bringing some backwards-incompatible changes.
The goal is to make the library more flexible and easier to use from an
application requiring DS402 functions, especially when controlling several
synchronized motors.

CAUTION: The following changes will likely require adaptation if you are
         using the respective functions in your application code!

* The BaseNode402.homing() method no longer changs the DS402 power state
  machine.  A suitable state needs to be set by the application before using
  it. (#249)

* Remove the broken parameter set_new_home from the BaseNode402.homing()
  implementation, which did not work as intended. (#250)

* After homing, restore the previous operation mode only when explicitly
  requested using the restore_op_mode parameter. (#262)

* Minimize side-effects of operation mode switching.  Do not clear the
  target values and keep the power state unchanged.  This must now be
  handled in the application, but enables some use-cases where switching
  modes on the fly is desirable. (#251)

* Changes to the Controlword no longer trigger an immediate RPDO
  transmission if already configured as periodic.  That is the case when
  .start() has been called or the .period property was set externally.  For
  an RPDO whose transmission type indicates that it needs a SYNC object to
  apply, the immediate sending is also skipped.  Check the .is_periodic
  property to see whether this will change anything in your usage.

* The .op_mode property is now handled via PDO if possible, otherwise
  setup_402_state_machine() logs a warning and falls back to SDO, just like
  for the Statusword.  Note that if the PDO is expected periodically, the
  getter still blocks (up to 0.2 seconds) until a TPDO update was
  received. (#257)

* Reduce overhead in setup_402_state_machine() procedure by not switching
  the NMT state or the power state automatically.  The application should be
  responsible for that.  Only the NMT STOPPED state will now trigger an
  error before trying to read SDOs.  Reading the PDO configuration can even
  be skipped altogether using the optional read_pdos parameter. (#259)


Enhancements:

* The documentation page concerning device profiles was extended to include
  the relevant API description for the BaseNode402 class.

* All timeout and waiting durations have been moved to constants in the
  BaseNode402 class.  They can thus now be overridden per instance.

* BaseNode402.is_homed() to check whether homing is necessary, switching the
  operation mode automatically as required. (#248)

* Make PDO subscription available without calling read() or save(). (#253)

* The DS402 power state machine is now more flexible by trying to reach any
  target state even if there is no direct transition.  Some bugs were fixed
  as well, such as not trying transitions which only the drive itself can
  trigger. (#264)


Run-time optimizations:

* Supported operation modes are now cached internally to avoid reading
  object 0x6502 repeatedly as it should never change. (#247)

* Several actions which depend on an updated Statusword value now wait for a
  TPDO reception (up to 0.2 seconds), if the PDO is expected periodically.
  This can reduce some delays previously caused by checking loops with fixed
  delays. (#263)

Once we are certain where this should be documented, I will gladly adjust the formatting accordingly. While writing it up, I noticed a bug which will be fixed by #277.

[af-silva]

Great work @acolomb, for me is a go.

[friederschueler]

Status of this issue? PR is merged. Will close it for now.

[acolomb]

I think all concerns have been addressed, thus fine with closing.