•  

Comment Results

Review Name Created Custom Fields Content
UI-DEN-1434-1 10 Mar 2020

RESOLVED.

HD-SPR3-1 10 Oct 2019

Do we need to define 1 in a variable instead of using a magic number?

HD-DEN-759-1 12 Nov 2019

The Debug and Release folders will be removed from the repository by removing them from the git commit list and then .gitignore will be effective. We do not need to keep them in the remote repository. We also had to make sure that we can both build in Debug and Release mode with no issues.

HD-SPR4-1 24 Oct 2019

Renamed #define to clarify # of bits.

DIALIN-1 12 Dec 2019

RESOLVED.

HD-DEN-759-1 14 Nov 2019

Value "7", which is ...?

DIALIN-1 26 Nov 2019

Not a big fan of sleep() in general.

HD-DEN-759-1 14 Nov 2019

Value "9", which is ...?

HD-SPR4-1 24 Oct 2019

MASK_OFF_MSB (indicates how many bits), e.g., MASK_OFF_4_MSB

DG-DEN-1125-1 06 Jan 2020

Done

HD-SPR4-1 25 Oct 2019

RESOLVED IN CODE WALKTHROUGH

HD-MASTER-1 01 Oct 2019

Extra space?

HD-SPR4-1 25 Oct 2019

as is RESOLVED IN CODE WALKTHROUGH

DIALIN-1 17 Dec 2019

RESOLVED

HD-SPR4-1 25 Oct 2019

as is RESOLVED IN CODE WALKTHROUGH

HD-MASTER-1 01 Oct 2019

There is no extra space.

HD-SPR3-1 25 Oct 2019

RESOLVED IN CODE WALKTHROUGH

HD-MASTER-1 30 Sep 2019

Does the variable supposed to be U32 instead of uint32_t?

HD-MASTER-1 01 Oct 2019

Fixed.

HD-MASTER-1 30 Sep 2019

Do we need this space?

HD-MASTER-1 01 Oct 2019

I like to put a blank line between "system" includes and "app" includes.

HD-MASTER-1 01 Oct 2019

Fixed.

HD-MASTER-1 01 Oct 2019

Fixed.

HD-MASTER-1 01 Oct 2019

Isn't lamp state 1 and 2 LAMP_STATE_ON or LAMP_STATE_OFF? If so, I suggest to correct comments

HD-MASTER-1 01 Oct 2019

Can you spell out what does CPLD stand for at least one here?

HD-MASTER-1 01 Oct 2019

Coding standard only requires when there are operators in the expansion.

HD-MASTER-1 01 Oct 2019

Coding standard only requires when there are operators in the expansion.

HD-MASTER-1 01 Oct 2019

These are 2-state patterns to support blinking. If blinking, then yes it could be ON and OFF (or vice-versa). If no blink, then both states would be ON. If color shouldn't even be on at all, both states would be OFF.

HD-MASTER-1 01 Oct 2019

PIN_SIGNAL_STATE_T is not defined in this h file.
Shouldn't there be the h file (defining PIN_SIGNAL_STATE_T) here instead of the c file?

HD-MASTER-1 01 Oct 2019

Use parenthesis around the function.

HD-MASTER-1 01 Oct 2019

Same as before

HD-MASTER-1 04 Oct 2019

I abbreviated them to a fixed 4 characters to help make transition table (in .c file) more readable.

HD-MASTER-1 04 Oct 2019

Recommend complete spell out, e.g., from MODE_FAUL to MODE_FAULT

HD-MASTER-1 04 Oct 2019

Now reviewed.

HD-MASTER-1 04 Oct 2019

Now reviewed.

HD-MASTER-1 04 Oct 2019

Now reviewed.

HD-MASTER-1 04 Oct 2019

Now reviewed.

HD-MASTER-1 04 Oct 2019

Now reviewed.

HD-MASTER-1 05 Oct 2019

Reviewed.

HD-MASTER-1 06 Oct 2019

Reviewed.

HD-MASTER-1 06 Oct 2019

Reviewed.

HD-SPR3-1 10 Oct 2019

Fixed.

HD-SPR3-1 11 Oct 2019

I think this need to be round parethenses

HD-SPR3-1 14 Oct 2019

Addressed in code walk-through.

HD-SPR3-1 10 Oct 2019

General comment. We need to define these constant with #define

HD-SPR3-1 14 Oct 2019

Leaving as is because it is HALCoGen generated code.

HD-SPR3-1 10 Oct 2019

create a temp #define for 0 that indicates intend

HD-SPR3-1 14 Oct 2019

Addressed in code walk-through.

HD-SPR3-1 10 Oct 2019

define 2 as a #define

HD-SPR3-1 14 Oct 2019

Used #define.