[Maint] Cleanup Hpi refactoring

[RDoerfel]

This PR follows up on #884, cleans up some stuff and improves the documentation. Further, I tried to apply better mutexing in the hpi plugin.

[codecov]

Codecov Report

Merging #890 (753eee2) into main (bd31738) will increase coverage by 6.02%.
The diff coverage is 0.70%.

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   30.20%   36.22%   +6.02%     
==========================================
  Files         452      199     -253     
  Lines       39208    11811   -27397     
==========================================
- Hits        11841     4279    -7562     
+ Misses      27367     7532   -19835     

Impacted Files	Coverage Δ
libraries/disp/viewers/helpers/scalecontrol.h	0.00% <ø> (ø)
libraries/inverse/hpiFit/hpifit.h	0.00% <ø> (-60.00%)	⬇️
libraries/inverse/hpiFit/hpimodelparameters.h	0.00% <0.00%> (ø)
libraries/inverse/hpiFit/sensorset.h	0.00% <0.00%> (ø)
libraries/inverse/hpiFit/signalmodel.h	0.00% <0.00%> (ø)
libraries/rtprocessing/rthpis.cpp	1.78% <0.00%> (-0.07%)	⬇️
libraries/rtprocessing/rthpis.h	0.00% <ø> (ø)
libraries/utils/file.cpp	0.00% <ø> (ø)
libraries/utils/ioutils.cpp	23.68% <0.00%> (-6.32%)	⬇️
libraries/utils/kmeans.cpp	0.27% <ø> (ø)
... and 295 more

[RDoerfel]

@juangpc could you have a look at this one?

[juangpc]

Nice.
I'll get back to you later with some comments.

[juangpc]

Hi Ruben, the code looks fine.
I can't fetch it and test it myself, but since it is restricted to the hpi plugin and related structs, which you know very well, I think it is safe and we should merge it whenever you feel it is ready.

A few minor comments, only because sometimes you share that you'd like to discuss and learn about code.

• I'm pretty sure that you'd agree that the multithread protection is distracting from the logic of the different routines. That is something that would make a lot of sense to hide in the details of those particular settings. One way to do that is to define a set of getters and setters for the variables you want to protect. And have the protection implemented only in those methods, and importantly, only once. Those methods could be private or public, it depends on your own criteria, but let me tell you that there is nothing wrong on having a class consuming its own public api, on the contrary I like it a lot, because it enforces order in the code.
  A different route, would be to use atomic variables. Both implemented in Qt and in the standard library. That would be nice, and you'd gain efficiency (ask Gabriel, he knows! 😄 ) Though not all the members you want to protect are booleans... but I think you know what I mean.
• This doesn't belong to this pr but (only, because you've asked me to talk about these things on the past) I'd like you to reflect on the methods that start with on.... (like onDoSingleHpiFit, or onDoFreqOrder). Probably always, but specially when these methods relate to a GUI element. This kind of construction do compile and do the work, so "they work", however, in my opinion they "are wrong". And again, they compile and do the work, so they are not wrong, but... any code that introduces "unnecessary complexity" to the codebase is, I'm arguing, "wrong". I'd like to know your opinion on the use of these constructions. And hey, please alleviate the weight of the word "wrong" for me, allow me to try to trigger your interest.
• One last thing. How safe is the landing when there are only 3 hpi coils defined?
  Thanks for working on this.

[RDoerfel]

Thank's for the comments! Here are some thoughts:

• Multithread protection: I agree, it is pretty annoying :D I like the idea of having this hidden in functions. But what were atomic variables again? not sure if I know what you mean by that ...

• Complexity of GUI - plugin interaction: I think it is quite annoying to have the actual GUI outside of the plugin, especially when the control windows are so closely tight up. Also, you sort of do things in both, plugin and GUI. This feels a bit wrong. However, I do like that there is not too much GUI handling in the plugin-backend.

[juangpc]

Hi, I think atomic variables are a valuable thing to know about. If you don't have time, let me just say that they are a set of variables provided by some languages that already implement some kind of protection mechanism in case different want to read/write a variable at the same time. So it is something else different from a mutex, but it solves the same problem. And it is faster (than mutexed code).

Other comments are there to try to help you (and me) improve your code (and mine).

[RDoerfel]

@juangpc I just read up on atomic variables. Very cool. So instead of:

bool myTempVariable = false;
m_mutex.lock();
myTempVariable = m_myMemberVariable;
m_mutex.unlock();

if(myTemplateVaiable) {
    // do stuff
}

I could just do:

atomic<bool> m_myMemberVariable; // in header 
if(m_myMemberVariable) {
    // do stuff
}

This is pretty neat. Especially since regarding cppreference, this could be used for custom classes and pointers as well. So would you recommend converting bigger objects (QVector, FiffInfo, Eigen::Matrix) to atomic variables as well?