2.9 Gmoccapy: fix issues #3447 and #982

[Sigma1912]

This fixes the two bugs in issue #3447 and also the one in #982

• Fix the display of very large numbers for spindle speed and 'Vc' in the gui when using G96
• Fix 'spindle.0.speed-out' pins being reset to wrong values when using G96
• Fix wrong spindle speed set when using M1 with optional stop enabled

NOTE: Generally the python status channel can not always be relied on reflecting the current state.
Examples:

• It does not correctly reflect the actual spindle speed commanded by motion when using G96 as it will display either the optional 'D' value or the internal default +-'1e30'
• It does not correctly reflect the state of the machine when using M1 and optional stop enabled

Because of this 'self.stat.spindle[0]['speed']' is no longer used

• The 'Spindle [rpm]' value will now track hal pin 'spindle.0.speed-out'
• The 'S' value in the 'Gcodes' frame now tracks 'self.stat.settings[2]'

[Sigma1912]

There is still a bit of a blemish, using G96 without a 'D' word makes the 'spindle.0.speed-out' value climb to 1e30 when X approaches zero (this is to be expected). Of course any actual spindle will max out at a realistic speed but the GUI will try to display the 'spindle.0.speed-out' value and thus the problem with the very very large number reappears.

[Sigma1912]

Looks like none of the other GUIs handle this case so it's probably fine like this.

So this PR is ready as far as I am concerned.

[BsAtHome]

The 1e30 value is a signalling value to indicate to set the maximum (emc/rs274ngc/interp_convert.cc:4473 in 2.9 or line 5013 in master). The value is, apparently, an arbitrary large value to signal the specific condition that the D-word is missing.

As there does not seem to be a way to get the actual maximum value for the spindle, then the signalling value should be taken as an instruction to display (for example) the word "maximum" instead of an obviously wrong large value. This is still sub-optimal because the GUI should be able to tell the user what the maximum is. However, it is better to use the signalling value than try to display it.

