John, Am 06.03.2014 um 04:43 schrieb John Kasunich <jmkasun...@fastmail.fm>:
> > > On Sun, Mar 2, 2014, at 02:29 PM, Michael Haberler wrote: >> >> I have read up on the data flow in stepgen.c and I found the following for >> normal (i.e. not disabled, and setup disregarded): >> >> make_pulses reads: >> stepgen->accum >> stepgen->target_addval >> stepgen->deltalim >> >> make_pulses writes: >> stepgen->accum >> >> update_freq reads: >> stepgen->accum >> >> update_freq writes: >> stepgen->target_addval >> stepgen->deltalim >> >> Now stepgen->accum can be taken out of the picture for considering >> transactions since that is the single scalar value read back by update_freq >> (it could be replaced by an atomic 64bit fetch which is supported on all >> architectures, but that is a side issue). >> >> The interesting case are the two values which update_freq writes. I assume >> you are referring to those? >> >> I can see how scheduling comes in here the way this is coded, but I think >> this is basically a complicated way to achieve an atomic message transmit >> from update_freq to make_pulses. >> > > Actually, I don't think there is any need to worry about atomic updates of > target_addval and deltalim. > > The relevant code is this (from make_pulses, in the faster thread): > > /* update addval (ramping) */ > old_addval = stepgen->addval; > target_addval = stepgen->target_addval; > if (stepgen->deltalim != 0) { > /* implement accel/decel limit */ > if (target_addval > (old_addval + stepgen->deltalim)) { > /* new value is too high, increase addval as far as > possible */ > new_addval = old_addval + stepgen->deltalim; > } else if (target_addval < (old_addval - stepgen->deltalim)) { > /* new value is too low, decrease addval as far as possible > */ > new_addval = old_addval - stepgen->deltalim; > } else { > /* new value can be reached in one step - do it */ > new_addval = target_addval; > } > } else { > /* go to new freq without any ramping */ > new_addval = target_addval; > } > /* save result */ > stepgen->addval = new_addval; > > "addval" is the value that is added to the accumulator every base period. In > other > words, it is the instantaneous velocity. > > "deltalim" is the maximum allowed change in addval per base period. It is > based > on the maximum allowable acceleration parameter. > > In normal LCNC configurations, the acceleration is set at startup and never > changes. But the stepgen was designed to be more versatile than that, and > it needs to work correctly even if the acceleration limit is changed on the > fly. > But there is no need for an acceleration change to be atomic with the velocity > command. > > There is a bit of sloppiness in this code. Deltalim isn't treated with > the same care as addval. You'll notice that stepgen->addval and > stepgen->target_addval are copied from the structure to local > variables at the beginning of this code snipped, and then the new > value of addval is copied back to the structure at the end. > stepgen->target_addval is modified by the slow thread, and if > we allow multi-core, it is subject to being changed at any time. > If it changes before it is copied to the local target_adval, the > change will take effect this time, if not, it will take effect the > next time. The value is read only once. > > Now look at stepgen->deltalim. It is also written by the slow > thread, and if we allow multi-core, it can change at any time. > The code snippet above has five references to stepgen->deltalim. > It would be possible for the value to change after the first > conditional "if (stepgen->deltalim != 0) {", but before the second > one "if (target_addval > (old_addval + stepgen->deltalim)) ", or > after the second conditional but before the line that uses the > value: "new_addval = old_addval + stepgen->deltalim;" > > When that code was written, the assumption was rate monotonic > and single core. SMP was around at that time, but RTAPI limited > all realtime threads to a single core. Under those assumptions, > the code is fine. The low speed thread can't sneak in and > change the value at an unexpected time. > > But under the new assumption that threads can be running on > different cores, the slow thread might be running (and changing > stepgen->deltalim) at the same time as the fast thread is running. > > The answer is to treat deltalim like addval. Read stegen->deltalim > a single time, at the start of the code, and copy it to a local > variable. Then the five references to deltalim can use the local. > Any change made to stepgen->deltalim by the slow thread will > either be before the single read, and take effect this time, or > after the read, and take effect next time. There is no risk of > using one value during a comparison and another value during > the subsequent assignment, because the slow thread cannot > change the local variable. > > I wrote this code, and to be honest I don't know why I treated > addval with more care than deltalim. Under the assumptions > of that time, both were safe from modification by the slow > thread, and neither one needed to be copied to a local. Under > the new assumptions, both are vulnerable, and both need to > be copied to locals. > > Because velocity and acceleration limits are independent > attributes, there is no need to worry about synchronization > of the two values to each other. thanks for going through this. I think we have reached clarification of a not-so-well understood corner of HAL which was unclear to some (well at least me). Can I summarize what I read from the above as follows: - there are two pieces of state (addval and deltalim) passed from servo to base in stepgen.c - for correctness of stepgen operation it is not essential these values origin from the same invocation of update-freq, meaning make-pulses is not a critical region with respect to access of these values - as a consequence, the threads executing update-freq and make-pulses are not subject to any priority and non-preemption preconditions does this reflect our common understanding? - Michael ------------------------------------------------------------------------------ Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce. With Perforce, you get hassle-free workflows. Merge that actually works. Faster operations. Version large binaries. Built-in WAN optimization and the freedom to use Git, Perforce or both. Make the move to Perforce. http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk _______________________________________________ Emc-developers mailing list Emc-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/emc-developers