On 05/31/2016 03:54 PM, Sebastian Kuzminsky wrote:
> On 05/31/2016 02:00 PM, John Morris wrote:
>> I've enabled remap of the M62 thru M66 codes [1].  Is this of interest
>> for merge into LinuxCNC?
>
> In my opinion: yes.

Great!  Then in addition to the below, I went ahead and rounded out the 
patch by adding M67 and M68.

>
>
>> Seb, I borrowed your 'statbuffer-g5x-abort' test for this.  Proper
>> testing of calling the original e.g. M64 from within the remapped M64
>> needs bath interp and HAL, plus the connecting machinery, so it was a
>> timely example for me.  Thanks!
>
> I see that you import hal, but i don't see you use it.  You're verifying
> the dout settings by looking in the stat buffer, which is probably fine,
> and which *should* agree with Motion's digital-out hal pins.
>
> If you wanted to add that to the test, take a look at how (for example)
> the io-startup test does it.  I think this improvement is optional,
> since that's not at the core of what the test is testing.

I meant to do this before, but hadn't found the right example to crib 
from.  ;)  Done.

>
>
>> The M66 test is also a good example of how to remap a code that returns
>> INTERP_EXECUTE_WAIT.
>>
>> [1]: https://github.com/LinuxCNC/linuxcnc/commit/ffac8964
>
> This is very cool.  I very much appreciate your thorough testing and
> bugfixing in this tricky area.
>
>
> Looking at the history of ffac8964 i see a commit from Norbert
> (d3820714) that's not in any mainline branch, but that also looks like
> it doesn't belong with the remap work you're doing.  There's a commit in
> master (389bba5ff) that matches it.  An off-by-one rebase perhaps?

Yup.  Fixed.

>
> I see that you brought back to life a commented-out call to
> read_inputs() in Inter::synch(), which seems right to me (though i dont
> know the details).  Do you know why it was disabled?
>
>

The `read_inputs()` call was moved back and forth between 
`Interp::synch()` and `Interp::_read()` in 424eb992 and earlier in 
e200945b.  I didn't try to understand why, and lazily decided to enable 
it in `synch()`.  That works, but makes lot more calls than necessary.

Better is to call `read_inputs()` from `interp_python.cc` in 
`Interp::pycall()` before calling `generator_next()`.  That ensures that 
if the generator function needs the result of the M66 after continuing 
from the `yield`, it's available.  I added this case to the unit test, 
since users may need this in python remaps.

Thanks for looking at this, Seb!

        John

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
_______________________________________________
Emc-developers mailing list
Emc-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/emc-developers

Reply via email to