Split mobiflight.cpp into multiple files

[elral]

@MobiFlight-Admin @neilenns @GioCC
During creating the last pull request it was some kind of "difficult" to keep the overview in mobiflight.cpp. A lot of scrolling through the file was required to find all related parts of the code.
PlatformIO has better capabilities to use multiple files than the Arduino IDE, and discussions in the past shows that all of us agree to split mobiflight.cpp into severall files.
@neilenns has done this already by setting up an additional file where all devices and other parts are set up in a class, I did it in the past not using classes but single files for each "functional group".

I will add an example of the file structure I have setup, but this should not mean that my proposal should be implementes. It is mean as a starting point to discuss how to split mobilflight.cpp and to habe an issue to keep track on this topic.

If one would like to have a deeper look into the file structure and the files itself, see my branch https://github.com/elral/MobiFlight-FirmwareSource/tree/multiple_files

[GioCC]

I absolutely subscribe to this idea. In the weekend I'll have a look, but I'm confident of your judgment.

[neilenns]

I would love this. It will reduce the chance of merge conflicts when people are working on different parts of the firmware at the same time.

Instead of adding additional files can we move the methods to static methods on the associated classes? Then everything for analog would live in MFAnalog, everything for output would live in MFOutput, etc. (I don't know if this would actually work, just wondering if it is possible)

[elral]

@neilenns This was exactly the point where I was unsure and thought about a couple of days. In the end I came up to move it into separate files because I treated the classes as a kind of "library" and that I was unsure if this will work to have the existing functions in the files for the classes (in the mean time a saw it this way in another project).

This weekend I could set it up e.g. for the analogIn in this way to see how it works / look like to evaluate if this makes more sense.

[neilenns]

If static methods work the advantage would be everything about each module type would live in one place. We'd never have to go "wait, is that method in output.cpp or MFOutput.cpp? 🤔"

[GioCC]

I agree with @neilenns , in fact, that's exactly one of the first thoughts that came through my mind as I went to browse the code.
Although it's also true that there is a sensible distinction between the content of the two file sets. I have to think about it a little (I'm still on the job as of now).

[GioCC]

OK, I have had a look and I'm ready to give you my 2cent feedback.

First of all, full thumbs up for the code split, but I think that was already settled.

About the alternative to join all code pertaining to a device vs having two distinct files (i.e. "MFButton.cpp" vs "MFButton.cpp"+"button.cpp"), I had a little afterthought. (I'll use "button" as example)

Although I initially agreed with @neilenns 's proposal, which is appealing since it would be great to have a single file (of reasonable size) for each device, on second thought I would rather dismiss that as logically incorrect (therefore misleading).

While the "MFButton" file holds the code for the "button" object itself, the "button.cpp" part is purely client code, which should be decoupled from the device class; even more so, it has very little reason to be part of the class as static methods.

Bottom line, I would definitely keep the two files separate, maybe slightly changing some naming if it helps (e.g. "button.cpp" in ".\inputs" rather that ".\MFinputs" etc).

[neilenns]

One way I've handled this in the past is to put Add-style methods into a static DeviceManager class. That would solve the problem of them going somewhere that makes sense conceptually, instead of in the actual device class.

Unfortunately I'm not sure that really makes things much better, as it would just move the bulk of the length of mobiflight.cpp into another file that would also be quite long?

[GioCC]

@neilenns 101% agree. In order to keep things most manageable, one could have e.g. two classes as InputDeviceManager and OutputDeviceManager. Maybe not really conceptually justified, but that would at least partially help.

Beside that (and I did not want to go that far for fear of digressing), it would be a good step towards a full objectification of the code.

[elral]

@neilenns and @GioCC thanks for your comments!

Just for my understanding (I am not soo familiar with the wording :( ), setting up two classes would mean e.g. for the inputs:
InputDeviceManager::InputDeviceManager() <- hmmhmm, but what should be initialized
InputDeviceManager::Analog_update()
InputDeviceManager::Analog_readBuffer()
.....
InputDeviceManager::Encoder_attach()
InputDeviceManager::Encoder_update()
....
and the some for all the outputs.

Due to my less experience I am wondering, if

• this really helps to get a better overview
• splitting up the device (e.g. in case of further updates)
• does it have same/less/more memory consumption
• must be really everything OOP in embedded systems or wouldn't it be better to choose the best out of both "worlds"
• InputDeviceManager and OutpuDeviceManager would be declared just one in contrast to the devices (where I absolutely understand the requirement for classes as they can be used severall times)

These are really serious questions from my side to get a better understanding due to my leck of experience.

To have a file per device with the static functions and the classes included seems for me the best way for now in regard of flexibility, seperating the devices and getting a good overview (I set it up accordingly for the MFAnalog in my branch).

[neilenns]

I would do the managers as fully static classes so they don't require memory allocation and wouldn't have a constructor.

I'll take a peek later today and mock up and example. I'm not at all sure it is a good approach 😂

[GioCC]

Hoping to be on topic with what you ask, this would be my answers:

• using classes here would not contribute much, if anything, in terms of clarity; it would make the code a little more uniform in case it should ever be decided to go towards a "pure" C++ structure (see also below)
• ("splitting up the device" is probably related to the previous point or the next one; can't read a question)
• Memory consumption would probably not vary at all, and if even, very marginally
• OOP, particularly in embedded systems, is NOT mandatory. It can be applied wherever it one feels it can help in any way (usually at no cost), but mixed C/C++ programs are perfectly fine; if anywhere, the embedded domain is where this is most true.
• As @neilenns has pointed out, by nature both "managers" would be fully static (ie one copy and static allocation); the only practical difference would merely be the code syntax.
  To all intents and purposes, in our case saying "let's group all these functions in a file named XXX" or "let's write these functions as methods of a static class XXX in file XXX" is perfectly equivalent.
  The only advantage I see beyond the immediate cosmetic "OOPfication" would be the future possibility of going further in building the class by encapsulating also its pertaining data; but, at this point, this would also become a significant issue to tackle. That would imply not just a code regroup, but probably a careful refactoring of the rest of the code too, with a different impact.

At the risk of being annoying, let me advocate again that my preference would definitely go towards keeping the actual device classes separated from the client "stub" functions; these latter can then be grouped in two "inputMgr"/"OutputMgr" files (quite regardless whether C or C++), but even individual files would be ok if that is deemed more convenient.

[neilenns]

Yup. And in my mind the only advantage to the static class is making it clear when those functions are called where they're coming from instead of just being random functions that magically somehow work. Of course that could also be accomplished with a namespace instead of a static class...

[GioCC]

making it clear when those functions are called where they're coming from instead of just being random functions that magically somehow work

Also true!

[elral]

@GioCC @neilenns thanks again for your comments.

I will try to summarize the comments and to see, if I have understood everything correct.

• The device classes (MFAnalog.cpp, ...) should be kept as they are.
• The "manager" functions (do we find a better name ;) ) should be bundled in a file for input and a file for output devices. There will be a third one for the config and cmdmessenger stuff.
• These manager functions could be unchanged C functions (with a namespace) or preferred moved into static classes.

That means at least to get a common proposel we have to decide if we stay with the existing C functions (don't they have already a namespace*?) or to move them to static libraries.

It would be great if @MobiFlight-Admin could comment on this too.

*) I am afraid that I do not have the same understanding (or knowledge) of the definition of a namespace. My understanding is that all functions shoud have the same syntax and it should be clear what the functions for which device does. For me this is fullfilled (except less minor naming differences) for the existing naming.

[GioCC]

That would be a defined scenario; I, personally, would be fully satisfied with that.
C functions w/namespace would be fine by me (that would actually make them C++ functions ;) ).
(config and cmdmessenger could remain in two different files IMHO.)

[elral]

OK, thanks for the big support from @GioCC!
With this help I moved the input devices to one file with namespaces (hopefully, was the first time ;) )
A short check if this is what you mean would help me before moving the output devices to the second file.

