hdfirmware / firmware

App

Clone Tools

Stats

Commits this week: 0
Total Committers: 17
Last Commit: 19 Nov 25
Commits this week: 0

Lines of code count not available

Commit Activity

Commits By Day In Week
52 week commits volume
Commits By Day In Week
Commits by day
Commits By Day In Week
Commits by hour
Commits By Hour In Day
Commit calendar
 

Most active committers (90 days)

loading
loading
  • last updated a few minutes ago
All Commits Reviews
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Wednesday 12 May 2021
3:31 pm

Peman Montazemi <pmontazemi@diality.com>

DEN-7860: Addressed part of code review comments

Options
    • -20
    • +23
    ./Controllers/Bubble.c
    • -0
    • +1
    ./Services/AlarmMgmt.h
    • -2
    • +2
    ./Services/FPGA.c
    • -1
    • +5
    ./Tasks/TaskPriority.c
Tuesday 11 May 2021
3:39 pm

Sean Nash

I think this makes sense. We've done the same in other monitors/controllers like valves where there are several similar components handled in same driver.

I think this makes sense. We've done the same in other monitors/controllers like valves where there are several similar components handled in same driver.

3:36 pm

Sean Nash

Maybe cast the status params as U32 as well so there won't be an issue within the broadcast function (see my comment there for context).

Maybe cast the status params as U32 as well so there won't be an issue within the broadcast function (see my comment there for context).

3:34 pm

Sean Nash

For blood leak sensor, we didn't want to go to normal state until it was zeroed and self-tested. For bubble detector, do we want to be stuck in init state until self-test is requested? I think if w...

For blood leak sensor, we didn't want to go to normal state until it was zeroed and self-tested. For bubble detector, do we want to be stuck in init state until self-test is requested?
I think if we keep this state machine as is, we will want to request self-test in POST so that we can move on to normal state quickly. Then still request another self-test in each pre-treatment as we are now doing.

3:30 pm

qnguyen  (deleted user)

Should we broadcast both bubble detectors' data in one message?

Should we broadcast both bubble detectors' data in one message?

3:30 pm

Sean Nash

Same. Validate bubble param at top of function before using it. Then no need for else below.

Same. Validate bubble param at top of function before using it. Then no need for else below.

3:28 pm

Sean Nash

Should validate bubble param up top before you use it. S/W fault if invalid. Then don't need the else below.

Should validate bubble param up top before you use it. S/W fault if invalid. Then don't need the else below.

3:24 pm

Sean Nash

Initialize the other 3 fields of the override structure.

Initialize the other 3 fields of the override structure.

3:21 pm

Sean Nash

I agree we want to send the two status enums as a U32 over the CAN. I think it's better to assign w/ cast the two status to local U32s before memcpy into payload. That way, memcpy is given 4 bytes ...

I agree we want to send the two status enums as a U32 over the CAN. I think it's better to assign w/ cast the two status to local U32s before memcpy into payload. That way, memcpy is given 4 bytes to copy. As is, I worry compiler will only have 1 or 2 bytes for the enum passed into the function and we're forcing memcpy to copy 4 bytes - could corrupt the upper bytes of the two status fields in the message payload.

3:12 pm

Sean Nash

Code below looks like this message is expected to have a payload of type OVERRIDE_PUMP_SET_PT_PAYLOAD_T. Not sure why a bubble test request would have a pump type payload, maybe it had what you nee...

Code below looks like this message is expected to have a payload of type OVERRIDE_PUMP_SET_PT_PAYLOAD_T. Not sure why a bubble test request would have a pump type payload, maybe it had what you needed (an index). But this structure will have a length larger than 0. Should be "if ( sizeof( OVERRIDE_PUMP_SET_PT_PAYLOAD_T ) == message->hdr.payloadLen ).

3:06 pm

Sean Nash

I think it's worth checking that the self-test mode was entered as expected.

I think it's worth checking that the self-test mode was entered as expected.

3:03 pm

Sean Nash

Same comment as above.

Same comment as above.

3:03 pm

Sean Nash

No return value for this function.

No return value for this function.

3:03 pm

Sean Nash

Looks like fpgaActuatorSetPoints is an output and no inputs.

Looks like fpgaActuatorSetPoints is an output and no inputs.

3:02 pm

Sean Nash

This should be a s/w fault.

This should be a s/w fault.

2:54 pm

Sean Nash

Agreed. This is for me to add a check for this error. Will remove comment when I'm done.

Agreed. This is for me to add a check for this error. Will remove comment when I'm done.

2:54 pm

Sean Nash

Is this still an issue?

Is this still an issue?

2:53 pm

Sean Nash

Agreed. This is for me to add a check for this error. I'll remove comment when I'm done.

Agreed. This is for me to add a check for this error. I'll remove comment when I'm done.

2:53 pm

Sean Nash

Is this still an issue?

Is this still an issue?

2:01 pm

Sean Nash

Fixed merge conflicts.

Options
    • -23
    • +28
    ./Controllers/BloodFlow.c
    • -0
    • +12
    ./Controllers/DialOutFlow.c
    • -0
    • +1
    ./HDCommon.h
    • -9
    • +9
    ./Services/FPGA.c
1:44 pm

Sean Nash

DEN-7605: Added pump direction error counter check and alarm. Added build switch to allow change to treatment duration to as low as 1 minute.

Options
    • -2
    • +14
    ./Controllers/BloodFlow.c
    • -1
    • +13
    ./Controllers/DialInFlow.c
    • -0
    • +12
    ./Controllers/DialOutFlow.c
    • -0
    • +1
    ./HDCommon.h
    • -0
    • +4
    ./Modes/ModeTreatment.c
    • -9
    • +9
    ./Services/FPGA.c
11:50 am

Peman Montazemi <pmontazemi@diality.com>

DEN-7860: Finished implementing HD Air Bubble Detectors

Options
    • -35
    • +16
    ./Controllers/Bubble.c
    • -1
    • +1
    ./Controllers/Bubble.h
    • -4
    • +4
    ./Modes/SelfTests.c
    • -10
    • +14
    ./Services/FPGA.c
    • -4
    • +4
    ./Services/FPGA.h
    • -3
    • +38
    ./Services/SystemCommMessages.c
    • -0
    • +3
    ./Services/SystemCommMessages.h
10:50 am

qnguyen  (deleted user)

Why should we need a separate mask for bubble test state? I thought the normal input status mask should be sufficient to see the faked air bubble.

Why should we need a separate mask for bubble test state? I thought the normal input status mask should be sufficient to see the faked air bubble.

10:48 am

qnguyen  (deleted user)

Incorrect enum.

Incorrect enum.

10:46 am

qnguyen  (deleted user)

Incorrect casting type.

Incorrect casting type.

10:45 am

qnguyen  (deleted user)

Re-word comment.

Re-word comment.

10:43 am

qnguyen  (deleted user)

Recommend converting to a local variable rather than an array.

Recommend converting to a local variable rather than an array.

10:43 am

qnguyen  (deleted user)

Recommend converting to a local variable rather than an array.

Recommend converting to a local variable rather than an array.

10:41 am

qnguyen  (deleted user)

Recommend converting to a local variable rather than an array.

Recommend converting to a local variable rather than an array.

10:37 am

qnguyen  (deleted user)

These declarations seem odd. Please re-check.

These declarations seem odd. Please re-check.