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

Review Summary: No summary


----------------------------------------
File: dialin/hd/air_bubbles.py
Revision Comment by Sean Nash on 18 May 2021, 16:04 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9927

  Should we have a get function for air_bubbles_state too?

  Reply by pmontazemi on 19 May 2021, 10:37
  > Blood leak currently outputs the states of the state machine
  > to the console as it changes. If we want to get the state, we
  > will need to implement it here and for Blood Leak as well.
  > How are the other Dialin API libraries handling it? We need
  > to be consistent across here.

    Reply by Sean Nash on 19 May 2021, 13:50
    > I think older classes may not have get functions for
    > everything, but Sarina and others prefer get functions to
    > accessing properties directly so we're trying to create get
    > functions for every collected data.

      Reply by pmontazemi on 19 May 2021, 17:04
      > Addressed.

        Reply by Sean Nash on 20 May 2021, 10:35
        > RESOLVED.

Revision Comment by plucia on 18 May 2021, 16:10 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9931

  Missing typing information for parameters and return type

  Reply by pmontazemi on 19 May 2021, 10:31
  > Addressed.

    Reply by plucia on 19 May 2021, 11:05
    > RESOLVED

Revision Comment by Sean Nash on 18 May 2021, 16:05 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9928

  param is "status", not detected.

  Reply by pmontazemi on 19 May 2021, 10:02
  > Addressed.

    Reply by Sean Nash on 19 May 2021, 13:52
    > RESOLVED.

Revision Comment by Sean Nash on 18 May 2021, 16:08 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9930

  Should have definitions for 0 and 1 similar to fluid/air.

  Reply by pmontazemi on 19 May 2021, 10:01
  > Addressed.

    Reply by Sean Nash on 19 May 2021, 13:52
    > RESOLVED.

Revision Comment by plucia on 18 May 2021, 16:11 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9932

  Missing typing information for parameters and return type

  Reply by pmontazemi on 19 May 2021, 10:31
  > Addressed.

    Reply by plucia on 19 May 2021, 11:03
    > RESOLVED

Revision Comment by plucia on 18 May 2021, 16:11 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9933

  Missing typing information for parameters and return type

  Reply by pmontazemi on 19 May 2021, 09:51
  > Are you referring to comment section below? There I state
  > @param ms: integer (in ms) etc. If you are referring to
  > something else, please provide an example of what you are
  > expecting to see as I do not understand the comment.

    Reply by plucia on 19 May 2021, 09:56
    > Instead of 
    > {code}
    > cmd_air_bubbles_data_broadcast_interval_override(self, ms,
    > reset=NO_RESET): {code}
    > you need
    > {code}
    > cmd_air_bubbles_data_broadcast_interval_override(self, ms:
    > int, reset: int = NO_RESET) -> int: {code}
    > It is necessary to have typing information in commands so
    > the service app can call them correctly
    > Also, putting the typing information in the docstring is
    > good, but it's better to have it in the function so that it
    > can be dynamically interpreted when the code is inspected

      Reply by pmontazemi on 19 May 2021, 10:09
      > Addressed.

        Reply by plucia on 19 May 2021, 11:05
        > RESOLVED

Revision Comment by Sean Nash on 21 May 2021, 16:25 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c10016

  Is this print meant to be permanent?  Or is it debug code?

  Reply by pmontazemi on 24 May 2021, 09:02
  > Removed.

    Reply by Sean Nash on 24 May 2021, 13:11
    > RESOLVED.

Revision Comment by plucia on 19 May 2021, 10:02 
https://devapps.diality.us/cru/DIALIN-DEN-7860-1#c9942

  This should be checked in the function

  Reply by pmontazemi on 19 May 2021, 10:35
  > None of the functions we have check for any of these
  > conditions. If we want to implement this in the code, we will
  > need to remove this comment from all *.py Dialin API files
  > and implement the checks in all *.py Dialin API files. Please
  > let me know how you would like to proceed.

    Reply by plucia on 19 May 2021, 10:59
    > I wasn't aware the ms needed to be in multiples of 10ms.
    > I've created a case DIAL-115 to add constraint enforcement
    > across dialin. 
    > For now, I think new code should incorporate some form of
    > constraint enforcement. So if you could just apply it here
    > in DIAL-115 I will address this in other parts of dialin.
    > I think the docstring describing the constraint is useful
    > and should be kept if it's already there even if we add
    > code to check the constraint as well.

      Reply by Sean Nash on 19 May 2021, 13:46
      > Actually they should be in multiples of 50ms (general
      > task).  I mistakenly had a few functions written with
      > 10ms (priority task) but that is not correct.  I don't
      > think it would cause an error, but non-multiple intervals
      > will be floored to nearest lower multiple of 50 ms by the
      > f/w I suspect.

        Reply by pmontazemi on 19 May 2021, 17:16
        > Addressed. [~snash] [~plucia] Please check and resolve.

          Reply by plucia on 21 May 2021, 06:56
          > RESOLVED


---

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

Title: DIALIN-DEN-7860_HD Dev Bubble Detector Driver
Statement of Objectives:


State: Closed

Summary: 

Author: pmontazemi
Moderator: pmontazemi
Reviewers: (0 active, 2 completed*)
     Sean Nash   (*)
     plucia   (*)