Repository navigation
halcompile: allocate fake params for personality-masked params - #4637
grandixximo wants to merge 2 commits into
Conversation
|
The correct fix in bldc would be to fix the calls to hal_param_new_X(). When a personality mask/confition is specified on the component's param declaration, then the allocation should be the target type or a fake. The generated code in bldc: if(personality & 0x04) {
r = hal_param_new_sint(comp_id, HAL_RO, &(inst->offset_measured_p), 0,
"%s.offset-measured", prefix);
if(r != 0) return r;
}Should in reality have been: if(personality & 0x04) {
r = hal_param_new_sint(comp_id, HAL_RO, &(inst->offset_measured_p), 0,
"%s.offset-measured", prefix);
} else {
r = hal_param_new_fake(comp_id, (hal_refs_u *)&(inst->offset_measured_p));
}
if(r != 0) return r;That would have fixed it once and for all, including for the users. This can be done in halcompile and no change in bldc.comp required. |
…ccess" This reverts commit 3822d1e.
A param masked out by personality was never allocated, so its accessor handle stayed NULL and reading it killed the realtime thread (LinuxCNC#4625). Allocate a fake param in the personality else branch, per BsAtHome's review: the handle always points at HAL-owned storage, exported or not, and a masked param reads back zero.
7f07f66 to
3624a45
Compare
|
@BsAtHome Shaped as you said: revert plus hal_param_new_fake() in the personality else branch. The generated code now matches your sketch exactly, bldc.comp untouched. One open point from the earlier review round. @rene-dev argued on #4626: "imho it still is a logic error if you read a pin that you didnt create, and returning 0 masks this error." With the fake allocation the read still returns 0 silently. Do you want a warn-once message on first access of a personality-masked param, or is silent zero acceptable? |
And that is exactly the gremlin we have been fighting while doing the getter/setter conversion. Parameters have been used as (very) expensive variables and that should be fixed. The fake parameter was introduced as a stopgap to keep current code running. However, it still needs review and fixing. In the end, that fake allocation needs to go. We need to be firm and tell everybody who uses params (and pins) as storage that it will not work. For C-based code, this is done for some instances already. For .comp based code, well, there are a few more things we need to look at. |
|
One more shape for the longer term, meeting input if nothing else. The out-of-tree problem does not go away with roadmap firmness: a .comp author we cannot reach, reading a personality-masked param, gets a dead realtime thread once fakes are removed, with no diagnostic. NULL-checked accessors cover it without any storage. For personality-guarded params, halcompile could generate: (h ? hal_get_sint(h) : (warn_once("param 'x' accessed but masked by personality"), 0))No backing store, no fake, one truth. The logic error is loud (@rene-dev's point from #4626), fakes can die on schedule, and out-of-tree comps degrade to a logged warning and zero instead of a SIGSEGV. Cost is one predictable branch per access plus a byte of once-state per guarded param. Not asking to hold this PR for it; #4637 as-is is the right fix today. Candidate for the "when fakes go" milestone. |
|
This adds a condition to each and every access. That is problematic. We must first ensure that we do not have any cases in-tree before we can put something like this to warn out-of-tree. And then, we need to have a clear path how this pattern must be rewritten such that the condition+warning goes away. That is the really tricky part. |
Reshaped per @BsAtHome's review on #4626: the fix belongs in halcompile, using the fake allocator, not an instance-side backing store.
bldc.comp needs no change. Verified: cfg="a" completes case 0x02 (phase-angle tracks the 90 degree lead), cfg="hq" unchanged, full tree rebuild clean.
Fixes #4625.