Found while reviewing the getter/setter conversion of the motion module (#4286), which faithfully reproduces this master code.
src/emc/motion/homing.c, base_write_homing_out_pins() (around line 547):
if(!(*(addr->homing) && !H[jno].homing && homing_active)) {
// Prevent race condition.
// ... Do not update the homing status when going from homing --> not homing
// and the state machine is still active. The homing status deassertion
// must be delayed until the state machine is done.
*(addr->homing) = H[jno].homing;
}
*(addr->homing) = H[jno].homing; // OUTThe if guard exists to skip the pin update in one specific case (homing -> not homing while the state machine is still active), so the homing pin deassertion is delayed and a new homing command cannot fail on a stale state.
But the very next line writes the same value to the same pin unconditionally. Whenever the guard skips its own write, the following line does it anyway. The guard has no effect; the race it describes is not actually prevented.
Two possible fixes, depending on which behavior is intended:
- The delayed deassertion is needed: delete the unconditional write (keep only the guarded one).
- The eager update has proven fine in practice: delete the dead guard and its comment.
I do not know which is correct; the comment suggests (1), but the code has effectively been doing (2) for a long time without obvious fallout.
Found while reviewing the getter/setter conversion of the motion module (#4286), which faithfully reproduces this master code.
src/emc/motion/homing.c,base_write_homing_out_pins()(around line 547):The
ifguard exists to skip the pin update in one specific case (homing -> not homing while the state machine is still active), so thehomingpin deassertion is delayed and a new homing command cannot fail on a stale state.But the very next line writes the same value to the same pin unconditionally. Whenever the guard skips its own write, the following line does it anyway. The guard has no effect; the race it describes is not actually prevented.
Two possible fixes, depending on which behavior is intended:
I do not know which is correct; the comment suggests (1), but the code has effectively been doing (2) for a long time without obvious fallout.