[neilenns]

@elral Do you have a branch for it? Where? :D

[elral]

@neilenns yes, in my first post is the link. Once everything is "done" I will issue a pull request.

[neilenns]

@elral Did you push your latest changes? https://github.com/elral/MobiFlight-FirmwareSource/tree/multiple_files still has individual files like you originally had, I don't see namespaces for it.

[elral]

@neilenns I "just" did it for the input devices, outputs will be done when it goes into the correct direction
https://github.com/elral/MobiFlight-FirmwareSource/blob/multiple_files/src/inputMgr.cpp
and accordingly the .h file and changes in the other files changed today

[neilenns]

Ah, I see it now.

This wasn't quite what I was thinking. I was thinking there would just be a single InputManager namespace with all of the relevant methods in it (handleInputShifterOnChange, etc.). That would work for most methods, but things like Add are problematic (I hadn't thought about the Add methods).

As done now it really feels odd. There's no advantage to a single file with a bunch of namespaces in it, it just makes yet another big file which is what we're trying to get away from. And the namespaces being called Button etc just make it clear they could live as static methods on the existing MFButton class instead...

[GioCC]

@neilenns sorry this time I have to disagree (on one point). Whatever form they take, those methods have no business being attached as part of the MF_xxxx classes... I believe this is the one thing, if any, we have to stay away from.

[neilenns]

Yeah, I'm fine with that, I'm just saying the current namespace example isn't any better than what we have now.

Maybe separate files + namespace is the answer?

[GioCC]

Why not? Albeit debatable, I still find it better (smaller files, but not as dispersed as all separate files).

Right now, I'm fighting hard to keep myself from trying to undertake a more thorough (but sensible) refactoring... and I know that I will lose against myself. With that perspective, this solution (or an equivalent one) would be already a step in the right direction, but I admit that it's something that's only in my mind for now.

[neilenns]

Why not? Albeit debatable, I still find it better (smaller files, but not as dispersed as all separate files).

It's still a large file and has multiple things in it that don't really have anything to do with each other. Something about a single source file with that many namespaces in it too just feels... wrong. If I want to go make a change to one of those methods I'm still opening a huge file and having to ctrl+F to find what I need.

The thing I didn't like about single files was how the functions would just magically appear. Putting a namespace around the stuff in single files would solve that issue and give a matching structure to the existing MF* files these methods relate to.

