This is a list of all comments for HD-DEN-8679-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Dara Navaei on 24 July 2021, 16:04 https://devapps.diality.us/cru/HD-DEN-8679-1#c10428 Does the payload length does not match with the payload? In the payload, you have 1 U32 and 2 F32s so should not the length be U32 + 2 * F32? Reply by qnguyen on 27 July 2021, 10:25 > Fixed. Reply by Sean Nash on 27 July 2021, 15:52 > The first sizeof() has ALARM_ID_T. You converted that > param to a U32 (which is appropriate) and so you should > have U32 in that sizeof(). Reply by qnguyen on 29 July 2021, 12:41 > Done. Reply by Sean Nash on 29 July 2021, 13:03 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 27 July 2021, 15:53 https://devapps.diality.us/cru/HD-DEN-8679-1#c10468 First sizeof() should have U32 (same as above). Reply by qnguyen on 29 July 2021, 12:41 > Done. Reply by Sean Nash on 29 July 2021, 12:51 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/DGInterface.c Revision Comment by Dara Navaei on 05 August 2021, 20:42 https://devapps.diality.us/cru/HD-DEN-8679-1#c10601 dgSubMode is missing. Reply by qnguyen on 06 August 2021, 09:57 > Done. Reply by Dara Navaei on 09 August 2021, 12:07 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModePostTreat.c Revision Comment by Sean Nash on 26 July 2021, 09:30 https://devapps.diality.us/cru/HD-DEN-8679-1#c10433 Why not zero record here? I see it is zeroed just prior to creation at end of treatment, but that means this record may have prior treatment's data until current treatment is over. Reply by qnguyen on 27 July 2021, 10:37 > If fault mode calls the treatment log data collect function, > the record can be zeroed also. Reply by qnguyen on 02 August 2021, 10:43 > Done. Reply by Sean Nash on 02 August 2021, 12:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 July 2021, 09:34 https://devapps.diality.us/cru/HD-DEN-8679-1#c10434 I think we set UF volume to zero in TreatmentParams mode, so this condition should not be necessary. Reply by qnguyen on 27 July 2021, 10:48 > Removed. Reply by Sean Nash on 29 July 2021, 12:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 July 2021, 09:39 https://devapps.diality.us/cru/HD-DEN-8679-1#c10435 Are there ever any reject reasons? Is this message requested by UI or sent unsolicited when ready? Reply by qnguyen on 27 July 2021, 10:48 > This message is requested by UI. UI wants to keep it > consistent with other request message, in case there is > reject reason in the future. Reply by Sean Nash on 29 July 2021, 12:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeTreatment.c Revision Comment by Sean Nash on 26 July 2021, 09:41 https://devapps.diality.us/cru/HD-DEN-8679-1#c10436 I think this will be the time we initiate a treatment. Is that what we want? Or should we move this to transition function to capture time of treatment start? Reply by qnguyen on 27 July 2021, 10:33 > The transition function will call this init function. Reply by Sean Nash on 29 July 2021, 12:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 July 2021, 09:49 https://devapps.diality.us/cru/HD-DEN-8679-1#c10437 User may not have changed all 4 settings. Should we only send events for those that actually changed? Reply by qnguyen on 27 July 2021, 11:11 > Sent events only for changed settings. Reply by Sean Nash on 29 July 2021, 12:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Drivers/Battery.c Revision Comment by Dara Navaei on 24 July 2021, 15:28 https://devapps.diality.us/cru/HD-DEN-8679-1#c10423 I recommend to explicitly name the input and output variables. Reply by qnguyen on 27 July 2021, 10:52 > Done. I prefer generally output description, so refactor is > easier. Normally, a refactor will often make these variables > in the doxygen comment obsolete (variable name change). Reply by Dara Navaei on 29 July 2021, 13:06 > RESOLVED in CODE WALKTRHOUGH. Revision Comment by Dara Navaei on 24 July 2021, 15:26 https://devapps.diality.us/cru/HD-DEN-8679-1#c10422 Reverse the order of the condition. Reply by qnguyen on 27 July 2021, 10:19 > Done. Reply by Dara Navaei on 29 July 2021, 13:06 > RESOLVED in CODE WALKTRHOUGH. Revision Comment by Dara Navaei on 24 July 2021, 15:25 https://devapps.diality.us/cru/HD-DEN-8679-1#c10421 Why do you need an else here to zero the counter? Do you not need to 0 it anyways? Reply by Sean Nash on 26 July 2021, 09:12 > Why would we zero it when no AC detected? Reply by qnguyen on 27 July 2021, 10:15 > The else is when AC power detected. Added clear alarm > condition. Reply by Dara Navaei on 29 July 2021, 13:08 > RESOLVED in CODE WALKTRHOUGH. Revision Comment by Dara Navaei on 24 July 2021, 15:34 https://devapps.diality.us/cru/HD-DEN-8679-1#c10424 How do you get the status of the battery charge in the self test before you check the whether it is in range? Reply by qnguyen on 27 July 2021, 10:21 > It is part of background task in the battery monitor exec > function. The initial value is 0, we will wait here till we > get status. Reply by Dara Navaei on 29 July 2021, 13:05 > RESOLVED in CODE WALKTRHOUGH. Revision Comment by Dara Navaei on 24 July 2021, 15:49 https://devapps.diality.us/cru/HD-DEN-8679-1#c10425 Is it safe to stay in while loop until the hardware responds? I understand that you have a timeout but I thought we still should not do that. Reply by Sean Nash on 26 July 2021, 09:14 > If typical h/w response is very quick (e.g. < 1ms) and worst > case is still pretty quick (e.g. < 5ms), then I think this is > ok with the t/o there to prevent getting stuck in loop > forever. > Also, battery monitor is called from background task so we > can be a little more relaxed about this. Reply by Dara Navaei on 29 July 2021, 13:05 > RESOLVED in CODE WALKTRHOUGH. Revision Comment by Dara Navaei on 24 July 2021, 15:51 https://devapps.diality.us/cru/HD-DEN-8679-1#c10426 The name of the function does not match with the actual name. Reply by qnguyen on 27 July 2021, 10:24 > Fixed. Reply by Dara Navaei on 29 July 2021, 13:04 > RESOLVED in CODE WALKTRHOUGH. ---------------------------------------- File: firmware/App/Drivers/Battery.h Revision Comment by Dara Navaei on 24 July 2021, 15:54 https://devapps.diality.us/cru/HD-DEN-8679-1#c10427 Do we not need a battery remaining percent override function for testing? Reply by qnguyen on 29 July 2021, 13:03 > Added additional functions. Reply by Dara Navaei on 29 July 2021, 13:04 > RESOLVED in CODE WALKTRHOUGH. ---------------------------------------- File: firmware/App/Modes/ModeInitPOST.c Revision Comment by Dara Navaei on 05 August 2021, 20:41 https://devapps.diality.us/cru/HD-DEN-8679-1#c10600 This header is no longer needed. Reply by qnguyen on 06 August 2021, 09:57 > Removed. Reply by Dara Navaei on 09 August 2021, 12:08 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/Rinseback.c Revision Comment by Sean Nash on 26 July 2021, 09:52 https://devapps.diality.us/cru/HD-DEN-8679-1#c10439 Seems like the two states given as data are not very meaningful. Consider just setting before/after data to zero for this event. Reply by qnguyen on 27 July 2021, 10:27 > Done. Reply by Sean Nash on 27 July 2021, 15:48 > I think the data are floats, so 0 should be 0.0. Reply by qnguyen on 02 August 2021, 10:43 > Fixed. Reply by Sean Nash on 02 August 2021, 12:55 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeTreatmentParams.c Revision Comment by Sean Nash on 26 July 2021, 09:50 https://devapps.diality.us/cru/HD-DEN-8679-1#c10438 Not sure this variable (and related get function) are needed. Reply by qnguyen on 27 July 2021, 11:04 > Set UF volume initially to 0 and removed this variable and > related function. Reply by Sean Nash on 29 July 2021, 12:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: HDDefs.h Revision Comment by Sean Nash on 26 July 2021, 09:08 https://devapps.diality.us/cru/HD-DEN-8679-1#c10429 Why are DG definitions in HDDefs.h? If needed by both stacks, should we create a FWDefs.h common to both f/w stacks? Reply by qnguyen on 27 July 2021, 10:49 > [~dnavaei] Please take a look. Reply by Dara Navaei on 30 July 2021, 13:57 > I moved this enum to DGDefs.h in HD-DEN-9054. Reply by Sean Nash on 02 August 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-8679-1 https://devapps.diality.us/cru/HD-DEN-8679-1 Title: HD-DEN-8679_HD Dev Treatment Log Statement of Objectives: State: Closed Summary: Author: qnguyen Moderator: qnguyen Reviewers: (1 active, 2 completed*) Sean Nash (*) Dara Navaei (*) pmontazemi