AC Indicator #29

abukhalaf · 2020-04-08T20:51:19Z

Utilizing ASSIST_CONTROL flag in the main code. It can be utilized in the display code as well, but not attempting this in this push.

Added visual indicators (icons) to distinguish Patient-triggered vs. Time-triggered inhale cycles. They are added to the last row to avoid conflicts with the first row. This can be made prettier if cm H2O units are moved to the knobs instead of cluttering the display.

Tested on the Ambu Bag and all is running as expected.

…itivity pot

…art of exhale

Display.cpp

LICENSE

e-vent.ino

Display.cpp

amadoantonini

Great initiative getting started with the icons!
Let's address #50 though at the same time.

Display.cpp

Display.h

e-vent.ino

amadoantonini

In general I find that by extending what was already somewhat complicated code to do a more complex behavior it became unnecessarily confusing.
In particular, we need to simplify the Display class perhaps by using a display manager library.

Display.h

e-vent.ino

Indicators.h

Display.h

Display.cpp

Alarms.h

abukhalaf · 2020-07-02T15:28:58Z

The following commit 152e899 simplifies further the code.

This has been tested on ventilator 3.1-002 and box 1.0-002.

teddyort

Great start! There's a number of minor details I pointed out that need fixing.

The only big issue is confusing way the alarms are now structured with mixing between confirm alarms, and regular alarms, and lots of redundant and hardcoding to treat them differently.

To clean it up, we should abstract the alarm concept so that it applies equally well to all types of alarms. Perhaps two separt alarms instances (one for the safety conditions and one for the knobs) would be appropriate.

teddyort · 2020-07-20T18:55:13Z

Indicators.h

+    B00100,
+    B00000};
+
+    #endif


Should be left indented

teddyort · 2020-07-20T19:01:58Z

e-vent.ino

+          displ.showPatientIcon();
+        } else {
+          displ.showTimeIcon();
+        }


We shouldn't have separate if blocks that both check patientTriggered state next to each other

teddyort · 2020-07-20T19:06:44Z

e-vent.ino

