This is a list of all comments for HD-DEN-15104-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 12 May 2023, 10:33 https://devapps.diality.us/cru/HD-DEN-15104-1#c17389 Make U32. Reply by wbracken on 12 May 2023, 13:01 > Done Reply by Sean Nash on 12 May 2023, 17:26 > Now it's gone. No longer needed? Reply by wbracken on 14 May 2023, 11:28 > No longer needed. Reply by Sean Nash on 14 May 2023, 15:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 10:56 https://devapps.diality.us/cru/HD-DEN-15104-1#c17397 I think these variable declarations should stay here. Why move to function? Reply by wbracken on 12 May 2023, 13:01 > This code is identical to the DG implementation. Initially > they were only included in debug mode. Seemed like it made > sense to move to function as they are only used in the > function. Reply by Sean Nash on 12 May 2023, 17:29 > Per my other comment, please avoid in-function static > variables. Reply by wbracken on 14 May 2023, 12:39 > Moved to here from function. Reply by Sean Nash on 14 May 2023, 15:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 10:34 https://devapps.diality.us/cru/HD-DEN-15104-1#c17390 This doesn't seem to ever change. Why not just initialize this value once on reset? Reply by wbracken on 12 May 2023, 13:07 > Removed. Replaced fpgaReadByteSize with sizeof( > FPGA_SENSORS_T ) in the two statements that it was being > used. Reply by Sean Nash on 12 May 2023, 17:26 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 10:35 https://devapps.diality.us/cru/HD-DEN-15104-1#c17391 Why is this in two places? And why set every 10ms when it appears to be constant? Reply by Sean Nash on 12 May 2023, 17:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 11:09 https://devapps.diality.us/cru/HD-DEN-15104-1#c17404 Do we need to check for these in out exec? Reply by wbracken on 12 May 2023, 13:02 > Removed Reply by Sean Nash on 12 May 2023, 17:32 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 10:52 https://devapps.diality.us/cru/HD-DEN-15104-1#c17394 Why aren't these declared at top of file and initialized in init function like everything else. Reply by wbracken on 12 May 2023, 13:39 > This code is identical to the DG implementation. Initially > they were only included in debug mode. Seemed like it made > sense to move to function as they are only used in the > function. Reply by Sean Nash on 12 May 2023, 17:28 > Not a fan of in function statics. Probably due to > Vectorcast having a hard time initializing them before > function is called. Please avoid doing this. Reply by Sean Nash on 14 May 2023, 15:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 10:55 https://devapps.diality.us/cru/HD-DEN-15104-1#c17396 This is HD f/w. Why are you looking at DG mode? Reply by wbracken on 12 May 2023, 13:13 > Cut and paste error from DG. Corrected. Reply by Sean Nash on 12 May 2023, 17:28 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 11:11 https://devapps.diality.us/cru/HD-DEN-15104-1#c17405 Remove blank line. Reply by wbracken on 12 May 2023, 13:15 > Removed. Reply by Sean Nash on 12 May 2023, 17:33 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/PresOccl.c Revision Comment by Sean Nash on 12 May 2023, 10:58 https://devapps.diality.us/cru/HD-DEN-15104-1#c17398 Is there more to be done here (TODO)? Reply by wbracken on 12 May 2023, 11:44 > Nothing else needs to be done. Reply by Sean Nash on 12 May 2023, 12:53 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by Sean Nash on 12 May 2023, 11:07 https://devapps.diality.us/cru/HD-DEN-15104-1#c17403 Are these used anywhere? Reply by wbracken on 12 May 2023, 13:16 > They were used when FE OE generated software fault. Removed. Reply by Sean Nash on 12 May 2023, 17:31 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 11:05 https://devapps.diality.us/cru/HD-DEN-15104-1#c17402 Add space between ( and TRUE. Or just remove condition and always set flag to FALSE here. Reply by wbracken on 12 May 2023, 13:19 > Added space. I modified slightly, Since this is called > outside of the interrupt where the sci2FEOEError is being set > I believe it should get the value and only reset it if was > TRUE. Reply by Sean Nash on 12 May 2023, 17:31 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Controllers/Valves.c Revision Comment by Sean Nash on 12 May 2023, 11:01 https://devapps.diality.us/cru/HD-DEN-15104-1#c17399 Don't we have an enum for these values? Reply by wbracken on 12 May 2023, 13:28 > There are enums for the valves. However, the enum value does > not correspond to their position in the valve status returned > from the FPGA. For example, VDI is enumerated to be the > first value in the enum but is the second 2nd group of 3 bits > (0 relative) returned by the FPGA. Reply by Sean Nash on 12 May 2023, 17:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 11:01 https://devapps.diality.us/cru/HD-DEN-15104-1#c17400 Use enums instead of literals. Reply by wbracken on 12 May 2023, 13:31 > Corrected Reply by Sean Nash on 12 May 2023, 17:30 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 12 May 2023, 11:02 https://devapps.diality.us/cru/HD-DEN-15104-1#c17401 Remove blank line. Reply by wbracken on 12 May 2023, 13:22 > Removed Reply by Sean Nash on 12 May 2023, 17:31 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-15104-1 https://devapps.diality.us/cru/HD-DEN-15104-1 Title: HD-DEN-15104_HD Fpga Communications Statement of Objectives: State: Closed Summary: Author: wbracken Moderator: wbracken Reviewers: (3 active, 3 completed*) Sean Nash (*) Darren Cox (*) jtaylor (*) Michael Garthwaite Dara Navaei Steve Jarpe