search

LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.81k stars 1.16k forks source link

Improves named IO pins #2794

Closed havardAasen closed 10 months ago

havardAasen commented 10 months ago

There is still some limitations to the named arrays. If both names_dout and names_din is set, they have to be the exact same size. The same is true for names_ain and names_aout. Currently, we do not verify this.

Marking this as draft to see if the solution is accepted, I also thought to update the man-page for the named pins names_dout, names_din and names_ain, names_aout.

There is also the issue with not sending in the max length to `count_names(), not sure if you want that in 2.9 as well @andypugh?

andypugh commented 10 months ago

With this patch num_dio or num_aio is set to the minimum of count(inputs) and count(outputs). I feel it might be better to take the max(count(in), count(out) and then modify the check here: https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/motion/motion.c#L489C5-L489C5 to create default-named pins when it runs out of names. Currently users can define names for lots of inputs (say) and then not have them be created unless they have an equal number of outputs.

havardAasen commented 10 months ago

Are you thinking we can split up the input and output? Like 15 inputs and 5 outputs, and of those inputs 10 is named and 5 is default-named, with something similar with the outputs, but only 5 outputs? Input and output does not have same amount of pins.

Or is the intention that if I want to create 10 named inputs and 5 named outputs, It would actually generate 10 named inputs, 5 named outputs + 5 default-named outputs? Input and output has same amount of pins.

This last one would give a limitation of 32 named pins, not sure if this will become a problem.

andypugh commented 10 months ago

I was thinking of the latter, but may have missed a detail. Do the IOs of each type share an array with each being input or output (I would check, but it's late here, and probably later where you are). My (un-checked) impression was that they were separate but of equal size.

I am really keen to get 2.9.2 out (for reasons due to the Debian bugfix timescale) Do you think that the current partial fix is good enough for the moment?

havardAasen commented 10 months ago

I was thinking of the latter, but may have missed a detail. Do the IOs of each type share an array with each being input or output (I would check, but it's late here, and probably later where you are). My (un-checked) impression was that they were separate but of equal size.

You might be correct , I based my assumption on these lines https://github.com/LinuxCNC/linuxcnc/blob/1db56381a8a1bb336b4cc9ab424d736eaecede64/src/emc/motion/emcmotcfg.h#L38-L40 and since it combines input and output pins into the same variable I assumed the same register. But as you say, it would make more sense if they were separate.

I am really keen to get 2.9.2 out (for reasons due to the Debian bugfix timescale) Do you think that the current partial fix is good enough for the moment?

Not sure if I want to have any opinion on that, for 2.9.1

motmod num_misc_error=10 # Creates 10 error pins
motmod names_misc_errors=one,two # Creates 0 error pins

In it's current state, this has been flipped

motmod num_misc_error=10 # Creates 0 error pins
motmod names_misc_errors=one,two # Creates 2 error pins

The num_misc_error was the only one documented in 2.9.1 as well.

andypugh commented 10 months ago

As you spotted, I managed to commit an incomplete fix. I have now committed the fix that I intended (and tested). I think that what you have here is better, but I think it needs more careful assessment. The issue with the error pins was a clear bug, whereas the general improvement affects code that has (apparently) been working OK for some time. (Maybe not that long, I wasn't even aware that you could give names to the IO pins)

havardAasen commented 10 months ago

As you spotted, I managed to commit an incomplete fix. I have now committed the fix that I intended (and tested). I think that what you have here is better, but I think it needs more careful assessment.

Are you thinking on which solution to go for?

The issue with the error pins was a clear bug, whereas the general improvement affects code that has (apparently) been working OK for some time. (Maybe not that long, I wasn't even aware that you could give names to the IO pins)

Not sure if you have looked it up, but linking in the discussion in #1349 and the initial pr #1352 for the named IO pins.

Now that @andypugh fixed the bug in 2.9 this could target master instead, and we might be a bit more flexible with the solution and scope.

I don't think it is to hard to have equal amount of pins, where some is named and some isn't, splitting them up is a bit more work, though not by much.

havardAasen commented 10 months ago

Rebased against master and added the necesarry documentation, the documentation for the error pins which is located in 2.9 is not included in this commit.

Don't like the global variables, but not sure how to solve it without moving everything into the same function.

andypugh commented 10 months ago

Related to this (but not an immediate change) is https://github.com/LinuxCNC/linuxcnc/issues/2532