switchkins: allow more than three kinematics types - #4372
Conversation
kinematicsSwitch() stored the requested type in switchkins_type and only then ran the switch statement that validates it, so an out of range request left the module pointing at a kinematics that does not exist. The switch itself returned -1 and motion raised its error flag, but switchkins_type kept the bad value, so every kinematicsForward() and kinematicsInverse() call after that failed too and printed switchkins: Forward BAD switchkins_type </7> once per servo cycle for as long as the machine stayed up. Validate the request first and return without touching switchkins_type, which leaves the running kinematics in place. With the range checked up front the default arm of the switch is unreachable, so it goes away.
switchkins.c dispatched on switchkins_type with a three way switch and created a fixed kinstype.is-0/1/2, so a module could never provide more than three kinematics. Hold the setup, forward and inverse functions in arrays and dispatch by index. switchkinsSetup() still provides types 0,1,2 exactly as before, so no kinematics module changes and out of tree modules keep compiling. A module wanting more calls the new switchkinsRegister() from within switchkinsSetup(), once per additional type. The kinstype.is-N pins are created in a loop, which leaves the names of the first three unchanged.
| int switchkinsRegister(int ktype, KS kset, KF kfwd, KI kinv) | ||
| { | ||
| if (ktype < 3 || ktype >= SWITCHKINS_MAX_TYPES) { | ||
| rtapi_print_msg(RTAPI_MSG_ERR, | ||
| "switchkinsRegister: BAD switchkins_type <%d>" | ||
| " (must be 3..%d)\n", | ||
| ktype, SWITCHKINS_MAX_TYPES - 1); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Wouldn't it be more consistent to allow switchkinsRegister to supplant the old switchkinsSetup?
The real issue is to deal with is empty/skipping slots. That problem will be present also for any entry larger than 3.
There was a problem hiding this comment.
Supplanting switchkinsSetup(): agreed the split is inconsistent. It cannot be replaced without making it optional, since every module defines it and rtapi_app_main() calls it unconditionally. Either convert the eight in-tree modules and drop it, which breaks out of tree modules at compile time, or declare it weak and call it only if present, which I can only test on uspace. Cheap step either way is dropping the ktype < 3 restriction so registration covers 0 to 2 as well. Which end state do you want?
Gaps: already refused at module load, not at switch time. scarakins registering type 5 without 3 and 4 gives
Switchkins FAIL scarakins:<Missing setup function>
The message is the problem, it names neither the type nor the gap. Same area: a double registration silently overwrites, and nobody checks the return of switchkinsRegister(). I will name the missing type, reject double registration, and fail the load on a rejected registration.
There was a problem hiding this comment.
The path of least resistance would be to drop the ktype < 3 condition.
In the long run, we should make it generic. I'm also confused a bit because there are examples in hal/components that do their own switch. Maybe this is the reason why the current construct feels messy and inconsistent.
There was a problem hiding this comment.
Done, in a separate commit.
ktype runs from 0 now, and the count comes from the highest type filled by either route, so a module can register all of them and leave the switchkinsSetup() arguments alone. A type has to come from one route or the other. Registering one that switchkinsSetup() already filled, leaving a gap, and an out of range type are all refused at load time, naming the type at fault.
On the components with their own switch: millturn, xyzab_tdr_kins, xyzacb_trsrn and xyzbca_trsrn in src/objects/hal/components implement kinematicsSwitch(), forward, inverse and their own kinstype.is-N pins without linking switchkins.o, so there are two implementations of the same thing in the tree. They also all have the assign before validate bug the first commit fixes. Folding them onto switchkins.c looks doable now the type count is not fixed. Do you want that in this PR or a separate one?
For context on where I am going with this: I have G12.1 and G13.1 working locally to select the kinematics from G-code, with the interpreter tracking which one is active and exposing it as #<_kins_type>. Selecting the kinematics through a HAL pin and then forcing a queue buster with M66 E0 L0 is the wrong shape: it is a state change the interpreter has to know about, and today it does not, so #<_kins_type> and anything built on it can disagree with what motion is running. The intention is to make G-code the proper way to switch and deprecate the HAL route, which is also what makes more than three types worth having.
Registration was restricted to types 3 and up, which left types 0,1,2 arriving one way and the rest another. Allow any type from 0, so registration is the general mechanism and the switchkinsSetup() arguments are a shorthand for the first three. A type has to come from one route or the other. Registering one that switchkinsSetup() already filled in is refused, and a rejected registration now fails the module load instead of only printing, since no caller checks the return value. The count of provided types is taken from the highest one filled by either route rather than assumed to be three, so a module can register all of them. A type left out below that is a gap, and the load time message now names the type and which of the three functions is missing instead of saying only "Missing setup function".
2 participants
switchkins.cdispatched onswitchkins_typewith a three way switch statement and created a fixed set ofkinstype.is-0/1/2pins, so a kinematics module could never offer more than three kinematics. This changes the dispatch to arrays indexed by type, and adds a way for a module to provide more.Nothing changes for any existing module or configuration.
What a module sees
switchkinsSetup()keeps its exact prototype and still provides types 0, 1 and 2. None of the eight in-tree kinematics modules is touched, and out of tree modules keep compiling unchanged.A module that wants more calls the new export from inside its own
switchkinsSetup(), once per additional type:ktyperuns from 3 toSWITCHKINS_MAX_TYPES - 1, and every type below the highest one registered has to be provided.SWITCHKINS_MAX_TYPESgoes from 3 to 9. It is now only a ceiling on the array sizes rather than the number of types in use, so a module still provides exactly three unless it registers more, and the extra headroom costs a few unused array entries.HAL pins
The
kinstype.is-Npins are created in a loop, one per provided type. For a module providing three types that produceskinstype.is-0,kinstype.is-1andkinstype.is-2, the same names as before, so nothing that nets those pins breaks. A module providing five getskinstype.is-0throughkinstype.is-4.A setup routine now runs once per type it is registered for, so a routine used for two types must not create the same pin twice. Noted in the docs.
The first commit is a separate bug fix
kinematicsSwitch()stored the requested type inswitchkins_typeand only then ran the switch statement that validates it. The switch returned -1 and motion raised its error flag, butswitchkins_typekept the bad value, so the module was left pointing at a kinematics that does not exist. EverykinematicsForward()andkinematicsInverse()call after that failed as well, printingonce per servo cycle for as long as the machine stayed up. On the scara sim,
M68 E3 Q7reproduces it on master.The fix validates the request first and returns without touching
switchkins_type, which leaves the running kinematics in place. It stands on its own and does not depend on the rest, which is why it is a separate commit.Testing
All eight switchable modules (
scarakins,genhexkins,genserkins,pumakins,three21kins,5axiskins,xyzac-trt-kins,xyzbc-trt-kins) load underhalrunand create exactly the same three pins, with the same names, as they do on master.On
configs/sim/axis/vismach/scara/scara.ini:M68 E3 Q1selects kinematics 1,M68 E3 Q0selects 0.M68 E3 Q7is refused, kinematics 1 keeps running, and there is no per cycle log spam.With
scarakinstemporarily registering two extra types (a local test edit, not part of this PR), the same config giveskinstype.is-0throughkinstype.is-4and switches to type 4 and back.Why
I have a machine that needs more than three kinematics for the same config. There is no in-tree module that needs a fourth type today, so the second commit is enabling work rather than something with a visible user in this repository. I would rather add the capability in a shape you are happy with than carry a fork of
switchkins.c.I looked at doing this with a second array based
switchkinsSetup()prototype selected by#if, but that duplicates every dispatch site and every module's setup function for something that is decided at compile time anyway. A registration call seemed like much less to maintain.