This is a list of all comments for HD-DEN-11980-1.

Review Summary: No summary


----------------------------------------
File: firmware/App/Modes/ModeTreatmentParams.c
Revision Comment by Sean Nash on 16 February 2022, 09:59 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11936

  Add one more blank line here.

  Reply by Michael Garthwaite on 16 February 2022, 12:33
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:33
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:04 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11940

  Remove blank lines.

  Reply by Michael Garthwaite on 16 February 2022, 12:32
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:33
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:09 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11944

  Function name is misleading.  Sounds like Dialin is sending us
  new parameters, but it is only requesting current params be
  sent.  I would change to something like
  testSendCurrentTreatmentParameters().

  Reply by Michael Garthwaite on 16 February 2022, 12:32
  > Renamed to testSendCurrentTreatmentParameters(). thanks!

    Reply by Sean Nash on 17 February 2022, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:04 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11941

  Check editor settings.  Tabs should be set to 4 spaces.

  Reply by Michael Garthwaite on 16 February 2022, 12:31
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:33
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:05 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11942

  Add a blank line between declarations and code.

  Reply by Michael Garthwaite on 16 February 2022, 12:31
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:05 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11943

  Check that Dialin has logged in before acting on a Dialin
  request (TRUE == isTestingActivated()).

  Reply by Michael Garthwaite on 16 February 2022, 12:31
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:32
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 09:59 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11935

  Remove extra blank lines.

  Reply by Michael Garthwaite on 16 February 2022, 12:31
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:34
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemCommMessages.c
Revision Comment by Sean Nash on 16 February 2022, 10:02 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11938

  Remove blank line.

  Reply by Michael Garthwaite on 16 February 2022, 12:34
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:33
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 10:02 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11939

  Remove blank line.

  Reply by Michael Garthwaite on 16 February 2022, 12:34
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:33
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: Utilities.c
Revision Comment by Sean Nash on 16 February 2022, 09:50 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11933

  Remove extra blank line.

  Reply by Michael Garthwaite on 16 February 2022, 10:13
  > Fixed. Thanks!

    Reply by Sean Nash on 17 February 2022, 10:34
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: AlarmDefs.h
Revision Comment by Sean Nash on 16 February 2022, 09:47 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11930

  I see why you did this - make sure alarm list script(s) by Dara
  are updated accordingly.

  Reply by Michael Garthwaite on 16 February 2022, 13:55
  > Please see comment below regarding the changes in common from
  > this code review.

    Reply by Sean Nash on 17 February 2022, 10:37
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 16 February 2022, 09:48 
https://devapps.diality.us/cru/HD-DEN-11980-1#c11931

  Why does it look like you added this table when I know it was
  there before?

  Reply by Michael Garthwaite on 16 February 2022, 09:59
  > I believe this was caused from us branching out of master to
  > have develop & staging branches for common. I will review
  > this as this was not expected.

    Reply by Michael Garthwaite on 16 February 2022, 13:52
    > Dara and I have found out that this code review for common
    > is misconfigured. The source and destination branches are
    > misconfigured. The branch to review should have been from
    > DEN-11980_sw_dev_sprint_64 and the "branch from" should
    > have been from staging. This code review has staging as the
    > branch to review with master as the "branch review."

      Reply by Sean Nash on 17 February 2022, 10:34
      > RESOLVED in CODE WALKTHROUGH.


---

ID: HD-DEN-11980-1 https://devapps.diality.us/cru/HD-DEN-11980-1

Title: HD-DEN-11980_SW Dev Sprint 64
Statement of Objectives:


State: Closed

Summary: 

Author: Michael Garthwaite
Moderator: Michael Garthwaite
Reviewers: (0 active, 3 completed*)
     hnguyen   (*)
     Sean Nash   (*)
     Dara Navaei   (*)