•  

Comment Results

Review Name Created Custom Fields Content
UI-DEN-4860-BLE-1 12 Jan 2021

same

UI-DEN-4860-BLE-1 13 Jan 2021

Done

UI-DEN-4860-BLE-1 13 Jan 2021

Done

UI-DEN-4860-BLE-1 13 Jan 2021

Done

UI-DEN-6349-1 13 Jan 2021

RESOLVED

UI-DEN-6349-1 08 Jan 2021

please whenever possible use the QtQuick versions instead of the QtQuick.controls.
So in this case TextInput can be used instead of TextField.
And then please remove the "import QtQuick.Controls 2.12"

UI-DEN-6349-1 13 Jan 2021

RESOLVED

UI-DEN-6349-1 13 Jan 2021

I've moved the comment back so it's inside the if statement

UI-DEN-6349-1 13 Jan 2021

RESOLVED

UI-DEN-6349-1 13 Jan 2021

RESOLVED

UI-DEN-4860-BLE-1 13 Jan 2021

By connmand did you mean command?
Is this correct?
and if it is what is that?
Does it mean connection manager daemon?

DG-DEN-5855-1 10 Dec 2020

Misspell "Publish"

HD-DEN-5674-2 30 Dec 2020

should not instead of shouldn't.

HD-DEN-5674-2 30 Dec 2020

Fixed.

UI-DEN-5282-1 29 Oct 2020

RESOLVED.

UI-DEN-5282-1 29 Oct 2020

RESOLVED.

DG-DEN-3421-2-1 13 Nov 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-5887-1 30 Dec 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-5282-1 29 Oct 2020

RESOLVED.

DG-DEN-3421-2-1 13 Nov 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-5282-1 29 Oct 2020

RESOLVED.

UI-DEN-5751-1 01 Feb 2021

RESOLVED

DG-DEN-3421-2-1 13 Nov 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-5674-2 30 Dec 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-7135-1 12 Apr 2021

RESOLVED

HD-DEN-5674-2 30 Dec 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-5674-2 30 Dec 2020

RESOLVED in CODE WALKTHROUGH.

HD-DEN-5674-2 30 Dec 2020

Implemented TODO. Removed comment.

DG-DEN-5846-1 30 Nov 2020

Done

DG-DEN-3421-2-1 14 Nov 2020

You are right. Removed the if statement.

DG-DEN-3421-2-1 13 Nov 2020

Misspell REACOTRS and SELECTD?

DG-DEN-3421-2-1 13 Nov 2020

Do we need this if statement? if the reactor is off, then a request to turn off should generate a TRUE too?

DG-DEN-3504-1 13 Nov 2020

I don't see this function actually exiting the mode. Is it caller's responsibility to return to re-circ mode after calling this function?

HD-DEN-5674-2 30 Dec 2020

Fixed.

DG-DEN-5846-1 30 Nov 2020

Done

UI-DEN-4964-1 03 Jan 2021

Please change it to DGVersions as you have stated in the infoText function. this should be exactly the text user may search for in the log file to find the logged message.

UI-DEN-4964-1 03 Jan 2021

RESOLVED

DG-DEN-5846-1 30 Nov 2020

Added a check for minimum RPM assuming the RPMs should never be zero.

UI-DEN-4964-1 03 Jan 2021

RESOLVED

DG-DEN-5846-1 30 Nov 2020

I agree. It must have been from the past. Changed it to 1Hz.

UI-DEN-4964-1 29 Oct 2020

When this code review started the Doxygenization wasn't done.
So assumed that this code could be included in master and covered as part of the Doxygenization.
Now that the Doxygenization is done, all the classes shall have the documentation with messaging information.
Please add to all your classes.
For example on how to do please refer to the Model/View classes.
And when doc generated it will also be used for SDD.
it should contain:

  • \brief
  • \details
  • Message information
  • Message Payload information as each field in a line (see example below)
  • \sa(see also) which has related classes to this class
  • Logging info which at least has 3 lines (see example below and follow the HTML syntax for formating in doxygen please.)


An example provided. please look at more examples for your specific case in the code on the master branch.

/*!
 * \brief   The MAdjustSalineResponse class
 * \details The Saline Bolus adjustment response model
 *
 * | MSG | CAN ID | M.Box | Type | Ack | Src | Dest | Description           |
 * |:---:|:------:|:-----:|:----:|:---:|:---:|:----:|:---------------------:|
 * | 20  | 0x020  | 6     | Rsp  | Y   | HD  | UI   | Saline Bolus Response |
 *
 * | Payload  ||
 * |          ||
 * | #1:(U32) | \ref Data::mAccepted |
 * | #2:(U32) | \ref Data::mReason   |
 * | #3:(U32) | \ref Data::mTarget   |
 *
 * \sa Data
 * \sa MAdjustSalineReq : Saline Bolus Request
 * \sa MTreatmentSaline : Saline Bolus Data
 *
 * <h2 class="groupheader">Logging info</h2>
 * |              ||
 * |              ||
 * | typeText     | Event        |
 * | unitText     | HD           |
 * | infoText     | AdjustSaline |
 *
 */
HD-DEN-4641-1 01 Dec 2020

RESOLVED in CODE WALKTHROUGH.

DG-DEN-5846-1 01 Dec 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-4964-1 05 Jan 2021

RESOLVED

DG-DEN-5846-1 01 Dec 2020

RESOLVED in CODE WALKTHROUGH.

DG-DEN-5846-1 01 Dec 2020

RESOLVED in CODE WALKTHROUGH.

DG-DEN-5846-1 30 Nov 2020

Don't need "name" in these enum and type names and maybe add "DG" (e.g. DG_Fans, NUM_OF_DG_FANS and DG_FANS_T). If keeping "name", at least fix grammar (e.g. fan_names, NUM_OF_FAN_NAMES and FAN_NAMES_T).

DG-DEN-5846-1 30 Nov 2020

Fix comment. "They should be above...".

DG-DEN-5855-1 09 Dec 2020

RESOLVED in CODE WALKTHROUGH.

UI-DEN-6349-1 13 Jan 2021

Yes, the styling has been updated in the new file