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.78k stars 1.15k forks source link

motion.c: Correct copy-paste errors in handling of miscellanous error inputs #2780

Closed ArchMachineCo closed 9 months ago

ArchMachineCo commented 9 months ago

Correction to lines 348-351 to fix error when using "names_misc_errors" to create named misc error pins.

hansu commented 9 months ago

Thanks for your contribution! I cannot say anything about the content, but it would be nice to have a more meaningful title and commit message.

havardAasen commented 9 months ago

I've read through it again, I believe the change you do on line 349 is correct, but line 350 should be removed, I also think this should be targeted for 2.9, though @andypugh might want the final say on that.

ArchMachineCo commented 9 months ago

Do we know the original intent with this block? I mean, it compares the numbers of output names to input names.

The purpose of this portion of the code is to set the number of misc_error pins. Currently, its uzing the wrong variables in its comparison.

Misc_error pins are created by either using num_misc_error=x, or by using name_misc_errors="error1,error2....", but you cannot use both.

So there is a check to see if num_misc_error is used on line 344, and then if num_misc_error is used, it sets the number of misc error pins = to the value of num_misc_error.

If name_misc_errors is used, then it SHOULD set the number of misc error pins equal to the count of names in the array name_misc_errors.

Utilization of num_misc_error, AND name_misc_errors are both good options, and should remain in the code, the problem currently is that if you specify name_misc_errors="error1,error2.....", the number of misc error pins is set to the count of names in the array of digital output pins (names_dout).

PS: im very new to github and dont really know how to operate commits, pulls and such, so i apologize if im going about this incorrectly.

andypugh commented 9 months ago

This looks like a simple copy/paste error from duplicating the digital IO pins.

But it's all a bit of a mess.

static int num_misc_error = DEFAULT_MISC_ERROR; /* default number of misc errors */
RTAPI_MP_INT(num_misc_error, "number of misc error inputs");
...
  if(num_misc_error && (names_misc_errors[0])){
    rtapi_print_msg(RTAPI_MSG_ERR, _("MOTION: Can't specify both names and number for misc error\n"));
    return -1;
  }
  else if(names_misc_errors[0]){
    num_misc_error = count_names(names_dout);
    num_misc_error = (num_misc_error > count_names(names_din)) ? num_misc_error : count_names(names_din);
  }
  else if(!num_misc_error){
    num_misc_error = DEFAULT_MISC_ERROR;
  }

If DEFAULT_MISC_ERROR was ever > 0 then in the case where an error name was given, the first conditional would be satisfied, and an error message would be given, disallowing the use of names.

Then, whilst DEFAULT_MISC_ERROR == 0, the last condition always passes, but has no effect.

I will try to create a proper fix.

ArchMachineCo commented 9 months ago

Yes i believe it did appear to be a copy paste error. Glad to hear itll be worked out. Thanks!

andypugh commented 9 months ago

I think that this is closed by my similar, but different, fix https://github.com/LinuxCNC/linuxcnc/commit/74288982e684e03b567d9850efdd105dc209b1f4

ArchMachineCo commented 9 months ago

Awesome, thanks so much!

Excuse my noob-ness, but how do I go about getting these changes on my machine?

andypugh commented 9 months ago

If you wait a few days there should be a new 2.9.2 release (There is a Debian-related issue that I am not sure how to address) If you don't want to wait that long, then you can add the buildbot to your apt sources.list, of just grab the latest .deb file and install that. http://buildbot2.highlab.com