knipknap / better-tool-library

A FreeCAD addon and command line tool for managing tool libraries
MIT License
20 stars 10 forks source link

Crash report (support imperial units in the Feeds&Speeds calculator) #3

Closed ShamanTcler closed 1 year ago

ShamanTcler commented 1 year ago

Win 11 FreeCad ver 0.21.0 (build 33682) Better Tool Lib ( no version number, but "up-to-date" 8/8/2023

Tool displays fine. Go to Feeds & Speeds Added a machine: OneFinity set some resonable machine values, returned ..

then crash,

Reloaded....

Feeds & Speeds loads with a bunch of red/orange issues.

Red: Axial Force 0.00 N ...min/max range given Mrr= 0.00 min/max range given Power= 2.2 Kw ...... This is in fact what I put in ... limit was 2.2 max Torque= 0.00

Orange: Deflection 0.02 ,im/max given RPM= 59188 .... I don't recall entering this

So it appears to be a matter of setting reasonable defaults.

Also it was not clear how to add/edit machines except through the tool editor.

ShamanTcler commented 1 year ago

MachineDef

BitScreen

knipknap commented 1 year ago

Those results look wrong indeed. Could you show your tool parameters?

Given the tiny stickout of just 3.75mm... this looks like the tool would be like 0.something mm thick? That would lead to extremely low forces and may explain at least part of the result, though I guess it should still have found at least something.

ShamanTcler commented 1 year ago

This was a "kick the tires" exercise. So pretty much took defaults when able. BallEndMill

Ball2

Note all my dims are in inches ... might that be an issues?

knipknap commented 1 year ago

Ohh, I see... unfortunately, imperial units are not supported by the calculator; this is a documented limitation, see here. I am sure that is what causes the issues.

I should probably detect if there is anything imperial going on and print an error instead of letting the calculator fail.

ShamanTcler commented 1 year ago

I have not looked at you code, but what I have done in the past is:

1) Copy all parameters to temporary working variables 2) Check units, if need be, Set a flag, change working variables 3) Do the calcs (only need to validate once) 4) Check Flag, if set change units on returning vars 6) Return variables

I know simple, but simple can be good

ShamanTcler commented 1 year ago

So I glanced at you code, well done, however some comments are best left out: ;)

image

knipknap commented 1 year ago

Sure, this requires just adding support in the presentation layer. I even made things extra easy in the backend: All of the properties have get_imperial() and set() methods that convert the units for you. So the backend is fully prepared.

All that needs to be done is

Should be straight forward, go for it!

knipknap commented 1 year ago

Current master now supports imperial in all input fields, and serializes the units as well.

The calculator output is still metric, but that is a different matter. At least the calculator should no longer fail.

knipknap commented 1 year ago

Closing this issue as the original problem should be resolved. If the original problem reappears, please feel free to reopen.

As for imperial output, that should be a new issue, and I do accept patches, but am not planning on working on it myself.

ShamanTcler commented 1 year ago

Sounds good and thanks