This is a list of all comments for BOOTLOADER-LEAH-1415-1. Review Summary: No summary ---------------------------------------- File: firmware/App/Modes/ModeStandby.c Revision Comment by Vinayakam Mani on 04 September 2024, 09:31 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20163 Are we going to generate design document out from the code? if so, can we add doxygen headers/group for all .c/.h files? Reply by Dara Navaei on 30 September 2024, 16:06 > Please see the updated code with the doxygen comments. Revision Comment by Sean Nash on 01 October 2024, 09:09 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20344 Change "is the" to "is in". Remove extra "and". Revision Comment by Sean Nash on 01 October 2024, 09:10 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20346 If we are in the check image process, that means we are wanting to jump to the app (assuming check is successful). If we get an update command from UI during the check, I think that should take precedence - abort the check and service the update request. Revision Comment by Sean Nash on 01 October 2024, 09:09 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20345 Do this TODO. Revision Comment by Sean Nash on 01 October 2024, 09:12 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20347 Should we reset state to something valid? Or s/w reset? Try to recover in some way? Revision Comment by Sean Nash on 01 October 2024, 09:14 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20348 I think it's better (and maybe coding standard) to assign the state (from get function) to a local variable and use that variable in the switch. Revision Comment by Sean Nash on 01 October 2024, 09:15 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20349 Use #define. Remove high number. Revision Comment by Sean Nash on 01 October 2024, 09:27 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20350 I don't see this variable used in this function. Revision Comment by Sean Nash on 01 October 2024, 09:28 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20352 I don't understand either of these TODOs. Are they needed? Revision Comment by Sean Nash on 01 October 2024, 09:28 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20351 Why is this interrupt protected? Revision Comment by Sean Nash on 01 October 2024, 09:39 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20357 This else could be removed if you just set next state to idle at top of function. Revision Comment by Sean Nash on 01 October 2024, 09:30 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20353 I don't think retries are needed here. We should message UI to inform on the problem though. Revision Comment by Sean Nash on 01 October 2024, 09:36 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20355 Shouldn't this state at least transition back to update state when new update request is made? Where does that happen and why doesn't it happen here? Revision Comment by Sean Nash on 01 October 2024, 09:38 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20356 Why is this infinite loop needed (or even wanted)? ---------------------------------------- File: firmware/App/Services/NVDataMgmt.c Revision Comment by Sean Nash on 01 October 2024, 10:38 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20395 Remove blank line. ---------------------------------------- File: firmware/App/Tasks/TaskBG.c Revision Comment by Sean Nash on 01 October 2024, 10:07 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20369 No watchdog manager? ---------------------------------------- File: firmware/App/Modes/ModeUpdate.c Revision Comment by Sean Nash on 01 October 2024, 09:45 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20358 Doxygen comment needed. Revision Comment by Sean Nash on 01 October 2024, 09:46 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20359 Seems like more is happening here than init. Some resetting too. Revision Comment by Sean Nash on 01 October 2024, 09:55 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20363 Why return anything? Your OperationModes unit doesn't not even look at it. Revision Comment by Sean Nash on 01 October 2024, 09:47 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20360 Why zero? Why even return anything? Your OperationModes unit doesn't even look at return value. Revision Comment by Sean Nash on 01 October 2024, 09:48 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20361 Don't call get function in a switch. Is get function getting a state or a command? Seems like a command, so why is "State" in the function name? Revision Comment by Sean Nash on 01 October 2024, 09:50 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20362 Don't call get function in a switch. ---------------------------------------- File: firmware/App/Services/CommBuffers.c Revision Comment by Sean Nash on 01 October 2024, 09:57 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20365 Name of #define and doxygen comment do not make clear what max is for (i.e. download buffer). Revision Comment by Sean Nash on 01 October 2024, 10:09 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20370 Structure should be moved up to definitions section. Revision Comment by Sean Nash on 01 October 2024, 10:10 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20371 Where is this enumeration defined? Revision Comment by Sean Nash on 01 October 2024, 10:12 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20372 I don't think you are doing a double buffer. Remove this note. Revision Comment by Sean Nash on 01 October 2024, 10:13 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20373 You are not doing double buffers. Remove this note. Revision Comment by Vinayakam Mani on 04 September 2024, 10:19 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20165 Do we need this header? Reply by Dara Navaei on 30 September 2024, 16:06 > I have left the separating comments for future use. Revision Comment by Sean Nash on 01 October 2024, 10:13 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20374 Remove reference to double buffer. ---------------------------------------- File: firmware/App/Services/Interrupts.c Revision Comment by Sean Nash on 01 October 2024, 10:36 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20394 FPGA unit does not need to know when a transmit has completed? ---------------------------------------- File: firmware/App/Tasks/TaskTimer.c Revision Comment by Sean Nash on 01 October 2024, 10:04 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20367 When are we doing this? ---------------------------------------- File: firmware/source/sys_main.c Revision Comment by Sean Nash on 30 August 2024, 12:27 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20109 Shouldn't these be in user code (2)? Reply by Dara Navaei on 30 September 2024, 16:08 > Correct. I moved them. ---------------------------------------- File: firmware/App/Services/CopyFlashAPI2RAM.asm Revision Comment by Vinayakam Mani on 04 September 2024, 10:21 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20166 close the above header Reply by Dara Navaei on 30 September 2024, 16:05 > Done ---------------------------------------- File: firmware/App/Services/FPGA.c Revision Comment by Sean Nash on 01 October 2024, 10:29 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20389 Remove test code. Revision Comment by Sean Nash on 01 October 2024, 10:29 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20390 Add a blank line to separate code with different commenting styles - Doxygen gets confused if you don't. Revision Comment by Sean Nash on 01 October 2024, 10:30 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20391 Remove test code. Revision Comment by Sean Nash on 01 October 2024, 10:31 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20392 Returns TRUE if flash complete, FALSE if not. ---------------------------------------- File: firmware/App/Services/Comm.c Revision Comment by Sean Nash on 01 October 2024, 09:56 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20364 Add a blank line and an end of group marker at bottom of file. ---------------------------------------- File: firmware/App/Services/Download.c Revision Comment by Sean Nash on 01 October 2024, 10:15 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20375 Can we remove this now? Revision Comment by Sean Nash on 01 October 2024, 10:16 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20376 Move constants and structs up to definitions section. Revision Comment by Sean Nash on 01 October 2024, 10:16 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20377 Can we remove this now? Revision Comment by Sean Nash on 01 October 2024, 10:17 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20378 thisStackMailBox is an input? Revision Comment by Sean Nash on 01 October 2024, 10:18 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20379 When do you plan to do this? Revision Comment by Sean Nash on 01 October 2024, 10:19 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20380 Remove this function. Revision Comment by Sean Nash on 01 October 2024, 10:20 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20381 Remove test code. Revision Comment by Sean Nash on 01 October 2024, 10:21 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20382 Remove test code. Revision Comment by Sean Nash on 01 October 2024, 10:21 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20383 When do you plan to do this? Revision Comment by Sean Nash on 01 October 2024, 10:21 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20384 Why is this code commented out? Revision Comment by Sean Nash on 01 October 2024, 10:25 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20385 If send fails, I think we abort, try to send error msg to UI, and go back to standby. Revision Comment by Sean Nash on 01 October 2024, 10:26 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20386 When do you plan to do this? Revision Comment by Sean Nash on 01 October 2024, 10:27 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20387 Remove test code. Revision Comment by Sean Nash on 01 October 2024, 10:27 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20388 Should only be one blank line at end of file. Should also be an end group marker down here. ---------------------------------------- File: firmware/App/Tasks/TaskPriority.h Revision Comment by Sean Nash on 01 October 2024, 10:05 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1#c20368 I guess you're not using this #define, but is there any harm in keeping it? --- ID: BOOTLOADER-LEAH-1415-1 https://devapps.diality.us/cru/BOOTLOADER-LEAH-1415-1 Title: BOOTLOADER-LEAH-1415_BL Leahi Bootloader Statement of Objectives: State: Review Author: Dara Navaei Moderator: Dara Navaei Reviewers: (6 active, 0 completed*) Sean Nash Tiffany Mejia Vinayakam Mani Michael Garthwaite Behrouz NematiPour Daniel Ho