search

sparkfunX / Desktop-PickAndPlace-CHMT36VA

Tools and conversion scripts for the CHM-T36VA Desktop Pick and Place from SparkFun
GNU General Public License v3.0
66 stars 32 forks source link

Hardcoded value of FEEDER_NOMOUNT = 30 is problematic #28

Closed owendelong closed 5 years ago

owendelong commented 5 years ago

I'm using the Berniewa fork, but this applies to both his fork and SparkfunX Core

In Eagle-Conversion/ConvertToCharm.ulp, Sparkfun Line 73, Berniewa line 58, there is a value defined for FEEDER_NOMOUNT = 30.

If you have a larger machine (e.g. CHMT560P4), then unless you dig into the source code, you find yourself with a real head-scratcher as to why your feeder loaded in position 30 always gets tagged "NO Mount".

I found that it was easy to change this value to 999 without causing any noticeable harm. Obviously, I updated my feeder list spreadsheet accordingly.

Thanks for this very useful software.

owendelong commented 5 years ago

At the very least, I'd like to encourage documentation of the presence of this variable and its default value in the readme file, so that it doesn't just produce an unexpected result that is challenging to troubleshoot.

nseidle commented 5 years ago

Thanks for the issue and solution! Yep, it's hard coded (bad us!). Mind doing a PR for us since you've already got the fix working?

owendelong commented 5 years ago

Uh, OK... One-line PR generated. lol

owendelong commented 5 years ago

FWIW, the hard coding isn't so bad, IMHO. Its the fact that it's hard coded to a value that interferes with actual usable values in an unexpected way. Imagine my surprise when I'm looking over my place list and I see nice results for feeders 1,2,3,4,...24,26,28,33,37... Why is my chip in feeder 30 marked as "No Mount"?

(My machine is a CHMT560P4, so I've got 60 feeder positions.) I believe there are (or are likely to be in the future) machines with upwards of 200 feeders. As such, I figured picking a nice high value like 999 would simplify things.

I notice that there's also a "Max Feeder" definition. I changed it accordingly in my local, but someone more versed in ULP surgery may want to make that one configurable in the UI. My fix was just to use a way out of bounds number for FEEDER_NO_MOUNT since I think a 999 position machine is unlikely without requiring other significant changes to the software here.

nseidle commented 5 years ago

Thanks for the PR and sorry for the slow response! Artemis had me tied up for a bit.