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

Review Summary: No summary


----------------------------------------
File: firmware/App/Services/SystemCommMessages.c
Revision Comment by wbracken on 21 February 2023, 14:23 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16504

  Extra blank line.

  Reply by Sean Nash on 27 February 2023, 10:13
  > 2 lines appropriate around banner separators like this.

    Reply by wbracken on 27 February 2023, 10:51
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by wbracken on 21 February 2023, 14:23 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16505

  Extra blank line.

  Reply by wbracken on 27 February 2023, 10:51
  > RESOLVED IN CODE WALKTHROUGH

Revision Comment by wbracken on 09 March 2023, 11:35 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16671

  Change cmd to prompt

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by wbracken on 17 March 2023, 15:25
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by Sean Nash on 08 March 2023, 19:03 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16652

  Add space after "(".

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by Sean Nash on 20 March 2023, 10:17
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 27 February 2023, 10:18 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16518

  I think FALSE was more appropriate.  Last param indicates
  whether this ACK msg is an ACK (T) or NAK (F).

  Reply by Michael Garthwaite on 08 March 2023, 09:42
  > FALSE will be determined by our automagic script to be
  > ACK_REQUIRED in the excel sheet. This will cause
  > discrepancies between the readers of that sheet and what the
  > FW does.

    Reply by Sean Nash on 08 March 2023, 19:05
    > I think that's true in calls to serialize function, but I
    > don't think this function wants this kind of flag (i.e.
    > it's not looking for whether to require ACK or not - it
    > wants to know if we are ACKing (TRUE) or NAKing (FALSE)
    > this command).  Here, we are NAKing the command, so FALSE
    > was appropriate.

      Reply by Michael Garthwaite on 09 March 2023, 10:53
      > How do we wish to handle the output of the automated
      > script? It currently does not handle conditional
      > statements to determine ack requiredness. 
      > 
      > If set to false, the output is:
      > 
      > 0x4A	 MSG_ID_UI_PATIENT_DISCONNECTION_CONFIRM	 hdfirmware
      > response	 COMM_BUFFER_OUT_CAN_HD_2_UI	ACK_REQUIRED
      > PAYLOAD
      > 
      > which is incorrect to what the code is doing. We have 2
      > different implementations of calling sendAckResponseMsg()
      > 
      > 
      > 1. sendAckResponseMsg( (MSG_ID_T)message->hdr.msgID,
      > COMM_BUFFER_OUT_CAN_HD_2_UI, FALSE ); 
      > # if we dont meet an expected condition. Later in the
      > handler we will call a response message with the response
      > message calling:
      > serializeMessage( msg, COMM_BUFFER_OUT_CAN_DG_2_HD,
      > ACK_REQUIRED ); 
      > 
      > 2. sendAckResponseMsg( (MSG_ID_T)message->hdr.msgID,
      > COMM_BUFFER_OUT_CAN_HD_2_UI, result ); 
      > # a conditional based on?
      > 
      > Please review the FW_Message_list.csv within latest
      > develop & staging builds to understand how the automated
      > script is determining "ack requireness"

        Reply by Michael Garthwaite on 17 March 2023, 14:33
        > Reverted change. We will revisit this in an upcoming
        > branch.

          Reply by Sean Nash on 20 March 2023, 10:19
          > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 08 March 2023, 19:08 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16654

  FALSE was right.

  Reply by Michael Garthwaite on 17 March 2023, 14:33
  > Fixed. Thanks!

    Reply by Sean Nash on 20 March 2023, 10:17
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by wbracken on 10 March 2023, 13:19 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16680

  Isn't deleted statement correct?

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by wbracken on 17 March 2023, 15:23
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by wbracken on 10 March 2023, 13:17 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16678

  Isn't delete statement correct?

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by wbracken on 17 March 2023, 15:23
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by wbracken on 10 March 2023, 13:18 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16679

  Isn't deleted statement correct?

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by wbracken on 17 March 2023, 15:22
    > RESOLVED IN CODE WALKTHROUGH

Revision Comment by wbracken on 10 March 2023, 13:20 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16681

  Isn't deleted statement correct?

  Reply by Michael Garthwaite on 17 March 2023, 14:32
  > Fixed. Thanks!

    Reply by wbracken on 17 March 2023, 15:21
    > RESOLVED IN CODE WALKTHROUGH

----------------------------------------
File: firmware/App/Services/SystemCommMessages.h
Revision Comment by Sean Nash on 27 February 2023, 10:21 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16519

  Remove 2 blank lines.

  Reply by Michael Garthwaite on 08 March 2023, 16:52
  > Fixed. Thanks!

    Reply by Sean Nash on 08 March 2023, 19:13
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 27 February 2023, 10:22 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16520

  Should pack structure to 1 byte so sizeof() gives you 1 byte
  size.

  Reply by Michael Garthwaite on 27 February 2023, 10:30
  > Can you clarify? Is U08 no longer the size of 1 byte?

    Reply by Sean Nash on 27 February 2023, 10:33
    > Compiler will align structures to 32-bits by default.  Use
    > "#pragma pack(push,1)" above structure declaration and
    > "#pragma pack(pop)" below declaration to tell the compiler
    > to pack/compress structures to 1 byte (8-bit) alignment.

      Reply by Michael Garthwaite on 08 March 2023, 16:52
      > Fixed. Thanks!

        Reply by Sean Nash on 08 March 2023, 19:13
        > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 27 February 2023, 10:23 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16521

  Should pack structure to 1 byte so sizeof() gives you 1 byte
  size.

  Reply by Michael Garthwaite on 08 March 2023, 16:52
  > Fixed. Thanks!

    Reply by Sean Nash on 08 March 2023, 19:14
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/SystemComm.c
Revision Comment by Sean Nash on 10 March 2023, 09:48 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16672

  Why removed?

  Reply by Michael Garthwaite on 10 March 2023, 10:14
  > DG no longer sends this message. We determine DG check-in by
  > receiving the DG op mode command.

    Reply by Sean Nash on 10 March 2023, 10:16
    > Has DG check-in msg ID been made available for re-purpose?
    > RESOLVED in CODE WALKTHROUGH.

----------------------------------------
File: firmware/App/Services/AlarmMgmt.h
Revision Comment by Sean Nash on 08 March 2023, 19:00 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16649

  Add blank line before comment.

  Reply by Michael Garthwaite on 17 March 2023, 14:33
  > Fixed. Thanks!

    Reply by Sean Nash on 20 March 2023, 09:26
    > RESOLVED in CODE WALKTHROUGH.

Revision Comment by Sean Nash on 08 March 2023, 19:01 
https://devapps.diality.us/cru/HD-DEN-14689-1#c16651

  Remove extra blank line.

  Reply by Michael Garthwaite on 17 March 2023, 14:33
  > Fixed. Thanks!

    Reply by Sean Nash on 20 March 2023, 09:27
    > RESOLVED in CODE WALKTHROUGH.


---

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

Title: HD-DEN-14689_Messaging Bugs_HDFW
Statement of Objectives:


State: Closed

Summary: 

Author: Michael Garthwaite
Moderator: Michael Garthwaite
Reviewers: (5 active, 1 completed*)
     Sean Nash   (*)
     wbracken
     Dara Navaei
     Darren Cox
     jtaylor
     Steve Jarpe