added velocity calculation to StepDirListener for use in FF

[Copper280z]

This adds the option to use the StepDirListener velocity, as calculated by measuring the time between pulses, with the velocity feed forward feature.

In initial testing this reduced position error significantly for constant velocity moves.

One issue I'd like commentary on is the implementation of zeroing out the velocity when no pulses have arrived for some time. As the velocity is updated in an ISR, if pulses stop coming the velocity isn't updated and will hang at the value at the end of the last ISR. I've dealt with this in the cleanLowVelocity function, which needs to be called in the main loop somewhat frequently.

[runger1101001]

Thank you for this. We're just before a release, so if you don't mind I will hold off merging this until after the release 2.3.1 and it will be part of the next release.

Looking over the code, I had the following thoughts:

• Perhaps if people are actually using this class we should take the time to make it work really well.
• the getVelocityValue() function can only be called from inside the handle() function. We should make it private, or just move its code into handle()
• cleanLowVelocity() is not safe from the concurrency point of view. It uses multiple member fields of the class to do its calculations which get changed in the interrupt routine. So if interrupted at the wrong time it would do its calculation with values that don't belong together. In practice it doesn't really look like it would cause a problem in this particular function.
• we should keep track of the total number of step pulses received. That would allow checking for missed steps by a different protocol layer.

[runger1101001]

Actually its on its own branch so we could merge it, but its only in draft status right now anyways I see...

[Copper280z]

I'm with you on making it work well, I don't want to include buggy features. I'm also ok with missing this release.

I'd be happy to work on this some before merging, or merge to the FF branch then do another PR for the changes you mentioned.

[phpkadir]

this is awesome addition for arduinoFOC.

[Copper280z]

I think I handled the interrupt safety concerns, I think leaving getVelocityValue is a good idea in case you want to get the value without having attached a pointer that's updated in the ISR.

This does store the net number of step inputs received, in "count", which is akin to the absolute position. Are you suggesting we store the total number of received pulses, irrespective of the dir pin state?

I'm not sure if updating an attached pointer like this is safe in the general sense, on 32bit platforms a float access should be atomic (right?), but I don't know about an 8bit. The worst case as it sits (assuming atomic access for floats) might be getting an old position but a new velocity when the main loop code reads from these pointer addresses. Practically speaking, I don't think this is a large problem. Because of this safety uncertainty, I added the noInterrupts/interrupts calls in getValue and getVelocityValue, so that those could possibly become the suggested user interface.

[runger1101001]

This seems good to me now.

it might be worth simplifying the API a bit and cleaning up the interrupts/noInterrupts code by having a getValue() - external API - and a getValueInternal() - also called by the handle() routine, where the noInterrupts only is used on the external API. Same for velocity.

The API where the value is set to the provided pointers - I am not sure I get it. It has a few potential pitfalls:

• users should not set the motor.target or things like this as the pointer value. The interrupts can happen at any time, and generally we want to update our parameters at well-defined points in time, and ensure calculations happen using the same value.
• users need to be aware that they may need to use the volatile keyword on the float target variable, depending on the situation
• users need to get the values inside a noInterrupts block if they want velocity and position values guaranteed to come from the same calculation

Anyways, it was like that before so you're just keeping the API consistent, its the right thing to do.

The PR is still in draft status so you have to do something to make it active if we want to merge it :-)

[Copper280z]

I set this to ready to review a while back, is there anything else to do?

[runger1101001]

Sorry about that! I lost track of when it left draft mode.

[runger1101001]

It doesn't seem to compile on ESP32, but since its on the feature branch still anyways, lets merge it and fix it there...