Split off detection code/refactor mcpwm_foc into smaller files

[Teslafly]

mcpwm_foc.c is currently ~4000 lines long. It is a bit hard to find things in it and it could use some refactoring into smaller more specific functions. The RL and encoder/hall detection code in particular is pretty huge and not related to commutation.

The pid speed/position control loops could probably be pulled out too, among other things.

Is splitting up mcpwm_foc.c on the table? or do you really want to keep that all in one file for whatever reason? The split up files should likely go into an FOC folder.

[vedderb]

These functions deal with many parts of the internal state and configuration of mcpwm_foc. To make that functionality from the outside would require to make interfaces to modify the state in a meaningful way, preferably without making the interface of mcpwm_foc too cluttered and obscure. This can be probably done, but there would be more code in total with functions in mcpwm_foc that are only required to access enough of the internal state to make the detection functions work from the outside. Also, if I want to update the detection functions later (they have seen a lot of updates over the years), I do not only have to think about how to do update them, but also how to update the interface to mcpwm_foc to make that possible and hopefully also remove old unused parts of the interface.

To me that seems to makes the code overall harder to follow and update without much gain. There are also a bunch of detection functions in conf_general that don't need as much of the internal state, which is why there are there. I could possibly make a separate detection file for them at some point, but again, I'm not sure that there is a much gain in that

I general I don't see the merit of splitting very specific functionality into many files and functions that are only going to be used for one thing. The inherent complexity of the problem is still there, it just becomes spread among more files that I have to jump between and there are more interfaces to deal with. Unfortunately the problem itself is complex, and in order to work on mcpwm_foc at all a lot of knowledge and experience about motor control is required.

[ghost]

Sorry to jump into this thread with a different refactoring question: There are a lot of #ifdef HW_HAS_DUAL_MOTORS statements, and I wonder if some of them could be refactored? I could make a PR.

For example

float mcpwm_foc_get_abs_motor_current_filtered_motor(bool is_second_motor) {
#ifdef HW_HAS_DUAL_MOTORS
    return (is_second_motor ? &m_motor_2 : &m_motor_1)->m_motor_state.i_abs_filter;
#else
    (void)is_second_motor;
    return m_motor_1.m_motor_state.i_abs_filter;
#endif
}

mc_state mcpwm_foc_get_state_motor(bool is_second_motor) {
#ifdef HW_HAS_DUAL_MOTORS
    return (is_second_motor ? &m_motor_2 : &m_motor_1)->m_state;
#else
    (void)is_second_motor;
    return m_motor_1.m_state;
#endif
}

could be changed in quite a few places to the below pattern, which I find more readable and would shorten mcpwm_foc.c :

#ifdef HW_HAS_DUAL_MOTORS
#define M_MOTOR(is_second_motor) (is_second_motor ? &m_motor_2 : &m_motor_1)
#else
#define M_MOTOR(is_second_motor) (&m_motor_1)
#endif

float mcpwm_foc_get_abs_motor_current_filtered_motor(bool is_second_motor) {
    (void)is_second_motor;
    return M_MOTOR(is_second_motor)->m_motor_state.i_abs_filter;
}

mc_state mcpwm_foc_get_state_motor(bool is_second_motor) {
    (void)is_second_motor;
    return M_MOTOR(is_second_motor)->m_state;
}

There is irreducible complexity, but there is also repeated logic.

[vedderb]

There are always things that can be improved. I like that style better too, so please make a PR on the dev branch. Just a small comment; I would prefer to have it like this:

#ifdef HW_HAS_DUAL_MOTORS
#define M_MOTOR(is_second_motor) (is_second_motor ? &m_motor_2 : &m_motor_1)
#else
#define M_MOTOR(is_second_motor) ((void)is_second_motor;&m_motor_1)
#endif

float mcpwm_foc_get_abs_motor_current_filtered_motor(bool is_second_motor) {
    return M_MOTOR(is_second_motor)->m_motor_state.i_abs_filter;
}

mc_state mcpwm_foc_get_state_motor(bool is_second_motor) {
    return M_MOTOR(is_second_motor)->m_state;
}

or even a static function that returns the motor, it will be optimized to the same thing as the macro anyway.

Also, if possible, it would be great if you can test the code on single and dual hardware to make sure that nothing broke. In this case it might even make sense to just compare the checksum of the binaries before and after the change, as it should compile to the same thing.

[Teslafly]

From what I see, the detection functions really only rely on the motor state struct motor_all_state_t *motor = motor_now() and may call 1-2 private functions.

yes they do modify the timers, but they always put them back at the end of the function. is the timer modification the part you are trying to keep inside mcpwm_foc?

The interface functions that grab and set motor_all_state_t values are also quite a bit of code that mostly access values through motor_now()

[vedderb]

The motor state struct should really not be accessible outside of mcpwm. It has nothing to do with the interface, it is a collection of the private state of each motor. No one should mess with it unless they know exactly what they are doing. If you write an application and there is a function for getting and modifying the motor state it becomes tempting to do so, which is asking for trouble.

[Teslafly]

If people really want to access the motor state externally, there's not much stopping them with full access to the source code. and you would reject any pr that attempted to push such a change upstream. If you look at other motor control codebases, most have more split up control files.

How do you develop on mcpwm_foc.c? I have taken to having 2 vscode instances, each with dual pane tabs pointing to different parts of mcpwm_foc. I am finding it difficult to quickly find the functions I want to edit because the file is so cluttered with multiple different functionalities. when searching for what I need it often takes me out of my groove. There are legitimate mental reasons to have smaller files that are "pre sorted" https://codecraft.co/2013/03/21/small-files-are-your-friends/

[vedderb]

The interface tells the user what is sensible to access, and what it not. The motor state is deliberately not made accessible in the interface, because that is not intended. Of course anyone can go and make getters for all private variables in any file if they want to mess with them, but then they should expect problems. If they mess with what is exposed in the header they should expect it to behave well.

One option to deal with this in C (as C lacks many of the tools that e.g. C++ offers for these things) would be to make a section with functions in the header and comment that these are not intended to be used unless you know what you are doing. These functions could then expose some of the internal state so that the detection functions can be moved to their own file. However, I personally find it at least just as easy to navigate between the functions in the same file, so I don't really see the problem that you see. Splitting it to folders and files, keeping all headers up to date and coming up with names for functions is more troublesome to me. Keep in mind that this is not the same as the split between files that deal with communication or with functions that are used from many places, this is only for the purpose of splitting a large file in several smaller files. The separate communication files (can, uart, usb etc.) all have clear interfaces that will stay mostly the same and are natural to split at. The 4000 lines in mcpwm would turn into 5000 lines in several files and deal with the same collection of complicated state. Changing one file is then likely to break all of them, and I have to go around and fix them. More work for solving a problem that I barely see.

The detection functions in conf_general came out naturally as it was possible to make them without requiring the internal state of mcpwm, and that might be the case for some of the other functions some day too. The lock rotor functions came quite recently as that was useful to do from the outside, and with more things like that it might come naturally. I see no strong arguments for forcing them out though. You might have different opinions on that, and maybe I will some day too, but for now I am not convinced and won't make a major split and scatter the internal state among several files.

[vedderb]

Just one more comment: if this is important to you and you are up for giving the split a go, please do so. We can then have a look together and if something nicer comes out, I will merge it.