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

Kv rating fix #347

Merged
merged 2 commits into from
Dec 2, 2023
Merged

Kv rating fix #347

merged 2 commits into from
Dec 2, 2023

Conversation

nmscode
Copy link
Contributor

@nmscode nmscode commented Nov 27, 2023

fixes: #344

Need to verify that this is the correct math/logic. It assumes sinusoidal bemf. Perhaps can add an option for more trapezoidal bemf motors.

@runger1101001
Copy link
Member

Thank you very much for contributing this!

I'm going to hold off merging it until after the next release, which I hope will be quite soon. That will give us some time to then test the effects on the dev branch.

Introducing this change will correct the calculations, but it will have the side effect of changing the behaviour of existing setups. Many people will have "tuned" their settings, picking a KV value for themselves which differs from the actual one but works well in practice. People will have to change these values after we introduce the fix. So its something we need to do carefully.

@runger1101001 runger1101001 self-assigned this Nov 28, 2023
@runger1101001 runger1101001 added this to the 2.3.3_Release milestone Nov 28, 2023
@runger1101001
Copy link
Member

I have another question: why not pre-multiply the KV_rating value by the factor of _SQRT3/_RPM_TO_RADS in the init method, and then not have to do this multiplication/division each time through the loop?

@nmscode
Copy link
Contributor Author

nmscode commented Nov 28, 2023

That is also a possibility. I was not sure if the KV_rating variable might be used for something else and would be needed without the multiplier. However, I can make the change so that it is pre-multiplied if that is preferable.

@Candas1
Copy link
Collaborator

Candas1 commented Nov 29, 2023

I also have the feeling this will be confusing if KV_rating is used somewhere else.
And someone changing kv_rating after the constructor should know that as well.
It could be this is optimized by the compiler anyway.

@runger1101001
Copy link
Member

You're right - it would be confusing to have the parameter so named, but the value represent a different quantity.

So keep as is, but perhaps remove the unneeded brackets from the calculation - not sure if the compiler would respect the order implied by the parenthesis or optimize it away, but in this case we'd prefer the brackets around the two constants to make it clear to the compiler?

@askuric
Copy link
Member

askuric commented Dec 2, 2023

This looks good to me.
I've added sqrt(2) in order for the people to avoid setting too high KV values, to the KV values that people set relatively similar to the ones given by the manufacturers.
Honestly, I've found sqrt(2) to be empirically a good number. But I did not go any further with it.

This solution is much better both in terms of the code that is easy to understand(sqrt(3) instead sqrt(2)), and keeping the KV_rating the same as the one user has set will make it much easier to set for the user.

@runger1101001 runger1101001 merged commit 186e5cf into simplefoc:dev Dec 2, 2023
16 checks passed
@runger1101001
Copy link
Member

So it's merged! :-) Thank you very much for the contribution and the discussion!

@Candas1
Copy link
Collaborator

Candas1 commented Jul 8, 2024

Hi Guys,

Is the statement in the docs about increasing the kv by 10-20% still valid now ?
https://docs.simplefoc.com/voltage_torque_mode#voltage-control-with-current-estimation-and-back-emf-compensation

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants