This is a list of all comments for HD-DEN-1778-1. Review Summary: No summary ---------------------------------------- File: NVDataMgmt.c Revision Comment by Sean Nash on 17 March 2020, 10:36 https://devapps.diality.us/cru/HD-DEN-1778-1#c1442 Should this be "JOB" count instead of "QUEUE" count? Reply by Dara Navaei on 19 March 2020, 00:06 > This #define is used for checking the number queues > available. I changed it to MIN_QUEUE_NEEDED_FOR_DATA_LOG Reply by Sean Nash on 19 March 2020, 08:21 > My point was that there is only one queue and the queue > holds some number of jobs or memory operations. So the > count is not a queue count (because there's only one). Reply by Dara Navaei on 19 March 2020, 13:07 > Understood. I changed it to MIN_JOBS_NEEDED_FOR_DATA_LOG Reply by Sean Nash on 23 March 2020, 10:25 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:39 https://devapps.diality.us/cru/HD-DEN-1778-1#c1443 We are storing events in this one log, not data logs. Reply by Dara Navaei on 19 March 2020, 13:14 > True. Changed it to MAX_NUM_OF_EVENTS_IN_SECTOR3 and changed > it to all of them Reply by Sean Nash on 23 March 2020, 10:26 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:40 https://devapps.diality.us/cru/HD-DEN-1778-1#c1444 I thought the mfg. record would be stored in EEPROM (sector 0)? Reply by Dara Navaei on 19 March 2020, 13:14 > This #define was never used and I will remove it. If you look > at the POST functions, Mfg log is read from EEPROM at the > beginning of Sector0. Reply by Sean Nash on 23 March 2020, 10:25 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:41 https://devapps.diality.us/cru/HD-DEN-1778-1#c1445 Were you going to remove this state? Reply by Dara Navaei on 19 March 2020, 13:27 > I didn't remove it but it only goes to fault if the state > machine ends up in default. In that case, it set the SW fault > alarm and go to default. Should I go to Idle instead? Reply by Sean Nash on 23 March 2020, 10:24 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:42 https://devapps.diality.us/cru/HD-DEN-1778-1#c1446 What is the purpose of this "none" operation? Reply by Dara Navaei on 19 March 2020, 13:30 > I wanted the default operation be write to it does not > accidentally write or read something. Reply by Sean Nash on 24 March 2020, 10:10 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:43 https://devapps.diality.us/cru/HD-DEN-1778-1#c1447 Sometime we call it a job and sometimes a memory operation. We should probably pick one and be consistent. I think non-volatile memory operation is best. Reply by Dara Navaei on 19 March 2020, 16:09 > So the struct that has the details (see above please) has is > called memory operations. The array of type this struct is > called jobs and from then jobs is used. Only this struct has > memory named. Reply by Sean Nash on 23 March 2020, 10:22 > Keeping as is. RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 10:44 https://devapps.diality.us/cru/HD-DEN-1778-1#c1448 Were you going to add a flag field for when this record is found corrupted (and set to full until uploaded to HD)? Reply by Dara Navaei on 19 March 2020, 13:31 > Yes, the Bool flag isHdrCorrupted in the LOG_HEADER_T is that > flag. It is false unless the CRC fails in POST. Reply by Sean Nash on 23 March 2020, 10:19 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:37 https://devapps.diality.us/cru/HD-DEN-1778-1#c1449 Why is this a byte pointer instead of a CALIBRATION_DATA_T pointer? Reply by Dara Navaei on 19 March 2020, 14:58 > Done Reply by Sean Nash on 23 March 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:38 https://devapps.diality.us/cru/HD-DEN-1778-1#c1450 Why is this a byte pointer instead of a SERVICE_DATA_T pointer? Reply by Dara Navaei on 19 March 2020, 15:41 > Done Reply by Sean Nash on 23 March 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:43 https://devapps.diality.us/cru/HD-DEN-1778-1#c1451 Why is this a byte pointer instead of a LOG_DATA_T pointer? And if it was a LOG_DATA_T pointer, would we need a length since length is fixed at 32 bytes? Reply by Dara Navaei on 19 March 2020, 16:01 > Done Reply by Sean Nash on 23 March 2020, 10:18 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:46 https://devapps.diality.us/cru/HD-DEN-1778-1#c1452 Is minutes right? Or was it supposed to be hours? Reply by Dara Navaei on 19 March 2020, 13:40 > Changed it hours. Reply by Sean Nash on 23 March 2020, 10:17 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 19 March 2020, 09:23 https://devapps.diality.us/cru/HD-DEN-1778-1#c1468 What if the queue is full, what does the caller of this function see? Just status = FALSE? Is this acceptable by the caller? Reply by Dara Navaei on 19 March 2020, 13:55 > Yes we only return a bool to the caller. If they request a > write and get a FALSE, it means their request has failed and > they have to try again. It is mentioned in the Interface > section in AE. Otherwise, we have to return enums instead of > Boolean. Reply by pmontazemi on 20 March 2020, 08:45 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 19 March 2020, 09:24 https://devapps.diality.us/cru/HD-DEN-1778-1#c1469 Is this always guaranteed? i.e. is there a check on buffer's data needed before doing a memcpy to avoid failure of this action? Reply by Dara Navaei on 20 March 2020, 15:24 > I added a check to all the get functions to check for NULL > pointer. Reply by pmontazemi on 23 March 2020, 10:27 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:50 https://devapps.diality.us/cru/HD-DEN-1778-1#c1453 Remove fault state? On default, s/w fault and set state back to Idle. Reply by Dara Navaei on 19 March 2020, 13:38 > Done. It goes back to Idle Reply by Sean Nash on 23 March 2020, 10:16 > RESOLVED in CODE WALKTHROUGH. Revision Comment by pmontazemi on 19 March 2020, 09:26 https://devapps.diality.us/cru/HD-DEN-1778-1#c1470 Is bootloaderFlag a global variable or static variable in the scope of this file? Reply by Dara Navaei on 19 March 2020, 14:37 > The bootloader flag is a static variable in the scope of this > file. It is returned using the getBootloaderFlag function. Reply by pmontazemi on 20 March 2020, 08:40 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:54 https://devapps.diality.us/cru/HD-DEN-1778-1#c1454 Looks like we're going to queue the job even if the queue was full. Seems wrong. Reply by Dara Navaei on 24 March 2020, 10:08 > Done Reply by Sean Nash on 24 March 2020, 10:09 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:56 https://devapps.diality.us/cru/HD-DEN-1778-1#c1456 Shouldn't this be checking if queue is empty? Reply by Dara Navaei on 20 March 2020, 19:48 > Done Reply by Sean Nash on 23 March 2020, 10:13 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:56 https://devapps.diality.us/cru/HD-DEN-1778-1#c1457 Should we get this job if the queue was empty above? Reply by Dara Navaei on 20 March 2020, 19:48 > Done Reply by Sean Nash on 23 March 2020, 10:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 17 March 2020, 11:57 https://devapps.diality.us/cru/HD-DEN-1778-1#c1458 Why subtract one more? Reply by Dara Navaei on 19 March 2020, 13:42 > It was my mistake. Fixed it. Reply by Sean Nash on 23 March 2020, 10:12 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 23 March 2020, 08:27 https://devapps.diality.us/cru/HD-DEN-1778-1#c1497 Count needs thread protection too. Check enqueue for same issue. Reply by Dara Navaei on 23 March 2020, 08:59 > Done. I don't check for any of the counts in enqueue > function. Reply by Sean Nash on 23 March 2020, 10:11 > RESOLVED in CODE WALKTHROUGH. Revision Comment by Sean Nash on 19 March 2020, 08:30 https://devapps.diality.us/cru/HD-DEN-1778-1#c1465 If you only have one data, use the 1 data macro. Reply by Dara Navaei on 19 March 2020, 13:43 > Done Reply by Sean Nash on 23 March 2020, 10:11 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: MsgDefs.h Revision Comment by pmontazemi on 19 March 2020, 08:56 https://devapps.diality.us/cru/HD-DEN-1778-1#c1466 This is dangerous, there should always be a reason why any action is rejected. Reply by Dara Navaei on 19 March 2020, 13:52 > [~snash] Could you please respond? Reply by pmontazemi on 20 March 2020, 08:50 > Please elaborate on the comment related to //< No reason. Reply by Sean Nash on 23 March 2020, 08:26 > Done. Reply by pmontazemi on 23 March 2020, 10:09 > RESOLVED in CODE WALKTHROUGH. ---------------------------------------- File: FlashDriver/CGT.gcc.h Revision Comment by pmontazemi on 19 March 2020, 08:58 https://devapps.diality.us/cru/HD-DEN-1778-1#c1467 What are we using TI's code generation tool for? (to generate what code?) Reply by Dara Navaei on 19 March 2020, 13:53 > The FlashDriver folder contains all the files and the library > for F021 related work which they are not shown here. They are > all needed to be able to EEPROM work. I added CGT.gcc.h last > night so I can compile in VectoCAST as well since we use > MinGW in there. Reply by pmontazemi on 20 March 2020, 08:46 > RESOLVED in CODE WALKTHROUGH. --- ID: HD-DEN-1778-1 https://devapps.diality.us/cru/HD-DEN-1778-1 Title: DEN-HD-1778_NV Data Management Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (0 active, 2 completed*) Sean Nash (*) pmontazemi (*)