simplefoc / Arduino-FOC

Arduino FOC for BLDC and Stepper motors - Arduino Based Field Oriented Control Algorithm Library
https://docs.simplefoc.com
MIT License
1.95k stars 511 forks source link

Change init logic - remove params from initFOC #283

Closed runger1101001 closed 1 year ago

runger1101001 commented 1 year ago

This is an API change: I propose to remove the params from initFOC.

If you look at the way it works now, the semantics are actually quite complicated - with int being used internally but Direction enum in the params, and the params overwriting the values potentially already set in the object. There's also no way to set direction without also setting the zero angle via the params.

All in all this is quite complex (and has led me to be bug-hunting for the second time now :-( ) so I propose the following change (in this PR):

I've updated the one example I could find referring to this. If you agree to this change, then I'll update the docs accordingly.

Another small change in this PR: \r is added to the isSentinel() characters, to make things work right even if the user sets the wrong EOL in his console...

askuric commented 1 year ago

I like it, you're right that its better. This was already an implemented behavior, but it was not the one we pushed as a default one. I like that ìnitFOC` stays clean.

In terms of \r I have no real opinions, the only issue that I can see is if we manage well the case when the both are set \n\r. Will this create some issues? I guess no but just in case.

runger1101001 commented 1 year ago

So I searched the code for all uses of isSentinel() and from what I can see it should not cause any problems. There are many parts where it tries to convert the remaining string with atoi(), but this was already working with \n or \r - it should be the same for \n\r. And the buffer itself is constant size, it doesn't matter if there's an extra character in there that doesn't get used. The loop in commander.run() will run one extra time for the extra \n but AFAICT nothing will happen. In the end, people should set \n as the line ending...

So I will merge this and update the docs in the next few days :-)