Gmoccapy: Multiple sim configs fail to launch

[Sigma1912]

Testing on current master:

Example

None of the Gmoccapy sim configs in configs/sim/gmoccapy/non_trivial_kinematics launch.

Possible problem

Comparing the terminal output to 2.9.10 when lauching configs/sim/gmoccapy/non_trivial_kinematics/Gantry/gantry_mm.ini points to a possible clash between the gmoccapy-internal ini value check and the new linuxcnc ini parser

2.9.10 (config launches):
[Gmoccapy.GMOCCAPY.GETINIINFO][WARNING] Wrong entry [DISPLAY] CYCLE_TIME in INI File! Will use gmoccapy default 150 (getiniinfo.py:56)

current master (config fails)

/home/user/git/linuxcnc-master/configs/sim/gmoccapy/non_trivial_kinematics/Gantry/gantry_mm.ini:9: warning: Trailing character(s) in signed integer conversion ([DISPLAY]CYCLE_TIME='0.200')

Further testing shows that a missing [DISPLAY] CYCLE_TIME entry in configs/sim/gmoccapy'/moccapy.ini is handled by gmoccapy and does not cause a launch failure.

[Gmoccapy.GMOCCAPY.GETINIINFO][WARNING] Wrong entry [DISPLAY] CYCLE_TIME in INI File! Will use gmoccapy default 150 (getiniinfo.py:56)

However in 2.9.10 the same missing ini value causes the launch to fail with this error:

[Gmoccapy][ERROR]  Found an error!
The following information may be useful in troubleshooting:

