This is a list of all comments for HD-DEN-7860-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/Bubble.c Revision Comment by qnguyen on 11 May 2021, 10:37 https://devapps.diality.us/cru/HD-DEN-7860-1#c9803 These declarations seem odd. Please re-check. Reply by pmontazemi on 17 May 2021, 10:53 > Addressed. Reply by qnguyen on 17 May 2021, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:24 https://devapps.diality.us/cru/HD-DEN-7860-1#c9843 Initialize the other 3 fields of the override structure. Reply by pmontazemi on 17 May 2021, 10:08 > We have not done this for any of the following features. > After talking to [~snash], below is the TODO list. > > - HD Air Trap (TODO [~snash]) > - DG/HD Fluid Leak (DG TODO [~snash], HD TODO [~pmontazemi]) > - HD Blood Leak (TODO [~pmontazemi]) > - HD ADA/ADV Air Bubbles (TODO [~pmontazemi]) Reply by Sean Nash on 18 May 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:41 https://devapps.diality.us/cru/HD-DEN-7860-1#c9804 Recommend converting to a local variable rather than an array. Reply by pmontazemi on 17 May 2021, 10:52 > Addressed. Reply by qnguyen on 17 May 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:31 https://devapps.diality.us/cru/HD-DEN-7860-1#c9846 For blood leak sensor, we didn't want to go to normal state until it was zeroed and self-tested. For bubble detector, do we want to be stuck in init state until self-test is requested? I think if we keep this state machine as is, we will want to request self-test in POST so that we can move on to normal state quickly. Then still request another self-test in each pre-treatment as we are now doing. Reply by pmontazemi on 12 May 2021, 14:55 > We plan on doing self-test as part of priming, so we confirm > the bubble detector reads air on a new cartridge, then we > check that we read fluid when we have completed priming. I > need to add these checks. The air bubble detector alarm, > however, should only be active during treatment (includes > blood prime and rinseback), and outside of that we just > monitor the detectors but never alarm. Therefore, we do not > need to do the self-test in POST and it is fine to keep > handleInitBubbleState(...) as is. > > Note: No code change to handleInitBubbleState(...). Reply by Sean Nash on 17 May 2021, 10:39 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:43 https://devapps.diality.us/cru/HD-DEN-7860-1#c9805 Recommend converting to a local variable rather than an array. Reply by pmontazemi on 17 May 2021, 10:53 > Addressed. Reply by qnguyen on 17 May 2021, 10:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:28 https://devapps.diality.us/cru/HD-DEN-7860-1#c9844 Should validate bubble param up top before you use it. S/W fault if invalid. Then don't need the else below. Reply by pmontazemi on 17 May 2021, 10:06 > Addressed. Reply by Sean Nash on 17 May 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:43 https://devapps.diality.us/cru/HD-DEN-7860-1#c9806 Recommend converting to a local variable rather than an array. Reply by pmontazemi on 17 May 2021, 10:55 > Addressed. Reply by qnguyen on 17 May 2021, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:30 https://devapps.diality.us/cru/HD-DEN-7860-1#c9845 Same. Validate bubble param at top of function before using it. Then no need for else below. Reply by pmontazemi on 17 May 2021, 10:06 > Addressed. Reply by Sean Nash on 17 May 2021, 10:41 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:45 https://devapps.diality.us/cru/HD-DEN-7860-1#c9807 Re-word comment. Reply by pmontazemi on 17 May 2021, 13:31 > Addressed. Reply by qnguyen on 18 May 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:46 https://devapps.diality.us/cru/HD-DEN-7860-1#c9808 Incorrect casting type. Reply by pmontazemi on 17 May 2021, 13:32 > Addressed. Reply by qnguyen on 18 May 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:36 https://devapps.diality.us/cru/HD-DEN-7860-1#c9847 Maybe cast the status params as U32 as well so there won't be an issue within the broadcast function (see my comment there for context). Reply by pmontazemi on 12 May 2021, 14:22 > Compiler gives warnings with U32 casting, left code as is. Reply by Sean Nash on 18 May 2021, 10:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:48 https://devapps.diality.us/cru/HD-DEN-7860-1#c9809 Should we broadcast both bubble detectors' data in one message? Reply by Sean Nash on 11 May 2021, 15:39 > I think this makes sense. We've done the same in other > monitors/controllers like valves where there are several > similar components handled in same driver. Reply by pmontazemi on 17 May 2021, 10:49 > Addressed. Reply by qnguyen on 17 May 2021, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:48 https://devapps.diality.us/cru/HD-DEN-7860-1#c9810 Incorrect enum. Reply by pmontazemi on 17 May 2021, 13:32 > Addressed. Reply by qnguyen on 18 May 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by qnguyen on 14 May 2021, 10:39 https://devapps.diality.us/cru/HD-DEN-7860-1#c9860 This seems to be conflict with FPGA_BLOOD_LEAK_ZERO_STATE_MASK if we are reading from the same FPGA gpio. Reply by pmontazemi on 17 May 2021, 09:40 > Removed ADA/ADV bubble state test masks. Reply by qnguyen on 17 May 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:49 https://devapps.diality.us/cru/HD-DEN-7860-1#c9811 Why should we need a separate mask for bubble test state? I thought the normal input status mask should be sufficient to see the faked air bubble. Reply by Sean Nash on 11 May 2021, 15:05 > I think it's worth checking that the self-test mode was > entered as expected. Reply by pmontazemi on 17 May 2021, 09:40 > Removed ADA/ADV bubble state test masks as there are no > separate FPGA registers that output these variables. Reply by qnguyen on 17 May 2021, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 11 May 2021, 10:30 https://devapps.diality.us/cru/HD-DEN-7860-1#c9802 These should be bit 2 (0x04) and bit 3 (0x08). Reply by pmontazemi on 17 May 2021, 10:53 > Addressed. Reply by qnguyen on 17 May 2021, 10:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 13 May 2021, 13:21 https://devapps.diality.us/cru/HD-DEN-7860-1#c9859 Are we adding an alarm here later? Reply by pmontazemi on 17 May 2021, 09:47 > Addressed. Reply by Dara Navaei on 17 May 2021, 10:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:03 https://devapps.diality.us/cru/HD-DEN-7860-1#c9837 Looks like fpgaActuatorSetPoints is an output and no inputs. Reply by pmontazemi on 17 May 2021, 13:36 > Corrected comments for HD Fluid Leak, HD Blood Leak, and HD > Air Bubbles > > [~snash] TODO Please correct header comment for DG Fluid > Leak. Reply by Sean Nash on 18 May 2021, 08:49 > I don't see changes to HD fluid leak. DG fluid leak > function header looks ok as is. RESOLVED in CODE > WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:03 https://devapps.diality.us/cru/HD-DEN-7860-1#c9838 No return value for this function. Reply by pmontazemi on 17 May 2021, 13:39 > Corrected for both HD Blood Leak and HD Air Bubbles. Reply by Sean Nash on 18 May 2021, 10:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:03 https://devapps.diality.us/cru/HD-DEN-7860-1#c9839 Same comment as above. Reply by pmontazemi on 17 May 2021, 13:39 > Addressed. Reply by Sean Nash on 18 May 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:02 https://devapps.diality.us/cru/HD-DEN-7860-1#c9836 This should be a s/w fault. Reply by pmontazemi on 17 May 2021, 11:17 > Addressed. Reply by Sean Nash on 18 May 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 11 May 2021, 15:15 https://devapps.diality.us/cru/HD-DEN-7860-1#c9842 I agree we want to send the two status enums as a U32 over the CAN. I think it's better to assign w/ cast the two status to local U32s before memcpy into payload. That way, memcpy is given 4 bytes to copy. As is, I worry compiler will only have 1 or 2 bytes for the enum passed into the function and we're forcing memcpy to copy 4 bytes - could corrupt the upper bytes of the two status fields in the message payload. Reply by pmontazemi on 17 May 2021, 14:22 > Addressed. Reply by Sean Nash on 18 May 2021, 10:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 11 May 2021, 15:10 https://devapps.diality.us/cru/HD-DEN-7860-1#c9841 Code below looks like this message is expected to have a payload of type OVERRIDE_PUMP_SET_PT_PAYLOAD_T. Not sure why a bubble test request would have a pump type payload, maybe it had what you needed (an index). But this structure will have a length larger than 0. Should be "if ( sizeof( OVERRIDE_PUMP_SET_PT_PAYLOAD_T ) == message->hdr.payloadLen ). Reply by pmontazemi on 17 May 2021, 14:22 > Addressed. Reply by Sean Nash on 18 May 2021, 08:45 > payload is of type TEST_OVERRIDE_ARRAY_PAYLOAD_T (12 > bytes). TEST_OVERRIDE_PAYLOAD_T (used below) is only 8 > bytes. Change below to array version of test payload type. Reply by pmontazemi on 18 May 2021, 09:00 > Addressed. Reply by Sean Nash on 18 May 2021, 10:35 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.h Revision Comment by qnguyen on 19 May 2021, 10:41 https://devapps.diality.us/cru/HD-DEN-7860-1#c9949 Recommend remove 'S' from "BUBBLES" to be consistent with other messages. Reply by pmontazemi on 19 May 2021, 10:59 > Same response here. Reply by qnguyen on 20 May 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by qnguyen on 19 May 2021, 10:41 https://devapps.diality.us/cru/HD-DEN-7860-1#c9948 Recommend remove 'S' from "BUBBLES" to be consistent with other messages. Reply by pmontazemi on 19 May 2021, 10:59 > Data is packaging several pieces of information for both ADA > and ADV air bubble detectors, hence, the grammar is plural. > The other override functions are commanding on only one of > the detectors depending on the index, hence, the grammar is > singular. Reply by qnguyen on 20 May 2021, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 13 May 2021, 13:02 https://devapps.diality.us/cru/HD-DEN-7860-1#c9857 It looks like you have not added these functions to SystemComm.c. They need to be added there otherwise the commands will not be handled. Reply by pmontazemi on 17 May 2021, 09:59 > Addressed. Reply by Dara Navaei on 17 May 2021, 10:57 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/SelfTests.c Revision Comment by qnguyen on 18 May 2021, 09:36 https://devapps.diality.us/cru/HD-DEN-7860-1#c9910 Remove extra line. Reply by pmontazemi on 18 May 2021, 10:33 > Addressed. Reply by qnguyen on 18 May 2021, 10:38 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-7860-1 https://devapps.diality.us/cru/HD-DEN-7860-1 Title: HD-DEN-7860_HD Dev Bubble Detector Driver Statement of Objectives: State: Closed Summary: Author: pmontazemi Moderator: pmontazemi Reviewers: (0 active, 3 completed*) qnguyen (*) Sean Nash (*) Dara Navaei (*)