AC Indicator#29
Conversation
amadoantonini
left a comment
amadoantonini
left a comment
There was a problem hiding this comment.
Great initiative getting started with the icons!
Let's address #50 though at the same time.
amadoantonini
left a comment
amadoantonini
left a comment
There was a problem hiding this comment.
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.
|
The following commit 152e899 simplifies further the code. This has been tested on ventilator 3.1-002 and box 1.0-002. |
teddyort
left a comment
teddyort
left a comment
There was a problem hiding this comment.
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.
| B00100, | ||
| B00000}; | ||
|
|
||
| #endif |
| displ.showPatientIcon(); | ||
| } else { | ||
| displ.showTimeIcon(); | ||
| } |
There was a problem hiding this comment.
We shouldn't have separate if blocks that both check patientTriggered state next to each other
| if (now() - tCycleTimer > tEx + MIN_PEEP_PAUSE) { | ||
| pressureReader.set_peep(); | ||
|
|
||
| displ.writePEEP(round(pressureReader.peep())); |
There was a problem hiding this comment.
Why write PEEP here when it will get written along with all the other pressures on line 268?
| if (enteringState) { | ||
| enteringState = false; | ||
| goToPositionByDur(roboclaw, BAG_CLEAR_POS, motorPosition, tEx - (now() - tCycleTimer)); | ||
| displ.hideTimePatientIcon(); |
There was a problem hiding this comment.
Why hide the icon here? Seems it should stay on until the next trigger...
| @@ -121,27 +127,41 @@ class Display { | |||
| Display(LiquidCrystal* lcd, const float& trigger_threshold): | |||
There was a problem hiding this comment.
Comment on this class needs updating
| 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()) { |
There was a problem hiding this comment.
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
| 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) |
There was a problem hiding this comment.
Why to footer width when some alarms are in the header?
| NOT_CONFIRM_TV, | ||
| NOT_CONFIRM_RR, | ||
| NOT_CONFIRM_IE, | ||
| NOT_CONFIRM_AC, |
There was a problem hiding this comment.
Why do we need separate alarms for each not_confirm? The old way was better since it's really the same alarm
| 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); |
There was a problem hiding this comment.
Is all this duplicating of NOT_CONFIRM really needed?
| 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;} |
There was a problem hiding this comment.
And here again... this will be very hard to maintain
3 participants
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.