Traceback (most recent call last):
  File "/home/user/git/linuxcnc-master/bin/gmoccapy", line 6575, in <module>
    app = gmoccapy(sys.argv)
          ^^^^^^^^^^^^^^^^^^
  File "/home/user/git/linuxcnc-master/bin/gmoccapy", line 514, in __init__
    GLib.timeout_add( cycle_time, self._periodic )  # time between calls to the function, in milliseconds
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3/dist-packages/gi/overrides/GLib.py", line 612, in timeout_add
    return GLib.timeout_add(priority, interval, function, *user_data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: Argument 1 does not allow None as a value
 (gmoccapy:75)

which indicates a problem with a value returned from ini parsing/checking

linuxcnc/src/emc/usr_intf/gmoccapy/gmoccapy.py

Lines 513 to 514 in 0bb86dd

	cycle_time = self.get_ini_info.get_cycle_time()
	GLib.timeout_add( cycle_time, self._periodic ) # time between calls to the function, in milliseconds

linuxcnc/src/emc/usr_intf/gmoccapy/getiniinfo.py

Lines 49 to 57 in 0bb86dd

	def get_cycle_time(self):
	temp = self.inifile.getint("DISPLAY", "CYCLE_TIME")
	try:
	return temp
	except:
	message = ("Wrong entry [DISPLAY] CYCLE_TIME in INI File! ")
	message += ("Will use gmoccapy default 150")
	LOG.warning(message)
	return 150

[BsAtHome]

2.9.10 (config launches):
[Gmoccapy.GMOCCAPY.GETINIINFO][WARNING] Wrong entry [DISPLAY] CYCLE_TIME in INI File! Will use gmoccapy default 150 (getiniinfo.py:56)

This is because [DISPLAY]CYCLE_TIME is converted to integer and that fails on a floating point value. The result is invalid and the default kicks in.

current master (config fails)
/home/user/git/linuxcnc-master/configs/sim/gmoccapy/non_trivial_kinematics/Gantry/gantry_mm.ini:9: warning: Trailing character(s) in signed integer conversion ([DISPLAY]CYCLE_TIME='0.200')

In master it also converts to integer (straight forward conversion), but it fails because the new INI-parser sees trailing context. The result is zero (0) and not an exception.

Both 2.9 and master are wrong. The [DISPLAY]CYCLE_TIME is a floating point number (value in seconds) and must be read like that. The real conversion must be:

    def get_cycle_time(self):
        return int(self.inifile.getreal("DISPLAY", "CYCLE_TIME", fallback=0.15) * 1000.0)

The 2.9 branch needs to be fixed to read:

    def get_cycle_time(self):
        try:
            temp = self.inifile.find("DISPLAY", "CYCLE_TIME")
            return int(float(temp) * 1000.0)
        except:
             message = ("Wrong entry [DISPLAY] CYCLE_TIME in INI File! ") 
             message += ("Will use gmoccapy default 150") 
             LOG.warning(message) 
             return 150 

[grandixximo]

@BsAtHome confirmed on master. The chain, as far as I can tell:

get_cycle_time() in src/emc/usr_intf/gmoccapy/getiniinfo.py now does:

temp = self.inifile.getint("DISPLAY", "CYCLE_TIME")
try:
    return temp
except:
    ... return 150

With the new parser, getint() on CYCLE_TIME = 0.200 returns 0 (with the trailing-characters warning); on a missing entry it returns None. Neither raises, so the except is dead code. 0 flows into GLib.timeout_add(0, self._periodic) (gmoccapy.py:514), spinning the main loop; None gives the TypeError traceback.

In 2.9 this was find() + int(temp) inside the try, so int("0.200") raised and the fallback to 150 ran. The behavior change comes from the find() => getint() swap in 2f57090: the old code relied on int() raising, the new accessor never raises and signals trouble via its return value.

Fix direction: restore find() + int() (exact 2.9 semantics), or is there a preferred new-API idiom? Note fallback=150 alone does not cover "0.200", since conversion succeeds and returns 0 rather than falling back.

I audited the other conversions in 2f57090. Most are fine, but the same trap appears in a few more places:

• getiniinfo.py:get_tool_sensor_data() + gmoccapy.py:1102: if not xpos or not ypos or ... now sees 0.0 as falsy, so a tool setter at X=0 or Y=0 disables tool measurement. Old find() returned the truthy string "0".
• Same pattern in share/gscreen/skins/silverdragon/silverdragon_handler.py:246 and :274.
• lib/python/gladevcp/macro_widget.py:56: invalid NO_FORCE_HOMING used to raise and the except set no_home_required = 1; now getbool silently returns False, inverting the failure mode.

Restoring the 2.9 form of get_cycle_time() locally makes these configs launch normally again.

[Sigma1912]

[DISPLAY]CYCLE_TIME is a floating point number (value in seconds)

Seems that there might be diverting ideas about the unit in different GUIs. Gmoccapy apparently expects a value in _milli_seconds:

linuxcnc/src/emc/usr_intf/gmoccapy/gmoccapy.py

Line 512 in 0bb86dd

# CYCLE_TIME = time, in milliseconds, that display will sleep between polls

Also confirmed by looking at various sim configs:

./configs/sim/qtdragon_hd/qtdragon_hd_xyz/qtdragon_next_xyz.ini:46:CYCLE_TIME = 100
./configs/sim/qtdragon_hd/qtdragon_hd_xyz/qtdragon_next_xyz.ini:112:CYCLE_TIME = 0.010
...
./configs/sim/axis/random_tc.ini:9:CYCLE_TIME = 0.100
./configs/sim/axis/random_tc.ini:42:CYCLE_TIME = 0.001
...
./configs/sim/gmoccapy/lathe_configs/lathe_C.ini:19:CYCLE_TIME =            100
./configs/sim/gmoccapy/lathe_configs/lathe_C.ini:69:CYCLE_TIME =            0.001

[BsAtHome]

BTW, if the message is to be retained in master, then it can be:

    def get_cycle_time(self):
        temp = self.inifile.getreal("DISPLAY", "CYCLE_TIME")
        if temp is not None and temp >= 0.001;
            return int(temp * 1000.0)
         message = ("Wrong entry [DISPLAY] CYCLE_TIME in INI File! ")
         message += ("Will use gmoccapy default 0.150")
         LOG.warning(message)
         return 150

The message in 2.9 should also say "0.150" instead of 150.

[BsAtHome]

• getiniinfo.py:get_tool_sensor_data() + gmoccapy.py:1102: if not xpos or not ypos or ... now sees 0.0 as falsy, so a tool setter at X=0 or Y=0 disables tool measurement. Old find() returned the truthy string "0".
• Same pattern in share/gscreen/skins/silverdragon/silverdragon_handler.py:246 and :274.
• lib/python/gladevcp/macro_widget.py:56: invalid NO_FORCE_HOMING used to raise and the except set no_home_required = 1; now getbool silently returns False, inverting the failure mode.

Thanks for looking deeper into this. The problem is that the old code has been winging it at interpreting INI values. That was the whole point in creating the new INI-parser, to leave the value parsing to the parser. Now there are discovered multiple instances where local interpretations/assumptions are no longer valid. These code snippets need to be fixed so that work properly. Especially the boolean issues are again a good example why the old ways were not nice. The whole boolean misinterpretation stuff prompted the new INI-parser in the first place.

[grandixximo]

@BsAtHome getreal * 1000 silently breaks gmoccapy's own configs: gmoccapy.ini, 9_axis.ini, lathe_C.ini and friends carry [DISPLAY]CYCLE_TIME = 100 or 150 in milliseconds, matching gmoccapy's documented default of 150 ms. Those become 100 to 150 s poll periods. Both units exist in the wild under the same key, and the GUI reading it defines which applies. No fixed formula reads both correctly.

So I'd keep gmoccapy on its documented milliseconds: a non-integer value is a config error, warn, fall back to 150. That is exactly the 2.9 behavior and never silently misreads a valid config. A doc note stating the unit would help.

And parser-side, worth considering: getint returning a plausible-looking 0 for "0.200" is what turned this into a silent hang. Treating trailing junk as conversion failure (warning + fallback) would have surfaced it immediately as the config error it is.

[BsAtHome]

Making the parser fail on that kind of input may break other things. That is why it warns. The whole issue stems from inappropriate formats and incomplete parsing in the first place. Fixing all instances where INI stuff is interpreted wrongly is hard because we are pullingdragging legacy and is a slow and painful process.

For the fix in this case. Maybe:

def get_cycle_time(self):
        temp = self.inifile.getint("DISPLAY", "CYCLE_TIME")
        if temp is not None and temp > 0;
            return temp
         message = ("Wrong entry [DISPLAY] CYCLE_TIME in INI File! ")
         message += ("Will use gmoccapy default 150")
         LOG.warning(message)
         return 150

[BsAtHome]

• lib/python/gladevcp/macro_widget.py:56: invalid NO_FORCE_HOMING used to raise and the except set no_home_required = 1; now getbool silently returns False, inverting the failure mode.

The original code did:

no_home_required = int(self.ini.find("TRAJ", "NO_FORCE_HOMING") or 0)

That defaults to 0 (int(None or 0) --> 0) on not found. Yes, it would have triggered a different situation on invalid boolean. However, the boolean saga is why the new parser was created and it will warn. For the situation at hand, the default is returned as indicated by the code. So I see no problem with that. The code has the wrong value in the except clause IMO because it now has two distinct defaults that contradict. FWIW, the whole except clause could only be triggered by ini parsing failure or int() failure. When the ini file fails to parse, then your problems are, hm, how to say, a lot worse. The wrong boolean value in the NO_FORCE_HOMING is of no real consequence to have it default as is it was not present.