search

LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.81k stars 1.16k forks source link

/src/emc/usr_intf/emcrsh.cc function getToolOffset. Wrong param in snprintf. #2784

Open johnchikov opened 11 months ago

johnchikov commented 11 months ago

I think in emcrsh.cc (line 1677) use wrong argument type in snprintf instead of %f is %d

smoe commented 11 months ago

This refers to https://github.com/LinuxCNC/linuxcnc/blob/7d9b5e504c1121f6d0be36120cd03a75ebf206da/src/emc/usr_intf/emcrsh.cc#L1677 and the question what the type emcStatus->task.toolOffset.tran.z may possibly be. That should be a double, right? And then, indeed, a "%f" should possibly substitute the "%d".

If that is truly an error then this is also in version 2.9, 2.8, 2.7 and earlier, so one could assume.

smoe commented 11 months ago

I just checked. It is a double since the ToolOffset is an EmcPose , trans is a PmCartesian of which the coordinates are doubles.

rmu75 commented 11 months ago

%f (or %g / %e) is the correct specifier for a double in printf, so as it is now it probably prints some garbled value. Even if repaired that method is somewhat incomplete as there may very well be additional offsets (besides Z).

smoe commented 10 months ago

Since the value was garbled, the function cannot have been used much and, we have some freedom to correct it. What do you suggest? Add Z to the function name - and add variants for X and Y?

rmu75 commented 10 months ago

This is a can of worms. Not sure if we want to open it. in emcrsh after a quick glance, WCS offset commands return X, Y, Z and A, B, C offsets, linuxcnc knows of additional U, V, W offsets (linear, like X, Y, Z). I'm not sure what their purpose is (maybe in hot wire foam cutting?), but to be complete, all these values should be reported and probably also available as tool offset. Maybe make it depend on axis_mask.

Changing that would also affect / need changing the "set" commands in emcrsh.

heeplr commented 10 months ago

@johnchikov can you retry with the current master? I did some cleanup in linuxcncrsh that has been merged yesterday. I fixed this issue among others (and I hope I didn't introduce new ones) :)

Also, if you actually use linuxcncrsh, it'd be cool if you could do some real-world testing. Maybe we could bake that into a test with simulated hardware. Currently the tests for linuxcncrsh are not complete.

rmu75 commented 10 months ago

I'd say with 2.9 we only fix the %d and think about what offsets to display/read in 2.10