Index: code_review/.gitignore =================================================================== diff -u --- code_review/.gitignore (revision 0) +++ code_review/.gitignore (revision 59b505667dc760aa3b4cf77f64aae3b8cca150dd) @@ -0,0 +1 @@ +.idea/ Index: code_review/UI-Diagram-Behrouz.png =================================================================== diff -u Binary files differ Index: code_review/mvc_review.md =================================================================== diff -u --- code_review/mvc_review.md (revision 0) +++ code_review/mvc_review.md (revision 59b505667dc760aa3b4cf77f64aae3b8cca150dd) @@ -0,0 +1,258 @@ +### Denali UI Code Structure Review, Part 2 + +#### Peter Lucia +plucia@diality.com + +Apr 6, 2020 + +#### Model View Controller + +While it may require more components, it will be more easily maintainable, so I think it's a good choice. + +- https://en.wikibooks.org/wiki/C%2B%2B_Programming/Code/Design_Patterns#Model-View-Controller_(MVC) +"Newcomers will probably see this "MVC" model as wasteful, mainly because you are working with many +extra objects at runtime, when it seems like one giant object will do. ***But the secret to the MVC +pattern is not writing the code, but in maintaining it, and allowing people to modify the code +without changing much else."*** + +From Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides - Design patterns: Elements of Reusable Object-Oriented +Software +- "MVC consists of three kinds of objects. The Model is the application object, the View is +its screen presentation, and the Controller defines the way the user interface reacts to +user input." + +- "The view should ensure that its appearance reflects the state of the model. Whenever the model's data changes, the model +notifies views that depend on it. In response, each view gets an opportunity to update itself. This approach lets you +attach multiple views to a model to provide different presentations. You can also create new views for a model without +rewriting it. " + + +From the UI Application SW Data Adjustment diagram, the GUI is pictured as sending a request to VGui to make a change when +the user requests it. This request is then handled by cApp (Application Controller), then passed to the message dispatcher to +send down to the firmware. There is a back and forth between the UI and Firmware, and then CMessage dispatcher updates +the Model, which then automatically updates the View, which the diagram shows is binded to the QML screens. + +This process seems to align with the MVC design pattern. + +Questions: +1. What is the View block between the Model and GUI? +2. How decoupled are our views, controllers, and models are from one another? Could we easily attach new views to a model +to provide new presentations? + +![Architecture](UI-Diagram-Behrouz.png) + +It seems that in the context of MVC, our "view" really is both QML + C++ associated View classes that +hand off data directly to the QML. + +For example, looking at SettingsHome.qml: + +The following model data + +``` +Repeater { + model: [ + vTreatmentDialysateFlow.dialysateFlow_FlowSetPoint .toFixed(2) , + vTreatmentDialysateFlow.dialysateFlow_MeasuredFlow .toFixed(2) , + vTreatmentDialysateFlow.dialysateFlow_RotorSpeed .toFixed(2) , + vTreatmentDialysateFlow.dialysateFlow_MotorSpeed .toFixed(2) , + vTreatmentDialysateFlow.dialysateFlow_MotorCtlSpeed .toFixed(2) , + vTreatmentDialysateFlow.dialysateFlow_MotorCtlCurrent.toFixed(2) , + "%" + vTreatmentDialysateFlow.dialysateFlow_PWMDutyCycle .toFixed(2) +] + +``` +is exposed by vtreatmentdialysateflow.h/.cpp: +``` +class VTreatmentDialysateFlow : public QObject +{ + Q_OBJECT + + PROPERTY( qint32 , dialysateFlow_FlowSetPoint , 0, false) + PROPERTY( float , dialysateFlow_MeasuredFlow , 0, false) + PROPERTY( float , dialysateFlow_RotorSpeed , 0, false) + PROPERTY( float , dialysateFlow_MotorSpeed , 0, false) + PROPERTY( float , dialysateFlow_MotorCtlSpeed , 0, false) + PROPERTY( float , dialysateFlow_MotorCtlCurrent, 0, false) + PROPERTY( float , dialysateFlow_PWMDutyCycle , 0, false) + + VIEW_DEC(VTreatmentDialysateFlow, DialysateFlowData) + +}; +``` +and the properties are updated by the MDialysateFlow model's Data struct, found in mtreatmentflows.h: + +``` + +class MDialysateFlow : public MFlow { +public: + struct Data { + qint32 mFlowSetPoint = 0; /*!< vFlowSetPoint - Flow Set Point value of type signed int extracted out */ + float mMeasuredFlow = 0; /*!< vMeasuredFlow - Measured Flow value of type float extracted out */ + float mRotorSpeed = 0; /*!< vRotorSpeed - Rotor Speed value of type float extracted out */ + float mMotorSpeed = 0; /*!< vMotorSpeed - Motor Speed value of type float extracted out */ + float mMotorCtlSpeed = 0; /*!< vMotorCtlSpeed - Motor Controller Speed value of type float extracted out */ + float mMotorCtlCurrent = 0; /*!< vMotorCtlCurrent - Motor Controller Current value of type float extracted out */ + float mPWMDutyCycle = 0; /*!< vPWMDtCycle - PWM Duty Cycle in % value of type float extracted out */ + }; + +... + +typedef Model::MDialysateFlow::Data DialysateFlowData; + + +``` + +Are two cpp classes necessary to expose the model's data to QML? +Could it be done with just one model class? + +I took a look at the macros which handle the signal slot mapping. Now, I can see how it's +useful to dynamically add signals and slots with macros. Maybe it could save lines of code? + +Take this call in applicationcontroller.cpp for example: + +``` + ACTION_RECEIVE_MODEL_BRIDGE_CONNECTIONS(_MessageDispatcher) + +``` + +Which calls the following macro in mmodel.h: + +```batch +#define ACTION_RECEIVE_MODEL_BRIDGE_CONNECTIONS(vSOURCE) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, BloodFlowData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, DialysateFlowData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, OutletFlowData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, TreatmentTimeData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, PressureOcclusionData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, AlarmStatusData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, PowerOffData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, TreatmentRangesData ) \ + ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, AdjustBloodDialysateResponseData ) +``` + +Where ACTION_RECEIVE_BRIDGE_CONNECTION is defined in main.h. That just points +to the macro ACTION_METHOD_BRIDGE_CONNECTION, hardcoding didActionReceive and onActionReceive +as the signal and slot names. + +``` + +#define ACTION_METHOD_BRIDGE_CONNECTION(vMETHOD, vSOURCE, vTYPE) \ + connect(&vSOURCE, SIGNAL(did##vMETHOD(const vTYPE &)), \ + this , SLOT( on##vMETHOD(const vTYPE &))); +//--------------------------------------------------------------------------------// +#define ACTION_RECEIVE_BRIDGE_CONNECTION(vSOURCE, vTYPE) \ + ACTION_METHOD_BRIDGE_CONNECTION(ActionReceive, vSOURCE, vTYPE) + +``` + +That's three macros to accomplish the following. + +```batch + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const BloodFlowData&)), \ + this , SLOT( onActionReceive(const BloodFlowData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const DialysateFlowData&)), \ + this , SLOT( onActionReceive(const DialysateFlowData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const OutletFlowData&)), \ + this , SLOT( onActionReceive(const OutletFlowData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const TreatmentTimeData&)), \ + this , SLOT( onActionReceive(const TreatmentTimeData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const PressureOcclusionData&)), \ + this , SLOT( onActionReceive(const PressureOcclusionData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const AlarmsStatusData&)), \ + this , SLOT( onActionReceive(const AlarmsStatusData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const PowerOffData&)), \ + this , SLOT( onActionReceive(const PowerOffData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const TreatmentRangesData&)), \ + this , SLOT( onActionReceive(const TreatmentRangesData&))); + connect(&_MessageDispatcher, SIGNAL(didActionReceive(const AdjustBloodDialysateResponseData&)), \ + this , SLOT( onActionReceive(const AdjustBloodDialysateResponseData&))); +``` +In my opinion the above macros hide the true intention of the code. The signals/slots makes it simpler and +clearer what the intention is. 16 lines with macros vs. 18 lines with signals / slots. So, you're not really saving lines +of code, but instead are just placing the mappings somewhere else instead of inside the class they affect. +So, I think this could be simplified. + +The following macro, however, I think saves you lots of lines of code, and is particularly useful way +of automating setup of Q_PROPERTIES. Nice! + +```batch + +#define PROPERTY(vTYPE , vVARIABLE , vDEFVALUE, vALWAYSEMIT ) \ + Q_PROPERTY(vTYPE vVARIABLE \ + READ vVARIABLE \ + WRITE vVARIABLE \ + NOTIFY vVARIABLE##Changed) \ + Q_SIGNALS: \ + void vVARIABLE##Changed \ + ( const vTYPE & v##vVARIABLE ); \ + private: \ + vTYPE _##vVARIABLE = vDEFVALUE; \ + vTYPE vVARIABLE () const { \ + return _##vVARIABLE ; \ + } \ + void vVARIABLE ( const vTYPE & v##vVARIABLE ) { \ + static bool init = false; \ + if ( vALWAYSEMIT \ + || ! init \ + || _##vVARIABLE != v##vVARIABLE ) { \ + DEBUG_PROPERTY_CHANGED(vVARIABLE) \ + init = true; \ + _##vVARIABLE = v##vVARIABLE; \ + emit vVARIABLE##Changed( _##vVARIABLE ); \ + } \ + } + +``` + +Other topics: + +It seems that there was an intention to create a message model class that we haven't yet had time for. + +from messageglobals.h: +``` +struct Message { // TODO : Should be converted to MessageModel class // no time left for now !!! + Can_Id can_id; + Sequence sequence = 0; // seq 0 is invalid + Gui::GuiActionType actionId = Gui::GuiActionType::Unknown; +... +``` + + + +We might consider adding a controllers folder. Right now, the applicationcontroller is in the +parent directory, while the gui controller is in the gui directory. + +Otherwise, the organization looks good: +```batch +abstract/ +applicationcontroller.cpp* +applicationcontroller.h* +applicationpost.cpp* +applicationpost.h* +canbus/ +configuration/ +gui/ +main.h +maintimer.cpp* +maintimer.h* +model/ +storage/ +threads.cpp +threads.h +utility/ +view/ + +``` + +QML: + +The QML code is well structured. The grouping makes sense. Lots of reusable components, which is great. +I think using multiple groups of stacks - one for Manager, Treatment, and Settings should be okay, too. + +``` +components/ +dialogs/ +globals/ +main.qml* +pages/ +``` Index: code_review/review.md =================================================================== diff -u --- code_review/review.md (revision 0) +++ code_review/review.md (revision 59b505667dc760aa3b4cf77f64aae3b8cca150dd) @@ -0,0 +1,265 @@ +### Denali UI Code Structure Review + +#### Peter Lucia +plucia@diality.com + +Apr 3, 2020 + +#### Model View Controller + +While it may require more components, it will be more easily maintainable, so I think it's a good choice. + +- https://en.wikibooks.org/wiki/C%2B%2B_Programming/Code/Design_Patterns#Model-View-Controller_(MVC) +"Newcomers will probably see this "MVC" model as wasteful, mainly because you are working with many +extra objects at runtime, when it seems like one giant object will do. ***But the secret to the MVC +pattern is not writing the code, but in maintaining it, and allowing people to modify the code +without changing much else."*** + +It seems we have many models and many views, but only one main controller class, applicationcontroller.cpp, and two +other helper controller classes: messagedispatcher.cpp and guicontroller.cpp. + +As we add views and models, we may want to think about ways of carefully splitting this up into multiple controllers +that have their own domain of responsibility. + + +#### Use of Singletons + +From main.h: + +``` +// TODO : A singleton parent class needs to be created +// to taking care of the Threading, init, quit, and so + +// TODO : +// - We still need to work on threading on other classes +// - We need to have a singleton parent class +// - Some code has been added to debug can interface (We still have swap frames) +#define SINGLETON(vCLASS) \ +private: \ + explicit vCLASS(QObject *parent = nullptr); \ + virtual ~vCLASS() { } \ + vCLASS(vCLASS const &) = delete; \ + vCLASS & operator = (vCLASS const &) = delete; \ +public: \ + static vCLASS &I() { \ + static vCLASS _instance; \ + return _instance; \ + } \ + +``` + +It appears that we are using a form of the Meyers Singleton pattern, which it seems the online community endorses as +being thread safe. + +- https://stackoverflow.com/questions/12248747/singleton-with-multithreads +"In C++11, the following is guaranteed to perform thread-safe initialization:" +``` +static Singleton* getSingletonInstance() +{ + static Singleton instance; + return &instance; +} +``` + +- https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton +“The beauty of the ***Meyers Singleton*** in C++11 is that it's automatically thread safe." + +- From Modern C++ Design: Generic Programming and Design Patterns Applied, the Meyers Singleton is as follows: +``` + +Singleton& Singleton::Instance() +{ + static Singleton obj; + return obj; +} + +``` + +Could there be any flexibility lost by using a macro in main.h for the singleton constructor? What if we eventually need +to pass class-specific parameters to the singleton constructor? + +If we're going to use macros a lot, it's probably a good idea to add a doxygen style code documentation above them in the +so it's clearer what their intended use and function is. + +It seems like the PROPERTY macro as it seems like it will simplify creating properties and lines of code. I like it. +Do you think it could limit or restrict us in any way? + +``` +#define PROPERTY(vTYPE , vVARIABLE , vDEFVALUE, vALWAYSEMIT ) \ + Q_PROPERTY(vTYPE vVARIABLE \ + READ vVARIABLE \ + WRITE vVARIABLE \ + NOTIFY vVARIABLE##Changed) \ + Q_SIGNALS: \ + void vVARIABLE##Changed \ + ( const vTYPE & v##vVARIABLE ); \ + private: \ + vTYPE _##vVARIABLE = vDEFVALUE; \ + vTYPE vVARIABLE () const { \ + return _##vVARIABLE ; \ + } \ + void vVARIABLE ( const vTYPE & v##vVARIABLE ) { \ + static bool init = false; \ + if ( vALWAYSEMIT \ + || ! init \ + || _##vVARIABLE != v##vVARIABLE ) { \ + DEBUG_PROPERTY_CHANGED(vVARIABLE) \ + init = true; \ + _##vVARIABLE = v##vVARIABLE; \ + emit vVARIABLE##Changed( _##vVARIABLE ); \ + } \ + } +``` + +#### Coding Style and Documentation + +Coding style could be more consistent. + +I'm a big fan of automating code styling with tools like cpplint. This avoids spending valuable time in code reviews +fretting over styling issues that can easily be automated away. + +Here is one example of the code styling inconsistencies I noticed: + +The closed parentheses () are spaced far away from the method names here in guiview.h: + +``` + +private slots: + void onActionReceive (GuiActionType vAction, const QVariantList &vData); // UI <= HD/DG + + void doUSBDriveMount (); + void doUSBDriveRemove(); + + void doExport (); + +public slots: // is public since will be used in the UI and is in the same thread. + void doActionTransmit(GuiActionType vAction, const QVariantList &vData); // UI => HD/DG + void doActionTransmit(GuiActionType vAction, const QVariant &vData); // UI => HD/DG + void doUSBDriveUmount(); + void doExportLog (); + + + +``` + + +But then if we go to messagedispatcher.h, for example, the spacing is different; + + +``` +public slots: + bool init(); + bool init(QThread &vThread); + +private slots: + void quit(); + +public: + void enableConsoleOut(bool vEnable) { _builder.enableConsoleOut(vEnable); } + +private: + void initConnections(); + + void initThread(QThread &vThread); + void quitThread(); +``` + +Could use more intention-revealing names. For example, the Data struct used in many model classes isn't particularly +revealing. Could it be renamed to something else that would give more of a hint about what is included in the structure? + +The prefix choices of "m", and "v" don't signify a clear meaning to me. +Typically, the "m" prefix is used to indicate a member variable of a class. +The "v" prefix seems to indicate a local variable. +It seems the underscore prefix is used to indicate that a function or variable should be private. PEP8 in python follows +this convention as well. + +Since our development environments easily provide this information with a hover or Ctrl+Click easy navigation to the +definition of variables and function names, I'm wondering if we really need these single character prefixes. +I think the code could be more readable without them, but if you prefer to keep them that is absolutely fine with me too. + +In many model classes, I see the Data struct is defined like the following example in mpoweroff.h: +``` +public: + QString stringPrefix = "PowerOff"; + struct Data { + quint8 mStatus = 0; + bool mShutDown = false; + }; + +``` + +As the return type of the data() function, it's not quite clear at first what it does. Since the data() function is +in a bunch of model classes, maybe adding the prefix, "get" to it would make its use clearer. + +``` + Data data() const; +``` + +The naming for signals could be made clearer. +From our coding standard "4. Functions and variables should have descriptive and meaningful names or +well-commented declarations; variables with a small scope can have short names." + +For example, how about `receivedAction` instead of `didActionReceive` or `mountedUSB` instead of `didUSBDriveMount` +in guicontroller.h. + +``` + +signals: + void didActionReceive (GuiActionType vAction, const QVariantList &vData); // UI <= HD/DG + void didActionTransmit(GuiActionType vAction, const QVariantList &vData); // UI => HD/DG + + void didUSBDriveMount (); + void didUSBDriveUmount(); + void didUSBDriveRemove(); + + void didExportLog(); + void didExport (); +``` + +In messageglobals.h, the Can_Id uses a mix of snake and mixedCase. Couldn't it just be mixedCase? Is the "e" character +prefix really needed if the IDE can tell us it's an enum? +According to SOP-0026, Software Coding Standards, C++.docx, "Function/Method" and variable names must be Camel Case" +``` +enum Can_Id : quint16 { + eChlid_LOWEST = 0x7FF, + eChlid_NONE = 0x7FF, + + // Broadcasts + //// Alarm + eChlid_HD_Alarm = 0x001, ///< HD alarm broadcast + eChlid_DG_Alarm = 0x002, ///< DG alarm broadcast + eChlid_UI_Alarm = 0x004, ///< UI alarm broadcast [Out] + //// Sync + eChlid_HD_Sync = 0x040, ///< HD sync broadcast + eChlid_DG_Sync = 0x080, ///< DG sync broadcast + eChlid_UI_Sync = 0x200, ///< UI sync broadcast [Out] + + // UI not listening + eChlid_HD_DG = 0x008, ///< HD => DG + eChlid_DG_HD = 0x010, ///< DG => HD + + // UI is listening + eChlid_HD_UI = 0x020, ///< HD => UI + eChlid_UI_HD = 0x100, ///< UI => HD [Out] + + // UI lessens occasionally + eChlid_DG_UI = eChlid_DG_Alarm, ///< No direct channel has been defined between DG&UI, May be required for logging + eChlid_UI_DG = eChlid_UI_Sync , ///< No direct channel has been defined between DG&UI, May be required for logging +}; + + +``` + +The styling in the cpp views is a mix of snake case and mixed case as well: + +For example, in vtreatmentbloodflow.cpp: +``` + + bloodFlow_FlowSetPoint (vData.mFlowSetPoint ); + bloodFlow_MeasuredFlow (vData.mMeasuredFlow ); + bloodFlow_RotorSpeed (vData.mRotorSpeed ); + +``` + +A minor point, but I did come across a few self describing functions that don't have doxygen comments (format.cpp and mtreatmenttime.cpp) +If we have code coverage requirements for documentation purposes, I suppose this will eventually need to be addressed.