This is a list of all comments for DG-DEN-11928-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/ConcentratePumps.h Revision Comment by Sean Nash on 28 February 2022, 14:19 https://devapps.diality.us/cru/DG-DEN-11928-1#c12114 Why is this changed? Will we never want to go faster in any mode? Reply by hnguyen on 01 March 2022, 13:58 > Changed CONCENTRATE_PUMP_MAX_SPEED from 40.0 to 48.0. Reply by Sean Nash on 04 March 2022, 13:44 > So do we need a different #define for the 40? Reply by Sean Nash on 04 March 2022, 14:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.h Revision Comment by Sean Nash on 28 February 2022, 14:22 https://devapps.diality.us/cru/DG-DEN-11928-1#c12115 These should probably go in calibration record - talk to Dara to get that started. Leave these here until it is ready. Also, add a blank line between public definitions comment and any #defines. Reply by hnguyen on 01 March 2022, 13:54 > Added a blank line. > Asked Dara to add acid and bicard values to the calibration > record on 2/8/2022. Reply by Dara Navaei on 22 March 2022, 13:45 > Please do not remove the #defines until the calibration > records are updated. Reply by hnguyen on 22 March 2022, 14:12 > Ok Reply by Sean Nash on 22 March 2022, 17:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by hnguyen on 22 March 2022, 14:10 https://devapps.diality.us/cru/DG-DEN-11928-1#c12604 Those #define below will not be removed until they are added into the calibration records. #define ACID_NORMAL_CONDUCTIVITY 11645.05 #define BICARB_NORMAL_CONDUCTIVITY 13734.88 Reply by Dara Navaei on 18 October 2023, 21:28 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 28 February 2022, 14:55 https://devapps.diality.us/cru/DG-DEN-11928-1#c12124 These 2 rates look strange because values are clearly in L/min units. Reply by hnguyen on 03 March 2022, 10:55 > Because the RO pump target flow rate is in liter per minute > I updated RO_PUMP_400_ML_PER_MIN from 0.4 to 400.0 > RO_PUMP_800_ML_PER_MIN from 0.8 to > 800.0 > Added #define MILLILITERS_PER_LITER > 1000.0 and use this value in > setROPumpTargetFlowRateLPM( RO_PUMP_800_ML_PER_MIN / > MILLILITERS_PER_LITER, TARGET_RO_PRESSURE_PSI ); Reply by Sean Nash on 04 March 2022, 14:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:58 https://devapps.diality.us/cru/DG-DEN-11928-1#c12125 These conductivity thresholds should probably come from DG calibration record. Talk to Dara. Reply by hnguyen on 01 March 2022, 17:28 > Sent Dara email asking him to put those numbers in the DG > calibration record. Reply by Sean Nash on 22 March 2022, 17:36 > Maybe remove thresholds for now as we are not using them > yet. Reply by hnguyen on 22 March 2022, 17:47 > Removed 2 unused thresholds: > > #define CONDUCTIVITY_WHEN_ACID_JUG_EMPTY > 10000.0 > #define CONDUCTIVITY_WHEN_BICARB_JUG_EMPTY > 12000.0 Reply by Sean Nash on 23 March 2022, 14:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:00 https://devapps.diality.us/cru/DG-DEN-11928-1#c12126 Can we prime lines in < 2 seconds? Reply by hnguyen on 01 March 2022, 17:30 > I forgot the pumps run at 48 mL/min not 48 mL/sec. > > total line volume + 25% = 60mL + 25% of 60mL = 75 mL. > > Concentrate pumps run at 48 mL/ min. > > ( 75 mL / 48 mL ) * 60 seconds = 93.75 seconds (used 95 > seconds for PRIME_CONCENTRATE_LINES_TIME_OUT_MS) Reply by Sean Nash on 04 March 2022, 14:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:17 https://devapps.diality.us/cru/DG-DEN-11928-1#c12532 Do we need this bottles need prime flag? Isn't this redundant with existing isThisFirstFill flag? Reply by hnguyen on 21 March 2022, 17:58 > Removed fillBottlesNeedPrimeFlag and use the existing > isThisFirstFill Reply by Sean Nash on 22 March 2022, 17:07 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:09 https://devapps.diality.us/cru/DG-DEN-11928-1#c12712 Indent comment properly. Reply by hnguyen on 30 March 2022, 13:44 > Aligned this comment with other comments > ///< Used to schedule dialysate fill data publication to CAN > bus. Reply by Sean Nash on 31 March 2022, 13:23 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:03 https://devapps.diality.us/cru/DG-DEN-11928-1#c12127 Array name and comment should indicate this is for flushing bubbles after priming concentrate lines. Reply by hnguyen on 01 March 2022, 15:52 > Renamed roPumpSpeed to roPumpFlushBubblesSpeed and updated > comments Reply by Sean Nash on 04 March 2022, 14:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:10 https://devapps.diality.us/cru/DG-DEN-11928-1#c12713 delete one of these blank lines. Reply by hnguyen on 30 March 2022, 13:46 > deleted blank line Reply by Sean Nash on 31 March 2022, 13:23 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:10 https://devapps.diality.us/cru/DG-DEN-11928-1#c12714 Stagger? Reply by Dara Navaei on 30 March 2022, 21:38 > Done. Reply by Sean Nash on 31 March 2022, 13:22 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:25 https://devapps.diality.us/cru/DG-DEN-11928-1#c12131 If alarm is recoverable, we should go to paused state. If not, we should not delete this line of code so we can exit fill mode. Otherwise, we get stuck here forever. Reply by hnguyen on 01 March 2022, 16:14 > Added requestNewOperationMode( DG_MODE_GENE ); Reply by Sean Nash on 04 March 2022, 14:30 > Alarm is recoverable. Should go to fill paused state. > Paused state should go back to this test inlet water > quality state when alarm acknowledged. Reply by hnguyen on 23 March 2022, 14:16 > Sean is OK with current implementation. Reply by Sean Nash on 23 March 2022, 14:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:26 https://devapps.diality.us/cru/DG-DEN-11928-1#c12132 Does this alarm need persistence? Looks like it used to have a timeout but now it's immediate. Reply by hnguyen on 01 March 2022, 16:26 > Reverted the code with timeout. Need to add timeout > requirement in the Inlet Water Check in the DG SRS. Reply by Sean Nash on 04 March 2022, 14:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 16:30 https://devapps.diality.us/cru/DG-DEN-11928-1#c12144 Put "none" in function headers where there is nothing in that section (throughout this module). Reply by hnguyen on 01 March 2022, 17:47 > Done. Reply by Sean Nash on 04 March 2022, 14:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:32 https://devapps.diality.us/cru/DG-DEN-11928-1#c12133 Do both lines prime in same amount of time? Are volumes same? Is 2 seconds the right amount of time? Reply by hnguyen on 03 March 2022, 11:06 > Changed the PRIME_CONCENTRATE_LINES_TIME_OUT_MS to 95 > seconds. Reply by Sean Nash on 04 March 2022, 14:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:15 https://devapps.diality.us/cru/DG-DEN-11928-1#c12128 You must set isThisFirstFill to FALSE somewhere. This may not be the right place anymore - but it looks ok to me - why deleted? Reply by hnguyen on 01 March 2022, 17:02 > Set isThisFirstFill = FALSE per comment. Reply by Sean Nash on 04 March 2022, 14:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:15 https://devapps.diality.us/cru/DG-DEN-11928-1#c12531 Does this function need to return anything? Does caller even look for a return value? Reply by hnguyen on 18 March 2022, 11:23 > Removed return. Reply by hnguyen on 21 March 2022, 18:01 > Removed setBottlesNeedPrimeFlag and use function > setThisFirstFillFlag(). Reply by Sean Nash on 22 March 2022, 17:08 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:36 https://devapps.diality.us/cru/DG-DEN-11928-1#c12134 Is 400 mL/min rate correct? I thought it was 800. Reply by hnguyen on 01 March 2022, 16:44 > According Blain's PowerPoint, for bicard, the RO Pump speed > is 400mL/min. 800mL/min is for acid. Reply by Sean Nash on 04 March 2022, 14:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:37 https://devapps.diality.us/cru/DG-DEN-11928-1#c12135 Put space between if and (. Reply by hnguyen on 01 March 2022, 16:47 > Done. Reply by Sean Nash on 04 March 2022, 14:21 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:44 https://devapps.diality.us/cru/DG-DEN-11928-1#c12137 Is this equation correct? Doesn't look right to me. Reply by hnguyen on 01 March 2022, 16:47 > According to Blain, the original equation is: (CD1 - CD2) / > ( (CD1 + CD2) / 2 ) ) > +/- 5% which is the same as 2 * > (CD1 - CD2) / (CD1 + CD2) > +/- 5% Reply by Sean Nash on 04 March 2022, 13:59 > Ok, I see that. I don't think averageConductivity is the > right name for this. Looks more like a > pctDifferenceInConductivity or something like that. Reply by hnguyen on 07 March 2022, 11:16 > Updated code to check for the difference in percent of > conductivity between CD1 and CD2 and make sure it is < 5% > otherwise alarm. Reply by Sean Nash on 22 March 2022, 17:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:57 https://devapps.diality.us/cru/DG-DEN-11928-1#c12139 Do we need to set RO pump rate here before we move on to produce/delivery dialysate? Reply by hnguyen on 01 March 2022, 17:05 > RO pump rate is already set in the previous state. Reply by Sean Nash on 04 March 2022, 14:03 > But is it set to correct rate for product/delivery? Check > with Dara. Produce/delivery rate should be set by HD I > think, so may not be same as set in previous state. Reply by hnguyen on 07 March 2022, 11:22 > set the RO pump rate to > setROPumpTargetFlowRateLPM( > getTargetFillFlowRateLPM(), TARGET_RO_PRESSURE_PSI ); Reply by Sean Nash on 22 March 2022, 17:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 March 2022, 14:00 https://devapps.diality.us/cru/DG-DEN-11928-1#c12378 Is ACID_TEST_CD2_TCD the right reference value for this check? Should we even be using the isValueWithinPercentRange function here? I think averageConductivity is already a percentage so you can just see if it's less than 5%. Reply by hnguyen on 07 March 2022, 11:19 > Updated code to check for the difference in percent of > conductivity between CD1 and CD2 and make sure it is < 5% > otherwise alarm. Reply by Sean Nash on 22 March 2022, 17:23 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:13 https://devapps.diality.us/cru/DG-DEN-11928-1#c12715 Doesn't next state need acid zeroed here? Reply by Dara Navaei on 31 March 2022, 12:46 > It is zeroed in transition function and it is not used until > the acid state. Reply by Sean Nash on 31 March 2022, 13:17 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:51 https://devapps.diality.us/cru/DG-DEN-11928-1#c12138 Do we still want to check temperature here? I thought this was no longer being done. Check with Dara. Reply by hnguyen on 01 March 2022, 17:59 > According to SRSDG 216, While in Fill-Dialysate Production > state, the DG Software shall transition to Fill-Deliver > Dialysate state when the dialysate temperature is in target > set temperature ± 2° C and water quality is good > (conductivity, pressure, temperature) for at least 3 s ± 100 > ms. Reply by Sean Nash on 04 March 2022, 14:07 > Let's review requirement - may be obsolete. Reply by Sean Nash on 23 March 2022, 14:46 > Reviewed requirement - it is obsolete. Reply by hnguyen on 23 March 2022, 15:07 > Removed ( TRUE == checkDialysateTemperature() ) Reply by Sean Nash on 31 March 2022, 13:24 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 15:59 https://devapps.diality.us/cru/DG-DEN-11928-1#c12140 What is the point of turning off the pumps here? Reply by hnguyen on 01 March 2022, 18:04 > Removed the lines > requestConcentratePumpOff( CONCENTRATEPUMPS_CP1_ACID > ); > requestConcentratePumpOff( > CONCENTRATEPUMPS_CP2_BICARB ); > > The concentrate pumps are already OFF prior to transitioning > to produce dialysate state. Reply by Sean Nash on 04 March 2022, 14:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 16:02 https://devapps.diality.us/cru/DG-DEN-11928-1#c12141 Do we still need temperature check here? Reply by hnguyen on 01 March 2022, 17:17 > According to SRSDG 397 we need to check the dialysate > temperature within +/- 2 deg C. > Systems need to review the DGSRS and provide comments. Reply by Sean Nash on 04 March 2022, 14:04 > Let's review that requirement - may be obsolete. Reply by Sean Nash on 23 March 2022, 14:45 > Reviewed requirement - it is obsolete. Reply by hnguyen on 23 March 2022, 15:01 > Removed > 1. static BOOL checkDialysateTemperature( void ) > declaration > 2. ( TRUE == checkDialysateTemperature() ) reference > 3. static BOOL checkDialysateTemperature( void ) > definition Reply by Sean Nash on 31 March 2022, 13:24 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 16:06 https://devapps.diality.us/cru/DG-DEN-11928-1#c12142 Is this average calculation correct? Do we need to weight samples based on CP rate? Reply by hnguyen on 03 March 2022, 11:20 > I am not clear on this question, do you want to use the load > cell to weight samples based on the concentrate pump rate? Reply by Sean Nash on 22 March 2022, 17:25 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:25 https://devapps.diality.us/cru/DG-DEN-11928-1#c12533 Aren't we doing this check at end of drain mode? I don't think we should be checking here. Reply by hnguyen on 18 March 2022, 16:56 > The fillStatus.fillEmptyAcidBottleDetected is set to TRUE to > signal drain mode that empty bottle is detected > This flag is checked at the end of the drain mode. Reply by Sean Nash on 22 March 2022, 17:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 17:04 https://devapps.diality.us/cru/DG-DEN-11928-1#c12642 Remove commented out alarms. Reply by hnguyen on 22 March 2022, 18:18 > Removed line > > //activateAlarmNoData( > ALARM_ID_DG_ACID_BOTTLE_LOW_VOLUME ); Reply by Sean Nash on 23 March 2022, 14:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 17:07 https://devapps.diality.us/cru/DG-DEN-11928-1#c12643 Remove commented out requests to go to idle mode. Reply by hnguyen on 22 March 2022, 18:18 > Removed line > > //requestNewOperationMode( DG_MODE_GENE ); Reply by Sean Nash on 23 March 2022, 14:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 16:28 https://devapps.diality.us/cru/DG-DEN-11928-1#c12143 testValue is a parameter - not an input. There are 2 other parameters. Need to add @param for each. Reply by hnguyen on 01 March 2022, 17:25 > Added param: testValue, baseValue, percentFactor Reply by Sean Nash on 04 March 2022, 14:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:33 https://devapps.diality.us/cru/DG-DEN-11928-1#c12536 I don't see idle mode looking at this flag - comment appears to be wrong. Reply by hnguyen on 18 March 2022, 16:54 > Idle mode calls > BOOL isAvgConductivityOutOfRange( void ) > { > return fillStatus.fillAvgConductivityOutOfRange; > } Reply by Sean Nash on 22 March 2022, 17:00 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:35 https://devapps.diality.us/cru/DG-DEN-11928-1#c12537 2nd alarm probably doesn't need any data. 1st alarm will log the data. Reply by hnguyen on 18 March 2022, 12:59 > Replaced with activateAlarmNoData. Reply by Sean Nash on 22 March 2022, 16:58 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:39 https://devapps.diality.us/cru/DG-DEN-11928-1#c12538 This request for idle mode is already present at bottom of function. Reply by hnguyen on 18 March 2022, 12:49 > Removed requestNewOperationMode( DG_MODE_GENE ); Reply by Sean Nash on 22 March 2022, 16:57 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:39 https://devapps.diality.us/cru/DG-DEN-11928-1#c12539 This request for idle mode is already present at bottom of function. Reply by hnguyen on 18 March 2022, 12:50 > Removed requestNewOperationMode( DG_MODE_GENE ); Reply by Sean Nash on 22 March 2022, 16:56 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ConductivitySensors.c Revision Comment by Sean Nash on 28 February 2022, 14:27 https://devapps.diality.us/cru/DG-DEN-11928-1#c12116 This (2000) is a minimum for a warning. Alarm if > 2200. Sensor should normally be below 2000 (so not a minimum really). I think we still want a minimum. Not sure if 100 was correct before. Check with Systems. Reply by hnguyen on 01 March 2022, 13:47 > Checked with Systems. Conductivity min value is 100. > #define COND_SENSOR_CPI_MAX_VALUE 2200 > > #define COND_SENSOR_CPI_WARNING_HIGH 2200 > #define COND_SENSOR_CPI_WARNING_LOW 2000 > #define COND_SENSOR_CPI_MIN_VALUE 100 Reply by Sean Nash on 04 March 2022, 14:36 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 March 2022, 13:41 https://devapps.diality.us/cru/DG-DEN-11928-1#c12372 Do we need a warning high when we already have a max above with same value? Reply by hnguyen on 04 March 2022, 13:56 > Replaced COND_SENSOR_CPI_MAX_VALUE with > COND_SENSOR_CPI_WARNING_HIGH > and removed #define COND_SENSOR_CPI_MAX_VALUE Reply by Sean Nash on 22 March 2022, 17:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:31 https://devapps.diality.us/cru/DG-DEN-11928-1#c12117 I think we still need this alarm. There should be 2 alarms. One for alarm (>=2200) and one for warning (>=2000 and <2200). Reply by hnguyen on 01 March 2022, 13:03 > Added ALARM_ID_INLET_WATER_HIGH_CONDUCTIVITY = 158, > ///< Inlet water conductivity too high in AlarmDefs.h Reply by Sean Nash on 04 March 2022, 14:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 March 2022, 13:41 https://devapps.diality.us/cru/DG-DEN-11928-1#c12373 Why is alarm commented out? It is still checked below so we should initialize it here. Reply by hnguyen on 04 March 2022, 14:12 > Added ALARM_ID_INLET_WATER_HIGH_CONDUCTIVITY to AlarmDefs.h > Uncommented > initPersistentAlarm(ALARM_ID_INLET_WATER_HIGH_CONDUCTIVITY Reply by Sean Nash on 22 March 2022, 17:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:38 https://devapps.diality.us/cru/DG-DEN-11928-1#c12119 Too low should be < 100 (check w/ Systems). There should be 2 too high alarms: 1) warning (>=2000) and 2) alarm (>=2200). Reply by hnguyen on 01 March 2022, 13:44 > Updated code to include checking for high conductivity Reply by Sean Nash on 04 March 2022, 14:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 22 March 2022, 13:48 https://devapps.diality.us/cru/DG-DEN-11928-1#c12596 Per our coding style, please use ternary for these two booleans: isConductTooLow = ( conductivity <= COND_SENSOR... ? TRUE : FALSE ) Reply by hnguyen on 22 March 2022, 14:08 > Updated to > BOOL const isConductTooLow = ( conductivity <= > COND_SENSOR_CPI_MIN_VALUE ) ? TRUE : FALSE; > > BOOL const isConductTooHigh = ( conductivity >= > COND_SENSOR_CPI_WARNING_HIGH ) ? TRUE : FALSE ; Reply by Dara Navaei on 22 March 2022, 16:26 > RESOLVED in CODE WALTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 08:52 https://devapps.diality.us/cru/DG-DEN-11928-1#c12528 Last parameter should be warning low threshold I think. Reply by hnguyen on 18 March 2022, 11:08 > Changed COND_SENSOR_CPI_WARNING_HIGH to > COND_SENSOR_CPI_WARNING_LOW Reply by Sean Nash on 22 March 2022, 17:19 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by Sean Nash on 28 February 2022, 14:48 https://devapps.diality.us/cru/DG-DEN-11928-1#c12120 These are both maximums. Should be something like MAX_INPUT_WATER_TEMP_ALARM and MAX_INPUT_WATER_TEMP_WARNING. Reply by hnguyen on 01 March 2022, 15:20 > Renamed #define to > > #define MIN_WATER_TEMPERATURE_WARNING_LOW_RANGE > 22U > #define MAX_WATER_TEMPERATURE_WARNING_LOW_RANGE > 24U > #define MIN_WATER_TEMPERATURE_WARNING_HIGH_RANGE > 37U > #define MAX_WATER_TEMPERATURE_WARNING_HIGH_RANGE > 39U > #define MIN_WATER_TEMPERATURE_ALARM > 22U > #define MAX_WATER_TEMPERATURE_ALARM > 39U Reply by Sean Nash on 04 March 2022, 14:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:49 https://devapps.diality.us/cru/DG-DEN-11928-1#c12121 These are both minimums. Should be something like MIN_INPUT_WATER_TEMP_WARNING and MIN_INPUT_WATER_TEMP_ALARM. Reply by hnguyen on 01 March 2022, 15:21 > Renamed #define to > > #define MIN_WATER_TEMPERATURE_WARNING_LOW_RANGE > 22U > #define MAX_WATER_TEMPERATURE_WARNING_LOW_RANGE > 24U > #define MIN_WATER_TEMPERATURE_WARNING_HIGH_RANGE > 37U > #define MAX_WATER_TEMPERATURE_WARNING_HIGH_RANGE > 39U > #define MIN_WATER_TEMPERATURE_ALARM > 22U > #define MAX_WATER_TEMPERATURE_ALARM > 39U Reply by Sean Nash on 04 March 2022, 14:35 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 04 March 2022, 13:54 https://devapps.diality.us/cru/DG-DEN-11928-1#c12375 Why are we not initializing these alarms? Reply by hnguyen on 07 March 2022, 10:29 > Uncommented and recompiled without errors Reply by Sean Nash on 22 March 2022, 17:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:52 https://devapps.diality.us/cru/DG-DEN-11928-1#c12122 I think we still want to distinguish between low and high temp here - so need two different alarms. Reply by Sean Nash on 04 March 2022, 14:34 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 28 February 2022, 14:53 https://devapps.diality.us/cru/DG-DEN-11928-1#c12123 Replace this out of range alarm with 2 separate alarms (as I stated above): 1) inlet water too high (>39) and inlet water too low (<22). Reply by hnguyen on 01 March 2022, 15:23 > Added high and low temperature check > BOOL isWaterTempTooHigh = ( temperature > > MAX_WATER_TEMPERATURE_ALARM ); > > BOOL isWaterTempTooLow = ( temperature < > MIN_WATER_TEMPERATURE_ALARM ); Reply by Sean Nash on 04 March 2022, 14:34 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: AlarmDefs.h Revision Comment by Dara Navaei on 22 March 2022, 13:39 https://devapps.diality.us/cru/DG-DEN-11928-1#c12593 Add doxygen comment. Reply by hnguyen on 22 March 2022, 18:00 > Added doxygen comment > > ALARM_ID_INLET_WATER_HIGH_CONDUCTIVITY = 106, > ///< DG inlet water conductivity is greater than threshold Reply by Dara Navaei on 23 March 2022, 14:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 08:33 https://devapps.diality.us/cru/DG-DEN-11928-1#c12524 Is this the bad fill alarm (first one)? If so, let's change name to FILL_CONDUCTIVITY_OUT_OF_RANGE and I think clear immediate should be TRUE and no clear should be FALSE and clear only (UsrAck here) should be TRUE. Not sure if TxLog should be TRUE - I don't think so. Reply by hnguyen on 18 March 2022, 10:53 > Done. Reply by Sean Nash on 22 March 2022, 17:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:03 https://devapps.diality.us/cru/DG-DEN-11928-1#c12707 Is this alarm really logged to treatment log? Reply by hnguyen on 30 March 2022, 14:05 > Changed TxLog field from TRUE to FALSE Reply by Sean Nash on 31 March 2022, 13:20 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 08:42 https://devapps.diality.us/cru/DG-DEN-11928-1#c12526 This looks like the second bad fill alarm. ClrIm should be FALSE, NoClr should be FALSE, and NoRes/NoRin/NoEnd should be FALSE. Reply by hnguyen on 18 March 2022, 10:58 > Done. Reply by Sean Nash on 22 March 2022, 17:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 08:38 https://devapps.diality.us/cru/DG-DEN-11928-1#c12525 Are these the alarms for when a bottle volume prediction is < 10% left? If so, NoRin and NoEnd should be FALSE and ClrOnly (UsrAck here) should be FALSE. Reply by hnguyen on 18 March 2022, 10:42 > Updated NoRin, NoEnd, ClrOnly to FALSE. Reply by Sean Nash on 22 March 2022, 17:22 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: DGDefs.h Revision Comment by Sean Nash on 15 March 2022, 16:45 https://devapps.diality.us/cru/DG-DEN-11928-1#c12522 Flush lines state is obsolete now. Let's remove it and go straight to flush water state. Reply by hnguyen on 22 March 2022, 11:00 > Removed flush lines Reply by Sean Nash on 22 March 2022, 17:22 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 22 March 2022, 13:41 https://devapps.diality.us/cru/DG-DEN-11928-1#c12594 Add doxygen comment. Reply by hnguyen on 22 March 2022, 18:01 > Added doxygen comment > > DG_HANDLE_BAD_FILL_STATE_START = 0, > ///< Bad fill start state Reply by Dara Navaei on 23 March 2022, 14:18 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.h Revision Comment by hnguyen on 18 March 2022, 16:03 https://devapps.diality.us/cru/DG-DEN-11928-1#c12570 Renamed setBottlesNeedPrimeFlag() to setThisFirstFillFlag() Reply by Dara Navaei on 18 October 2023, 21:30 > RESOLVED in CODE WALKTHROUGH Revision Comment by Sean Nash on 18 March 2022, 09:30 https://devapps.diality.us/cru/DG-DEN-11928-1#c12534 This get function is never called outside of fill mode - should be private (static) inside .c file. Reply by hnguyen on 18 March 2022, 12:55 > Changed this function to private. Reply by hnguyen on 18 March 2022, 16:00 > This function is obsolete since the fillBottlesNeedPrimeFlag > is replaced by isThisFirstFill flag in ModeFill.c Reply by Sean Nash on 22 March 2022, 17:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:32 https://devapps.diality.us/cru/DG-DEN-11928-1#c12535 This function is never called outside of fill mode - should be private (static) inside .c file. Reply by hnguyen on 21 March 2022, 17:56 > isAvgConductivityOutOfRange is now called in ModeGenIdle Reply by Sean Nash on 22 March 2022, 17:01 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by Sean Nash on 18 March 2022, 08:47 https://devapps.diality.us/cru/DG-DEN-11928-1#c12527 I think 0xAD msg ID is taken. Looks like you haven't done a pull for common in a while - please do that. Also, when adding new messages, start in our shared message list spreadsheet - we need to keep that spreadsheet up to date. Reply by hnguyen on 21 March 2022, 17:12 > Assigned MSG_ID_DG_FILL_MODE_DATA = 0xAE, ///< DG > broadcast of fill mode data such as used acid & bicarb volume > Updated Message List.xlsx (Denali Tab, Row = 142) with 0xAE00 > DG Fill Mode Data Reply by Sean Nash on 22 March 2022, 17:20 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeGenIdle.c Revision Comment by Sean Nash on 18 March 2022, 09:40 https://devapps.diality.us/cru/DG-DEN-11928-1#c12540 Don't need this new #define. Already have 0.3 defined above with TARGET_RO_FLOW_RATE_L. Reply by hnguyen on 18 March 2022, 13:01 > Removed TARGET_FLUSH_WATER_RO_FLOW_RATE_L Reply by Sean Nash on 22 March 2022, 16:56 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:45 https://devapps.diality.us/cru/DG-DEN-11928-1#c12541 Don't need this volume variable. You are only initializing it and then never using it. Reply by hnguyen on 18 March 2022, 13:03 > Removed flushWaterVolumeL. Reply by Sean Nash on 22 March 2022, 16:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:05 https://devapps.diality.us/cru/DG-DEN-11928-1#c12709 I don't see this getting initialized anywhere. Should be initialized in init function. Consider stagger when initializing it. Reply by hnguyen on 30 March 2022, 15:26 > Initialized hdLostCommStartTime_ms > in void initGenIdleMode( void ) Reply by Dara Navaei on 30 March 2022, 21:33 > It is initialized in the init function and it is staggered. Reply by Sean Nash on 31 March 2022, 13:20 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 09:58 https://devapps.diality.us/cru/DG-DEN-11928-1#c12542 We can't initialize bad fill state here because this init function gets called every time we come back to idle mode. I think we should initialize the badFillState in the start state when we decide to go to the flush water (normal) state. Reply by hnguyen on 18 March 2022, 13:23 > Corrected > Initialized in case DG_GEN_IDLE_MODE_STATE_START: > > // Execute current generation idle state > switch ( genIdleState ) > { > case DG_GEN_IDLE_MODE_STATE_START: > badFillState = DG_HANDLE_BAD_FILL_STATE_START; > genIdleState = handleIdleStartState(); > break; Reply by Sean Nash on 22 March 2022, 16:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 22 March 2022, 14:35 https://devapps.diality.us/cru/DG-DEN-11928-1#c12613 We should stop all the actuators prior to transitioning to standby. Reply by hnguyen on 22 March 2022, 15:06 > When transition to > case DG_MODE_STAN: > currentSubMode = transitionToStandbyMode(); > break; > transitionToStandbyMode is called and deenergizeActuators() > is invoked to init all actuators to desired state. > > // Turn off the UV reactors > turnOffUVReactor( INLET_UV_REACTOR ); > turnOffUVReactor( OUTLET_UV_REACTOR ); > > // De-energize all the valves > setValveStateDelayed( VPI, VALVE_STATE_CLOSED, > DELAY_VALVE_MS ); > setValveStateDelayed( VBF, VALVE_STATE_CLOSED, > DELAY_VALVE_MS ); > setValveStateDelayed( VSP, VALVE_STATE_CLOSED, > DELAY_VALVE_MS ); > setValveStateDelayed( VPD, VALVE_STATE_DRAIN_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VPO, VALVE_STATE_NOFILL_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VDR, VALVE_STATE_DRAIN_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VRC, VALVE_STATE_DRAIN_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VRO, VALVE_STATE_R1_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VRD1, VALVE_STATE_CLOSED, > DELAY_VALVE_MS ); > setValveStateDelayed( VRD2, VALVE_STATE_CLOSED, > DELAY_VALVE_MS ); > setValveStateDelayed( VRI, VALVE_STATE_R1_C_TO_NO, > DELAY_VALVE_MS ); > setValveStateDelayed( VRF, VALVE_STATE_R2_C_TO_NO, > DELAY_VALVE_MS ); > > requestConcentratePumpOff( CONCENTRATEPUMPS_CP1_ACID ); > requestConcentratePumpOff( CONCENTRATEPUMPS_CP2_BICARB ); > signalROPumpHardStop(); > signalDrainPumpHardStop(); > stopHeater( DG_PRIMARY_HEATER ); > stopHeater( DG_TRIMMER_HEATER ); Reply by Dara Navaei on 22 March 2022, 16:25 > RESOLVED in CODE WALTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 16:43 https://devapps.diality.us/cru/DG-DEN-11928-1#c12625 Can't initialize this hear. Move to start state handler where we decide to do normal workflow. Reply by hnguyen on 23 March 2022, 10:44 > Initialized badFillState where it is declared in the private > data section. Reply by Sean Nash on 23 March 2022, 14:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:06 https://devapps.diality.us/cru/DG-DEN-11928-1#c12710 I would move this to end of function in case state machine changes the state. Reply by hnguyen on 30 March 2022, 13:40 > Moved publishBadFillSubstates(); to the end of > execGenIdle function Reply by Sean Nash on 31 March 2022, 13:21 > RESOLVED in WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:16 https://devapps.diality.us/cru/DG-DEN-11928-1#c12550 I don't think we should be using bad fill to make this decision. It will work first time but bad fill workflow will do 2 more fills that hopefully will not be bad fills and yet we still want to stay in the bad fill state until we finish the bad fill workflow. So what I think we should be doing is having fill mode signal idle mode (call a signal function from fill mode when bad avg conductivity detected which sets a flag here in idle mode). Reply by hnguyen on 18 March 2022, 15:43 > Removed isBadFill() function and used signal function > isAvgConductivityOutOfRange( void ); Reply by hnguyen on 23 March 2022, 13:41 > Added signal function setBadAvgConductivityDetectedFlag( > BOOL flag ) in ModeGenIdle. Reply by Sean Nash on 23 March 2022, 14:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:09 https://devapps.diality.us/cru/DG-DEN-11928-1#c12543 Should just set result to this state above at declaration to simplify. Reply by Sean Nash on 22 March 2022, 16:55 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:11 https://devapps.diality.us/cru/DG-DEN-11928-1#c12548 We need to wait in this state (don't fill yet) if the first alarm has not yet been cleared by user (indicating user has confirmed replacement of bottles). So need an "if" here. Also, we need to do something so that the next fill will only go to 1,000 mL. Reply by hnguyen on 18 March 2022, 13:59 > Wait here for alarm is no longer active, then transition to > fill mode. Reply by Sean Nash on 22 March 2022, 16:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Dara Navaei on 22 March 2022, 16:17 https://devapps.diality.us/cru/DG-DEN-11928-1#c12616 This needs to be a #define line BAD_FILL_TARGET_VOLUME_ML Reply by hnguyen on 22 March 2022, 17:54 > Removed U32 fillToVolumeML = 1000; > Added #define BAD_FLUSH_FILL_TARGET_VOLUME_ML 1000 Reply by Dara Navaei on 23 March 2022, 14:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:09 https://devapps.diality.us/cru/DG-DEN-11928-1#c12545 Should just set result to this state above at declaration to simplify. Reply by hnguyen on 18 March 2022, 13:30 > Done. Reply by Sean Nash on 22 March 2022, 16:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:14 https://devapps.diality.us/cru/DG-DEN-11928-1#c12549 Need to make sure this fill will target the normal fill volume (not 1,000 mL this time). Reply by hnguyen on 21 March 2022, 17:46 > Added > startFillCmd( targetFillVolumeML, > getTargetFillFlowRateLPM() ); // refill to the saved target > fill volume (~1500 mL) Reply by Sean Nash on 22 March 2022, 16:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 16:40 https://devapps.diality.us/cru/DG-DEN-11928-1#c12624 Should this mode request be moved to last state as we transition to this state. Reply by hnguyen on 23 March 2022, 14:14 > Sean is OK with current implementation. Reply by Sean Nash on 23 March 2022, 14:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:09 https://devapps.diality.us/cru/DG-DEN-11928-1#c12546 Should just set result to this state above at declaration to simplify. Reply by hnguyen on 18 March 2022, 13:31 > Done. Reply by Sean Nash on 22 March 2022, 16:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:10 https://devapps.diality.us/cru/DG-DEN-11928-1#c12547 Should set back to start state for next time. Reply by hnguyen on 18 March 2022, 14:16 > Set result = DG_HANDLE_BAD_FILL_STATE_START; Reply by Sean Nash on 22 March 2022, 16:54 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 10:23 https://devapps.diality.us/cru/DG-DEN-11928-1#c12551 Would also set internal signal flag to FALSE here so next call to idle exec will get us back into the normal flush water state. Reply by hnguyen on 22 March 2022, 13:13 > Added internal signal flag handleBadFillFlag Reply by Sean Nash on 22 March 2022, 16:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 16:50 https://devapps.diality.us/cru/DG-DEN-11928-1#c12628 This mode request will not do anything. Need to change the main idle state to start state so it will take normal workflow path. Reply by hnguyen on 23 March 2022, 10:52 > Added > genIdleState = DG_GEN_IDLE_MODE_STATE_START; Reply by Sean Nash on 23 March 2022, 14:37 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 30 March 2022, 13:08 https://devapps.diality.us/cru/DG-DEN-11928-1#c12711 Add test functions banner separator here. Reply by hnguyen on 30 March 2022, 13:54 > Added test separator below > /************************************************************************* > * TEST SUPPORT FUNCTIONS > > *************************************************************************/ Reply by Sean Nash on 31 March 2022, 13:21 > RESOLVED in WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeDrain.c Revision Comment by Sean Nash on 18 March 2022, 09:00 https://devapps.diality.us/cru/DG-DEN-11928-1#c12530 I think we tare just about every time we drain - so this path is very common and you are not checking for empty bottles here. Reply by hnguyen on 21 March 2022, 18:24 > At the end of drain, check for empty bottles Reply by Sean Nash on 22 March 2022, 17:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 17:15 https://devapps.diality.us/cru/DG-DEN-11928-1#c12647 We might want to keep request for idle mode here. Reply by hnguyen on 22 March 2022, 18:08 > Uncommented requestNewOperationMode( ) Reply by Sean Nash on 23 March 2022, 14:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 17:16 https://devapps.diality.us/cru/DG-DEN-11928-1#c12648 Might want to move empty bottle checks above tare check. Reply by hnguyen on 22 March 2022, 18:15 > Moved check for empty bottles before if ( TRUE == > isReservoirTarePending() ) Reply by Sean Nash on 23 March 2022, 14:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 18 March 2022, 08:58 https://devapps.diality.us/cru/DG-DEN-11928-1#c12529 I think this should just be an else. Reply by hnguyen on 21 March 2022, 18:23 > Replaced > else if ( ( FALSE == isAlarmActive( > ALARM_ID_DG_ACID_BOTTLE_LOW_VOLUME ) ) || > ( FALSE == isAlarmActive( > ALARM_ID_DG_BICARB_BOTTLE_LOW_VOLUME ) ) ) > { > with > else > { Reply by Sean Nash on 22 March 2022, 17:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 22 March 2022, 17:18 https://devapps.diality.us/cru/DG-DEN-11928-1#c12649 Don't need this else and request for idle. Reply by hnguyen on 22 March 2022, 18:14 > Removed else requestNewOperationMode( DG_MODE_GENE ); Reply by Sean Nash on 23 March 2022, 14:31 > Retract issue - looking at latest code it is needed. Else > and mode change request restored. RESOLVED in CODE > WALKTHROUGH. --- ID: DG-DEN-11928-1 https://devapps.diality.us/cru/DG-DEN-11928-1 Title: DG-DEN-11928_DG Conductivity Update Statement of Objectives: State: Closed Summary: Author: hnguyen Moderator: hnguyen Reviewers: (2 active, 2 completed*) Sean Nash (*) Dara Navaei (*) Michael Garthwaite Behrouz NematiPour