This is a list of all comments for DG-DEN-2650-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Controllers/LoadCell.c Revision Comment by pmontazemi on 11 June 2020, 09:10 https://devapps.diality.us/cru/DG-DEN-2650-1#c2221 What is our plan to adjust these constants across machines? Reply by Sean Nash on 11 June 2020, 09:17 > I imagine we would settle on a nominal (theoretical) > gain/offset here and then apply calibration gain/offset from > NV memory afterward to get a final conversion. Reply by pmontazemi on 19 June 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 04 June 2020, 09:04 https://devapps.diality.us/cru/DG-DEN-2650-1#c2177 No longer a float? Reply by Sean Nash on 04 June 2020, 17:17 > Never was a float. Reply by pmontazemi on 05 June 2020, 10:48 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Pressures.c Revision Comment by pmontazemi on 30 April 2020, 16:42 https://devapps.diality.us/cru/DG-DEN-2650-1#c1723 Measured weight from load cells? Reply by Sean Nash on 30 April 2020, 16:48 > Fixed. Reply by pmontazemi on 06 May 2020, 10:38 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 30 April 2020, 21:27 https://devapps.diality.us/cru/DG-DEN-2650-1#c1725 Insert line. Reply by Sean Nash on 01 May 2020, 09:21 > Done Reply by pmontazemi on 06 May 2020, 10:37 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/ROPump.c Revision Comment by pmontazemi on 18 May 2020, 10:28 https://devapps.diality.us/cru/DG-DEN-2650-1#c1922 Replace with RO_FLOW_ADC_TO_LPM_FACTOR / (F32)(roFlow) Reply by Sean Nash on 18 May 2020, 10:53 > Agreed. Done. Reply by pmontazemi on 20 May 2020, 13:06 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 04 June 2020, 09:05 https://devapps.diality.us/cru/DG-DEN-2650-1#c2178 Why compare to 0 then set to 0.0 (float)? Reply by Sean Nash on 04 June 2020, 17:15 > This is trying to avoid divide by zero. Easier to compare > the average sum of readings (integer) to zero than the > average (float). Reply by pmontazemi on 05 June 2020, 10:47 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 04 June 2020, 16:16 https://devapps.diality.us/cru/DG-DEN-2650-1#c2182 Where is isROPumpOn declared? Reply by Dara Navaei on 04 June 2020, 16:29 > It is a static variable in the module Reply by pmontazemi on 05 June 2020, 10:43 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 19 June 2020, 09:13 https://devapps.diality.us/cru/DG-DEN-2650-1#c2447 Do we have dip switches on the DG Board? Reply by Sean Nash on 19 June 2020, 09:20 > Yes, there are 4 DIP switches that go to GPIO input pins on > the RM46 that we can use for engineering/test purposes. Reply by pmontazemi on 19 June 2020, 10:54 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by pmontazemi on 24 April 2020, 11:55 https://devapps.diality.us/cru/DG-DEN-2650-1#c1675 Remove commented code. Reply by Sean Nash on 27 April 2020, 09:34 > Done. Reply by pmontazemi on 28 April 2020, 08:29 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeRecirculate.c Revision Comment by pmontazemi on 27 April 2020, 10:52 https://devapps.diality.us/cru/DG-DEN-2650-1#c1709 Where does 0.1L come from? From SA? From HRS? From HDD? Reply by Sean Nash on 27 April 2020, 10:58 > This is just a placeholder. I do not believe this is > specified anywhere yet - still unknown - but f/w will need to > know this I imagine to determine when the line is cleared. Reply by pmontazemi on 28 April 2020, 08:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by pmontazemi on 19 June 2020, 09:16 https://devapps.diality.us/cru/DG-DEN-2650-1#c2452 There are many more valves on the DG Device, why setting only these ones? Reply by Sean Nash on 19 June 2020, 09:21 > The 4 reservoir valves are controlled by the HD when it's > connected and on. In Standby mode, the HD is connected and > on. Reply by pmontazemi on 19 June 2020, 10:56 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by pmontazemi on 22 April 2020, 13:41 https://devapps.diality.us/cru/DG-DEN-2650-1#c1627 Remove commented lines. Reply by Sean Nash on 22 April 2020, 16:40 > Done. Reply by pmontazemi on 23 April 2020, 10:33 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 04 June 2020, 11:52 https://devapps.diality.us/cru/DG-DEN-2650-1#c2181 Why do we need to remove this line? Reply by Dara Navaei on 04 June 2020, 16:28 > This was for testing only and will be removed Reply by pmontazemi on 05 June 2020, 10:44 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 11 May 2020, 16:43 https://devapps.diality.us/cru/DG-DEN-2650-1#c1878 ??? Reply by pmontazemi on 13 May 2020, 10:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Reservoirs.c Revision Comment by pmontazemi on 22 April 2020, 13:54 https://devapps.diality.us/cru/DG-DEN-2650-1#c1628 Any other conditions needed to be covered here? Or, only this double success condition? Reply by Sean Nash on 22 April 2020, 16:36 > There may be other conditions added later, but these are what > I know so far. To accept a fill command, you have to be in > recirculate mode and the requested fill to volume must be in > valid range. Reply by pmontazemi on 23 April 2020, 10:36 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/DGCommon.h Revision Comment by pmontazemi on 11 June 2020, 09:15 https://devapps.diality.us/cru/DG-DEN-2650-1#c2222 Clean up Reply by Sean Nash on 11 June 2020, 09:20 > I do remove build flags that I don't need anymore every so > often. If we're done with CAN testing I can remove that one. > I'm still using the rest of these from time to time. Reply by pmontazemi on 19 June 2020, 10:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by pmontazemi on 06 May 2020, 12:25 https://devapps.diality.us/cru/DG-DEN-2650-1#c1819 Insert blank line Reply by Sean Nash on 19 June 2020, 09:24 > I have the typedef immediately following the associated enum > to show they are coupled. Previously I had the typedef > included in the enum declaration but Qt didn't like that. Reply by Dara Navaei on 19 October 2023, 08:41 > RESOLVED in CODE WALKTHROUGH ---------------------------------------- File: firmware/App/Controllers/Heaters.c Revision Comment by pmontazemi on 15 May 2020, 09:47 https://devapps.diality.us/cru/DG-DEN-2650-1#c1920 If max duty cycle is 79.5 %, why are we setting this to 89 %? Same with the below two lines at 50 % against 25 %. Reply by Sean Nash on 18 May 2020, 10:46 > Ferdyan changed max to 89%. Fixed comment. Reply by pmontazemi on 20 May 2020, 13:05 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 15 May 2020, 09:48 https://devapps.diality.us/cru/DG-DEN-2650-1#c1921 Let's discuss what these should be. Reply by Sean Nash on 18 May 2020, 10:47 > We should cover this in Dara's code review. Reply by pmontazemi on 20 May 2020, 13:05 > RESOLVED in CODE WALKTHROUGH. Will address in Dara's code > walkthrough. Revision Comment by pmontazemi on 27 April 2020, 10:51 https://devapps.diality.us/cru/DG-DEN-2650-1#c1708 Empty? Reply by Sean Nash on 27 April 2020, 10:57 > Heaters code should be reviewed in Dara's Heaters and Temps > branch. I have a preliminary version of his code to support > hardware. Reply by pmontazemi on 28 April 2020, 08:30 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/TemperatureSensors.c Revision Comment by pmontazemi on 05 June 2020, 10:42 https://devapps.diality.us/cru/DG-DEN-2650-1#c2205 Value in mV Reply by Dara Navaei on 05 June 2020, 10:49 > Done Reply by pmontazemi on 05 June 2020, 10:49 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 05 June 2020, 10:41 https://devapps.diality.us/cru/DG-DEN-2650-1#c2203 Change it to TODO, instead. Reply by Dara Navaei on 05 June 2020, 10:49 > Done Reply by pmontazemi on 05 June 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 04 June 2020, 16:16 https://devapps.diality.us/cru/DG-DEN-2650-1#c2183 What does t = -1.0 mean? Is that a default temperature? Reply by Dara Navaei on 04 June 2020, 16:29 > If the cold junction temperature is not positive, I cannot > use the coefficients to calculate the temperature and I have > to use other coefficients which I did not implement since we > don't go to negative temperature. So if the cold junction is > negative, I set an alarm and set the temperature to -1.0 and > not 0 to show that cold junction became negative. Reply by pmontazemi on 05 June 2020, 10:42 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/AlarmMgmt.h Revision Comment by pmontazemi on 04 June 2020, 11:52 https://devapps.diality.us/cru/DG-DEN-2650-1#c2180 Why were these removed? Reply by Sean Nash on 04 June 2020, 17:13 > This looks like an issue with Dara's merge to this branch. > Looks like he corrected it. Reply by pmontazemi on 19 June 2020, 10:50 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-2650-1 https://devapps.diality.us/cru/DG-DEN-2650-1 Title: DG-DEN-2650_DG Firmware Infrastructure Statement of Objectives: State: Closed Summary: Author: Sean Nash Moderator: Sean Nash Reviewers: (1 active, 2 completed*) Dara Navaei (*) pmontazemi (*) qnguyen