Only send (costly) SW Stepper Enable when toggling

[0r31]

Hi,

I propose this PR to optimize enable write operations when using software enable feature. The goal is to avoid overload/latency on SPI/UART lines.

The implementation is pretty naive:

• Add an enable flag in the planner
• Update the flag for each axis everywhere DISABLE_AXIS_##AXIS/ENABLE_AXIS_##AXIS is used.

Feel free to discuss the usefulness of such PR.
Don't hesitate to provide implementation improvements (e.g. originally i wanted to add more and more macros to avoid repeating the same piece of code everywhere).

Regards,
Orel

[sjasonsmith]

A few general questions for you:

1. Does this solve a noticeable performance problem for you?

2. What hardware are you using this with?

[0r31]

Hi @sjasonsmith ,

First, thanks for your propositions ! The code will look nicier. Will do this asap.
What about the variable initialization in planner.cpp?
I can't replace xyz_bool_t Planner::axis_enabled{false}; by xyz_bool_t Planner::axis_enabled.set(false, false, false);, can I?

Then to answer your questions:

Does this solve a noticeable performance problem for you?

Short answer : Yes.
We faced micro-pauses in curves.
The root cause seems to be that for each step, we ensure enable is ... enabled. For hardware enable, no problem as it writes on a physical pin. For software enable, the registry of the driver is updated through SPI/UART which leads to a kind of latency.

Before:

After:

What hardware are you using this with?

I can't really answer as the board is not yet marketed. What i can say : it uses many TMCs in software serial mode + software enable.

Regards,
Orel

[sjasonsmith]

That is a pretty impressive difference in print performance. My primary concern was making sure that this was solving a real issue and wasn't change just for the sake of change.

I can't replace xyz_bool_t Planner::axis_enabled{false}; by set() usage, can I?

No, in this case initialization and setting are different. To be completely honest I'm not 100% sure of the exact initialization behavior this will have, I will need to refresh my memory.

What about the variable initialization in planner.cpp?

I will need to look at the code a little to see if anything else seems better.
I had the thought that maybe the checks could be moved inside the DISABLE_AXIS_X (and similar) macros. I don't know yet whether that is practical.

It's pretty late here at the moment, so I will probably look back at this tomorrow and get back to you.

[0r31]

I had the thought that maybe the checks could be moved inside the DISABLE_AXIS_X (and similar) macros. I don't know yet whether that is practical.

That's what i wanted to do initially but i didn't really manage to handle it correctly (planner not accessible in indirection.h) and I was focused on solving the issue.
Issue solved -> I've created this PR to get any advice and good practices in the implementation itself ;)

It's pretty late here at the moment, so I will probably look back at this tomorrow and get back to you.

We can take time on it, no hurry. The issue is solved for us and we can continue to work on board/machine development.

Good night and regards,
Orel

[rhapsodyv]

Isn't better (if is possible, of course), put IF_ENABLED(SOFTWARE_DRIVER_ENABLE, planner.axis_enabled.x = false); inside DISABLE_AXIS_X... and IF_ENABLED(SOFTWARE_DRIVER_ENABLE, planner.axis_enabled.x = true); inside ENABLE_AXIS_X, and so on?
Ok, you already talked about that before. :-)

[0r31]

@rhapsodyv : it is a good idea, but i wonder where to put the axis_enabled variable other than in planner.h so that it can be both unique and accessible in indirection.h.

As SOFTWARE_DRIVER_ENABLE is only used in trinamic stuff, I guess trinamic.h is a good bet, but not sure.

Regards,
Orel

[sjasonsmith]

I think I can get it all into the indirection files. I'm playing with ideas now, and will post something for you to test.

[sjasonsmith]

@0r31 take a look at the change I just pushed. I will need you to test it and verify it works as expected, for both enabling and disabling motors.

It seems to all resolve to the proper content in the macros when I examine in the IDE but I haven't actually run it on a printer yet.

[0r31]

Hi @sjasonsmith ,

Many thanks! It looks exactly as it should be!
Much cleaner than the first version...

Will test it properly (disable/enable/printing/...) next Monday morning as I have no access to the printer during the weekend.

Regards,
Orel

[sjasonsmith]

@0r31 it looks like thinkyhead ran with mine, made it a bit easier to understand, and merged it. Let me know on Monday if you have any issues with it.

[0r31]

Hi @sjasonsmith and @thinkyhead ,

Enabling/disabling Z4 is missing in ENABLE_AXIS_Z/DISABLE_AXIS_Z macros.

I am in the testing process (M17 and M18 first).

[sjasonsmith]

@0r31 fixed. Let us know if anything else seems amiss.

[0r31]

M17 X - 👌
M17 Y - 👌
M17 Z - 👌
M18 X - 👌
M18 Y - 👌
M18 Z - 👌
M17 - 👌
M18 - 👌

[0r31]

Print - 👌

No more micro-pauses, no more blobs, same behavior as what was observed after the first implementation version.

The topic is closed for me. Thanks guys!