Back
[00:47:50] <SWPadnos> heh - yeah, it was a long one ;)
[00:48:07] <SWPadnos> it took me longer to make a config that would reproduce the problem than it did to fix it :)
[00:57:36] <jmkasunich> the remaining code is rather sub-optimal
[00:58:06] <jmkasunich> you do an if on corr vs. filt, and the very next step is to take the difference of corr vs. filt
[00:58:55] <jmkasunich> plus, there is code to handle that diff being negative, which by definition can no longer ever be executed
[00:59:16] <jmkasunich> I guess neither of those things really matters though
[00:59:29] <SWPadnos> I wanted a minimal change set at first. if it works for others it can be optimized
[00:59:41] <jmkasunich> understood
[00:59:51] <SWPadnos> I only checked it on sim, with a new config that I had just created for the purpose
[01:00:40] <SWPadnos> if others verify that the problem is fixed, I can go through and take out the redundant code and do the backport
[01:03:48] <jmkasunich> one thing bugs me about that code
[01:04:00] <SWPadnos> it's too complex? :)
[01:04:37] <jmkasunich> if s_to_go > 0, do one thing, else if s_to_go < 0, do another thing, with an implied if s_to_go == 0, do nothing
[01:04:50] <SWPadnos> yep
[01:04:57] <jmkasunich> the odds of it being exactly zero aren't high, but otoh, that is the desired end state
[01:05:28] <SWPadnos> it should get to 0, since the clipping code will make backlash_filt = backlash_corr eventually
[01:06:16] <jmkasunich> just found the clipping code - line 1670, right?
[01:06:23] <jmkasunich> and its counterpart in the other branch
[01:06:29] <SWPadnos> err - somewhere around there, yes
[01:06:54] <SWPadnos> or is that eliminated by the s_to_go<0 thing
[01:08:02] <jmkasunich> the "s_to_go <= ds_stop + ds_vel" conditional is still functional
[01:08:24] <SWPadnos> ah, ok
[01:08:29] <jmkasunich> it's the if s_to_go > 0, else if s_to_go < 0 that is redundant
[01:08:47] <SWPadnos> ok, sure
[01:08:49] <jmkasunich> also redundant - the first three lines inside the outermost branch are the same in both paths
[01:09:00] <SWPadnos> yes, I noticed that
[01:09:19] <jmkasunich> ds_vel, dv_acc, and ds_stop should be calc'ed outside the branch
[01:09:25] <SWPadnos> and there are a couple that are just -v * <something> vs v * <somehting>
[01:09:27] <jmkasunich> s_to_go too for that matter
[01:09:28] <SWPadnos> sure
[01:09:44] <SWPadnos> that one does change order, but it can use fabs too
[01:10:07] <jmkasunich> well, I wasn't going to go so far as to eliminate the two paths
[01:10:22] <jmkasunich> but I'd calc s_to_go, the branch on its sign, and in the negative branch negate it
[01:10:46] <SWPadnos> I'm not sure that's a great savings
[01:11:05] <jmkasunich> well, in reality I'd replace it all with the simple tp
[01:11:07] <SWPadnos> maybe one less operand fetch, plus a tiny bit less code
[01:11:09] <SWPadnos> heh
[01:11:43] <SWPadnos> I don't know if the TP is quite right. it may be, but there was something nagging at me when I was looking at the code
[01:12:03] <SWPadnos> maybe it's got one derivative/integral too much or too little. don't remember
[01:13:42] <jmkasunich> the simple_tp you mean?
[01:13:55] <SWPadnos> yes
[01:14:12] <jmkasunich> I checked it last night, it is _exactly_ the same code as in the existing free mode planner, only repackaged to be more object oriented
[01:14:26] <SWPadnos> hm. ok
[01:14:31] <jmkasunich> and that code has been essentially untouched and working since january of 2006
[01:14:40] <SWPadnos> and backlash is just motion superimpose don normal motion
[01:15:51] <jmkasunich> basically, I changed things like joint->free_tp_active, to tp->active
[01:16:14] <jmkasunich> and added a "struct simple_tp" to the joint structure
[01:16:39] <SWPadnos> the simple TP should operate on a simple struct that has only current and target position/vel/acc, and limits
[01:16:41] <SWPadnos> plus maybe a few flags
[01:17:10] <jmkasunich> kinda like:
[01:17:15] <jmkasunich> typedef struct simple_tp_t {
[01:17:15] <jmkasunich> double pos_cmd;/* position command */
[01:17:15] <jmkasunich> double max_vel;/* velocity limit */
[01:17:15] <jmkasunich> double max_acc;/* acceleration limit */
[01:17:17] <jmkasunich> int enable;/* if zero, motion stops ASAP */
[01:17:19] <jmkasunich> double curr_pos;/* current position */
[01:17:21] <jmkasunich> double curr_vel;/* current velocity */
[01:17:25] <jmkasunich> int active;/* non-zero if motion in progress */
[01:17:27] <jmkasunich> } simple_tp_t;
[01:17:35] <SWPadnos> sure, could be ;)
[01:20:15] <jmkasunich> this is the diff (on a branch) where I replaced the old free tp with an instance of simple_tp
[01:20:18] <jmkasunich> http://cvs.linuxcnc.org/cgi-bin/cvsweb.cgi/emc2/src/emc/motion/control.c.diff?r1=1.141;r2=1.141.2.1
[01:21:42] <jmkasunich> it's very tempting to apply that to trunk
[01:22:03] <jmkasunich> but I will wait at _least_ long enough for someone to test your patch, and for it to be backported to 2.2.x
[01:23:29] <SWPadnos> yeah - I want to see a difference other than a scope trace on my laptop :)
[04:02:36] <CIA-32> EMC: 03cmorley 07v2_2_branch * 10emc2/src/hal/classicladder/edit.c: fix for erasing connection-with-top
[12:54:36] <jepler> SWPadnos: I don't think you'll get that until stuart tries the code
[12:55:03] <SWPadnos> * SWPadnos waits patiently
[12:56:05] <alex_joni> maybe drop a message in the same thread, so stuart tries it out :)
[12:56:08] <skunkworks> does stuart know? I remember only steves comment here on irc
[12:56:18] <skunkworks> yey
[12:56:25] <skunkworks> *heh
[12:56:31] <alex_joni> works in spanish
[12:56:37] <alex_joni> jej
[18:16:24] <LawrenceG> LawrenceG is now known as s1010s1101
[18:16:50] <s1010s1101> s1010s1101 is now known as LawrenceG
[18:44:06] <skunkworks> SWPadnos: success :)
[18:44:47] <SWPadnos> heh
[18:44:50] <SWPadnos> cool
[19:02:50] <issy> hi all
[19:06:15] <SWPadnos> hi