@@ -246,7 +247,7 @@ void loop() {

      if (now() - tCycleTimer > tEx + MIN_PEEP_PAUSE) {
        pressureReader.set_peep();
-
+        displ.writePEEP(round(pressureReader.peep()));


Why write PEEP here when it will get written along with all the other pressures on line 268?

teddyort · 2020-07-20T19:08:28Z

e-vent.ino

@@ -232,6 +232,7 @@ void loop() {
      if (enteringState) {
        enteringState = false;
        goToPositionByDur(roboclaw, BAG_CLEAR_POS, motorPosition, tEx - (now() - tCycleTimer));
+        displ.hideTimePatientIcon();


Why hide the icon here? Seems it should stay on until the next trigger...

teddyort · 2020-07-20T19:26:13Z

Display.h

@@ -121,27 +127,41 @@ class Display {
  Display(LiquidCrystal* lcd, const float& trigger_threshold):


Comment on this class needs updating

teddyort · 2020-07-20T21:14:08Z

Alarms.cpp

-      if (alarms_[i].isON()) {
+      aConfirmAlarm = (i==NOT_CONFIRM_TV || i==NOT_CONFIRM_RR ||
+                       i==NOT_CONFIRM_IE || i==NOT_CONFIRM_AC);
+      if (aConfirmAlarm && alarms_[i].isON()) {


This is confusing and brittle. Now that the confirm knobs and alarms are being written to different places, they should be treated differently. But hardcoding the list of which alarms are confirm alarms, and writing these almost duplicate functions is bad practice. We should abstract the logic of which alarms are in which cycle, so we don't need to hardcode checks and write code multiple times

teddyort · 2020-07-20T21:15:43Z

Alarms.h

@@ -177,7 +182,7 @@ class Alarm {
  // for at least `min_good_to_clear_` consecutive calls with different `seq`.   
  void setCondition(const bool& bad, const unsigned long& seq);

-  // Set the alarm text (trim or pad to display width)
+  // Set the alarm text (trim or pad to footer width)


Why to footer width when some alarms are in the header?

teddyort · 2020-07-20T21:17:10Z

Alarms.h

+    NOT_CONFIRM_TV,
+    NOT_CONFIRM_RR,
+    NOT_CONFIRM_IE,
+    NOT_CONFIRM_AC,


Why do we need separate alarms for each not_confirm? The old way was better since it's really the same alarm

teddyort · 2020-07-20T21:18:35Z

Alarms.h

+    alarms_[NOT_CONFIRM_TV] = Alarm("CONFIRM?            ", 1, 1, NOTIFY);
+    alarms_[NOT_CONFIRM_RR] = Alarm("CONFIRM?            ", 1, 1, NOTIFY);
+    alarms_[NOT_CONFIRM_IE] = Alarm("CONFIRM?            ", 1, 1, NOTIFY);
+    alarms_[NOT_CONFIRM_AC] = Alarm("CONFIRM?            ", 1, 1, NOTIFY);


Is all this duplicating of NOT_CONFIRM really needed?

teddyort · 2020-07-20T21:19:31Z

Alarms.h

+    if (key == display::DisplayKey::VOLUME)      { NOT_CONFIRM = NOT_CONFIRM_TV;}
+    if (key == display::DisplayKey::BPM)         { NOT_CONFIRM = NOT_CONFIRM_RR;}
+    if (key == display::DisplayKey::IE_RATIO)    { NOT_CONFIRM = NOT_CONFIRM_IE;}
+    if (key == display::DisplayKey::AC_TRIGGER)  { NOT_CONFIRM = NOT_CONFIRM_AC;}


And here again... this will be very hard to maintain

abukhalaf added 15 commits April 3, 2020 10:07

use the assit control flag with display, reverse map the trigger sens…

66aee68

…itivity pot

Merge branch 'develop' into AssistControl_cleanup

7c922fb

trigger sensitivty knob [2<5<off]

6445c58

Merge branch 'develop' into AssistControl_cleanup

8f02eb1

remove inhale type in favor to patienttriggered flag

253d924

Create LICENSE

6fadcbe

Update LICENSE

71bb064

Merge branch 'develop' into AssistControl_cleanup

75a1156

add patient and time icons

6d13a1e

update flag

7866ee0

adjust the patient icon

bc37ea5

Merge branch 'develop' into AssistControl_cleanup

45bb7c0

revert to [OFF, 2, 5] trigger sensitivty scale

414b9f3

revert to [OFF, 2, 5] trigger sensitivty scale

9337c35

adjust message length to avoid flickering; add hide icon called at st…

fcf83d7

…art of exhale

abukhalaf requested review from amadoantonini and teddyort April 8, 2020 20:51

amadoantonini reviewed Apr 13, 2020

View reviewed changes

Display.cpp Outdated Show resolved Hide resolved

LICENSE Outdated Show resolved Hide resolved

e-vent.ino Outdated Show resolved Hide resolved

Display.cpp Outdated Show resolved Hide resolved

Display.cpp Outdated Show resolved Hide resolved

abukhalaf added 8 commits April 13, 2020 13:24

Merge branch 'develop' into AssistControl_cleanup

6a8fe20

rename flag

a360e51

defined icon constants in header file

54176cf

Merge branch 'develop' into AssistControl_cleanup

6908c13

fix constant name

a329931

move indicators to row 3 column 11

053227a

Merge branch 'develop' into AssistControl_cleanup

31b0471

bug fix: ensure 2.0 AC knob setting is ON

deb5439

abukhalaf changed the title ~~AC Flag and AC Indicators~~ AC Indicator May 8, 2020

bug fix: ensure 2.0 AC knob setting is ON

419383f

amadoantonini reviewed May 9, 2020

View reviewed changes

Display.cpp Outdated Show resolved Hide resolved

Display.h Outdated Show resolved Hide resolved

Display.h Outdated Show resolved Hide resolved

e-vent.ino Outdated Show resolved Hide resolved

e-vent.ino Outdated Show resolved Hide resolved

abukhalaf marked this pull request as draft May 14, 2020 13:38

abukhalaf added 7 commits May 28, 2020 12:42

Merge branch 'develop' into AssistControl_cleanup

d0729ca

Adding Indicators, and Rearranging Knob Displaying

b299a07

Display Snooze Timer, Created Footer for Confirm

5ff896e

Ensure snooze timer disappears when no alarms

1215201

Eliminate a space overlap

b956a7f

Hide snooze timer for edge cases; unsnooze for no alarms

168de9f

Assign each knob a confim alarm, and cycle through

ea44e8c

abukhalaf marked this pull request as ready for review June 13, 2020 02:10

Display 1st PEEP measurement; Fix typo in logger

47c7caf

amadoantonini reviewed Jul 1, 2020

View reviewed changes

Simplify #29 and #50 refactoring

152e899

abukhalaf requested a review from amadoantonini July 2, 2020 15:29

teddyort requested changes Jul 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AC Indicator #29

AC Indicator #29

abukhalaf commented Apr 8, 2020 •

edited

Loading

amadoantonini left a comment

amadoantonini left a comment

abukhalaf commented Jul 2, 2020 •

edited

Loading

teddyort left a comment

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

teddyort Jul 20, 2020

		@@ -121,27 +127,41 @@ class Display {
		Display(LiquidCrystal* lcd, const float& trigger_threshold):

AC Indicator #29

Are you sure you want to change the base?

AC Indicator #29

Conversation

abukhalaf commented Apr 8, 2020 • edited Loading

amadoantonini left a comment

Choose a reason for hiding this comment

amadoantonini left a comment

Choose a reason for hiding this comment

abukhalaf commented Jul 2, 2020 • edited Loading

teddyort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abukhalaf commented Apr 8, 2020 •

edited

Loading

abukhalaf commented Jul 2, 2020 •

edited

Loading