[GioCC]

Well, not that large a file, considering it's still a fraction of the one that we wanted to split in the first place... but I see your point.
The other annoying thing about having many small files is also that you have less of an overview, and you have to open a lot of them if you look for something.
But I think that at this point any of these solutions could basically work, one way or another.

[elral]

The thing I didn't like about single files was how the functions would just magically appear. Putting a namespace around the stuff in single files would solve that issue and give a matching structure to the existing MF* files these methods relate to.

That would also my preferred solution. I like it more to have "multiple" flles instead of one big file, for me it's easier to know which file to open instead of searching in a big file. It gives also the possibility to use different inlude paths if a device should be included or excluded (instead of defines and encapsulating in the file, but (maybe?) more stuff in platformio.ini).
The namespace is something new for me, it's nice and seems to be usefull here. It forces (me) to have similiar names for similiar functions.

[GioCC]

Good then, looks like we finally have a solution! :D
However, at this point the whole question about namespaces becomes rather moot... their only actual purpose would be to allow you to call functions e.g. Button::Add() or Encoder::Add() instead of Button_Add() or Encoder_Add(), which seems pretty pointless.

[neilenns]

It's not pointless, it saves having to prefix all the function names with Button_ 😂 That's what namespaces are for!

[GioCC]

How is it different from prefixing them with Button:: (and possibly forgetting or mixing up some prefix)?
They are meant to be used elsewhere in the code, not just in their definition, and of course you can't group all of them under a same using...

[elral]

Puuuh, changed everything from the inputs back to multiple files w/ namespace and added namespace for the outputs. There are no more dependecies directly between the devices. Some failures from merging the outputshifters (renaming) are also corrected.
Hopefully now comes the finetuning ;)

What I also saw:
OnSetPin() for the outputs uses directly analogWrite(pin,state) and not the class function outputs[_pin].set(state) as pin != _pin. For now the hardware pin is transferred from the UI and not the x.th output. If this should be used the UI has to be changed but which is a problem regarding backwards compatibility.

[DocMoebiuz]

Hey cool! it is a lot better!!!! GREAT JOB.

Idea: Can we not bundle the MFButton.cpp and Button.cpp together in one directory and get rid of the MFModules folder and use MF Analog, MFButton, etc, instead? This way we would have both cpp-files in one place. I would even put the h-file there, but i think it is a platformio thing that they have to reside in libs?

I don't understand the specfic reason for splitting it up in MF_Inputs and MF_Outputs.

[elral]

For the folders there were only the reason that not all files should be on the same order and to group them. @DocMoebiuz I guess your proposal goes into the same direction I mentioned aboved.

.... It gives also the possibility to use different inlude paths if a device should be included or excluded (instead of defines and encapsulating in the file, but (maybe?) more stuff in platformio.ini).

This would mean for every device one folder with the required files (e.g. Analog.cpp, MFAnalog.cpp and their .h files). Within platformio.ini it will be defined which folder has to be used (which works also for the .h files, they must not be in include, see MFBoard.h)

[GioCC]

I personally don't think is a good idea. One folder per device would mean an awful fragmentation, with the result that navigating the source would be even worse than the situation we started from in the first place.

Also, conditional compilation by "hiding" folders wold not be a good idea either:

• the optional code would still need to be referenced in the main section(s) , therefore we would still need #defines anyway (unless we pepper the code with _weak stubs, which would further detract to clarity);
• it would not be as apparent if code is included or not;
• it would (slightly) complicate things for people wanting to use different build systems other than Platform.IO

[GioCC]

I would even put the h-file there, but i think it is a platformio thing that they have to reside in libs?

No, it can be definitely done - I usually find it much better that way, unless there are really lots of files (but even in that case, I split file groups keeping .c/cpp and .h/hpp together).

[neilenns]

I took a peek at the pull request and the general structure makes sense to me. It does look like the pull request incorporates a lot of other changes beyond this splitting into multiple files though. I see switches to using #pragma once, new #if statements for controlling debugging, new methods for setting the last button press time, etc. Was that intentional?

[elral]

Oh sorry, maybe I did too much :(
It was originally not intended, but while splitting the file I had a "deeper" look on these topics and tried to setup everything the same way. I commented this also in the Pull Request.
If it is too much for this pull request I will change it back.

Just one question, which part do you mean with "setting the last button press time"?

[neilenns]

Ultimately it's up to @DocMoebiuz, but personally I prefer pull requests that are "single subject". It has several advantages:

1. It makes it easier for code reviewers. They can go into the review knowing they are looking at a specific improvement/change, and watch for stray things that might have accidentally crept in. It also means they can assume all the changes are about that one improvement, and makes it easier to follow the logic behind the changes. With your current PR, for example, I have to wonder whether the #pragma once was necessary to make the refactor work.

2. It makes it easier to deal with bugs later on. If we merge the PR as is and something stops working it's harder to roll back individual changes and figure out what happened. As the PR is currently a future bug could be because of the refactor, the change to how items are initialized, etc. and we'd have to roll everything back instead of having individual, narrower scoped, changes.