This is a list of all comments for DG-DEN-15199-1. Review Summary: No summary ---------------------------------------- File: TestSupport.c Revision Comment by Sean Nash on 21 April 2023, 11:26 https://devapps.diality.us/cru/DG-DEN-15199-1#c17179 Add blank line between banner and #define. Reply by Dara Navaei on 21 April 2023, 14:45 > Done Reply by Sean Nash on 24 April 2023, 10:02 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:26 https://devapps.diality.us/cru/DG-DEN-15199-1#c17180 Add blank line between banner and #define. Reply by Dara Navaei on 21 April 2023, 14:45 > Done Reply by Sean Nash on 24 April 2023, 10:01 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:28 https://devapps.diality.us/cru/DG-DEN-15199-1#c17181 memset wants a byte. Just say 0 instead of using KEY #define. Also, TEST_CONFIG_T is an enum, so size is probably 1 and so this probably won't initialize whole array. Reply by Dara Navaei on 21 April 2023, 14:45 > I removed the memset macro. Reply by Sean Nash on 24 April 2023, 10:01 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:34 https://devapps.diality.us/cru/DG-DEN-15199-1#c17182 Also, Dialin must be logged in to accept a test config command. Reply by Dara Navaei on 24 April 2023, 09:13 > Done. Reply by Sean Nash on 24 April 2023, 09:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:35 https://devapps.diality.us/cru/DG-DEN-15199-1#c17183 Yes, s/w fault (for invalid config). Ignore command for not logged in. Reply by Dara Navaei on 24 April 2023, 09:14 > Done. Reply by Sean Nash on 24 April 2023, 09:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:37 https://devapps.diality.us/cru/DG-DEN-15199-1#c17184 Add check for Dialin logged in. Reply by Dara Navaei on 21 April 2023, 14:44 > This function will not even be called if Dialin is not logged > in. Did you want to check it here too? Reply by Sean Nash on 24 April 2023, 09:51 > Yes, as this function could be called from somewhere other > than message handler. > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:37 https://devapps.diality.us/cru/DG-DEN-15199-1#c17185 Yes, s/w fault (for invalid config). Ignore command for not logged in. Reply by Sean Nash on 24 April 2023, 09:52 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:40 https://devapps.diality.us/cru/DG-DEN-15199-1#c17188 Note in brief that function will return FALSE if Dialin not logged in. Reply by Dara Navaei on 21 April 2023, 14:39 > This function is a Dialin command so it will not even be > called if Dialin is not logged in. Did you want to check this > again in this function? Reply by Sean Nash on 24 April 2023, 09:58 > Not a Dialin command. Called by opmodes in firmware. Reply by Dara Navaei on 24 April 2023, 12:40 > Done Reply by Sean Nash on 26 April 2023, 08:48 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:40 https://devapps.diality.us/cru/DG-DEN-15199-1#c17187 testConfig is an input, not an output. Reply by Dara Navaei on 21 April 2023, 14:43 > Done Reply by Sean Nash on 24 April 2023, 09:42 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 21 April 2023, 11:39 https://devapps.diality.us/cru/DG-DEN-15199-1#c17186 Change "release" to "test". Reply by Dara Navaei on 21 April 2023, 14:43 > Done Reply by Sean Nash on 24 April 2023, 09:46 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 24 April 2023, 09:44 https://devapps.diality.us/cru/DG-DEN-15199-1#c17212 Still want check for logged in. Reply by Dara Navaei on 24 April 2023, 12:40 > Done Reply by Sean Nash on 26 April 2023, 08:49 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: TestSupport.h Revision Comment by Sean Nash on 21 April 2023, 11:25 https://devapps.diality.us/cru/DG-DEN-15199-1#c17178 Are DG and HD sharing this enum? Reply by Dara Navaei on 21 April 2023, 14:46 > Correct. This common code in between the two stacks. I can > separate them using #ifndef. Reply by Sean Nash on 24 April 2023, 10:03 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Modes/ModeFill.c Revision Comment by Sean Nash on 21 April 2023, 11:14 https://devapps.diality.us/cru/DG-DEN-15199-1#c17175 We want test configs be active only if explicitly enabled. Safest condition would be to say ( TRUE == getTest.... ). Make this change everywhere getTestConfigStatus function is used. Reply by Dara Navaei on 21 April 2023, 14:59 > Done Reply by Sean Nash on 24 April 2023, 10:04 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemComm.c Revision Comment by Sean Nash on 26 April 2023, 08:31 https://devapps.diality.us/cru/DG-DEN-15199-1#c17249 Remove "Been" from function name. And add a comment line above explaining what we're doing here. Reply by Dara Navaei on 26 April 2023, 08:52 > Done Reply by Sean Nash on 26 April 2023, 09:05 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: firmware/App/Services/SystemCommMessages.c Revision Comment by Sean Nash on 26 April 2023, 08:33 https://devapps.diality.us/cru/DG-DEN-15199-1#c17250 This is an output. Reply by Dara Navaei on 26 April 2023, 08:50 > Done Reply by Sean Nash on 26 April 2023, 09:04 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 April 2023, 08:35 https://devapps.diality.us/cru/DG-DEN-15199-1#c17251 Checking for isTestingActivated should be done in sendTestConfigStatusToDialin function (not here). Fix throughout new handlers. Reply by Dara Navaei on 26 April 2023, 08:49 > Done Reply by Sean Nash on 26 April 2023, 09:04 > I said fix throughout. Need fix in function below as well. Reply by Dara Navaei on 26 April 2023, 09:13 > Done Reply by Sean Nash on 26 April 2023, 09:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 26 April 2023, 08:38 https://devapps.diality.us/cru/DG-DEN-15199-1#c17252 Acking even if payload length > 0? Reply by Dara Navaei on 26 April 2023, 08:49 > Done. Reply by Sean Nash on 26 April 2023, 09:02 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: AlarmDefs.h Revision Comment by Sean Nash on 26 April 2023, 08:41 https://devapps.diality.us/cru/DG-DEN-15199-1#c17253 Why did we blank title/instructions? Reply by Dara Navaei on 26 April 2023, 08:42 > Brought the code back. Reply by Sean Nash on 26 April 2023, 09:02 > RESOLVED in CODE WALKTHROUGH. --- ID: DG-DEN-15199-1 https://devapps.diality.us/cru/DG-DEN-15199-1 Title: DG-DEN-15199_FW DN Sprint Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (4 active, 2 completed*) Sean Nash (*) jtaylor (*) Michael Garthwaite wbracken Darren Cox Steve Jarpe