Handling the signalling value should be done, regardless, because the value is used in a calculation (with spindle_override and a widget's value assigned to spindle_override_in) that will obviously result in wrong values. When you cannot tell the actual value, then the derived values must be displayed as unknowns too. Otherwise you give the user the wrong impression of something that is knowingly not true (rule number 1, never knowingly display bad data, but tell that the data is "bad" or "unknown" and why if known).

[Sigma1912]

Note though that we are not talking about the spindle feedback rpm we are talking about the commanded spindle rpm. In this sense it is not necessarily wrong information because it IS the value that 'spindle.0.speed-out' is set to by the motion controller. The flaw here is that the number does not fit the screen.
As an alternative, we could easily have the gui look for a INI file entry 'MAX_SPINDLE_SPEED' (as a matter of fact I believe that this is required to calibrate the output signal on some mesa boards) but displaying that value instead would actually not reflect the value of 'spindle.0.speed-out'.

One could argue that the motion controller (ie spindle.0 pins) should really not output spindle speed values that are beyond what the hardware can handle. After all we go through great pains to limit axis speeds to the limits defined in the ini file.

[BsAtHome]

Note though that we are not talking about the spindle feedback rpm we are talking about the commanded spindle rpm. In this sense it is not necessarily wrong information because it IS the value that 'spindle.0.speed-out' is set to by the motion controller.

It doesn't matter whether it is feedback or commanded. The value is wrong because the "value" is not a value, but a signal to interpreter(s) of that value that something else is happening. Hence, a signalling value. The GUI must handle that case and signal the operator accordingly.

The flaw here is that the number does not fit the screen. [snip]

No, the flaw is that a signalling value is used as an indicative (normal) value.

One could argue that the motion controller (ie spindle.0 pins) should really not output spindle speed values that are beyond what the hardware can handle. After all we go through great pains to limit axis speeds to the limits defined in the ini file.

That is true too. The whole concept of the 1e30 value as indication that the D-word is missing is probably the wrong way to handle the problem. But changing that would likely be much more difficult and may not be back-portable. Anyway, it would firstly involve to map the use-cases and see how a better solution would need to be designed.

[Sigma1912]

Note though that a missing 'D' value does not automatically mean that the commanded speed will actually reach +-1e30. It may even stay within the physical bounds of the spindle, it may be 1e10 or 1000000. It all depends on how close the X position value comes to zero.
Changing the default to say '999999999' instead of '1e30' would fix all the GUI display issues without ever keeping a real spindle from reaching max speed.

[BsAtHome]

Note though that a missing 'D' value does not automatically mean that the commanded speed will actually reach +-1e30. It may even stay within the physical bounds of the spindle, it may be 1e10 or 1000000. It all depends on how close the X position value comes to zero.

Of course I know. That is why I keep telling that the 1e30 is a signalling value. Is is a signal that the speed should be some maximum configured (supported).

Please note that the value zero (0) also is a signalling value used in the code which is handled separately.

Changing the default to say '999999999' instead of '1e30' would fix all the GUI display issues without ever keeping a real spindle from reaching max speed.

A GUI with lack on knowledge of the actual value can only guess a number. Guessing in the GUI is Bad Design^TM and leads to catastrophe because the operator may be fooled by what is displayed. Therefore, using any guessed number is bad because you cannot distinguish a bad number from a good number. If the GUI can't tell you the actual value, then it must tell the operator so and, for example, should write "Maximum" or "Overflow" instead of a number.

All of this is nothing else than adding another case in the GUI. You already handle the case where the value is zero (0). What I suggest is that you add a case where self.stat.spindle[0]['speed'] >= 1e30 and put in an indicative label for that specific condition, like in (just one example):

  if self.stat.spindle[0]['speed'] >= 1e30:
    self.widgets.active_speed_label.set_label("maximum")
    self.widgets.lbl_spindle_act.set_text("S unknown")
  else:
    if self.stat.spindle[0]['speed'] == 0:
      speed = self.stat.settings[2]
    else:
      speed = self.stat.spindle[0]['speed']
    self.widgets.active_speed_label.set_label("{0:.0f}".format(abs(speed)))
    self.widgets.lbl_spindle_act.set_text("S {0}".format(int(round(speed * self.spindle_override))))

Feel free to use any INI values if available or any other value that represents reality. But, signalling values should generally not be used as indicative values.

[Sigma1912]

But, signalling values should generally not be used as indicative values.

Which is why this

 else:
      speed = self.stat.spindle[0]['speed']

was changed to

 else:
      speed = hal.get_value("spindle.0.speed-out")

Because in G96 mode 'self.stat.spindle[0]['speed']' contains the signalling value while 'hal.get_value("spindle.0.speed-out")' contains the actual commanded value (which is the one I want to display in the GUI).

The only thing that should be handled now is for the GUI spindle display to increase to the maximum number that can be comfortably displayed in the allotted screen space (eg 99999999), then stay at that value and give an indication that the display is maxed out and the actual value is larger. That is pretty standard procedure in a display.

Thanks for the code example, although it would have to be something like this:

 if hal.get_value("spindle.0.speed-out") >= 999999999:
    self.widgets.active_speed_label.set_label("maximum")
    self.widgets.lbl_spindle_act.set_text("S Out of Range"

That should not be too difficult.

[Sigma1912]

A catch for large (positive and negative) S and Vc values has been added. One million rpm should be plenty but it can be adjusted:

        self.max_spindle_disp = 999999 # maximum display value for 'S' and 'Vc'

[Sigma1912]

This is ready to merge as far as I am concerned.

[BsAtHome]

From an interaction design and UX perspective, it is wrong to use decimals (a decimal value) if you want to indicate that a value is out of bounds. The use of "<999999" or ">999999" is misleading because the visual seen by the operator is a decimal number and you cannot guarantee a difference between a real value and the out of bounds value.

[BTW, the real problem is that you cannot immediately see that 999999 has a distinctive value that is not the same as 99999. We see in patterns and large repetitive sequences larger than four characters/digits are very hard to get right at a glance. You must look very carefully to get the number right and then you have to make the interpretative step of what it means. Therefore, the hurdle for our brain is very large to "see" what is going on.]

The correct way to indicate an overflow/underflow situation is to name it as such. There has to be a distinctive change that the eye (and brain) can catch that something is out of bounds or noteworthy. Therefore, a much better indication is calling it "<overflow" and ">overflow" or "<maximum" and ">maximum". That makes a distinct visual impression that cannot be mistaken for a numerical value.

[zz912]

Or easy "overflow ERROR"

[Sigma1912]

I appreciate your thoughts but in my opinion you are overthinking this.

All the '><999999' patch does is prevent the GUI trying to resize the window and display the crazy numbers 'spindle.0.speed-out' climbs to when using G96 without a D value and X goes to zero. This fixes a glitch in the GUI that's simply an irritation to the operator.
The fact that the 'spindle.0.speed-out' pins go to crazy large numbers is not an error the operator needs to be made aware of. The machine works perfectly fine because the spindle goes to max velocity as it should at the end of the cut and the program continues.

The actual bugs this PR solves are:

1. The GUI overriding 'spindle.0.speed-out' to a wrong value (ie the value given in the S word which is fine for G97 but is very wrong for G96). This bug is in the Gmoccapy code.
2. The GUI displaying the max spindle command value (ie either the default 1e30 or the optional D word) instead of the actual value of 'spindle.0.speed-out'. This is really a bug in the python status code but I can't fix that. (self.stat.spindle[0]['speed'] should be the same as 'spindle.0.speed-out')

[Sigma1912]

The next minor blemish in my eyes it the 'S' in front of the 'Spindle [rpm]' value. Because it is NOT really the S value passed in the Gcode.
But I don't want to pack that into this PR.

[hansu]

The next minor blemish in my eyes it the 'S' in front of the 'Spindle [rpm]' value. Because it is NOT really the S value passed in the Gcode.

I agree, the "S" does not belong there.

I also reworked the section with the velocities and feed rates. Feel free to say your opinion:

I could remove the "S" in this context.

Unfortunately I didn't find the time to finalize this Gmoccapy version, but it's almost done.

For a preview, see this branch: https://github.com/hansu/linuxcnc/commits/gmoccapy-3-5-1

[Sigma1912]

I could remove the "S" in this context.

Please do.

Thanks for the preview. Since you ask, I find the vertical offsets make things a bit busy. I've rearranged things slightly:

[hansu]

Thanks for the preview. Since you ask, I find the vertical offsets make things a bit busy. I've rearranged things slightly:

Looks better indeed, but also makes the slider much smaller 🤔

[Sigma1912]

Yes it does but considering that those sliders are really only a visual indicator and cannot be used to adjust the values then it may be worth trimming them abit.

[hansu]

If I remember correctly it worked earlier to modify the slider via touch without the +/- buttons? But maybe I am wrong...

[Sigma1912]

That's possible but it must have been quite a while ago as I still have an installation with Version 3.1.x and it's not working there.

[hansu]

Probably the versions before Gtk3.

[andypugh]

The GUI overriding 'spindle.0.speed-out' to a wrong value (ie the value given in the S word which is fine for G97 but is very wrong for G96). This bug is in the Gmoccapy code.

This is a serious bug (in my opinion) and it's an important fix.

The GUI displaying the max spindle command value (ie either the default 1e30 or the optional D word) instead of the actual value of 'spindle.0.speed-out'. This is really a bug in the python status code but I can't fix that. (self.stat.spindle[0]['speed'] should be the same as 'spindle.0.speed-out')

This I see as more of a graphical/GUI inelegance. (and the resize causes graphical glitches) so I am prepared to accept any improvement, even if not entirely optimal.

So I am happy to merge this to 2.9

[andypugh]

Just a thought... Should the spindle speed swap to units of mm/min when in G96 mode, and display the surface speed command?
On reflection I think probably not, but it is a valid option.

[hansu]

Just a thought... Should the spindle speed swap to units of mm/min when in G96 mode, and display the surface speed command?

I would not switch the units

[hansu]

@Sigma1912 what do you think about having the units not in bold? Here for example for Current Velocity:

[Sigma1912]

@Sigma1912 what do you think about having the units not in bold? Here for example for Current Velocity:

Yes, I think that is a good idea

[gmoccapy]

touch the unit stuff, why not placing them right of the numbers and not showing them in the frame title at all?

[hansu]

The correct way to indicate an overflow/underflow situation is to name it as such.

What about displaying "#####" which is common in spreadsheet applications when the column is too narrow?

[hansu]

touch the unit stuff, why not placing them right of the numbers and not showing them in the frame title at all?

Hmm I think for the spindle speed it is clear that it's rpm.

And for the feed rates, I think it would be too much to have it displayed at every value:

I think this looks quite nice:

[Sigma1912]

What about displaying "#####" which is common in spreadsheet applications when the column is too narrow?

In my opinion '>999999' is more informative to an operator than '#####' which could mean anything.

[Sigma1912]

Hmm I think for the spindle speed it is clear that it's rpm.

Not so clear for G96.