CAP1Sup / Intellistep

A rework of the terribly coded firmware from the BTT S42B v2 boards
GNU General Public License v3.0
45 stars 10 forks source link

Revert variable name 'softStepCNT' to 'desiredStep' #79

Closed IhorNehrutsa closed 3 years ago

IhorNehrutsa commented 3 years ago

https://github.com/CAP1Sup/Intellistep/blob/f292d5d595b82d5efa1879fca8f6c93d19482ed4/src/hardware/motor.h#L221-L222 The 'softStepCNT' was the 'desiredStep'

'desiredStep' expresses the purpose of a variable (essence, entity, nature).
'softStepCNT' expresses how a variable is handled (function, method).

The method may change. At first, the method was stepping interrupt, now the method is TIM2(CNT), DMA (TIM2 (CNT)) is possible in the future. But the 'entity' stays the same - the 'desiredStep'.

Renaming is not a difficult procedure in VSCode, but it may inconvenient, if you make big changes in potent branches. If you do not object to renaming, I may do it.

CAP1Sup commented 3 years ago

It's not really the desired step count, it's more of a software counter. That's why I renamed it. The desired step is derived using the specified method, not just the software version.

CAP1Sup commented 3 years ago

I don't think that it's even used anymore, now that the hardware counter is in place. Maybe we could get some minor performance benefits by removing it? It doesn't really make sense reflecting on it

xerootg commented 3 years ago

I haven't had time to look at the tim2 implementation, but if you are indeed referencing tim2->CNT then it sure seems like counting in software is just lost cycles.

On Thu, Jul 15, 2021, 15:15 Christian Piper @.***> wrote:

I don't think that it's even used anymore, now that the hardware counter is in place. Maybe we could get some minor performance benefits by removing it? It doesn't really make sense reflecting on it

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CAP1Sup/Intellistep/issues/79#issuecomment-881018245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6S6ST2W3AS26YNUG2I5P3TX5FWRANCNFSM5AOJS5JA .

CAP1Sup commented 3 years ago

Would you mind taking a look at it? You seem to know what you're doing. Myself? Not so much

IhorNehrutsa commented 3 years ago

Are you sure that int32_t is enough? https://github.com/IhorNehrutsa/Intellistep/blob/ad46de343e5d191641bd608baa850723b871c984/src/user/config.h#L17-L21

// The maximum number of steps per rev is 200*32==6400 (for 1.8 degree per step motor)
//                                    and 400*32==12800 (for 0.9 degree per step motor)
// int32_t allow to store +/- 2^31==2147483648 steps and 2^31/6400==335544 rev's (for 1.8 degree per step motor)
//                                                       2^31/12800==167772 rev's (for 0.9 degree per step motor)
typedef int32_t increments_t;

Maybe int64_t is required in the future.

Look here http://leadshine.com/series.aspx?type=products&category=stepper-products&producttype=stepper-drives&series=EM-S The lowest model EM415S has resolution 200, 400, 800, 1600, 3200, 6400, 12800, 25600 steps The highest model EM882S has incredible 40000 steps.

xerootg commented 3 years ago

It's on my 'todo' list 🤣 I promise I will look.

On Thu, Jul 15, 2021, 15:19 Christian Piper @.***> wrote:

Would you mind taking a look at it? You seem to know what you're doing. Myself? Not so much

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CAP1Sup/Intellistep/issues/79#issuecomment-881020211, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6S6SWZXTW3Q5RPWLZOE3LTX5GERANCNFSM5AOJS5JA .

CAP1Sup commented 3 years ago

@IhorNehrutsa I really don't think that we need a 64 bit integer. Most users will never exceed over a hundred, let alone a thousand or more revolutions.

IhorNehrutsa commented 3 years ago

If we stay on the in32_t size of the step counter, then softStepCNT is not needed. Edited: But 'desiredStep' is needed.

CAP1Sup commented 3 years ago

Sounds good to me

IhorNehrutsa commented 3 years ago

Oh no, there are other interfaces to set 'desiredStep' in PID mode. It may be SET_POSITION command through the UART or CAN.

CAP1Sup commented 3 years ago

The hardware counter can be set, I made a function for it

IhorNehrutsa commented 3 years ago

Maybe

IhorNehrutsa commented 3 years ago

335544 rev's / 1000RPM ~ 336 min == 5.6 hour in one direction for counter overflow. I vote for int32_t.

IhorNehrutsa commented 3 years ago

I am confused by 'increments_t'. The first association is increment/decrement, not a 'step'.

CAP1Sup commented 3 years ago

Sorry about that, I misunderstood. I adjusted the comments on your PR to make it more clear. I think that I can close this now, your solution was excellent