Fix spindle speed for remap

[SebKuzminsky]

The multi-spindle support that went into 2.8 broke remap's ability to read the requested spindle speed. The error can be seen by running sim/axis/remap/getting-started/demo.ini. The problem is that the interpreter doesn't have just one spindle speed now, it's got array of them, one for each spindle.

Remap uses the interpreter's internal interpreter python module to access the interpreter's internal state, including spindle speed. The interpmodule source was changed so it compiles with the new multi-spindle code, but fails at runtime (as demonstrated by that config).

This PR changes the interpmodule speed property from being a float to being an array, using the same infrastructure as the other arrays (active_g_codes, active_m_codes, etc). The remap example that touches spindle speed now works.

[andypugh]

Looks better than the current (broken) setup to me. (sorry about breaking it)

One concern is that the interface only knows how to handle spindle.0. Did you look at adding the selected spindle into the logic?

Even without that, though, it is at least less broken with this fix.

[SebKuzminsky]

One concern is that the interface only knows how to handle spindle.0. Did you look at adding the selected spindle into the logic?

I did not look into handling the selected spindle in the remap code, no. In fact, I didn't know there was even the concept of a selected spindle, or how to find out which one is selected...

The interpmodule exports the full array of spindle speeds, so if some intrepid remapper wanted to, it would now be somewhat easier.

There are also identical bugs in several more of interpmodule's spindle properties, which this PR does not address...

So yeah, this is a definite fix, but there's plenty of other broken stuff in this particular neighborhood...

[andypugh]

I didn't know there was even the concept of a selected spindle, or how to find out which one is selected...

It's a fairly weak concept, but it is needed to determine which spindle speed is being used for feed-per rev or spindle-synched motion

https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/rs274ngc/interp_internal.hh#L746

settings->active_spindle

[jepler]

Seems fine, though please finish talking through the default spindle aspect of it before merging.

[SebKuzminsky]

What's a typical gcode sequence demonstrating the use of active_spindle? Would something like S1000 $1 do it?

[SebKuzminsky]

@andypugh I added active_spindle as a property of interpmodule, and added a test that introspects a bit about spindle state. It does not appear that active_spindle is set in everyday use of S and M3/M4. What should I be testing here? Can you tell me if I'm on the right track?

[andypugh]

I changed my mind on "active_spindle" a few times, eventually deciding that it probably shouldn't be modal in the conventional sense. (I am open to discussion on this point. In fact I would welcome it)
But I decided that it had to exist for feed-per-rev and spindle-synched-motion.
So, really, it represents the number of the spindle that is controlling another motion.

But, I think it is soon enough in the history of the feature (like, there are few enough users) that we could consider making it more modal.

So:

M3 S100 $0
M3 S400 $1
M3 S1000

Would set spindle 1 to 1000, not spindle 0. (Without checking I don't actually remember which it does. It definitely sets one of them...)

[SebKuzminsky]

The G-code in your comment ends with spindle 0 at 1000 rpm and spindle 1 at 400 rpm.
I'm not a fan of modal behavior, so this behavior seems correct to me.

Since LinuxCNC (apparently) doesn't have a persistent/modal/default spindle, and defaults to spindle 0 unless the user specifies a different one with the $ word, I think the sim/axis/remap/getting-started config is fine as it is. The INI doesn't specify a [TRAJ]SPINDLE value, so the simulated machine only has one spindle anyway.

I modified the config to have multiple spindles, but remap doesn't know about the $ word and can't pass it through to the python remap function... And fixing that is beyond the scope of this PR.

Any objections to me merging this as it is?

[c-morley]

Thinking out loud...
How about having a M/g code to select the active spindle?
In the future there will probably be a need to have codes for spindle sync and one will need to pick the 'master' spindle.

[andypugh]

Any objections to me merging this as it is?

No, do it.

[SebKuzminsky]

How about having a M/g code to select the active spindle?
In the future there will probably be a need to have codes for spindle sync and one will need to pick the 'master' spindle.

I don't know what you mean by this.

The spindle-synchronized moves currently accept an optional $-word (eg G33.1) which specifies which spindle the cartesian motion should be synchronized with.

The non-synchronized commands that affect a spindle (e.g. M3/M4/M5/S) also all take an optional $ argument to select the spindle, or default to spindle 0 (as verified above).

So I guess I don't understand what the current active_spindle Interpreter variable does, or what @c-morley is suggesting above.