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?

electronic-leadscrew/els-f280049c/StepperDrive.h

Line 144 in 892a21f

this->currentPosition--;

I noticed floating point constant expressions in your code.

electronic-leadscrew/els-f280049c/Encoder.cpp

Line 83 in 892a21f

if( count > _ENCODER_MAX_COUNT/2 ) {

electronic-leadscrew/els-f280049c/Encoder.cpp

Line 87 in 892a21f

rpm = count * 60 * RPM_CALC_RATE_HZ / 4096;

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

electronic-leadscrew/els-f280049c/Core.cpp

Line 47 in 892a21f

if( reverse )

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

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

electronic-leadscrew/els-f280049c/StepperDrive.h

Line 115 in 892a21f

switch( this->state ) {

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

electronic-leadscrew/els-f280049c/Encoder.cpp

Line 89 in 892a21f

if( rpm > 2000 ) {

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 ***@***.***> 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 <#1?email_source=notifications&email_token=AAZLSHEHGIX2DQMVULTTXMTQTC3ADA5CNFSM4HHEHHQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDVNUFI#issuecomment-552262165>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZLSHH5GXWJWCNDB2R5TALQTC3ADANCNFSM4HHEHHQA> .