This is a list of all comments for DIALIN-DEN-2652-1.

Review Summary: No summary


----------------------------------------
File: tests/SeansUFTest.py
Revision Comment by pmontazemi on 19 May 2020, 23:04 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1934

  Why does test script include the author's name in its file
  name?

  Reply by Sean Nash on 20 May 2020, 08:47
  > Should these test scripts even be code reviewed?  I view this
  > folder as a play area for developers.  I put my name in the
  > file name to help me find my scripts.

    Reply by pmontazemi on 20 May 2020, 09:50
    > Sure, this is a play space, the V&V ones, however, will be
    > much more controlled. But, I would still recommend using
    > the name of module or epic to avoid including personal
    > names.

      Reply by Sean Nash on 20 May 2020, 10:00
      > Done.

        Reply by pmontazemi on 20 May 2020, 13:42
        > RESOLVED.

----------------------------------------
File: dialin/dg/dialysate_generator.py
Revision Comment by plucia on 20 May 2020, 10:35 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1998

  Don't you need the first element in each tuple?

  Reply by pmontazemi on 20 May 2020, 11:19
  > I think this needs to be addressed by Sean.

    Reply by Sean Nash on 20 May 2020, 12:59
    > Fixed.

      Reply by plucia on 20 May 2020, 13:05
      > RESOLVED

----------------------------------------
File: dialin/dg/drain_pump.py
Revision Comment by plucia on 20 May 2020, 11:55 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2000

  Please use snake_case:
  [PEP8|https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles]
  bq. Function names should be lowercase, with words separated by
  underscores as necessary to improve readability.
  bq. Variable names follow the same convention as function
  names.

  Reply by Sean Nash on 20 May 2020, 13:00
  > Fixed.

    Reply by plucia on 20 May 2020, 13:06
    > RESOLVED

Revision Comment by plucia on 20 May 2020, 12:05 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2004

  See above comment

  Reply by plucia on 21 May 2020, 10:26
  > RESOLVED.

----------------------------------------
File: dialin/dg/hd_proxy.py
Revision Comment by plucia on 20 May 2020, 11:56 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2001

  It returns False otherwise, not 0

  Reply by plucia on 21 May 2020, 10:24
  > RESOLVED.

----------------------------------------
File: tests/SeansHDTestScript.py
Revision Comment by pmontazemi on 19 May 2020, 23:04 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1933

  Why does test script include the author's name in its file
  name?

  Reply by Sean Nash on 20 May 2020, 10:00
  > Fixed.

    Reply by pmontazemi on 20 May 2020, 13:02
    > RESOLVED.

----------------------------------------
File: dialin/dg/valves.py
Revision Comment by Sean Nash on 20 May 2020, 08:54 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1947

  These make it look like the valve states broadcast message has
  a 4-byte boolean for each valve, but I see the f/w is only
  sending one U16 with a bit for each valve.

  Reply by pmontazemi on 20 May 2020, 09:29
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 10:01
    > RESOLVED.

Revision Comment by Sean Nash on 20 May 2020, 08:55 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1948

  I suggest these should be booleans instead of 0/1.

  Reply by pmontazemi on 20 May 2020, 09:35
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 10:01
    > RESOLVED.

Revision Comment by Sean Nash on 20 May 2020, 09:53 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1995

  I think vst will be a single element array.  Should be vst[0] I
  think.
  Also, these & operation will yield zero or non-zero bit masks -
  I think we want boolean (energized/de-energized).

  Reply by pmontazemi on 20 May 2020, 13:58
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 10:00
    > RESOLVED.

Revision Comment by Sean Nash on 20 May 2020, 08:55 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1949

  unpack 'f' is for interpreting floats in a byte array.  For a
  U16, use unpack 'H'.

  Reply by pmontazemi on 20 May 2020, 09:48
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 10:01
    > RESOLVED.

Revision Comment by Sean Nash on 20 May 2020, 08:59 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c1950

  Will need some bit masking here to extract each valve from the
  U16.

  Reply by pmontazemi on 20 May 2020, 09:48
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 10:00
    > RESOLVED.

Revision Comment by Sean Nash on 20 May 2020, 14:03 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2039

  valve state will be an integer so use integer_to_bytearray().

  Reply by pmontazemi on 20 May 2020, 14:10
  > Addressed.

    Reply by Sean Nash on 21 May 2020, 09:59
    > RESOLVED.

----------------------------------------
File: tests/can_xmit_test.py
Revision Comment by plucia on 20 May 2020, 12:04 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2002

  Delete

  Reply by plucia on 21 May 2020, 10:07
  > RESOLVED.

----------------------------------------
File: tests/uf_test.py
Revision Comment by plucia on 20 May 2020, 12:04 
https://devapps.diality.us/cru/DIALIN-DEN-2652-1#c2003

  Delete

  Reply by Dara Navaei on 19 October 2023, 13:18
  > RESOLVED


---

ID: DIALIN-DEN-2652-1 https://devapps.diality.us/cru/DIALIN-DEN-2652-1

Title: DIALIN-DEN-2652_DG Valve Control 1 of 2
Statement of Objectives:


State: Closed

Summary: 

Author: pmontazemi
Moderator: pmontazemi
Reviewers: (3 active, 0 completed*)
     Sean Nash
     plucia
     Behrouz NematiPour