change external offset to float

[rene-dev]

change external offset to float, having an offset as integer makes absoluteley no sense.
in one place(eoffset_per_angle.comp) it is actually used as a fixed point integer, calculating the reverse scale!
please, someone check the plasmac changes.

[grandixximo]

If an integer offset makes no sense, does that also apply to axis.L.jog-counts? Same counts-times-scale interface, same design.

The s32 type comes from the MPG world: a handwheel emits integer pulses, counts*scale turns them into distance. Natively float sources needing conv_float_s32 is ugly, agreed. But is the answer to retype the pin, or to feed it better?

Every plasma config in the wild that nets an s32 signal into axis.z.eoffset-counts stops loading after this. HAL does no implicit s32-to-float conversion on nets, so there is no graceful path. What is the migration story for those configs?

Your own updown.comp change adds a count_f pin instead of retyping count. If preserving the s32 interface is the right call there, why is it the wrong call in motion?

And once the pin is float, what does "counts" still count? Would a separate float input (counts left untouched) reach the same goal with zero breakage?

[andypugh]

If an integer offset makes no sense, does that also apply to axis.L.jog-counts? Same counts-times-scale interface, same design

Jog-counts is typically scaled by a step-size under UI or separate switch control, so makes sense there.

Almost every use I have seen for external offsets has to jump through hoops to force the output of the calculations into counts+scale.

I think the original application was offsetting a path either an MPG but I can't recall what unusual application that was. Subsequent applications, like Sam's hexagonal boring would be better (and more accurately) served by a float pin rather than fixed-point.

[grandixximo]

The step-size switch sets jog-scale from the UI instead of HAL. Underneath it is the same counts times scale. What distinguishes them?

On accuracy: resolution is the scale value, range is 2^31 times the scale, and motion diffs the pin against its previous value and accumulates in double, so source-side rounding never accumulates. You said a float pin would serve applications like Sam's hexagonal boring better, and more accurately. More accurately how? Is there a concrete case where s32 counts at a chosen scale lose to a float, or is that assertion?

The integer-width question came up before with 64-bit encoder counts, and the answer was the new HAL API in #4247. I would rather Bertho weigh in on how this fits that track than settle it piecemeal here.

Still open: the migration story for existing configs, and what a retype achieves that an added float input does not.

[BsAtHome]

Just about every file touched in this PR and #4518 conflicts with #4247.

[snowgoer540]

please, someone check the plasmac changes.

Thank you for requesting this. If you can tag me next time, I have a much higher chance of seeing it as I don't check pull requests every day. I much prefer this over popping in, dropping a huge change, and then having to figure out how to fix what it broke, etc. ... ...

This pull request no longer cleanly applies. At any rate, I applied it and looked at it with respect to QtPlasmaC/PlasmaC.

It highlighted that something has changed with regard to how eoffsets are unwound, and probably associated accelerations, etc. and I will need some time to figure out exactly what broke and when. It probably makes sense to hold off on this change until that stuff can be figured out.

I don't think there's another component/GUI that uses eoffsets like PlasmaC does.

EDIT: I think eoffsets are actually OK and I might have spoke too soon. At any rate, if you can update this commit so it applies, I'll try it out on my actual plasma machine.

[rene-dev]

I rebased on master, and removed all the plasmac related changes, some plasmac developer should look into this. looks like they do everythting in integer, should be a seperate PR.
@snowgoer540 you can test, but is should work just fine.

[snowgoer540]

I rebased on master, and removed all the plasmac related changes, some plasmac developer should look into this. looks like they do everythting in integer, should be a seperate PR. @snowgoer540 you can test, but is should work just fine.

Will do, but I don't follow what the need for the change is. Could you explain it to me, bearing in mind I am not a software engineer/master programmer?

[grandixximo]

@snowgoer540 my reading, and rene can correct me: no bug is being fixed. rene's view (see #4099) is that physical quantities should be float pins and HAL rarely needs integer ones; this PR applies that to eoffset-counts. The practical gain is for things that calculate a distance (Z level compensation, eoffset_per_angle, the qtdragon spindle lift): today they divide by the scale and round to whole counts to feed the pin, with a float pin they can send the distance directly.

PlasmaC already converts distances to counts inside plasmac.comp (dividing by offset_scale), so nothing changes for it today; a float pin would let a later PR drop that conversion, which is the "separate PR" rene mentions. This PR keeps plasmac.comp calculating in whole counts and just changes the offset pins to float, which carry whole numbers exactly, so motion sees the same values and the stock configs keep working. What would stop loading is a user config that connects an integer signal to axis.*.eoffset-counts or plasmac.*-offset-counts, since HAL will not connect integer to float. Those configs can be fixed by putting the existing conv_sint_real component between the integer signal and the pin.

Note the rebased commit still touches plasmac.comp (offset pins become float, via a macro shim) and qtplasmac_handler.py (z_offset_counts pin type), so those are what to test on the machine.

[grandixximo]

@andypugh I owe you a correction. I went through public configs that use external offsets (GitHub, forum): outside plasma, sources converting a float into counts outnumber native integer ones roughly 11 designs to 4, and most of those conversions are copies of in-tree recipes this PR removes. Plasma does the same conversion inside plasmac.comp. So "jump through hoops" holds, and I withdraw my objection to the retype.

@rene-dev I built the PR and checked the nets in halrun: the stock plasmac net and conv_sint_real into the float pin link fine. A few sim configs still create sint pins for this net and fail to load with Signal 'eoffset_count' of type 'sint' cannot add pin 'axis.z.eoffset-counts' of type 'real':

• share/qtvcp/screens/woodpecker/woodpecker_handler.py:261 (eoffset-count), netted in configs/sim/woodpecker/woodpecker_postgui.hal and woodpecker_postgui_ya.hal: all woodpecker sims.
• configs/sim/qtvcp_screens/qtdragon/qtvcp/screens/qtdragon/qtdragon_handler.py:353 (eoffset-spindle-count), used by qtdragon_mpg.ini.
• For consistency: configs/sim/woodpecker/compensate.py:125 (counts, which its help file nets to the now real woodpecker.comp-count) and configs/sim/woodpecker/woodpecker_/woodpecker_handler.py:166.

Could you also add a line to "Updating Configuration Files for 2.10.y" in updating-linuxcnc.adoc: axis.L.eoffset-counts is now real, and a config feeding it from an integer signal needs conv_sint_real in between.

Optional: conv_sint_real already covers what updown.count-f adds, and scaled_sint_sums already has a float out-f, so updown.comp could stay untouched.