Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initFOC: remove unnecessary delay #370

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

Schnilz
Copy link
Contributor

@Schnilz Schnilz commented Jan 21, 2024

Hey, thanks for writing simpleFOC.
It is amazing how easy you made it conquer the otherwise complex topic of motor control!

I looked into skipping the direction and zero calibration done in initFOC with the find_sensor_offset_and_direction.ino example. This worked nicely but the function still block for a whole second which i didn't get.
From looking a the code i don't know why the delay is needed so i removed it, and my driver-code still works (but "boots" faster). If it is needed for the calibration or something, I would suggest moving it in the if statements so it gets skipped when not needed.

Thanks!

@runger1101001
Copy link
Member

Hey, thanks for your nice feedback, and thanks for the PR!

You're not the first to observe this, and also not the first to question it - I have done so myself.

These delays have been in the code since before I first started to help on it. Their purpose isn't completely clear to me.

The first delay, on line 154, could easily be added by the user if needed. My guess as it its purpose is that is allows the driver hardware, which was presumably enabled just before in motor.init(), has a chance to start up. 500ms is very long for this, in my experience most drivers have a wake up time measured in microseconds. But then I only work with little motors, it might be different for large inverters. In any case, the user could add the correct amount of delay themselves.

The second delay, in line 168, if I were to guess its purpose then it is to allow the motor to settle after the sensor alignment, before starting the current sense alignment. But there is already a delay in the sensor alignment function, having both is a little confusing. It's also not clear it's needed at this point, if there was no sensor alignment for example.

So I tend to agree with you that both can be removed, but its also clear that this will change the behaviour of SimpleFOC for all users, so even though the code change is tiny, I consider the impact to be large.

I'd like to check with Antun what he thinks about removing them.

@runger1101001 runger1101001 added the enhancement New feature or request label Jan 21, 2024
@askuric
Copy link
Member

askuric commented Jan 22, 2024

Hi guys,

This is perfectly reasonable.
I think that these delays are a remainder from the earlier code where the sensor and current alignement were implemented within initFOC and were not handled within their own functions.

I do agree with Richard's conclusions here and I am happy to merge this PR. We might consider also doing a pass over the other delays in the library to potentially reduce them. Especially since some of them have been set in the original Arduino UNO based code.

@runger1101001
Copy link
Member

Then lets merge it :-)

@runger1101001 runger1101001 merged commit ecfbcd2 into simplefoc:dev Jan 26, 2024
16 checks passed
@runger1101001 runger1101001 self-assigned this Jan 26, 2024
@runger1101001 runger1101001 added this to the 2.3.3_Release milestone Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants