Review code comments from Mike Lawrence

[clough42]

Mike left a comment on YouTube:

Mike Lawrence • 2 hours ago @Clough42

Did a quick look at the code. Would you prefer comments on GitHub?

Since this interrupt is modifying this->currentPosition-- don't you need synchronization (a semaphore, volatile, atomic_int, etc);to prevent main thread calls to incrementCurrentPosition(1) from losing the ISR change?

https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/StepperDrive.h#L144

I noticed floating point constant expressions in your code.

https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/Encoder.cpp#L83

https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/Encoder.cpp#L87

Check the assembler code to see if it's doing the division at runtime. Some compilers defer floating point math to runtime to avoid losing precision. If that's true then you can eliminate this work by doing the divide during initialization.

create a new constants (not macros) for these values and set once: _ENCODER_MAX_COUNT/2 60 * RPM_CALC_RATE_HZ / 4096

You can shorten https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/Core.cpp#L47

if(reverse ) { this->feedDirection = -1; } else { this->feedDirection = 1; }

to this->feedDirection = reverse ? -1 : 1;

https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/StepperDrive.h#L115

you can do a fast exit if no work if( this->desiredPosition == this->currentPosition ) return;

https://github.com/clough42/electronic-leadscrew/blob/892a21fc1712b56361fc15996be963822e62e7f3/els-f280049c/Encoder.cpp#L89

This code is not needed since the next line set previous: if( rpm > 2000 ) { previous = current; }

[clough42]

Removed the dead rpm>2000 code.

[clough42]

I think the race condition is real. Possible solutions:

1. Disable interrupts while calculating and setting the new value.
2. Use another register to dispatch the update to the interrupt handler.

[davidgeorge222]

As a thought, would it be practical to work out the spindle speed and the desired lead screw speed then use a timer to provide stepper drive signal as a back ground interrupt then the main program error checking? Also if you need PCB / Schematic / hardware help let me know Other thought I have is what happens if the lead screw stepper fails to step (lack of torque etc) is there an encoder to monitor this?

[clough42]

@davidgeorge222 This is possible, and this is the approach that many people have used for this kind of controller. On this particular TI DSP, it's usually done with a high-res PWM peripheral. It's possible to set up the PWM at the expected frequency and then interrupt on every pulse to make adjustments.

I haven't gone to that level yet, because it doesn't seem necessary. There are lots of details to work out in the electronics, the motors and controllers, the mechanical drivetrain, the user interface, error handling, safety, fault condition handling, etc. At this point, I have code that works to calculate the ratios, and I want to get all the other stuff working before spending too much time refining the software.

Making the software perfect doesn't make much sense if some other element of the system ends up causing a top-down conceptual redesign. Better to solve the other problems first, and then come back to refine the software.

[davidgeorge222]

@clough42 sounds like your all over this. If your running into problems with the electronics or motor drives let me know, as that's far more my thing than doing software.

[clough42]

In this case, the methods are in the header file so they will be duplicated in the compilation units where they are used and can be inlined by the compiler. These are either simple data access operations, functions performed in a time sensitive interrupt routine, or both.

On Sun, Nov 10, 2019, 5:50 PM Reddinr74 notifications@github.com wrote:

So, I'm a C guy and not so much c++ so I may be way off here. I have a question about the code structure. In C I've always read about and experienced function implementation in the .c files and declarations in the .h files. Is that not common practice in c++? The reason I ask is that it took me quite a while to find a couple of the functions because I didn't expect them to be in the header.

Also, your design development approach and explanations are top-notch in my book. Great to see. I'm looking forward to making an ELS for my own lathe.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/clough42/electronic-leadscrew/issues/1?email_source=notifications&email_token=AAZLSHEHGIX2DQMVULTTXMTQTC3ADA5CNFSM4HHEHHQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDVNUFI#issuecomment-552262165, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZLSHH5GXWJWCNDB2R5TALQTC3ADANCNFSM4HHEHHQA .